netfilter: x_tables: speed up iptables-restore
Subsystems (3)
Florian Westphal
maintainerPATCH v2 1/1 netfilter: x_tables: speed up iptables-restore
iptables-restore can take quite a long time when sytem is busy, in order of half a minute or more.
The main reason for this is the way ip(6)tables performs table swap, or, more precisely, expensive sequence lock synchronizations when reading counters.
When xt_replace_table assigns the new ruleset pointer, it does not wait for other processors to finish with old ruleset.
Instead it relies on the counter sequence lock in get_counters() to do this.
This works but this is very costly if system is busy as each counter read operation can possibly be restarted indefinitely.
Instead, make xt_replace_table wait until all processors are known to not use the old ruleset anymore.
This allows to read the old counters without any locking, no cpu is using the ruleset anymore so counters can't change either.
ipv4/netfilter/arp_tables.c | 22 ++++++++++++++++++++-- ipv4/netfilter/ip_tables.c | 23 +++++++++++++++++++++-- ipv6/netfilter/ip6_tables.c | 22 ++++++++++++++++++++-- netfilter/x_tables.c | 15 ++++++++++++--- 4 files changed, 73 insertions(+), 9 deletions(-)
Florian Westphal
maintainerPATCH v2 1/2 netfilter: x_tables: make xt_replace_table wait until old rules are not used anymore
xt_replace_table relies on table replacement counter retrieval (which uses xt_recseq to synchronize pcpu counters).
This is fine, however with large rule set get_counters() can take a very long time -- it needs to synchronize all counters because it has to assume concurrent modifications can occur.
Make xt_replace_table synchronize by itself by waiting until all cpus had an even seqcount.
This allows a followup patch to copy the counters of the old ruleset without any synchonization after xt_replace_table has completed.
---
v2: fix Erics email address
net/netfilter/x_tables.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)
net/netfilter/x_tables.c +12 -3
@@ -1153,6 +1153,7 @@ xt_replace_table(struct xt_table *table, int *error) {struct xt_table_info *private; +unsigned int cpu;int ret; ret =xt_jumpstack_alloc (newinfo);
@@ -1184,12 +1185,20 @@ xt_replace_table(struct xt_table *table,/* * Even though table entries have now been swapped, other CPU's -* may still be using the old entries. This is okay, because -* resynchronization happens because of the locking done -* during the get_counters() routine. +* may still be using the old entries... */ local_bh_enable (); +/* ... so wait for even xt_recseq on all cpus */ +for_each_possible_cpu (cpu) { +seqcount_t *s = &per_cpu (xt_recseq, cpu); + +while (raw_read_seqcount (s) &1 ) +cpu_relax (); + +cond_resched (); + } +#ifdef CONFIG_AUDIT if (audit_enabled) { audit_log(current->audit_context, GFP_KERNEL,
Signature · 1 line
2.13.6
Re: [PATCH v2 nf-next 1/2] netfilter: x_tables: make xt_replace_table wait until old rules are not used anymore
Quoted 44 lines · Florian Westphal
xt_replace_tablerelies on table replacement counter retrieval (which usesxt_recseqto synchronize pcpu counters).This is fine, however with large rule set
get_counters()can take a very long time -- it needs to synchronize all counters because it has to assume concurrent modifications can occur.Make
xt_replace_tablesynchronize by itself by waiting until all cpus had an even seqcount.This allows a followup patch to copy the counters of the old ruleset without any synchonization after
xt_replace_tablehas completed.Cc Dan Williams <[email protected]>Cc Eric Dumazet <[email protected]>Signed-off-by Florian Westphal <[email protected]>---
v2: fix Erics email addressnet/netfilter/x_tables.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-)net/netfilter/x_tables.c +2
-3@@ -1153,6 +1153,7 @@ xt_replace_table(struct xt_table *table, int *error) {struct xt_table_info *private; +unsigned int cpu;int ret; ret =xt_jumpstack_alloc (newinfo);@@ -1184,12 +1185,20 @@ xt_replace_table(struct xt_table *table,/* * Even though table entries have now been swapped, other CPU's -* may still be using the old entries. This is okay, because -* resynchronization happens because of the locking done -* during the get_counters() routine. +* may still be using the old entries... */ local_bh_enable ();
net/netfilter/x_tables.c xt_replace_table(struct xt_table *table, · line 1200
Quoted 10 lines
net/netfilter/x_tables.c +9
-0+/* ... so wait for even xt_recseq on all cpus */ +for_each_possible_cpu (cpu) { +seqcount_t *s = &per_cpu (xt_recseq, cpu); + +while (raw_read_seqcount (s) &1 ) +cpu_relax (); + +cond_resched (); + }
It seems that we could also check :
1) If low order bit of sequence is 0
Or
2) the value has changed
for_each_possible_cpu(cpu) {
seqcount_t *s = &per_cpu(xt_recseq, cpu);
u32 seq = raw_read_seqcount(s) ;
if (seq & 1) {
do {
cpu_relax();
} while (seq == raw_read_seqcount(s));Thanks !
Florian Westphal
maintainerRe: [PATCH v2 nf-next 1/2] netfilter: x_tables: make xt_replace_table wait until old rules are not used anymore
Quoted 17 lines · Eric Dumazet
net/netfilter/x_tables.c xt_replace_table(struct xt_table *table, · line 1200
Quoted 9 lines
net/netfilter/x_tables.c +9
-0+/* ... so wait for even xt_recseq on all cpus */ +for_each_possible_cpu (cpu) { +seqcount_t *s = &per_cpu (xt_recseq, cpu); + +while (raw_read_seqcount (s) &1 ) +cpu_relax (); + +cond_resched (); + }It seems that we could also check :
1) If low order bit of sequence is 0
Or
2) the value has changed
Quoted 9 lines
for_each_possible_cpu(cpu) {
seqcount_t *s = &per_cpu(xt_recseq, cpu);
u32 seq = raw_read_seqcount(s) ;
if (seq & 1) {
do {
cpu_relax();
} while (seq == raw_read_seqcount(s));Actually I first used
for_each_possible_cpu(cpu) {
seqcount_t *s = &per_cpu(xt_recseq, cpu);
(void)__read_seqcount_begin(s);
}
but it looked confusing (not paired with a _retry function). I'll respin with a loop like your suggestion above. Thanks!
Florian Westphal
maintainerPATCH v2 2/2 netfilter: x_tables: don't use seqlock when fetching old counters
In reply to Florian Westphalafter previous commit xt_replace_table will wait until all cpus had even seqcount (i.e., no cpu is accessing old ruleset).
Add a 'old' counter retrival version that doesn't synchronize counters. Its not needed, the old counters are not in use anymore at this point.
This speeds up table replacement on busy systems with large tables (and many cores).
---
v2: fix Erics email address
net/ipv4/netfilter/arp_tables.c | 22 ++++++++++++++++++++-- net/ipv4/netfilter/ip_tables.c | 23 +++++++++++++++++++++-- net/ipv6/netfilter/ip6_tables.c | 22 ++++++++++++++++++++-- 3 files changed, 61 insertions(+), 6 deletions(-)
net/ipv4/netfilter/arp_tables.c +20 -2
@@ -634,6 +634,25 @@ static void get_counters(const struct xt_table_info *t, } } +static void get_old_counters (const struct xt_table_info *t, +struct xt_counters counters[]) +{ +struct arpt_entry *iter; +unsigned int cpu, i; + +for_each_possible_cpu (cpu) { + i =0 ; + xt_entry_foreach(iter, t->entries, t->size) { +struct xt_counters *tmp; + + tmp =xt_get_per_cpu_counter (&iter->counters, cpu); +ADD_COUNTER (counters[i], tmp->bcnt, tmp->pcnt); + ++i; + } +cond_resched (); + } +} +static struct xt_counters *alloc_counters (const struct xt_table *table) {unsigned int countersize;
@@ -910,8 +929,7 @@ static int __do_replace(struct net *net, const char *name, (newinfo->number <= oldinfo->initial_entries))module_put (t->me); -/* Get the old counters, and synchronize with replace */ -get_counters (oldinfo, counters); +get_old_counters (oldinfo, counters);/* Decrease module usage counts and free resource */ loc_cpu_old_entry = oldinfo->entries;
net/ipv4/netfilter/ip_tables.c +21 -2
@@ -781,6 +781,26 @@ get_counters(const struct xt_table_info *t, } } +static void get_old_counters (const struct xt_table_info *t, +struct xt_counters counters[]) +{ +struct ipt_entry *iter; +unsigned int cpu, i; + +for_each_possible_cpu (cpu) { + i =0 ; + xt_entry_foreach(iter, t->entries, t->size) { +const struct xt_counters *tmp; + + tmp =xt_get_per_cpu_counter (&iter->counters, cpu); +ADD_COUNTER (counters[i], tmp->bcnt, tmp->pcnt); + ++i;/* macro does multi eval of i */ + } + +cond_resched (); + } +} +static struct xt_counters *alloc_counters (const struct xt_table *table) {unsigned int countersize;
@@ -1070,8 +1090,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, (newinfo->number <= oldinfo->initial_entries))module_put (t->me); -/* Get the old counters, and synchronize with replace */ -get_counters (oldinfo, counters); +get_old_counters (oldinfo, counters);/* Decrease module usage counts and free resource */ xt_entry_foreach (iter, oldinfo->entries, oldinfo->size)
net/ipv6/netfilter/ip6_tables.c +20 -2
@@ -800,6 +800,25 @@ get_counters(const struct xt_table_info *t, } } +static void get_old_counters (const struct xt_table_info *t, +struct xt_counters counters[]) +{ +struct ip6t_entry *iter; +unsigned int cpu, i; + +for_each_possible_cpu (cpu) { + i =0 ; + xt_entry_foreach(iter, t->entries, t->size) { +const struct xt_counters *tmp; + + tmp =xt_get_per_cpu_counter (&iter->counters, cpu); +ADD_COUNTER (counters[i], tmp->bcnt, tmp->pcnt); + ++i; + } +cond_resched (); + } +} +static struct xt_counters *alloc_counters (const struct xt_table *table) {unsigned int countersize;
@@ -1090,8 +1109,7 @@ __do_replace(struct net *net, const char *name, unsigned int valid_hooks, (newinfo->number <= oldinfo->initial_entries))module_put (t->me); -/* Get the old counters, and synchronize with replace */ -get_counters (oldinfo, counters); +get_old_counters (oldinfo, counters);/* Decrease module usage counts and free resource */ xt_entry_foreach (iter, oldinfo->entries, oldinfo->size)
Signature · 1 line
2.13.6