NUMA aware ipv4/netfilter/ip_tables.c
Subsystems (8)
PATCH 1/1 NUMA aware ipv4/netfilter/ip_tables.c
Hi
Part of the performance problem we have with netfilter is memory allocation is not NUMA aware, but 'only' SMP aware.
What do you think of this patch ?
Note : The allocation function is quite complex (copied from drivers/pci/pci-driver.c pci_call_probe()) because current kernel doesnt have a NUMA aware vmalloc() wrapper, maybe a future kernel will export such a common function ?
Thank you
net/ipv4/netfilter/ip_tables.c +95 -58
@@ -17,6 +17,7 @@#include <linux/skbuff.h> #include <linux/kmod.h> #include <linux/vmalloc.h> +#include <linux/mempolicy.h> #include <linux/netdevice.h> #include <linux/module.h> #include <linux/tcp.h>
@@ -82,11 +83,6 @@context stops packets coming through and allows user context to read the counters or update the rules. -To be cache friendly on SMP, we arrange them like so: -[ n-entries ] -... cache-align padding ... -[ n-entries ] -Hence the start of any table is given by get_table() below. */ /* The table itself */
@@ -104,7 +100,8 @@unsigned int underflow[NF_IP_NUMHOOKS ];/* ipt_entry tables: one per CPU */ -char entries[0 ]____cacheline_aligned ; +void *entry0 ;/* entry for the first possible cpu */ +void *entries[NR_CPUS ]; };static LIST_HEAD (ipt_target);
@@ -112,11 +109,6 @@static LIST_HEAD (ipt_tables);#define ADD_COUNTER (c,b,p) do { (c).bcnt += (b); (c).pcnt += (p); } while(0) -#ifdef CONFIG_SMP -#define TABLE_OFFSET (t,p) (SMP_ALIGN((t)->size)*(p)) -#else -#define TABLE_OFFSET (t,p) 0 -#endif #if 0 #define down (x) do { printk("DOWN:%u:" #x "\n", __LINE__); down(x); } while(0)
@@ -289,8 +281,7 @@read_lock_bh (&table->lock);IP_NF_ASSERT (table->valid_hooks & (1 << hook)); - table_base = (void *)table->private->entries - +TABLE_OFFSET (table->private,smp_processor_id ()); + table_base = (void *)table->private->entries[smp_processor_id ()]; e =get_entry (table_base, table->private->hook_entry[hook]);#ifdef CONFIG_NETFILTER_DEBUG
@@ -571,7 +562,7 @@for (hook =0 ; hook <NF_IP_NUMHOOKS ; hook++) {unsigned int pos = newinfo->hook_entry[hook];struct ipt_entry *e - = (struct ipt_entry *)(newinfo->entries + pos); + = (struct ipt_entry *)(newinfo->entry0 + pos);if (!(valid_hooks & (1 << hook)))continue ;
@@ -621,13 +612,13 @@ goto next; e = (struct ipt_entry *) - (newinfo->entries + pos); + (newinfo->entry0 + pos); }while (oldpos == pos + e->next_offset);/* Move along one */ size = e->next_offset; e = (struct ipt_entry *) - (newinfo->entries + pos + size); + (newinfo->entry0 + pos + size); e->counters.pcnt = pos; pos += size; }else {
@@ -644,7 +635,7 @@ newpos = pos + e->next_offset; } e = (struct ipt_entry *) - (newinfo->entries + newpos); + (newinfo->entry0 + newpos); e->counters.pcnt = pos; pos = newpos; }
@@ -874,11 +865,11 @@duprintf ("translate_table: size %u\n" , newinfo->size); i =0 ;/* Walk through entries, checking offsets. */ - ret =IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, + ret =IPT_ENTRY_ITERATE (newinfo->entry0, newinfo->size, check_entry_size_and_hooks, newinfo, - newinfo->entries, - newinfo->entries + size, + newinfo->entry0, + newinfo->entry0 + size, hook_entries, underflows, &i);if (ret !=0 )return ret;
@@ -911,20 +902,21 @@/* Finally, each sanity check must pass */ i =0 ; - ret =IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, + ret =IPT_ENTRY_ITERATE (newinfo->entry0, newinfo->size, check_entry, name, size, &i);if (ret !=0 ) { -IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, +IPT_ENTRY_ITERATE (newinfo->entry0, newinfo->size, cleanup_entry, &i);return ret; }/* And one copy for every other CPU */ -for (i =1 ; i <num_possible_cpus (); i++) { -memcpy (newinfo->entries +SMP_ALIGN (newinfo->size)*i, - newinfo->entries, -SMP_ALIGN (newinfo->size)); +for_each_cpu (i) { +if (newinfo->entries[i] != newinfo->entry0) +memcpy (newinfo->entries[i], + newinfo->entry0, + newinfo->size); }return ret;
@@ -940,15 +932,12 @@#ifdef CONFIG_NETFILTER_DEBUG { -struct ipt_entry *table_base; -unsigned int i; - -for (i =0 ; i <num_possible_cpus (); i++) { - table_base = - (void *)newinfo->entries - +TABLE_OFFSET (newinfo, i); +int cpu; - table_base->comefrom =0xdead57ac ; +for_each_cpu (cpu) { +struct ipt_entry *table_base = newinfo->entries[cpu]; +if (table_base) + table_base->comefrom =0xdead57ac ; } }#endif
@@ -990,9 +979,9 @@unsigned int cpu;unsigned int i; -for (cpu =0 ; cpu <num_possible_cpus (); cpu++) { +for_each_cpu (cpu) { i =0 ; - IPT_ENTRY_ITERATE(t->entries + TABLE_OFFSET(t, cpu), + IPT_ENTRY_ITERATE(t->entries[cpu], t->size, add_entry_to_counter, counters,
@@ -1026,7 +1015,7 @@write_unlock_bh (&table->lock);/* ... then copy entire thing from CPU 0... */ -if (copy_to_user (userptr, table->private->entries, total_size) !=0 ) { +if (copy_to_user (userptr, table->private->entry0, total_size) !=0 ) { ret = -EFAULT ; goto free_counters; }
@@ -1038,7 +1027,7 @@struct ipt_entry_match *m;struct ipt_entry_target *t; - e = (struct ipt_entry *)(table->private->entries + off); + e = (struct ipt_entry *)(table->private->entry0 + off); if (copy_to_user(userptr + off + offsetof(struct ipt_entry, counters), &counters[num],
@@ -1107,6 +1096,51 @@return ret; } +static void free_table_info (struct ipt_table_info *info) +{ +int cpu; +for_each_cpu (cpu) +vfree (info->entries[cpu]); +kfree (info); +} + +static struct ipt_table_info *alloc_table_info (unsigned int size) +{ +struct ipt_table_info *newinfo; +int cpu; + newinfo =kmalloc (sizeof (struct ipt_table_info ),GFP_KERNEL ); +if (!newinfo) +return NULL ; +memset (newinfo,0 ,sizeof (*newinfo)); +for_each_cpu (cpu) { +#ifdef CONFIG_NUMA +struct mempolicy *oldpol; +cpumask_t oldmask = current->cpus_allowed; +int node =cpu_to_node (cpu); +if (node >=0 &&node_online (node)) +set_cpus_allowed (current,node_to_cpumask (node)); +/* And set default memory allocation policy */ + oldpol = current->mempolicy; + current->mempolicy = &default_policy; +mpol_get (current->mempolicy); +#endif + newinfo->entries[cpu] =vmalloc (size); +#ifdef CONFIG_NUMA +set_cpus_allowed (current, oldmask); +mpol_free (current->mempolicy); + current->mempolicy = oldpol; +#endif +if (newinfo->entries[cpu] ==0 ) { +free_table_info (newinfo); +return NULL ; +if (!newinfo->entry0) + newinfo->entry0 = newinfo->entries[cpu]; + } + } +return newinfo; +} + +static int do_replace (void __user *user,unsigned int len) {
@@ -1127,12 +1161,10 @@if ((SMP_ALIGN (tmp.size) >>PAGE_SHIFT ) +2 > num_physpages)return -ENOMEM ; - newinfo =vmalloc (sizeof (struct ipt_table_info ) - +SMP_ALIGN (tmp.size) *num_possible_cpus ()); + newinfo =alloc_table_info (tmp.size);if (!newinfo)return -ENOMEM ; - -if (copy_from_user (newinfo->entries, user +sizeof (tmp), +if (copy_from_user (newinfo->entry0, user +sizeof (tmp), tmp.size) !=0 ) { ret = -EFAULT ; goto free_newinfo;
@@ -1185,8 +1217,8 @@/* Get the old counters. */ get_counters (oldinfo, counters);/* Decrease module usage counts and free resource */ -IPT_ENTRY_ITERATE (oldinfo->entries, oldinfo->size, cleanup_entry,NULL ); -vfree (oldinfo); +IPT_ENTRY_ITERATE (oldinfo->entry0, oldinfo->size, cleanup_entry,NULL ); +free_table_info (oldinfo);if (copy_to_user (tmp.counters, counters,sizeof (struct ipt_counters ) * tmp.num_counters) !=0 ) ret = -EFAULT ;
@@ -1198,11 +1230,11 @@module_put (t->me);up (&ipt_mutex); free_newinfo_counters_untrans: -IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, cleanup_entry,NULL ); +IPT_ENTRY_ITERATE (newinfo->entry0, newinfo->size, cleanup_entry,NULL ); free_newinfo_counters:vfree (counters); free_newinfo: -vfree (newinfo); +free_table_info (newinfo);return ret; }
@@ -1264,7 +1296,7 @@ } i =0 ; - IPT_ENTRY_ITERATE(t->private->entries, + IPT_ENTRY_ITERATE(t->private->entry0, t->private->size, add_counter_to_entry, paddc->counters,
@@ -1454,15 +1486,20 @@ {int ret;struct ipt_table_info *newinfo; -static struct ipt_table_info bootstrap - = {0 ,0 ,0 , {0 }, {0 }, { } }; +static struct ipt_table_info bootstrap = { + .size =0 , + .number =0 , + .initial_entries =0 , + .hook_entry = {0 }, + .underflow = {0 }, + .entry0 =NULL , + .entries = {NULL } + }; - newinfo =vmalloc (sizeof (struct ipt_table_info ) - +SMP_ALIGN (repl->size) *num_possible_cpus ()); + newinfo =alloc_table_info (repl->size);if (!newinfo)return -ENOMEM ; - -memcpy (newinfo->entries, repl->entries, repl->size); +memcpy (newinfo->entry0, repl->entries, repl->size); ret = translate_table(table->name, table->valid_hooks, newinfo, repl->size,
@@ -1470,13 +1507,13 @@ repl->hook_entry, repl->underflow);if (ret !=0 ) { -vfree (newinfo); +free_table_info (newinfo);return ret; } ret =down_interruptible (&ipt_mutex);if (ret !=0 ) { -vfree (newinfo); +free_table_info (newinfo);return ret; }
@@ -1505,7 +1542,7 @@return ret; free_unlock: -vfree (newinfo); +free_table_info (newinfo); goto unlock; }
@@ -1516,9 +1553,9 @@up (&ipt_mutex);/* Decrease module usage counts and free resources */ -IPT_ENTRY_ITERATE (table->private->entries, table->private->size, +IPT_ENTRY_ITERATE (table->private->entry0, table->private->size, cleanup_entry,NULL ); -vfree (table->private); +free_table_info (table->private); }/* Returns 1 if the port is matched by the range, 0 otherwise */
Quoted 6 lines · Eric dumazet
Hi
Part of the performance problem we have with netfilter is memory allocation is not NUMA aware, but 'only' SMP aware.
What do you think of this patch ?
...
Quoted 5 lines
net/ipv4/netfilter/ip_tables.c +5
-0+#ifdef CONFIG_NUMA +set_cpus_allowed (current, oldmask); +mpol_free (current->mempolicy); + current->mempolicy = oldpol; +#endif
net/ipv4/netfilter/ip_tables.c line 1142
Quoted 10 lines
net/ipv4/netfilter/ip_tables.c +10
-0+if (newinfo->entries[cpu] ==0 ) { +free_table_info (newinfo); +return NULL ; +if (!newinfo->entry0) + newinfo->entry0 = newinfo->entries[cpu]; + }// ERROR + } +return newinfo; +} +
Oops sorry for this misplaced brace. It should be :
Quoted 15 lines
net/ipv4/netfilter/ip_tables.c +15
-0+#ifdef CONFIG_NUMA +set_cpus_allowed (current, oldmask); +mpol_free (current->mempolicy); + current->mempolicy = oldpol; +#endif +if (newinfo->entries[cpu] ==0 ) { +free_table_info (newinfo); +return NULL ; + }// HERE +if (!newinfo->entry0) + newinfo->entry0 = newinfo->entries[cpu]; + } +return newinfo; +} +
Quoted 4 lines · Eric dumazet
Hi
Part of the performance problem we have with netfilter is memory allocation is not NUMA aware, but 'only' SMP aware.
How do you know? Did you measure it somehow?
Normally I would expect fire wall state to mostly fit into caches (1MB+) of modern Opterons and other NUMA systems.
Quoted 5 lines
What do you think of this patch ?
Note : The allocation function is quite complex (copied from
drivers/pci/pci-driver.cpci_call_probe())
because current kernel doesnt have a NUMA awarevmalloc()wrapper
Normal vmalloc is NUMA aware and allocates on the local node. Like all memory allocations
Quoted 3 lines
, maybe
a future kernel will export
such a common function ?
BTW if you want to put data onto a specific node you don't need to schedule there - it is enough to set the process mempolicy with sys_set_mempolicy. This is fine from kernel too, as long as you save/restore the old one (current->mempolicy)
-Andi
Quoted 1 line
Quoted 9 lines · Andi Kleen
Quoted 4 lines · Eric dumazet
Hi
Part of the performance problem we have with netfilter is memory allocation is not NUMA aware, but 'only' SMP aware.
How do you know? Did you measure it somehow?
Well, it seems that even a 1MB cache is not enough for this workload, on a dual Opterons 275 machine.
CPU: AMD64 processors, speed 2205.54 MHz (estimated)
Counted DATA_CACHE_MISSES events (Data cache misses) with a unit mask of 0x00 (No unit mask) count 5000
samples % symbol name 86104 13.3027 ipt_do_table 21508 3.3229 copy_user_generic_c 18618 2.8764 try_to_wake_up 18180 2.8087 memset 18085 2.7941 tcp_v4_rcv 16949 2.6185 schedule
# iptables -nvL | wc -l
190Quoted 6 lines
Normally I would expect fire wall state to mostly fit into caches (1MB+) of modern Opterons and other NUMA systems.
Quoted 2 lines
What do you think of this patch ?
Quoted 7 lines
Quoted 3 lines
Note : The allocation function is quite complex (copied from
drivers/pci/pci-driver.cpci_call_probe())
because current kernel doesnt have a NUMA awarevmalloc()wrapperNormal vmalloc is NUMA aware and allocates on the local node. Like all memory allocations
Yes, but I want to allocate percpu (and per node) data, using vmalloc(), not memory on the local node.
Quoted 10 lines
Quoted 4 lines
, maybe a future kernel will export
such a common function ?BTW if you want to put data onto a specific node you don't need to schedule there - it is enough to set the process mempolicy with
sys_set_mempolicy. This is fine from kernel too, as long as you save/restore the old one (current->mempolicy)
OK thanks, I will try this.
Eric
Hi all
Part of the performance problem we have with netfilter is memory allocation is not NUMA aware, but 'only' SMP aware (ie each CPU normally touch separate cache lines)
Even with small iptables rules, the cost of this misplacement can be high on common workloads.
Instead of using one vmalloc() area (located in the node of the iptables process), we now vmalloc() an area for each possible CPU, using NUMA policy (MPOL_PREFERRED) so that memory should be allocated in the CPU's node if possible.
If the size of ipt_table is small enough (less than one page), we use kmalloc_node() instead of vmalloc(), to use less memory (and less TLB entries) in small setups.
This patch try to use local node memory in expensive translate_table() function (and others), but doesnt bother to bind the task to the current CPU.
Note : I also optimize get_counters(), using a SET_COUNTER() for the first cpu, avoiding a memset() and ADD_COUNTER() if SMP on other cpus.
Thank you
net/ipv4/netfilter/ip_tables.c +157 -67
@@ -17,6 +17,7 @@#include <linux/skbuff.h> #include <linux/kmod.h> #include <linux/vmalloc.h> +#include <linux/mempolicy.h> #include <linux/netdevice.h> #include <linux/module.h> #include <linux/tcp.h>
@@ -82,11 +83,6 @@context stops packets coming through and allows user context to read the counters or update the rules. -To be cache friendly on SMP, we arrange them like so: -[ n-entries ] -... cache-align padding ... -[ n-entries ] -Hence the start of any table is given by get_table() below. */ /* The table itself */
@@ -104,19 +100,14 @@unsigned int underflow[NF_IP_NUMHOOKS ];/* ipt_entry tables: one per CPU */ -char entries[0 ]____cacheline_aligned ; +void *entries[NR_CPUS ]; };static LIST_HEAD (ipt_target);static LIST_HEAD (ipt_match);static LIST_HEAD (ipt_tables);#define ADD_COUNTER (c,b,p) do { (c).bcnt += (b); (c).pcnt += (p); } while(0) - -#ifdef CONFIG_SMP -#define TABLE_OFFSET (t,p) (SMP_ALIGN((t)->size)*(p)) -#else -#define TABLE_OFFSET (t,p) 0 -#endif +#define SET_COUNTER (c,b,p) do { (c).bcnt = (b); (c).pcnt = (p); } while(0)#if 0 #define down (x) do { printk("DOWN:%u:" #x "\n", __LINE__); down(x); } while(0)
@@ -289,8 +280,7 @@read_lock_bh (&table->lock);IP_NF_ASSERT (table->valid_hooks & (1 << hook)); - table_base = (void *)table->private->entries - +TABLE_OFFSET (table->private,smp_processor_id ()); + table_base = (void *)table->private->entries[smp_processor_id ()]; e =get_entry (table_base, table->private->hook_entry[hook]);#ifdef CONFIG_NETFILTER_DEBUG
@@ -562,7 +552,7 @@/* Figures out from what hook each rule can be called: returns 0 if there are loops. Puts hook bitmask in comefrom. */ static int -mark_source_chains (struct ipt_table_info *newinfo,unsigned int valid_hooks) +mark_source_chains (struct ipt_table_info *newinfo,unsigned int valid_hooks,void *entry0) {unsigned int hook;
@@ -571,7 +561,7 @@for (hook =0 ; hook <NF_IP_NUMHOOKS ; hook++) {unsigned int pos = newinfo->hook_entry[hook];struct ipt_entry *e - = (struct ipt_entry *)(newinfo->entries + pos); + = (struct ipt_entry *)(entry0 + pos);if (!(valid_hooks & (1 << hook)))continue ;
@@ -621,13 +611,13 @@ goto next; e = (struct ipt_entry *) - (newinfo->entries + pos); + (entry0 + pos); }while (oldpos == pos + e->next_offset);/* Move along one */ size = e->next_offset; e = (struct ipt_entry *) - (newinfo->entries + pos + size); + (entry0 + pos + size); e->counters.pcnt = pos; pos += size; }else {
@@ -644,7 +634,7 @@ newpos = pos + e->next_offset; } e = (struct ipt_entry *) - (newinfo->entries + newpos); + (entry0 + newpos); e->counters.pcnt = pos; pos = newpos; }
@@ -854,6 +844,7 @@ translate_table(const char *name, unsigned int valid_hooks, struct ipt_table_info *newinfo, + void *entry0, unsigned int size, unsigned int number, const unsigned int *hook_entries,
@@ -874,11 +865,11 @@duprintf ("translate_table: size %u\n" , newinfo->size); i =0 ;/* Walk through entries, checking offsets. */ - ret =IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, + ret =IPT_ENTRY_ITERATE (entry0, newinfo->size, check_entry_size_and_hooks, newinfo, - newinfo->entries, - newinfo->entries + size, + entry0, + entry0 + size, hook_entries, underflows, &i);if (ret !=0 )return ret;
@@ -906,25 +897,24 @@ } } -if (!mark_source_chains (newinfo, valid_hooks)) +if (!mark_source_chains (newinfo, valid_hooks, entry0))return -ELOOP ;/* Finally, each sanity check must pass */ i =0 ; - ret =IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, + ret =IPT_ENTRY_ITERATE (entry0, newinfo->size, check_entry, name, size, &i);if (ret !=0 ) { -IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, +IPT_ENTRY_ITERATE (entry0, newinfo->size, cleanup_entry, &i);return ret; }/* And one copy for every other CPU */ -for (i =1 ; i <num_possible_cpus (); i++) { -memcpy (newinfo->entries +SMP_ALIGN (newinfo->size)*i, - newinfo->entries, -SMP_ALIGN (newinfo->size)); +for_each_cpu (i) { +if (newinfo->entries[i] && newinfo->entries[i] != entry0) +memcpy (newinfo->entries[i], entry0, newinfo->size); }return ret;
@@ -940,15 +930,12 @@#ifdef CONFIG_NETFILTER_DEBUG { -struct ipt_entry *table_base; -unsigned int i; +int cpu; -for (i =0 ; i <num_possible_cpus (); i++) { - table_base = - (void *)newinfo->entries - +TABLE_OFFSET (newinfo, i); - - table_base->comefrom =0xdead57ac ; +for_each_cpu (cpu) { +struct ipt_entry *table_base = newinfo->entries[cpu]; +if (table_base) + table_base->comefrom =0xdead57ac ; } }#endif
@@ -972,6 +959,7 @@ }/* Gets counters. */ +#ifdef CONFIG_SMP static inline int add_entry_to_counter(const struct ipt_entry *e, struct ipt_counters total[],
@@ -982,22 +970,44 @@ (*i)++;return 0 ; } +#endif +static inline int +set_entry_to_counter (const struct ipt_entry *e, +struct ipt_counters total[], +unsigned int *i) +{ +SET_COUNTER (total[*i], e->counters.bcnt, e->counters.pcnt); + + (*i)++; +return 0 ; +}static void get_counters (const struct ipt_table_info *t,struct ipt_counters counters[]) {unsigned int cpu; +unsigned int curcpu =raw_smp_processor_id ();unsigned int i; -for (cpu =0 ; cpu <num_possible_cpus (); cpu++) { + i =0 ; +IPT_ENTRY_ITERATE (t->entries[curcpu], + t->size, + set_entry_to_counter, + counters, + &i); +#ifdef CONFIG_SMP +for_each_cpu (cpu) { +if (cpu == curcpu) +continue ; i =0 ; -IPT_ENTRY_ITERATE (t->entries +TABLE_OFFSET (t, cpu), +IPT_ENTRY_ITERATE (t->entries[cpu], t->size, add_entry_to_counter, counters, &i); } +#endif } static int
@@ -1009,6 +1019,7 @@struct ipt_entry *e;struct ipt_counters *counters;int ret =0 ; +void *loc_cpu_entry;/* We need atomic snapshot of counters: rest doesn't change (other than comefrom, which userspace doesn't care
@@ -1020,13 +1031,17 @@return -ENOMEM ;/* First, sum counters... */ -memset (counters,0 , countersize);write_lock_bh (&table->lock);get_counters (table->private, counters);write_unlock_bh (&table->lock); -/* ... then copy entire thing from CPU 0... */ -if (copy_to_user (userptr, table->private->entries, total_size) !=0 ) { +/* +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()]; +/* ... then copy entire thing ... */ +if (copy_to_user (userptr, loc_cpu_entry, total_size) !=0 ) { ret = -EFAULT ; goto free_counters; }
@@ -1038,7 +1053,7 @@struct ipt_entry_match *m;struct ipt_entry_target *t; - e = (struct ipt_entry *)(table->private->entries + off); + e = (struct ipt_entry *)(loc_cpu_entry + off); if (copy_to_user(userptr + off + offsetof(struct ipt_entry, counters), &counters[num],
@@ -1107,6 +1122,56 @@return ret; } +static void free_table_info (struct ipt_table_info *info) +{ +int cpu; +for_each_cpu (cpu) { +if (info->size <=PAGE_SIZE ) +kfree (info->entries[cpu]); +else +vfree (info->entries[cpu]); + } +kfree (info); +} + +static struct ipt_table_info *alloc_table_info (unsigned int size) +{ +struct ipt_table_info *newinfo; +int cpu; + newinfo =kzalloc (sizeof (struct ipt_table_info ),GFP_KERNEL ); +if (!newinfo) +return NULL ; + newinfo->size = size; +for_each_cpu (cpu) { +if (size <=PAGE_SIZE ) { + newinfo->entries[cpu] =kmalloc_node (size, +GFP_KERNEL , +cpu_to_node (cpu)); + }else { +#ifdef CONFIG_NUMA +struct mempolicy *oldpol; +struct mempolicy prefnodepolicy = { + .refcnt =ATOMIC_INIT (1 ), + .policy =MPOL_PREFERRED , + .v.preferred_node =cpu_to_node (cpu), + }; + oldpol = current->mempolicy; + current->mempolicy = &prefnodepolicy; +#endif + newinfo->entries[cpu] =vmalloc (size); +#ifdef CONFIG_NUMA + current->mempolicy = oldpol; +#endif + } +if (newinfo->entries[cpu] ==0 ) { +free_table_info (newinfo); +return NULL ; + } + } +return newinfo; +} + +static int do_replace (void __user *user,unsigned int len) {
@@ -1115,6 +1180,7 @@struct ipt_table *t;struct ipt_table_info *newinfo, *oldinfo;struct ipt_counters *counters; +void *loc_cpu_entry, *loc_cpu_old_entry;if (copy_from_user (&tmp, user,sizeof (tmp)) !=0 )return -EFAULT ;
@@ -1127,12 +1193,15 @@if ((SMP_ALIGN (tmp.size) >>PAGE_SHIFT ) +2 > num_physpages)return -ENOMEM ; - newinfo =vmalloc (sizeof (struct ipt_table_info ) - +SMP_ALIGN (tmp.size) *num_possible_cpus ()); + newinfo =alloc_table_info (tmp.size);if (!newinfo)return -ENOMEM ; - -if (copy_from_user (newinfo->entries, user +sizeof (tmp), +/* +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()]; +if (copy_from_user (loc_cpu_entry, user +sizeof (tmp), tmp.size) !=0 ) { ret = -EFAULT ; goto free_newinfo;
@@ -1143,10 +1212,9 @@ ret = -ENOMEM ; goto free_newinfo; } -memset (counters,0 , tmp.num_counters *sizeof (struct ipt_counters )); ret =translate_table (tmp.name, tmp.valid_hooks, - newinfo, tmp.size, tmp.num_entries, + newinfo, loc_cpu_entry, tmp.size, tmp.num_entries, tmp.hook_entry, tmp.underflow);if (ret !=0 ) goto free_newinfo_counters;
@@ -1185,8 +1253,9 @@/* Get the old counters. */ get_counters (oldinfo, counters);/* Decrease module usage counts and free resource */ -IPT_ENTRY_ITERATE (oldinfo->entries, oldinfo->size, cleanup_entry,NULL ); -vfree (oldinfo); + loc_cpu_old_entry = oldinfo->entries[raw_smp_processor_id ()]; +IPT_ENTRY_ITERATE (loc_cpu_old_entry, oldinfo->size, cleanup_entry,NULL ); +free_table_info (oldinfo);if (copy_to_user (tmp.counters, counters,sizeof (struct ipt_counters ) * tmp.num_counters) !=0 ) ret = -EFAULT ;
@@ -1198,11 +1267,11 @@module_put (t->me);up (&ipt_mutex); free_newinfo_counters_untrans: -IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, cleanup_entry,NULL ); +IPT_ENTRY_ITERATE (loc_cpu_entry, newinfo->size, cleanup_entry,NULL ); free_newinfo_counters:vfree (counters); free_newinfo: -vfree (newinfo); +free_table_info (newinfo);return ret; }
@@ -1235,6 +1304,7 @@struct ipt_counters_info tmp, *paddc;struct ipt_table *t;int ret =0 ; +void *loc_cpu_entry;if (copy_from_user (&tmp, user,sizeof (tmp)) !=0 )return -EFAULT ;
@@ -1264,7 +1334,12 @@ } i =0 ; - IPT_ENTRY_ITERATE(t->private->entries, +/* +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = t->private->entries[raw_smp_processor_id ()]; + IPT_ENTRY_ITERATE(loc_cpu_entry, t->private->size, add_counter_to_entry, paddc->counters,
@@ -1454,29 +1529,39 @@ {int ret;struct ipt_table_info *newinfo; -static struct ipt_table_info bootstrap - = {0 ,0 ,0 , {0 }, {0 }, { } }; +static struct ipt_table_info bootstrap = { + .size =0 , + .number =0 , + .initial_entries =0 , + .hook_entry = {0 }, + .underflow = {0 }, + .entries = {NULL } + }; +void *loc_cpu_entry; - newinfo =vmalloc (sizeof (struct ipt_table_info ) - +SMP_ALIGN (repl->size) *num_possible_cpus ()); + newinfo =alloc_table_info (repl->size);if (!newinfo)return -ENOMEM ; - -memcpy (newinfo->entries, repl->entries, repl->size); +/* +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()]; +memcpy (loc_cpu_entry, repl->entries, repl->size); ret =translate_table (table->name, table->valid_hooks, - newinfo, repl->size, + newinfo, loc_cpu_entry, repl->size, repl->num_entries, repl->hook_entry, repl->underflow);if (ret !=0 ) { -vfree (newinfo); +free_table_info (newinfo);return ret; } ret =down_interruptible (&ipt_mutex);if (ret !=0 ) { -vfree (newinfo); +free_table_info (newinfo);return ret; }
@@ -1505,20 +1590,25 @@return ret; free_unlock: -vfree (newinfo); +free_table_info (newinfo); goto unlock; }void ipt_unregister_table (struct ipt_table *table) { +void *loc_cpu_entry;down (&ipt_mutex);LIST_DELETE (&ipt_tables, table);up (&ipt_mutex); -/* Decrease module usage counts and free resources */ -IPT_ENTRY_ITERATE (table->private->entries, table->private->size, +/* Decrease module usage counts and free resources +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()]; +IPT_ENTRY_ITERATE (loc_cpu_entry, table->private->size, cleanup_entry,NULL ); -vfree (table->private); +free_table_info (table->private); }/* Returns 1 if the port is matched by the range, 0 otherwise */
net/ipv4/netfilter/ip_tables.c line 1160
Quoted 10 lines · Eric Dumazet
net/ipv4/netfilter/ip_tables.c +10
-0+#ifdef CONFIG_NUMA + struct mempolicy *oldpol; + struct mempolicy prefnodepolicy = { + .refcnt = ATOMIC_INIT(1), + .policy = MPOL_PREFERRED, + .v.preferred_node = cpu_to_node(cpu), + }; + oldpol = current->mempolicy; + current->mempolicy = &prefnodepolicy; +#endif
I would prefer if random code didn't mess with mempolicy internals like this. Better just call sys_set_mempolicy()
-Andi
Quoted 17 lines · Andi Kleen
Review
Quoted 10 lines · Eric Dumazet
(quoted diff) +10
-0+#ifdef CONFIG_NUMA + struct mempolicy *oldpol; + struct mempolicy prefnodepolicy = { + .refcnt = ATOMIC_INIT(1), + .policy = MPOL_PREFERRED, + .v.preferred_node = cpu_to_node(cpu), + }; + oldpol = current->mempolicy; + current->mempolicy = &prefnodepolicy; +#endifI would prefer if random code didn't mess with mempolicy internals like this. Better just call
sys_set_mempolicy()
Quoted 3 lines
-Andi
Thank you Andi
Is this new patch OK for you ?
Hi all
Part of the performance problem we have with netfilter is memory allocation is not NUMA aware, but 'only' SMP aware (ie each CPU normally touch separate cache lines)
Even with small iptables rules, the cost of this misplacement can be high on common workloads.
Instead of using one vmalloc() area (located in the node of the iptables process), we now vmalloc() an area for each possible CPU, using NUMA policy (MPOL_PREFERRED) so that memory should be allocated in the CPU's node if possible.
If the size of ipt_table is small enough (less than one page), we use kmalloc_node() instead of vmalloc(), to use less memory (and less TLB entries) in small setups.
This patch try to use local node memory in expensive translate_table() function (and others), but doesnt bother to bind the task to the current CPU.
Note : I also optimize get_counters(), using a SET_COUNTER() for the first cpu, avoiding a memset() and ADD_COUNTER() if SMP on other cpus.
Thank you
net/ipv4/netfilter/ip_tables.c +161 -67
@@ -17,6 +17,7 @@#include <linux/skbuff.h> #include <linux/kmod.h> #include <linux/vmalloc.h> +#include <linux/mempolicy.h> #include <linux/netdevice.h> #include <linux/module.h> #include <linux/tcp.h>
@@ -82,11 +83,6 @@context stops packets coming through and allows user context to read the counters or update the rules. -To be cache friendly on SMP, we arrange them like so: -[ n-entries ] -... cache-align padding ... -[ n-entries ] -Hence the start of any table is given by get_table() below. */ /* The table itself */
@@ -104,19 +100,14 @@unsigned int underflow[NF_IP_NUMHOOKS ];/* ipt_entry tables: one per CPU */ -char entries[0 ]____cacheline_aligned ; +void *entries[NR_CPUS ]; };static LIST_HEAD (ipt_target);static LIST_HEAD (ipt_match);static LIST_HEAD (ipt_tables);#define ADD_COUNTER (c,b,p) do { (c).bcnt += (b); (c).pcnt += (p); } while(0) - -#ifdef CONFIG_SMP -#define TABLE_OFFSET (t,p) (SMP_ALIGN((t)->size)*(p)) -#else -#define TABLE_OFFSET (t,p) 0 -#endif +#define SET_COUNTER (c,b,p) do { (c).bcnt = (b); (c).pcnt = (p); } while(0)#if 0 #define down (x) do { printk("DOWN:%u:" #x "\n", __LINE__); down(x); } while(0)
@@ -289,8 +280,7 @@read_lock_bh (&table->lock);IP_NF_ASSERT (table->valid_hooks & (1 << hook)); - table_base = (void *)table->private->entries - +TABLE_OFFSET (table->private,smp_processor_id ()); + table_base = (void *)table->private->entries[smp_processor_id ()]; e =get_entry (table_base, table->private->hook_entry[hook]);#ifdef CONFIG_NETFILTER_DEBUG
@@ -562,7 +552,7 @@/* Figures out from what hook each rule can be called: returns 0 if there are loops. Puts hook bitmask in comefrom. */ static int -mark_source_chains (struct ipt_table_info *newinfo,unsigned int valid_hooks) +mark_source_chains (struct ipt_table_info *newinfo,unsigned int valid_hooks,void *entry0) {unsigned int hook;
@@ -571,7 +561,7 @@for (hook =0 ; hook <NF_IP_NUMHOOKS ; hook++) {unsigned int pos = newinfo->hook_entry[hook];struct ipt_entry *e - = (struct ipt_entry *)(newinfo->entries + pos); + = (struct ipt_entry *)(entry0 + pos);if (!(valid_hooks & (1 << hook)))continue ;
@@ -621,13 +611,13 @@ goto next; e = (struct ipt_entry *) - (newinfo->entries + pos); + (entry0 + pos); }while (oldpos == pos + e->next_offset);/* Move along one */ size = e->next_offset; e = (struct ipt_entry *) - (newinfo->entries + pos + size); + (entry0 + pos + size); e->counters.pcnt = pos; pos += size; }else {
@@ -644,7 +634,7 @@ newpos = pos + e->next_offset; } e = (struct ipt_entry *) - (newinfo->entries + newpos); + (entry0 + newpos); e->counters.pcnt = pos; pos = newpos; }
@@ -854,6 +844,7 @@ translate_table(const char *name, unsigned int valid_hooks, struct ipt_table_info *newinfo, + void *entry0, unsigned int size, unsigned int number, const unsigned int *hook_entries,
@@ -874,11 +865,11 @@duprintf ("translate_table: size %u\n" , newinfo->size); i =0 ;/* Walk through entries, checking offsets. */ - ret =IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, + ret =IPT_ENTRY_ITERATE (entry0, newinfo->size, check_entry_size_and_hooks, newinfo, - newinfo->entries, - newinfo->entries + size, + entry0, + entry0 + size, hook_entries, underflows, &i);if (ret !=0 )return ret;
@@ -906,25 +897,24 @@ } } -if (!mark_source_chains (newinfo, valid_hooks)) +if (!mark_source_chains (newinfo, valid_hooks, entry0))return -ELOOP ;/* Finally, each sanity check must pass */ i =0 ; - ret =IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, + ret =IPT_ENTRY_ITERATE (entry0, newinfo->size, check_entry, name, size, &i);if (ret !=0 ) { -IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, +IPT_ENTRY_ITERATE (entry0, newinfo->size, cleanup_entry, &i);return ret; }/* And one copy for every other CPU */ -for (i =1 ; i <num_possible_cpus (); i++) { -memcpy (newinfo->entries +SMP_ALIGN (newinfo->size)*i, - newinfo->entries, -SMP_ALIGN (newinfo->size)); +for_each_cpu (i) { +if (newinfo->entries[i] && newinfo->entries[i] != entry0) +memcpy (newinfo->entries[i], entry0, newinfo->size); }return ret;
@@ -940,15 +930,12 @@#ifdef CONFIG_NETFILTER_DEBUG { -struct ipt_entry *table_base; -unsigned int i; +int cpu; -for (i =0 ; i <num_possible_cpus (); i++) { - table_base = - (void *)newinfo->entries - +TABLE_OFFSET (newinfo, i); - - table_base->comefrom =0xdead57ac ; +for_each_cpu (cpu) { +struct ipt_entry *table_base = newinfo->entries[cpu]; +if (table_base) + table_base->comefrom =0xdead57ac ; } }#endif
@@ -972,6 +959,7 @@ }/* Gets counters. */ +#ifdef CONFIG_SMP static inline int add_entry_to_counter(const struct ipt_entry *e, struct ipt_counters total[],
@@ -982,22 +970,44 @@ (*i)++;return 0 ; } +#endif +static inline int +set_entry_to_counter (const struct ipt_entry *e, +struct ipt_counters total[], +unsigned int *i) +{ +SET_COUNTER (total[*i], e->counters.bcnt, e->counters.pcnt); + + (*i)++; +return 0 ; +}static void get_counters (const struct ipt_table_info *t,struct ipt_counters counters[]) {unsigned int cpu; +unsigned int curcpu =raw_smp_processor_id ();unsigned int i; -for (cpu =0 ; cpu <num_possible_cpus (); cpu++) { + i =0 ; +IPT_ENTRY_ITERATE (t->entries[curcpu], + t->size, + set_entry_to_counter, + counters, + &i); +#ifdef CONFIG_SMP +for_each_cpu (cpu) { +if (cpu == curcpu) +continue ; i =0 ; -IPT_ENTRY_ITERATE (t->entries +TABLE_OFFSET (t, cpu), +IPT_ENTRY_ITERATE (t->entries[cpu], t->size, add_entry_to_counter, counters, &i); } +#endif } static int
@@ -1009,6 +1019,7 @@struct ipt_entry *e;struct ipt_counters *counters;int ret =0 ; +void *loc_cpu_entry;/* We need atomic snapshot of counters: rest doesn't change (other than comefrom, which userspace doesn't care
@@ -1020,13 +1031,17 @@return -ENOMEM ;/* First, sum counters... */ -memset (counters,0 , countersize);write_lock_bh (&table->lock);get_counters (table->private, counters);write_unlock_bh (&table->lock); -/* ... then copy entire thing from CPU 0... */ -if (copy_to_user (userptr, table->private->entries, total_size) !=0 ) { +/* +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()]; +/* ... then copy entire thing ... */ +if (copy_to_user (userptr, loc_cpu_entry, total_size) !=0 ) { ret = -EFAULT ; goto free_counters; }
@@ -1038,7 +1053,7 @@struct ipt_entry_match *m;struct ipt_entry_target *t; - e = (struct ipt_entry *)(table->private->entries + off); + e = (struct ipt_entry *)(loc_cpu_entry + off); if (copy_to_user(userptr + off + offsetof(struct ipt_entry, counters), &counters[num],
@@ -1107,6 +1122,60 @@return ret; } +static void free_table_info (struct ipt_table_info *info) +{ +int cpu; +for_each_cpu (cpu) { +if (info->size <=PAGE_SIZE ) +kfree (info->entries[cpu]); +else +vfree (info->entries[cpu]); + } +kfree (info); +} + +static struct ipt_table_info *alloc_table_info (unsigned int size) +{ +struct ipt_table_info *newinfo; +int cpu; + newinfo =kzalloc (sizeof (struct ipt_table_info ),GFP_KERNEL ); +if (!newinfo) +return NULL ; + newinfo->size = size; +for_each_cpu (cpu) { +if (size <=PAGE_SIZE ) { + newinfo->entries[cpu] =kmalloc_node (size, +GFP_KERNEL , +cpu_to_node (cpu)); + }else { +#ifdef CONFIG_NUMA +struct mempolicy *oldpol; +mm_segment_t oldfs =get_fs (); +DECLARE_BITMAP (mynode,MAX_NUMNODES ); + + oldpol = current->mempolicy; +get_pol (oldpol); +bitmap_zero (mynode,MAX_NUMNODES ); +set_bit (cpu_to_node (cpu), mynode); +set_fs (KERNEL_DS ); +sys_set_mempolicy (MPOL_PREFERRED , &mynode,MAX_NUMNODES ); +set_fs (oldfs); +#endif + newinfo->entries[cpu] =vmalloc (size); +#ifdef CONFIG_NUMA +mpol_free (current->mempolicy); + current->mempolicy = oldpol; +#endif + } +if (newinfo->entries[cpu] ==0 ) { +free_table_info (newinfo); +return NULL ; + } + } +return newinfo; +} + +static int do_replace (void __user *user,unsigned int len) {
@@ -1115,6 +1184,7 @@struct ipt_table *t;struct ipt_table_info *newinfo, *oldinfo;struct ipt_counters *counters; +void *loc_cpu_entry, *loc_cpu_old_entry;if (copy_from_user (&tmp, user,sizeof (tmp)) !=0 )return -EFAULT ;
@@ -1127,12 +1197,15 @@if ((SMP_ALIGN (tmp.size) >>PAGE_SHIFT ) +2 > num_physpages)return -ENOMEM ; - newinfo =vmalloc (sizeof (struct ipt_table_info ) - +SMP_ALIGN (tmp.size) *num_possible_cpus ()); + newinfo =alloc_table_info (tmp.size);if (!newinfo)return -ENOMEM ; - -if (copy_from_user (newinfo->entries, user +sizeof (tmp), +/* +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()]; +if (copy_from_user (loc_cpu_entry, user +sizeof (tmp), tmp.size) !=0 ) { ret = -EFAULT ; goto free_newinfo;
@@ -1143,10 +1216,9 @@ ret = -ENOMEM ; goto free_newinfo; } -memset (counters,0 , tmp.num_counters *sizeof (struct ipt_counters )); ret =translate_table (tmp.name, tmp.valid_hooks, - newinfo, tmp.size, tmp.num_entries, + newinfo, loc_cpu_entry, tmp.size, tmp.num_entries, tmp.hook_entry, tmp.underflow);if (ret !=0 ) goto free_newinfo_counters;
@@ -1185,8 +1257,9 @@/* Get the old counters. */ get_counters (oldinfo, counters);/* Decrease module usage counts and free resource */ -IPT_ENTRY_ITERATE (oldinfo->entries, oldinfo->size, cleanup_entry,NULL ); -vfree (oldinfo); + loc_cpu_old_entry = oldinfo->entries[raw_smp_processor_id ()]; +IPT_ENTRY_ITERATE (loc_cpu_old_entry, oldinfo->size, cleanup_entry,NULL ); +free_table_info (oldinfo);if (copy_to_user (tmp.counters, counters,sizeof (struct ipt_counters ) * tmp.num_counters) !=0 ) ret = -EFAULT ;
@@ -1198,11 +1271,11 @@module_put (t->me);up (&ipt_mutex); free_newinfo_counters_untrans: -IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, cleanup_entry,NULL ); +IPT_ENTRY_ITERATE (loc_cpu_entry, newinfo->size, cleanup_entry,NULL ); free_newinfo_counters:vfree (counters); free_newinfo: -vfree (newinfo); +free_table_info (newinfo);return ret; }
@@ -1235,6 +1308,7 @@struct ipt_counters_info tmp, *paddc;struct ipt_table *t;int ret =0 ; +void *loc_cpu_entry;if (copy_from_user (&tmp, user,sizeof (tmp)) !=0 )return -EFAULT ;
@@ -1264,7 +1338,12 @@ } i =0 ; - IPT_ENTRY_ITERATE(t->private->entries, +/* +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = t->private->entries[raw_smp_processor_id ()]; + IPT_ENTRY_ITERATE(loc_cpu_entry, t->private->size, add_counter_to_entry, paddc->counters,
@@ -1454,29 +1533,39 @@ {int ret;struct ipt_table_info *newinfo; -static struct ipt_table_info bootstrap - = {0 ,0 ,0 , {0 }, {0 }, { } }; +static struct ipt_table_info bootstrap = { + .size =0 , + .number =0 , + .initial_entries =0 , + .hook_entry = {0 }, + .underflow = {0 }, + .entries = {NULL } + }; +void *loc_cpu_entry; - newinfo =vmalloc (sizeof (struct ipt_table_info ) - +SMP_ALIGN (repl->size) *num_possible_cpus ()); + newinfo =alloc_table_info (repl->size);if (!newinfo)return -ENOMEM ; - -memcpy (newinfo->entries, repl->entries, repl->size); +/* +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()]; +memcpy (loc_cpu_entry, repl->entries, repl->size); ret =translate_table (table->name, table->valid_hooks, - newinfo, repl->size, + newinfo, loc_cpu_entry, repl->size, repl->num_entries, repl->hook_entry, repl->underflow);if (ret !=0 ) { -vfree (newinfo); +free_table_info (newinfo);return ret; } ret =down_interruptible (&ipt_mutex);if (ret !=0 ) { -vfree (newinfo); +free_table_info (newinfo);return ret; }
@@ -1505,20 +1594,25 @@return ret; free_unlock: -vfree (newinfo); +free_table_info (newinfo); goto unlock; }void ipt_unregister_table (struct ipt_table *table) { +void *loc_cpu_entry;down (&ipt_mutex);LIST_DELETE (&ipt_tables, table);up (&ipt_mutex); -/* Decrease module usage counts and free resources */ -IPT_ENTRY_ITERATE (table->private->entries, table->private->size, +/* Decrease module usage counts and free resources +* choose the copy that is on our node, +* but dont worry if the scheduler changes the CPU +*/ + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()]; +IPT_ENTRY_ITERATE (loc_cpu_entry, table->private->size, cleanup_entry,NULL ); -vfree (table->private); +free_table_info (table->private); }/* Returns 1 if the port is matched by the range, 0 otherwise */
PATCH 1/1 Adds sys_set_mempolicy() in include/linux/syscalls.h , Re: [PATCH, netfilter] NUMA aware ipv4/netfilter/ip_tables.c
In reply to Andi KleenQuoted 10 lines · Andi Kleen
On Tuesday 20 September 2005 11:47, Eric Dumazet wrote:
I would prefer if random code didn't mess with mempolicy internals like this. Better just call
sys_set_mempolicy()-Andi
OK but this prior patch seems necessary :
- Adds sys_set_mempolicy() in include/linux/syscalls.h
PATCH 1/1 Adds sys_set_mempolicy() in include/linux/syscalls.h
Quoted 9 lines · Andi Kleen
On Tuesday 20 September 2005 11:47, Eric Dumazet wrote:
I would prefer if random code didn't mess with mempolicy internals like this. Better just call
sys_set_mempolicy()
-Andi
OK but this prior patch seems necessary :
- Adds sys_set_mempolicy() in include/linux/syscalls.h
include/linux/syscalls.h +2 -0
@@ -508,5 +508,7 @@asmlinkage long sys_ioprio_set (int which,int who,int ioprio);asmlinkage long sys_ioprio_get (int which,int who); +asmlinkage long sys_set_mempolicy (int mode,unsigned long __user *nmask, +unsigned long maxnode);#endif
cover netfilter : 3 patches to boost ip_tables performance
Hi
I have reworked net/ipv4/ip_tables.c to boost its performance, and post three patches.
In oprofile results, ipt_do_table() was at the first position. It is now at 6th position, using 1/3 of the CPU it was using before. (Tests done on a dual Xeon i386 and a dual Opteron x86_64)
Short description :
1) No more central rwlock protecting each table (filter, nat, mangle, raw), but one lock per CPU. It avoids cache line ping pongs for each packet.
2) Loop unrollings and various code optimizations to reduce branches mispredictions.
3) NUMA aware allocation of memory (this part was posted earlier, but got some polishing problems)
Long description:
1) No more one rwlock_t protecting the 'curtain'
One major bottleneck on SMP machines is the fact that one rwlock is taken in ipt_do_table() entry and exit. That 2 atomic operations are the killer, and even if multiple readers can work together on the same table (using read_lock_bh()), the cache line containing rwlock still must be taken exclusively by each CPU at entry/exit of ipt_do_table().
As existing code already use separate copies of the data for each cpu, it is very easy to convert the central rwlock to separate rwlocks, allocated percpu (and NUMA aware).
When a cpu enters ipt_do_table(), it can locks its local copy, touching a cache line that is not used by other cpus. Further operations are done on 'local' copy of the data.
When a sum of all counters must be done, we can write_lock each part at a time, instead of locking all the parts, reducing the lock contention.
2) Loop unrolling
It seems that with current compilers and CFLAGS, the code fromip_packet_match() is very bad, using lot of mispredicted conditional branches I made some patches and generated code on i386 and x86_64 is much better.
3) NUMA allocation.
Part of the performance problem we have with netfilter is memory allocation is not NUMA aware, but 'only' SMP aware (ie each CPU normally touch separate cache lines)
Even with small iptables rules, the cost of this misplacement can be high
on common workloads.
Instead of using one vmalloc() area (located in the node of the iptables process), we now allocate an area for each possible CPU, using NUMA policy (MPOL_PREFERRED) so that memory should be allocated in the CPU's node if possible.
If the size of ipt_table is small enough (less than one page), we use kmalloc_node() instead of vmalloc(), to use less memory and less TLB entries) in small setups.
Note1 : I also optimized get_counters(), using a SET_COUNTER() for the first cpu, avoiding a memset() and ADD_COUNTER() if SMP on other cpus.
Note2 : This patch depends on another patch that declares sys_set_mempolicy() in include/linux/syscalls.h
( http://marc.theaimsgroup.com/?l=linux-kernel&m=112725288622984&w=2 )
Thank you
Eric Dumazet
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 8 lines · Eric Dumazet
Instead of using one
vmalloc()area (located in the node of the iptables process), we now allocate an area for each possible CPU, using NUMA policy (MPOL_PREFERRED) so that memory should be allocated in the CPU's node if possible.If the size of
ipt_tableis small enough (less than one page), we usekmalloc_node()instead ofvmalloc(), to use less memory and less TLB entries) in small setups.
Maybe we better introduce vmalloc_node() instead of improvising this for several subsystems? The e1000 driver has similar issues.
David S. Miller
maintainerRe: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
From: Christoph Lameter <[email protected]>
Date: Wed, 21 Sep 2005 15:43:29 -0700 (PDT)
Quoted 2 lines
Maybe we better introduce
vmalloc_node()instead of improvising this for several subsystems? The e1000 driver has similar issues.
I agree.
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 7 lines · David S. Miller
From: Christoph Lameter <[email protected]>
Date: Wed, 21 Sep 2005 15:43:29 -0700 (PDT)Quoted 2 lines
Maybe we better introduce
vmalloc_node()instead of improvising this for several subsystems? The e1000 driver has similar issues.I agree.
I did an implementation in June.
See http://marc.theaimsgroup.com/?l=linux-mm&m=111766643127530&w=2
Not sure if this will fit the bill. Never really tested it.
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 9 lines · Christoph Lameter
Quoted 6 lines · David S. Miller
From: Christoph Lameter <[email protected]>
Date: Wed, 21 Sep 2005 15:43:29 -0700 (PDT)Quoted 1 line
Maybe we better introduce
vmalloc_node()instead of improvising this for several subsystems? The e1000 driver has similar issues.
Quoted 8 lines
Quoted 1 line
I agree.
I did an implementation in June.
See http://marc.theaimsgroup.com/?l=linux-mm&m=111766643127530&w=2
Not sure if this will fit the bill. Never really tested it.
Maybe this simpler patch has more chances to be accepted ?
Thank you
[NUMA]
- Adds a vmalloc_node() function : A simple wrapper around vmalloc() to allocate memory from a preferred node.
This NUMA aware variant will be used by ip_tables and various network drivers.
include/linux/vmalloc.h +9 -0
@@ -1,6 +1,7 @@#ifndef _LINUX_VMALLOC_H#define _LINUX_VMALLOC_H +#include <linux/config.h> /* vmalloc_node() needs CONFIG_ options */ #include <linux/spinlock.h> #include <asm/page.h> /* pgprot_t */
@@ -32,6 +33,14 @@* Highlevel APIs for driver use */ extern void *vmalloc (unsigned long size); +#ifdef CONFIG_NUMA +extern void *vmalloc_node (unsigned long size,int node); +#else +static inline void *vmalloc_node (unsigned long size,int node) +{ +return vmalloc (size); +} +#endif extern void *vmalloc_exec (unsigned long size);extern void *vmalloc_32 (unsigned long size);extern void *__vmalloc (unsigned long size,unsigned int __nocast gfp_mask,pgprot_t prot);
mm/vmalloc.c +38 -1
@@ -19,6 +19,9 @@#include <asm/uaccess.h> #include <asm/tlbflush.h> +#ifdef CONFIG_NUMA +#include <linux/mempolicy.h> +#endif DEFINE_RWLOCK (vmlist_lock);struct vm_struct *vmlist;
@@ -471,7 +474,7 @@* Allocate enough pages to cover @size from the page level * allocator and map them into contiguous kernel virtual space. * -* For tight cotrol over page level allocator and protection flags +* For tight control over page level allocator and protection flags * use __vmalloc() instead. */ void *vmalloc (unsigned long size)
@@ -481,6 +484,40 @@EXPORT_SYMBOL (vmalloc); +#ifdef CONFIG_NUMA +/** +* vmalloc_node - allocate virtually contiguous memory +* +* @size: allocation size +* @node: preferred node +* +* This vmalloc variant try to allocate memory from a preferred node. +*/ +void *vmalloc_node (unsigned long size,int node) +{ +void *result; +struct mempolicy *oldpol = current->mempolicy; +mm_segment_t oldfs =get_fs (); +DECLARE_BITMAP (prefnode,MAX_NUMNODES ); + +mpol_get (oldpol); +bitmap_zero (prefnode,MAX_NUMNODES ); +set_bit (node, prefnode); + +set_fs (KERNEL_DS ); +sys_set_mempolicy (MPOL_PREFERRED , prefnode,MAX_NUMNODES ); +set_fs (oldfs); + + result =vmalloc (size); + +mpol_free (current->mempolicy); + current->mempolicy = oldpol; +return result; +} + +EXPORT_SYMBOL (vmalloc_node); +#endif +#ifndef PAGE_KERNEL_EXEC # define PAGE_KERNEL_EXEC PAGE_KERNEL#endif
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 20 lines · Eric Dumazet
mm/vmalloc.c +20
-0+#ifdef CONFIG_NUMA +/** +* vmalloc_node - allocate virtually contiguous memory +* +* @size: allocation size +* @node: preferred node +* +* This vmalloc variant try to allocate memory from a preferred node. +*/ +void *vmalloc_node (unsigned long size,int node) +{ +void *result; +struct mempolicy *oldpol = current->mempolicy; +mm_segment_t oldfs =get_fs (); +DECLARE_BITMAP (prefnode,MAX_NUMNODES ); + +mpol_get (oldpol); +bitmap_zero (prefnode,MAX_NUMNODES ); +set_bit (node, prefnode); +
mm/vmalloc.c line 516
Quoted 10 lines
mm/vmalloc.c +10
-0+set_fs (KERNEL_DS ); +sys_set_mempolicy (MPOL_PREFERRED , prefnode,MAX_NUMNODES ); +set_fs (oldfs); + + result =vmalloc (size); + +mpol_free (current->mempolicy); + current->mempolicy = oldpol; +return result; +}
No way, sorry. If you want a vmalloc node do it right.
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 25 lines · Christoph Hellwig
Quoted 24 lines · Eric Dumazet
mm/vmalloc.c +24
-0+#ifdef CONFIG_NUMA +/** +* vmalloc_node - allocate virtually contiguous memory +* +* @size: allocation size +* @node: preferred node +* +* This vmalloc variant try to allocate memory from a preferred node. +*/ +void *vmalloc_node (unsigned long size,int node) +{ +void *result; +struct mempolicy *oldpol = current->mempolicy; +mm_segment_t oldfs =get_fs (); +DECLARE_BITMAP (prefnode,MAX_NUMNODES ); + +mpol_get (oldpol); +bitmap_zero (prefnode,MAX_NUMNODES ); +set_bit (node, prefnode); + +set_fs (KERNEL_DS ); +sys_set_mempolicy (MPOL_PREFERRED , prefnode,MAX_NUMNODES ); +set_fs (oldfs); +
Quoted 8 lines
mm/vmalloc.c line 516
Quoted 6 lines
mm/vmalloc.c +6
-0+ result =vmalloc (size); + +mpol_free (current->mempolicy); + current->mempolicy = oldpol; +return result; +}No way, sorry. If you want a vmalloc node do it right.
The implementation looks fine to me, so I think it's already right.
-Andi
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 24 lines · Andi Kleen
Quoted 15 lines
Quoted 15 lines
mm/vmalloc.c +15
-0+void *vmalloc_node (unsigned long size,int node) +{ +void *result; +struct mempolicy *oldpol = current->mempolicy; +mm_segment_t oldfs =get_fs (); +DECLARE_BITMAP (prefnode,MAX_NUMNODES ); + +mpol_get (oldpol); +bitmap_zero (prefnode,MAX_NUMNODES ); +set_bit (node, prefnode); + +set_fs (KERNEL_DS ); +sys_set_mempolicy (MPOL_PREFERRED , prefnode,MAX_NUMNODES ); +set_fs (oldfs); +Quoted 8 lines
mm/vmalloc.c line 516
Quoted 6 lines
mm/vmalloc.c +6
-0+ result =vmalloc (size); + +mpol_free (current->mempolicy); + current->mempolicy = oldpol; +return result; +}No way, sorry. If you want a vmalloc node do it right.
Quoted 1 line
The implementation looks fine to me, so I think it's already right.
Umm, no - adding set_fs/get_fs mess for things like that is not right. If we want to go down the mempolicy-based route we need to add a proper kernel entry point for setting a mempolicy
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 1 line · Christoph Hellwig
Umm, no - adding set_fs/get_fs mess for things like that is not right.
I think it's fine. We're using it for various other interfaces too. In fact sys_set_mempolicy is already used elsewhere in the kernel too.
-Andi
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 6 lines · Andi Kleen
Quoted 1 line · Christoph Hellwig
Umm, no - adding set_fs/get_fs mess for things like that is not right.
I think it's fine. We're using it for various other interfaces too. In fact
sys_set_mempolicyis already used elsewhere in the kernel too.
It should really be do_set_mempolicy instead to be cleaner. I got a patch here that fixes the policy layer.
But still I agree with Christoph that a real vmalloc_node is better. There will be no fuzzing around with memory policies etc and its certainly better performance wise.
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 4 lines · Christoph Lameter
It should really be
do_set_mempolicyinstead to be cleaner. I got a patch here that fixes the policy layer.But still I agree with Christoph that a real
vmalloc_nodeis better. There will be no fuzzing around with memory policies etc and its certainly better performance wise.
vmalloc_node() should be seldom used, at driver init, or when a new ip_tables is loaded. If it happens to be a performance problem, then we can optimize it. Why should we spend days of work for a function that is yet to be used ?
Eric
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 2 lines · Eric Dumazet
vmalloc_node()should be seldom used, at driver init, or when a newip_tablesis loaded. If it happens to be a performance problem, then we can optimize it.
Allright. However, there are a couple of uses of vmalloc_node that I can see right now and they will likely increase in the future.
Quoted 1 line
Why should we spend days of work for a function that is yet to be used ?
I already did a vmalloc_node patch. So no need for spending days of work. The patch can wait until it becomes performance critical.
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
In reply to Eric DumazetQuoted 10 lines · Eric Dumazet
Quoted 4 lines · Christoph Lameter
It should really be
do_set_mempolicyinstead to be cleaner. I got a patch here that fixes the policy layer.
But still I agree with Christoph that a realvmalloc_nodeis better. There will be no fuzzing around with memory policies etc and its certainly better performance wise.
vmalloc_node()should be seldom used, at driver init, or when a newip_tablesis loaded. If it happens to be a performance problem, then we can optimize it. Why should we spend days of work for a function that is yet to be used ?
I see a contradiction in your sentence. "a new ip_tables is loaded" every time a user changes a single rule. There are numerous setups that dynamically change the ruleset (e.g. at interface up/down point, or even think of your typical wlan hotspot, where once a user is authorized, he'll get different rules.
Signature · 5 lines
- Harald Welte <[email protected]> http://netfilter.org/ ============================================================================ "Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed." -- Paul Vixie
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Here is an updated version of the vmalloc_node patch:
This patch adds
vmalloc_node(size, node) -> Allocate necessary memory on the specified node
and
get_vm_area_node(size, flags, node)
and the other functions that it depends on.
include/linux/vmalloc.h +21 -8
@@ -32,22 +32,35 @@ struct vm_struct {* Highlevel APIs for driver use */ extern void *vmalloc (unsigned long size); +extern void *vmalloc_node (unsigned long size,int node);extern void *vmalloc_exec (unsigned long size);extern void *vmalloc_32 (unsigned long size); -extern void *__vmalloc (unsigned long size,unsigned int __nocast gfp_mask,pgprot_t prot); -extern void *__vmalloc_area (struct vm_struct *area,unsigned int __nocast gfp_mask,pgprot_t prot); -extern void vfree (void *addr); +extern void *__vmalloc (unsigned long size,unsigned int __nocast gfp_mask, +pgprot_t prot,int node); +extern void *__vmalloc_area (struct vm_struct *area,unsigned int __nocast gfp_mask, +pgprot_t prot,int node); +extern void vfree (void *addr);extern void *vmap (struct page **pages,unsigned int count,unsigned long flags,pgprot_t prot);extern void vunmap (void *addr); - -/* -* Lowlevel-APIs (not for driver use!) + +/** +* get_vm_area - reserve a contingous kernel virtual area +* +* @size: size of the area +* @flags: %VM_IOREMAP for I/O mappings or VM_ALLOC +* +* Search an area of @size in the kernel virtual mapping area, +* and reserved it for out purposes. Returns the area descriptor +* on success or %NULL on failure. */ -extern struct vm_struct *get_vm_area (unsigned long size,unsigned long flags);extern struct vm_struct *__get_vm_area (unsigned long size,unsigned long flags, -unsigned long start,unsigned long end); +unsigned long start,unsigned long end,int node); +#define get_vm_area (__size, __flags) __get_vm_area((__size), (__flags), VMALLOC_START, \ + VMALLOC_END, -1) +#define get_vm_area_node (__size, __flags, __node) __get_vm_area((__size), (__flags), \ + VMALLOC_START, VMALLOC_END, __node)extern struct vm_struct *remove_vm_area (void *addr);extern struct vm_struct *__remove_vm_area (void *addr); extern int map_vm_area(struct vm_struct *area, pgprot_t prot,
mm/vmalloc.c +36 -28
145 lines not on this page · raw message
fs/xfs/linux-2.6/kmem.c +1 -1
@@ -55,7 +55,7 @@ kmem_alloc(size_t size, unsigned int __nif (size <MAX_SLAB_SIZE || retries >MAX_VMALLOCS ) ptr =kmalloc (size, lflags);else - ptr =__vmalloc (size, lflags,PAGE_KERNEL ); + ptr =__vmalloc (size, lflags,PAGE_KERNEL ,-1 );if (ptr || (flags & (KM_MAYFAIL |KM_NOSLEEP )))return ptr;if (!(++retries %100 ))
mm/page_alloc.c +1 -1
@@ -2542,7 +2542,7 @@ void *__init alloc_large_system_hash(conif (flags &HASH_EARLY ) table =alloc_bootmem (size);else if (hashdist) - table =__vmalloc (size,GFP_ATOMIC ,PAGE_KERNEL ); + table =__vmalloc (size,GFP_ATOMIC ,PAGE_KERNEL ,-1 );else {unsigned long order;for (order =0 ; ((1UL << order) <<PAGE_SHIFT ) < size; order++)
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 9 lines · Christoph Lameter
for (i = 0; i < area->nr_pages; i++) {mm/vmalloc.c +4
-1- area->pages[i] =alloc_page (gfp_mask); +if (node <0 ) + area->pages[i] =alloc_page (gfp_mask); +else + area->pages[i] =alloc_pages_node (node, gfp_mask,0 );if (unlikely (!area->pages[i])) {/* Successfully allocated i pages, free them in __vunmap() */ area->nr_pages = i;
...
mm/vmalloc.c EXPORT_SYMBOL(vmalloc); · line 512
Quoted 5 lines
void *vmalloc_exec(unsigned long size)
{mm/vmalloc.c +1
-1-return __vmalloc (size,GFP_KERNEL | __GFP_HIGHMEM,PAGE_KERNEL_EXEC ); +return __vmalloc (size,GFP_KERNEL | __GFP_HIGHMEM,PAGE_KERNEL_EXEC ,-1 ); }
Instead of hard-coding all of those -1's for the node to specify a default allocation, and changing all of those callers, why not:
void *__vmalloc_node(unsigned long size, unsigned int __nocast gfp_mask, pgprot_t prot, int node)
{
... existing vmalloc code here
}
void *__vmalloc(unsigned long size, unsigned int __nocast gfp_mask, pgprot_t prot)
{
__vmalloc_node(size, gfp_mask, prot, -1);
}A named macro is probably better than -1, but if it is only used in one place, it is hard to complain.
-- Dave
vmalloc_node
Quoted 2 lines · Dave Hansen
Instead of hard-coding all of those -1's for the node to specify a default allocation, and changing all of those callers, why not:
Done.
Quoted 3 lines
__vmalloc_node(size, gfp_mask, prot, -1);A named macro is probably better than -1, but if it is only used in one place, it is hard to complain.
-1 is used consistently in the *_node functions to indicate that the node is not specified. Should I replace -1 throughout the kernel with a constant?
Here is the updated vmalloc_node patch:
--
This patch adds
vmalloc_node(size, node) -> Allocate necessary memory on the specified node
and
get_vm_area_node(size, flags, node)
and the other functions that it depends on.
include/linux/vmalloc.h +23 -8
@@ -32,22 +32,37 @@ struct vm_struct {* Highlevel APIs for driver use */ extern void *vmalloc (unsigned long size); +extern void *vmalloc_node (unsigned long size,int node);extern void *vmalloc_exec (unsigned long size);extern void *vmalloc_32 (unsigned long size); -extern void *__vmalloc (unsigned long size,unsigned int __nocast gfp_mask,pgprot_t prot); -extern void *__vmalloc_area (struct vm_struct *area,unsigned int __nocast gfp_mask,pgprot_t prot); -extern void vfree (void *addr); +extern void *__vmalloc (unsigned long size,unsigned int __nocast gfp_mask, +pgprot_t prot); +extern void *__vmalloc_node (unsigned long size,unsigned int __nocast gfp_mask, +pgprot_t prot,int node); +extern void *__vmalloc_area (struct vm_struct *area,unsigned int __nocast gfp_mask, +pgprot_t prot,int node); +extern void vfree (void *addr);extern void *vmap (struct page **pages,unsigned int count,unsigned long flags,pgprot_t prot);extern void vunmap (void *addr); - -/* -* Lowlevel-APIs (not for driver use!) + +/** +* get_vm_area - reserve a contingous kernel virtual area +* +* @size: size of the area +* @flags: %VM_IOREMAP for I/O mappings or VM_ALLOC +* +* Search an area of @size in the kernel virtual mapping area, +* and reserved it for out purposes. Returns the area descriptor +* on success or %NULL on failure. */ -extern struct vm_struct *get_vm_area (unsigned long size,unsigned long flags);extern struct vm_struct *__get_vm_area (unsigned long size,unsigned long flags, -unsigned long start,unsigned long end); +unsigned long start,unsigned long end,int node); +#define get_vm_area (__size, __flags) __get_vm_area((__size), (__flags), VMALLOC_START, \ + VMALLOC_END, -1) +#define get_vm_area_node (__size, __flags, __node) __get_vm_area((__size), (__flags), \ + VMALLOC_START, VMALLOC_END, __node)extern struct vm_struct *remove_vm_area (void *addr);extern struct vm_struct *__remove_vm_area (void *addr); extern int map_vm_area(struct vm_struct *area, pgprot_t prot,
mm/vmalloc.c +40 -25
140 lines not on this page · raw message
Re: vmalloc_node
Quoted 5 lines · Christoph Lameter
Quoted 2 lines · Dave Hansen
Instead of hard-coding all of those -1's for the node to specify a default allocation, and changing all of those callers, why not:
Done.
That looks much nicer. Thanks!
Quoted 7 lines
Quoted 3 lines
__vmalloc_node(size, gfp_mask, prot, -1);A named macro is probably better than -1, but if it is only used in one place, it is hard to complain.
-1 is used consistently in the
*_nodefunctions to indicate that the node is not specified. Should I replace -1 throughout the kernel with a constant?
I certainly wouldn't mind. Giving it a name like NODE_ANY or NODE_UNSPECIFIED would certainly keep anyone from having to go dig into the allocator functions to decide what it actually does.
-- Dave
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
In reply to Harald WelteQuoted 13 lines · Harald Welte
Quoted 9 lines · Eric Dumazet
Quoted 2 lines · Christoph Lameter
It should really be
do_set_mempolicyinstead to be cleaner. I got a patch here that fixes the policy layer.
But still I agree with Christoph that a realvmalloc_nodeis better. There will be no fuzzing around with memory policies etc and its certainly better performance wise.
vmalloc_node()should be seldom used, at driver init, or when a newip_tablesis loaded. If it happens to be a performance problem, then we can optimize it. Why should we spend days of work for a function that is yet to be used ?
Quoted 6 lines
I see a contradiction in your sentence. "a new
ip_tablesis loaded" every time a user changes a single rule. There are numerous setups that dynamically change the ruleset (e.g. at interface up/down point, or even think of your typical wlan hotspot, where once a user is authorized, he'll get different rules.
But a user changing a single rule usually calls (fork()/exec()) a program called iptables. The underlying cost of all this, plus copying the rules to user space, so that iptables change them and reload them in the kernel is far more important than an hypothetical vmalloc_node() performance problem.
Eric
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 7 lines · Eric Dumazet
Quoted 5 lines · Harald Welte
I see a contradiction in your sentence. "a new
ip_tablesis loaded" every time a user changes a single rule. There are numerous setups that dynamically change the ruleset (e.g. at interface up/down point, or even think of your typical wlan hotspot, where once a user is authorized, he'll get different rules.
Quoted 5 lines
But a user changing a single rule usually calls (fork()/exec()) a program called iptables. The underlying cost of all this, plus copying the rules to user space, so that iptables change them and reload them in the kernel is far more important than an hypothetical
vmalloc_node()performance problem.
Yeah, if you're really worried about the cost of iptables manipulations, you should probably write your own happy little C program to atomically load, update, and store the rules. Even then, the cost of copying the whole ruleset to userspace for modification is probably greater than that of memory allocation issues, especially if the ruleset is large enough that memory allocation issues cause problems :-D
Cheers,
Kyle Moffett
-----BEGIN GEEK CODE BLOCK-----
Version: 3.12
GCM/CS/IT/U d- s++: a18 C++++>$ UB/L/X/*++++(+)>$ P+++(++++)>$ L++++(+ ++) E W++(+) N+++(++) o? K? w--- O? M++ V? PS+() PE+(-) Y+ PGP+++ t+(+ ++) 5 X R? tv-(--) b++++(++) DI+ D+ G e->++++$ h!*()>++$ r !y?(-) ------END GEEK CODE BLOCK------
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
In reply to Eric DumazetQuoted 2 lines · Eric Dumazet
I have reworked
net/ipv4/ip_tables.cto boost its performance, and post three patches.
Do you have any performance measurements?
- James
Signature · 2 lines
James Morris <[email protected]>
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 2 lines · James Morris
Do you have any performance measurements?
Yes, as I said in the first mail :
Quoted 3 lines
In oprofile results,
ipt_do_table()was at the first position. It is now at 6th position, using 1/3 of the CPU it was using before. (Tests done on a dual Xeon i386 and a dual Opteronx86_64)
On the dual opteron machine, with 40.000 packets coming per second, and 35.000 sent per second, the numbers were : 12.8 % before the patches, 4.4 % after the patches.
I dont have separate perf measurements for each patch.
Considering the fact that I inlined the read_lock_bh() call (not displayed in oprofile results, probably because of the special .spinlock.text section) that should have increased the profile of ipt_do_table(), thats a lot of CPU cycles
and mem bandwitdh that are available for other jobs.
Eric
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
In reply to Eric DumazetQuoted 3 lines
1) No more central rwlock protecting each table (filter, nat, mangle, raw), but one lock per CPU. It avoids cache line ping pongs for each packet.
Another useful change would be to not take the lock when there are no rules. Currently just loading iptables has a large overhead.
-Andi
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 7 lines · Andi Kleen
Quoted 2 lines
1) No more central rwlock protecting each table (filter, nat, mangle, raw), but one lock per CPU. It avoids cache line ping pongs for each packet.
Another useful change would be to not take the lock when there are no rules. Currently just loading iptables has a large overhead.
Unfortunatly there are allways rules, after the loading of iptables, at least for the "packet_filter" table.
Eric
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
In reply to Andi KleenQuoted 6 lines · Andi Kleen
Quoted 3 lines
1) No more central rwlock protecting each table (filter, nat, mangle, raw), but one lock per CPU. It avoids cache line ping pongs for each packet.
Another useful change would be to not take the lock when there are no rules. Currently just loading iptables has a large overhead.
This is partially due to the netfilter hooks that are registered (so we always take nf_hook_slow() in the NF_HOOK() macro).
The default policies inside an iptables chain are internally implemented as a rule. Thus, policies as built-in rules have packet/byte counters.
Therefore, without making a semantic change, we cannot do any of the following optimizations:
1) not take a lock when the chain is empty
2) not register at the netfilter hook when the chain is empty.
This is well-known, but I don't think we can change the semantics for the user during a stable kernel series. That's one point where not having 2.7.x really hurts.
Signature · 5 lines
- Harald Welte <[email protected]> http://netfilter.org/ ============================================================================ "Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed." -- Paul Vixie
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 10 lines · Harald Welte
Quoted 6 lines · Andi Kleen
Quoted 3 lines
1) No more central rwlock protecting each table (filter, nat, mangle, raw), but one lock per CPU. It avoids cache line ping pongs for each packet.
Another useful change would be to not take the lock when there are no rules. Currently just loading iptables has a large overhead.
This is partially due to the netfilter hooks that are registered (so we always take
nf_hook_slow()in theNF_HOOK()macro).
Not sure it's that. nf_hook_slow uses RCU, so it should be quite fast.
Quoted 2 lines
The default policies inside an iptables chain are internally implemented as a rule. Thus, policies as built-in rules have packet/byte counters.
That could be special cased and done lockless, with the counting done per CPU.
-Andi
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 2 lines · Andi Kleen
That could be special cased and done lockless, with the counting done per CPU.
It's also not very hard for iptables when verifying the table to conclude that there really isn't any "real" rules for a certain hook and then delete that hook registration (only policy ACCEPT rule found). Allowing you to have as many ip tables modules you like in the kernel, but only using the hooks where you have rules. Drawback is that you loose the packet counters on the policy.
Exception: iptable_nat. Needs the hooks for other purposes as well, not just the iptable so here the hooks can not be deactivated when there is no rules.
Regards
Henrik
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 5 lines · Henrik Nordstrom
Quoted 2 lines
That could be special cased and done lockless, with the counting done per CPU.
It's also not very hard for iptables when verifying the table to conclude that there really isn't any "real" rules for a certain hook
The detection should be quite straight-forward, yes.
Quoted 2 lines
Allowing you to have as many ip tables modules you like in the kernel, but only using the hooks where you have rules.
I totally agree, that from a current perspective, I think the concept of just loading a module (that has usage count 0) having severe impact on system performance is just wrong. But then, users are used to the current behaviour for almost five years now. Every decent script and/or piece of documentation should by now refer to the fact that loading a module implicitly registers it at the hooks, and that you only load those modules that you really need.
In an ideal world, you would load iptables, and userspace would configure a couple of chains, and then explicitly attach those chains to the netfilter hooks.
Therefore: Let's do this right next time, but live with that fact for now.
Quoted 1 line
Drawback is that you loose the packet counters on the policy.
That's the big problem. People rely on that fact, because they're used to it. If we introduce some new tool, or something that somehow works differently, I don't have a change. But silently changing the semantics with a kernel upgrade is not something that I'm willing to do on purpose.
Just imagine all those poor sysadmins who know nothing about current kernel development, and who upgrade their kernel because their distributor provides a new one - suddenly their accounting (which might be relevant for their business) doesn't work anymore :(
Quoted 3 lines
Exception:
iptable_nat. Needs the hooks for other purposes as well, not just the iptable so here the hooks can not be deactivated when there is no rules.
yes, even though we could make the ip_nat / iptable_nat split (that is introduced in 2.6.14) in a way that would make ip_nat.ko register those hooks that are implicitly needed, and iptable_nat only the user-visible chain-related hooks.
Signature · 5 lines
- Harald Welte <[email protected]> http://netfilter.org/ ============================================================================ "Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed." -- Paul Vixie
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 4 lines · Harald Welte
I totally agree, that from a current perspective, I think the concept of just loading a module (that has usage count 0) having severe impact on system performance is just wrong. But then, users are used to the current behaviour for almost five years now.
That doesn't mean it cannot be improved - and I think it should.
In a sense it's even getting worse: For example us losing the CONFIG option to disable local conntrack (Patrick has disabled it some time ago without even a comment why he did it) has a really bad impact in some cases.
Quoted 2 lines
Therefore: Let's do this right next time, but live with that fact for now.
Even with a "quite straight-forward" (quoting you) fix?
Quoted 4 lines
Just imagine all those poor sysadmins who know nothing about current kernel development, and who upgrade their kernel because their distributor provides a new one - suddenly their accounting (which might be relevant for their business) doesn't work anymore :(
Accounting with per CPU counters can be done fully lockless.
-Andi
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 2 lines · Andi Kleen
In a sense it's even getting worse: For example us losing the CONFIG
option to disable local conntrack (Patrick has disabled it some time ago without even a comment why he did it) has a really bad impact in some cases.
It was necessary to correctly handle locally generated ICMP errors.
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 7 lines · Patrick McHardy
Quoted 4 lines · Andi Kleen
In a sense it's even getting worse: For example us losing the CONFIG option to disable local conntrack (Patrick has disabled it some time ago without even a comment why he did it) has a really bad impact in some cases.
It was necessary to correctly handle locally generated ICMP errors.
Well you most likely wrecked local performance then when it's enabled.
-Andi
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 10 lines · Andi Kleen
Quoted 7 lines · Patrick McHardy
Quoted 4 lines · Andi Kleen
In a sense it's even getting worse: For example us losing the CONFIG option to disable local conntrack (Patrick has disabled it some time ago without even a comment why he did it) has a really bad impact in some cases.
It was necessary to correctly handle locally generated ICMP errors.
Well you most likely wrecked local performance then when it's enabled.
so you would favour a system that incorrectly deals with ICMP errors but has higher performance?
Signature · 5 lines
- Harald Welte <[email protected]> http://netfilter.org/ ============================================================================ "Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed." -- Paul Vixie
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 8 lines · Harald Welte
Quoted 7 lines · Andi Kleen
Quoted 6 lines · Patrick McHardy
Quoted 4 lines · Andi Kleen
In a sense it's even getting worse: For example us losing the CONFIG option to disable local conntrack (Patrick has disabled it some time ago without even a comment why he did it) has a really bad impact in some cases.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
It was necessary to correctly handle locally generated ICMP errors.
Well you most likely wrecked local performance then when it's enabled.
so you would favour a system that incorrectly deals with ICMP errors but has higher performance?
I would favour a system where development doesn't lose sight of performance. Perhaps there would be other ways to fix this problem without impacting performance unduly? Can you describe it in detail?
-Andi
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 5 lines · Andi Kleen
Quoted 3 lines · Harald Welte
Quoted 1 line · Andi Kleen
Well you most likely wrecked local performance then when it's enabled.
There are lots of other hooks and conntrack/NAT already have a quite large negative influence on performance. Do you have numbers that show that enabling this actually causes more than a slight decrease in performance? Besides, most distributors enable all these options anyway, so it only makes a difference for a small group of users.
Quoted 4 lines
Quoted 2 lines
so you would favour a system that incorrectly deals with ICMP errors but has higher performance?
I would favour a system where development doesn't lose sight of performance.
I don't think we do.
Quoted 2 lines
Perhaps there would be other ways to fix this problem without impacting performance unduly? Can you describe it in detail?
When an ICMP error is send by the firewall itself, the inner packet needs to be restored to its original state. That means both DNAT and SNAT which might have been applied need to be reversed. DNAT is reversed at places where we usually do SNAT (POST_ROUTING), SNAT is reversed where usually DNAT is done (PRE_ROUTING/LOCAL_OUT). Since locally generated packets never go through PRE_ROUTING, it is done in LOCAL_OUT, which required enabling NAT in LOCAL_OUT unconditionally. It might be possible to move this to some different hook, I didn't investigate it.
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 6 lines · Patrick McHardy
There are lots of other hooks and conntrack/NAT already have a quite large negative influence on performance. Do you have numbers that show that enabling this actually causes more than a slight decrease in performance? Besides, most distributors enable all these options anyway, so it only makes a difference for a small group of users.
I don't know about other distributions but SUSE at some point found that some web benchmarks dramatically improved in the default configuration when local conntrack was off. It was off then since ever.
Quoted 13 lines
Quoted 2 lines
Perhaps there would be other ways to fix this problem without impacting performance unduly? Can you describe it in detail?
When an ICMP error is send by the firewall itself, the inner packet needs to be restored to its original state. That means both DNAT and SNAT which might have been applied need to be reversed. DNAT is reversed at places where we usually do SNAT (
POST_ROUTING), SNAT is reversed where usually DNAT is done (PRE_ROUTING/LOCAL_OUT). Since locally generated packets never go throughPRE_ROUTING, it is done inLOCAL_OUT, which required enabling NAT inLOCAL_OUTunconditionally. It might be possible to move this to some different hook, I didn't investigate it.
This sounds wrong anyways. You shouldn't be touching conntrack state for ICMPs generated by routers because they can be temporary errors (e.g. during a routing flap when the route moves). Only safe way to handle this is to wait for the timeout which doesn't need local handling. And the firewall cannot be an endhost here.
-Andi
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
Quoted 4 lines · Andi Kleen
On Friday 07 October 2005 19:08, Patrick McHardy wrote:
I don't know about other distributions but SUSE at some point
found that some web benchmarks dramatically improved in the default configuration when local conntrack was off. It was off then since ever.
Interesting ..
Quoted 11 lines
Quoted 10 lines
When an ICMP error is send by the firewall itself, the inner packet needs to be restored to its original state. That means both DNAT and SNAT which might have been applied need to be reversed. DNAT is reversed at places where we usually do SNAT (
POST_ROUTING), SNAT is reversed where usually DNAT is done (PRE_ROUTING/LOCAL_OUT). Since locally generated packets never go throughPRE_ROUTING, it is done inLOCAL_OUT, which required enabling NAT inLOCAL_OUTunconditionally. It might be possible to move this to some different hook, I didn't investigate it.
Quoted 1 line
This sounds wrong anyways. You shouldn't be touching conntrack state for ICMPs generated by routers because they can be temporary errors (e.g. during a routing flap when the route moves). Only safe way to handle this is to wait for the timeout which doesn't need local handling. And the firewall cannot be an endhost here.
You misunderstood me. Its not about conntrack state, its about the inner packet contained in the ICMP message. If it was NATed by the box itself before the ICMP message is generated, it needs to be restored to its original state, otherwise the receipient will ignore it.
Re: [PATCH 0/3] netfilter : 3 patches to boost ip_tables performance
In reply to Harald WelteQuoted 4 lines · Harald Welte
Just imagine all those poor sysadmins who know nothing about current kernel development, and who upgrade their kernel because their distributor provides a new one - suddenly their accounting (which might be relevant for their business) doesn't work anymore :(
You seriously expect anyone is using the counters on the policy rule in otherwise completely empty rulesets? These counters is also available elsewhere (route and interface statistics) and more intuitively so.
Quoted 4 lines
yes, even though we could make the
ip_nat/iptable_natsplit (that is introduced in 2.6.14) in a way that would make ip_nat.ko register those hooks that are implicitly needed, andiptable_natonly the user-visible chain-related hooks.
Which adds additional overhead, so this approach is a bit counter-productive I would say.
Regards
Henrik
PATCH 1/1 netfilter : zap get_cpu()/put_cpu() calls from ip_tables
In reply to Eric DumazetThe 'ip_tables: NUMA-aware allocation' patch (http://www.kernel.org/git/?p=linux/kernel/git/davem/net-2.6.16.git;a=commit;h=6d1273850cac8db77c462caaa145b813d93adc55)
introduced a problem with get_cpu()/put_cpu()
The problem is that get_cpu() disables preemption and the thread is not allowed to sleep.
As the intent was only to give a hint, relax the enforcement and switch to a hint : If the current thread is preempted and migrated to another cpu, this is not a problem. Most of the time, thread wont be migrated anyway.
This patch removes get_cpu()/put_cpu() pairs and uses raw_smp_processor_id() were appropriate.
PATCH 1/1 netfilter : zap get_cpu()/put_cpu() calls from ip_tables
The 'ip_tables: NUMA-aware allocation' patch (http://www.kernel.org/git/?p=linux/kernel/git/davem/net-2.6.16.git;a=commit;h=6d1273850cac8db77c462caaa145b813d93adc55)
introduced a problem with get_cpu()/put_cpu()
The problem is that get_cpu() disables preemption and the thread is not allowed to sleep.
As the intent was only to give a hint, relax the enforcement and switch to a hint : If the current thread is preempted and migrated to another cpu, this is not a problem. Most of the time, thread wont be migrated anyway.
This patch removes get_cpu()/put_cpu() pairs and uses raw_smp_processor_id() were appropriate.
net/ipv4/netfilter/ip_tables.c +20 -18
@@ -988,11 +988,14 @@ {unsigned int cpu;unsigned int i; -unsigned int curcpu =get_cpu (); +unsigned int curcpu;/* Instead of clearing (by a previous call to memset()) * the counters and using adds, we set the counters -* with data used by current CPU */ +* with data used by 'current' CPU +* We dont care about preemption here. +*/ + curcpu =raw_smp_processor_id (); i =0 ; IPT_ENTRY_ITERATE(t->entries[curcpu],
@@ -1011,7 +1014,6 @@ counters, &i); } -put_cpu (); } static int
@@ -1029,7 +1031,7 @@(other than comefrom, which userspace doesn't care about). */ countersize =sizeof (struct ipt_counters ) * table->private->number; - counters =vmalloc (countersize); + counters =vmalloc_node (countersize,numa_node_id ());if (counters ==NULL )return -ENOMEM ;
@@ -1039,8 +1041,11 @@get_counters (table->private, counters);write_unlock_bh (&table->lock); -/* choose the copy that is on our node/cpu, ... */ - loc_cpu_entry = table->private->entries[get_cpu ()]; +/* choose the copy that is on our node/cpu, ... +* This choice is lazy (because current thread is +* allowed to migrate to another cpu) +*/ + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()];/* ... then copy entire thing ... */ if (copy_to_user (userptr, loc_cpu_entry, total_size) !=0 ) { ret = -EFAULT ;
@@ -1091,7 +1096,6 @@ } free_counters: -put_cpu ();vfree (counters);return ret; }
@@ -1189,7 +1193,7 @@return -ENOMEM ;/* choose the copy that is our node/cpu */ - loc_cpu_entry = newinfo->entries[get_cpu ()]; + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()];if (copy_from_user (loc_cpu_entry, user +sizeof (tmp), tmp.size) !=0 ) { ret = -EFAULT ;
@@ -1242,9 +1246,8 @@/* Get the old counters. */ get_counters (oldinfo, counters);/* Decrease module usage counts and free resource */ - loc_cpu_old_entry = oldinfo->entries[smp_processor_id ()]; + loc_cpu_old_entry = oldinfo->entries[raw_smp_processor_id ()];IPT_ENTRY_ITERATE (loc_cpu_old_entry, oldinfo->size, cleanup_entry,NULL ); -put_cpu ();free_table_info (oldinfo);if (copy_to_user (tmp.counters, counters,sizeof (struct ipt_counters ) * tmp.num_counters) !=0 )
@@ -1261,7 +1264,6 @@ free_newinfo_counters:vfree (counters); free_newinfo: -put_cpu ();free_table_info (newinfo);return ret; }
@@ -1303,7 +1305,7 @@if (len !=sizeof (tmp) + tmp.num_counters*sizeof (struct ipt_counters ))return -EINVAL ; - paddc =vmalloc (len); + paddc =vmalloc_node (len,numa_node_id ());if (!paddc)return -ENOMEM ;
@@ -1326,7 +1328,7 @@ i =0 ;/* Choose the copy that is on our node */ - loc_cpu_entry = t->private->entries[smp_processor_id ()]; + loc_cpu_entry = t->private->entries[raw_smp_processor_id ()]; IPT_ENTRY_ITERATE(loc_cpu_entry, t->private->size, add_counter_to_entry,
@@ -1525,8 +1527,10 @@if (!newinfo)return -ENOMEM ; -/* choose the copy on our node/cpu */ - loc_cpu_entry = newinfo->entries[get_cpu ()]; +/* choose the copy on our node/cpu +* but dont care of preemption +*/ + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()];memcpy (loc_cpu_entry, repl->entries, repl->size); ret = translate_table(table->name, table->valid_hooks,
@@ -1534,7 +1538,6 @@ repl->num_entries, repl->hook_entry, repl->underflow); -put_cpu ();if (ret !=0 ) {free_table_info (newinfo);return ret;
@@ -1584,10 +1587,9 @@up (&ipt_mutex);/* Decrease module usage counts and free resources */ - loc_cpu_entry = table->private->entries[get_cpu ()]; + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()];IPT_ENTRY_ITERATE (loc_cpu_entry, table->private->size, cleanup_entry,NULL ); -put_cpu ();free_table_info (table->private); }
net/ipv6/netfilter/ip6_tables.c +10 -13
@@ -1074,11 +1074,14 @@ {unsigned int cpu;unsigned int i; -unsigned int curcpu =get_cpu (); +unsigned int curcpu;/* Instead of clearing (by a previous call to memset()) * the counters and using adds, we set the counters -* with data used by current CPU */ +* with data used by 'current' CPU +* We dont care about preemption here. +*/ + curcpu =raw_smp_processor_id (); i =0 ; IP6T_ENTRY_ITERATE(t->entries[curcpu],
@@ -1097,7 +1100,6 @@ counters, &i); } -put_cpu (); } static int
@@ -1126,7 +1128,7 @@write_unlock_bh (&table->lock);/* choose the copy that is on ourc node/cpu */ - loc_cpu_entry = table->private->entries[get_cpu ()]; + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()];if (copy_to_user (userptr, loc_cpu_entry, total_size) !=0 ) { ret = -EFAULT ; goto free_counters;
@@ -1176,7 +1178,6 @@ } free_counters: -put_cpu ();vfree (counters);return ret; }
@@ -1271,7 +1272,7 @@return -ENOMEM ;/* choose the copy that is on our node/cpu */ - loc_cpu_entry = newinfo->entries[get_cpu ()]; + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()];if (copy_from_user (loc_cpu_entry, user +sizeof (tmp), tmp.size) !=0 ) { ret = -EFAULT ;
@@ -1324,9 +1325,8 @@/* Get the old counters. */ get_counters (oldinfo, counters);/* Decrease module usage counts and free resource */ - loc_cpu_old_entry = oldinfo->entries[smp_processor_id ()]; + loc_cpu_old_entry = oldinfo->entries[raw_smp_processor_id ()];IP6T_ENTRY_ITERATE (loc_cpu_old_entry, oldinfo->size, cleanup_entry,NULL ); -put_cpu ();free_table_info (oldinfo);if (copy_to_user (tmp.counters, counters,sizeof (struct ip6t_counters ) * tmp.num_counters) !=0 )
@@ -1343,7 +1343,6 @@ free_newinfo_counters:vfree (counters); free_newinfo: -put_cpu ();free_table_info (newinfo);return ret; }
@@ -1609,7 +1608,7 @@return -ENOMEM ;/* choose the copy on our node/cpu */ - loc_cpu_entry = newinfo->entries[get_cpu ()]; + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()];memcpy (loc_cpu_entry, repl->entries, repl->size); ret = translate_table(table->name, table->valid_hooks,
@@ -1617,7 +1616,6 @@ repl->num_entries, repl->hook_entry, repl->underflow); -put_cpu ();if (ret !=0 ) {free_table_info (newinfo);return ret;
@@ -1667,10 +1665,9 @@up (&ip6t_mutex);/* Decrease module usage counts and free resources */ - loc_cpu_entry = table->private->entries[get_cpu ()]; + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()];IP6T_ENTRY_ITERATE (loc_cpu_entry, table->private->size, cleanup_entry,NULL ); -put_cpu ();free_table_info (table->private); }
net/ipv4/netfilter/arp_tables.c +11 -13
@@ -814,11 +814,14 @@ {unsigned int cpu;unsigned int i; -unsigned int curcpu =get_cpu (); +unsigned int curcpu;/* Instead of clearing (by a previous call to memset()) * the counters and using adds, we set the counters -* with data used by current CPU */ +* with data used by 'current' CPU +* We dont care about preemption here. +*/ + curcpu =raw_smp_processor_id (); i =0 ; ARPT_ENTRY_ITERATE(t->entries[curcpu],
@@ -837,7 +840,6 @@ counters, &i); } -put_cpu (); } static int copy_entries_to_user(unsigned int total_size,
@@ -865,7 +867,7 @@get_counters (table->private, counters);write_unlock_bh (&table->lock); - loc_cpu_entry = table->private->entries[get_cpu ()]; + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()];/* ... then copy entire thing ... */ if (copy_to_user (userptr, loc_cpu_entry, total_size) !=0 ) { ret = -EFAULT ;
@@ -898,7 +900,6 @@ } free_counters: -put_cpu ();vfree (counters);return ret; }
@@ -996,7 +997,7 @@return -ENOMEM ;/* choose the copy that is on our node/cpu */ - loc_cpu_entry = newinfo->entries[get_cpu ()]; + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()];if (copy_from_user (loc_cpu_entry, user +sizeof (tmp), tmp.size) !=0 ) { ret = -EFAULT ;
@@ -1049,9 +1050,9 @@/* Get the old counters. */ get_counters (oldinfo, counters);/* Decrease module usage counts and free resource */ - loc_cpu_old_entry = oldinfo->entries[smp_processor_id ()]; + loc_cpu_old_entry = oldinfo->entries[raw_smp_processor_id ()];ARPT_ENTRY_ITERATE (loc_cpu_old_entry, oldinfo->size, cleanup_entry,NULL ); -put_cpu (); +free_table_info (oldinfo);if (copy_to_user (tmp.counters, counters,sizeof (struct arpt_counters ) * tmp.num_counters) !=0 )
@@ -1068,7 +1069,6 @@ free_newinfo_counters:vfree (counters); free_newinfo: -put_cpu ();free_table_info (newinfo);return ret; }
@@ -1295,7 +1295,7 @@ }/* choose the copy on our node/cpu */ - loc_cpu_entry = newinfo->entries[get_cpu ()]; + loc_cpu_entry = newinfo->entries[raw_smp_processor_id ()];memcpy (loc_cpu_entry, repl->entries, repl->size); ret = translate_table(table->name, table->valid_hooks,
@@ -1303,7 +1303,6 @@ repl->num_entries, repl->hook_entry, repl->underflow); -put_cpu ();duprintf ("arpt_register_table: translate table gives %d\n" , ret);if (ret !=0 ) {free_table_info (newinfo);
@@ -1354,10 +1353,9 @@up (&arpt_mutex);/* Decrease module usage counts and free resources */ - loc_cpu_entry = table->private->entries[get_cpu ()]; + loc_cpu_entry = table->private->entries[raw_smp_processor_id ()];ARPT_ENTRY_ITERATE (loc_cpu_entry, table->private->size, cleanup_entry,NULL ); -put_cpu ();free_table_info (table->private); }
Re: [PATCH (resent with the attachment !)] netfilter : zap get_cpu()/put_cpu() calls from ip_tables
Quoted 9 lines · Eric Dumazet
The '
ip_tables: NUMA-aware allocation' patch (http://www.kernel.org/git/?p=linux/kernel/git/davem/net-2.6.16.git;a=commit;h=6d1273850cac8db77c462caaa145b813d93adc55)introduced a problem with get_cpu()/put_cpu()
The problem is that
get_cpu()disables preemption and the thread is not allowed to sleep.As the intent was only to give a hint, relax the enforcement and switch to a hint : If the current thread is preempted and migrated to another cpu, this is not a problem. Most of the time, thread wont be migrated anyway.
This patch removes get_cpu()/put_cpu() pairs and uses
raw_smp_processor_id()were appropriate.
Looks good, thanks. I'm going to test it and pass it on to Dave if it works.
PATCH 1/3 netfilter : 3 patches to boost ip_tables performance
In reply to Eric DumazetPatch 1/3
1) No more one rwlock_t protecting the 'curtain'
One major bottleneck on SMP machines is the fact that one rwlock is taken in ipt_do_table() entry and exit. That 2 atomic operations are the killer, and even if multiple readers can work together on the same table (using read_lock_bh()), the cache line containing rwlock still must be taken exclusively by each CPU at entry/exit of ipt_do_table().
As existing code already use separate copies of the data for each cpu, it is very easy to convert the central rwlock to separate rwlocks, allocated percpu (and NUMA aware).
When a cpu enters ipt_do_table(), it can locks its local copy, touching a cache line that is not used by other cpus. Further operations are done on 'local' copy of the data.
When a sum of all counters must be done, we can write_lock each part at a time, instead of locking all the parts, reducing the lock contention.
Note1 : I also optimized get_counters(), using a SET_COUNTER() for the first cpu, avoiding a memset() and ADD_COUNTER() if SMP on other cpus.
include/linux/netfilter_ipv4/ip_tables.h +4 -0
@@ -445,7 +445,11 @@unsigned int valid_hooks;/* Lock for the curtain */ +#ifdef CONFIG_SMP +rwlock_t *lock_p;/* one lock per cpu */ +#else rwlock_t lock; +#endif /* Man behind the curtain... */ struct ipt_table_info *private;
net/ipv4/netfilter/ip_nat_rule.c +0 -1
@@ -93,7 +93,6 @@static struct ipt_table nat_table = { .name ="nat" , .valid_hooks =NAT_VALID_HOOKS , - .lock =RW_LOCK_UNLOCKED , .me =THIS_MODULE , };
net/ipv4/netfilter/iptable_filter.c +0 -1
@@ -77,7 +77,6 @@static struct ipt_table packet_filter = { .name ="filter" , .valid_hooks =FILTER_VALID_HOOKS , - .lock =RW_LOCK_UNLOCKED , .me =THIS_MODULE };
net/ipv4/netfilter/iptable_mangle.c +0 -1
@@ -107,7 +107,6 @@static struct ipt_table packet_mangler = { .name ="mangle" , .valid_hooks =MANGLE_VALID_HOOKS , - .lock =RW_LOCK_UNLOCKED , .me =THIS_MODULE , };
net/ipv4/netfilter/iptable_raw.c +0 -1
@@ -82,7 +82,6 @@static struct ipt_table packet_raw = { .name ="raw" , .valid_hooks =RAW_VALID_HOOKS , - .lock =RW_LOCK_UNLOCKED , .me =THIS_MODULE };
net/ipv4/netfilter/ip_tables.c +129 -13
264 lines not on this page · raw message
Re: [PATCH 1/3] netfilter : 3 patches to boost ip_tables performance
Quoted 3 lines · Eric Dumazet
Patch 1/3
1) No more one
rwlock_tprotecting the 'curtain'
I have no problem with this change "per se", but with the implementation.
As of now, we live without any ugly #ifdef CONFIG_SMP / #endif sections in the code - and if possible, I would continue this good tradition.
For example the get_counters() function. Wouldn't all the smp specific code (for_each_cpu(), ...) be #defined to nothing anyway?
And if we really need the #ifdef's, I would appreciate if those sectionas are as small as possible. in get_counters() the section can definitely be smaller, rather than basically having the whole function body separate for smp and non-smp cases.
Also, how much would we loose in runtime performance if we were using a "rwlock_t *" even in the UP case?. I mean, it's just one more pointer dereference of something that is expected to be in cache anyway, isn't it? This gets rid of another huge set of #ifdefs that make the code unreadable and prone to errors being introduced later on.
Signature · 5 lines
- Harald Welte <[email protected]> http://netfilter.org/ ============================================================================ "Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed." -- Paul Vixie
Re: [PATCH 1/3] netfilter : 3 patches to boost ip_tables performance
Quoted 7 lines · Harald Welte
Quoted 3 lines · Eric Dumazet
Patch 1/3
1) No more one
rwlock_tprotecting the 'curtain'
Quoted 8 lines
I have no problem with this change "per se", but with the implementation.
As of now, we live without any ugly #ifdef
CONFIG_SMP/ #endif sections in the code - and if possible, I would continue this good tradition.For example the
get_counters()function. Wouldn't all the smp specific code (for_each_cpu(), ...) be #defined to nothing anyway?
Well... not exactly, but you are right only the first loop (SET_COUNTER) will really do something. The if (cpu == curcpu) will be true but the compiler wont
know that, cpu and curcpu are still C variables.
Quoted 5 lines
And if we really need the #ifdef's, I would appreciate if those sectionas are as small as possible. in
get_counters()the section can definitely be smaller, rather than basically having the whole function body separate for smp and non-smp cases.
get_counters() is not critical, so I agree with you we can stick the general version (not the UP optimized one)
Quoted 7 lines
Also, how much would we loose in runtime performance if we were using a "
rwlock_t*" even in the UP case?. I mean, it's just one more pointer dereference of something that is expected to be in cache anyway, isn't it? This gets rid of another huge set of #ifdefs that make the code unreadable and prone to errors being introduced later on.
Well, in UP case, the rwlock_t is a nulldef.
I was inspired by another use of percpu data in include/linux/genhd.h
#ifdef CONFIG_SMP
struct disk_stats *dkstats;
#else
struct disk_stats dkstats;#endif
But if you dislike this, we can use pointer for all cases.
Eric
PATCH 2/3 netfilter : 3 patches to boost ip_tables performance
In reply to Eric DumazetPatch 2/3 (please apply after Patch 1/3)
2) Loop unrolling
It seems that with current compilers and CFLAGS, the code fromip_packet_match() is very bad, using lot of mispredicted conditional branches I made some patches and generated code on i386 and x86_64 is much better.
net/ipv4/netfilter/ip_tables.c +48 -21
@@ -125,6 +125,27 @@#define up (x) do { printk("UP:%u:" #x "\n", __LINE__); up(x); } while(0)#endif +/* +* Special macro to compare IFNAMSIZ 'strings', with mask. +* Best results if feeded with 3 args pointing to 'unsigned long' types +* Loop is manually unrolled for performance reasons. +* gcc generates code without branches, IFNAMSIZ being a constant +*/ +#define compare_if_lstrings (ret, devname, expect, expect_mask) \ + ret = ((devname)[0] ^ (expect)[0]) & (expect_mask)[0]; \ + if (IFNAMSIZ > sizeof(*devname)) \ + ret |= ((devname)[1] ^ (expect)[1]) & (expect_mask)[1]; \ + if (IFNAMSIZ > 2 * sizeof(*devname)) \ + ret |= ((devname)[2] ^ (expect)[2]) & (expect_mask)[2]; \ + if (IFNAMSIZ > 3 * sizeof(*devname)) \ + ret |= ((devname)[3] ^ (expect)[3]) & (expect_mask)[3]; \ +/* just in case IFNAMSIZ is enlarged */ \ +if (IFNAMSIZ >4 *sizeof (*devname)) { \ +int i; \ +for (i =4 ; (i <IFNAMSIZ /sizeof (*devname)); i++) \ + ret |= ((devname)[i] ^ (expect)[i]) & (expect_mask)[i]; \ + } +/* Returns whether matches rule or not. */ static inline int ip_packet_match(const struct iphdr *ip,
@@ -133,16 +154,22 @@const struct ipt_ip *ipinfo, int isfrag) { - size_t i; + int bool1, bool2;unsigned long ret;#define FWINV (bool,invflg) ((bool) ^ !!(ipinfo->invflags & invflg)) -if (FWINV ((ip->saddr&ipinfo->smsk.s_addr) != ipinfo->src.s_addr, -IPT_INV_SRCIP ) - ||FWINV ((ip->daddr&ipinfo->dmsk.s_addr) != ipinfo->dst.s_addr, -IPT_INV_DSTIP )) { -dprintf ("Source or dest mismatch.\n" ); + bool1 = ((ip->saddr&ipinfo->smsk.s_addr) != ipinfo->src.s_addr); + bool1 ^= !!(ipinfo->invflags &IPT_INV_SRCIP ); + + bool2 = ((ip->daddr&ipinfo->dmsk.s_addr) != ipinfo->dst.s_addr); + bool2 ^= !!(ipinfo->invflags &IPT_INV_DSTIP ); + +if ((bool1 | bool2) !=0 ) { +if (bool1) +dprintf ("Source%s mismatch.\n" , bool2 ?" and Dest" :"" ); +else +dprintf ("Dest mismatch.\n" ); dprintf("SRC: %u.%u.%u.%u. Mask: %u.%u.%u.%u. Target: %u.%u.%u.%u.%s\n", NIPQUAD(ip->saddr),
@@ -157,27 +184,26 @@return 0 ; } -/* Look for ifname matches; this should unroll nicely. */ -for (i =0 , ret =0 ; i <IFNAMSIZ /sizeof (unsigned long ); i++) { - ret |= (((const unsigned long *)indev)[i] - ^ ((const unsigned long *)ipinfo->iniface)[i]) - & ((const unsigned long *)ipinfo->iniface_mask)[i]; - } +compare_if_lstrings (ret, + (const unsigned long *)indev, + (const unsigned long *)ipinfo->iniface, + (const unsigned long *)ipinfo->iniface_mask); -if (FWINV (ret !=0 ,IPT_INV_VIA_IN )) { + bool1 =FWINV (ret !=0 ,IPT_INV_VIA_IN ); +if (bool1) {dprintf ("VIA in mismatch (%s vs %s).%s\n" , indev, ipinfo->iniface, ipinfo->invflags&IPT_INV_VIA_IN ?" (INV)" :"" );return 0 ; } -for (i =0 , ret =0 ; i <IFNAMSIZ /sizeof (unsigned long ); i++) { - ret |= (((const unsigned long *)outdev)[i] - ^ ((const unsigned long *)ipinfo->outiface)[i]) - & ((const unsigned long *)ipinfo->outiface_mask)[i]; - } +compare_if_lstrings (ret, + (const unsigned long *)outdev, + (const unsigned long *)ipinfo->outiface, + (const unsigned long *)ipinfo->outiface_mask); -if (FWINV (ret !=0 ,IPT_INV_VIA_OUT )) { + bool1 =FWINV (ret !=0 ,IPT_INV_VIA_OUT ); +if (bool1) {dprintf ("VIA out mismatch (%s vs %s).%s\n" , outdev, ipinfo->outiface, ipinfo->invflags&IPT_INV_VIA_OUT ?" (INV)" :"" );
@@ -185,8 +211,9 @@ }/* Check specific protocol */ -if (ipinfo->proto - &&FWINV (ip->protocol != ipinfo->proto,IPT_INV_PROTO )) { + bool1 =FWINV (ip->protocol != ipinfo->proto,IPT_INV_PROTO ) ; + bool1 &= (ipinfo->proto !=0 ); +if (bool1) {dprintf ("Packet protocol %hi does not match %hi.%s\n" , ip->protocol, ipinfo->proto, ipinfo->invflags&IPT_INV_PROTO ?" (INV)" :"" );
Re: [PATCH 2/3] netfilter : 3 patches to boost ip_tables performance
Quoted 3 lines · Eric Dumazet
Patch 2/3 (please apply after Patch 1/3)
2) Loop unrolling
First of all, thanks for your performance analysis and patches. I'm very inclined of merging them.
Quoted 4 lines
It seems that with current compilers and CFLAGS, the code from
ip_packet_match()is very bad, using lot of mispredicted conditional branches I made some patches and generated code on i386 andx86_64
is much better.
This only describes your "compare_if_lstrings()" changes, and I'm happy to merge them.
However, you also removed the use of the FWINV macro and replaced it by explicit code (including the bool1/bool2 variables, which are not really named intuitively). Why was this neccessary?
Signature · 5 lines
- Harald Welte <[email protected]> http://netfilter.org/ ============================================================================ "Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed." -- Paul Vixie
Re: [PATCH 2/3] netfilter : 3 patches to boost ip_tables performance
Quoted 16 lines · Harald Welte
Quoted 3 lines · Eric Dumazet
Patch 2/3 (please apply after Patch 1/3)
2) Loop unrolling
First of all, thanks for your performance analysis and patches. I'm very inclined of merging them.
Quoted 3 lines
It seems that with current compilers and CFLAGS, the code from
ip_packet_match()is very bad, using lot of mispredicted conditional branches I made some patches and generated code on i386 andx86_64is much better.
Quoted 7 lines
This only describes your "
compare_if_lstrings()" changes, and I'm happy to merge them.However, you also removed the use of the FWINV macro and replaced it by explicit code (including the bool1/bool2 variables, which are not really named intuitively). Why was this neccessary?
It was necessary to get the best code with gcc-3.4.4 on i386 and gcc-4.0.1 on x86_64
For example :
bool1 = FWINV(ret != 0, IPT_INV_VIA_OUT);
if (bool1) {
gives a better code than :
if (FWINV(ret != 0, IPT_INV_VIA_OUT)) {
(one less conditional branch)
Dont ask me why, it is shocking but true :(
Eric
Re: [PATCH 2/3] netfilter : 3 patches to boost ip_tables performance
On Thu, Sep 22, 2005 at 03:05:50PM +0200, Eric Dumazet wrote:
(...)
Quoted 8 lines
It was necessary to get the best code with gcc-3.4.4 on i386 and gcc-4.0.1 on
x86_64For example :
bool1 =
FWINV(ret != 0, IPT_INV_VIA_OUT);
if (bool1) {
Quoted 7 lines
gives a better code than :
if (
FWINV(ret != 0, IPT_INV_VIA_OUT)) {(one less conditional branch)
Dont ask me why, it is shocking but true :(
I also noticed many times that gcc's optimization of "if (complex condition)" is rather poor and it's often better to put it in a variable before. I even remember that if you use an intermediate variable, it can often generate a CMOV instruction on processors which support it, while it produces cond tests and jumps without the variable. Generally speaking, if you want fast code, you have to write it as a long sequence of small instructions, just as if you were writing assembly. As you said, shocking but true.
BTW, cheers for your optimizations !
Regards,
Willy
Re: [PATCH 2/3] netfilter : 3 patches to boost ip_tables performance
Quoted 19 lines · Willy Tarreau
On Thu, Sep 22, 2005 at 03:05:50PM +0200, Eric Dumazet wrote:
(...)Quoted 7 lines
It was necessary to get the best code with gcc-3.4.4 on i386 and gcc-4.0.1 on
x86_64For example :
bool1 =
FWINV(ret != 0, IPT_INV_VIA_OUT);
if (bool1) {Quoted 7 lines
gives a better code than :
if (
FWINV(ret != 0, IPT_INV_VIA_OUT)) {(one less conditional branch)
Dont ask me why, it is shocking but true :(
Quoted 7 lines
I also noticed many times that gcc's optimization of "if (complex condition)" is rather poor and it's often better to put it in a variable before. I even remember that if you use an intermediate variable, it can often generate a CMOV instruction on processors which support it, while it produces cond tests and jumps without the variable. Generally speaking, if you want fast code, you have to write it as a long sequence of small instructions, just as if you were writing assembly. As you said, shocking but true.
Even without CMOV support, the suggested patch helps :
Here is the code generated with gcc-3.4.4 on a pentium4 (i686) for :
/********************/
bool1 = ((ip->saddr&ipinfo->smsk.s_addr) != ipinfo->src.s_addr); bool1 ^= !!(ipinfo->invflags & IPT_INV_SRCIP);
bool2 = ((ip->daddr&ipinfo->dmsk.s_addr) != ipinfo->dst.s_addr); bool2 ^= !!(ipinfo->invflags & IPT_INV_DSTIP);
if ((bool1 | bool2) != 0) {
/********************/
cb: 0f b6 56 53 movzbl 0x53(%esi),%edx cf: 8b 46 08 mov 0x8(%esi),%eax #ip->saddr d2: 23 47 0c and 0xc(%edi),%eax #ipinfo->smsk.s_addr d5: 0f b6 da movzbl %dl,%ebx d8: 3b 06 cmp (%esi),%eax #ipinfo->src.s_addr da: 88 55 cf mov %dl,0xffffffcf(%ebp) dd: 89 da mov %ebx,%edx df: 0f 95 c0 setne %al e2: c1 ea 03 shr $0x3,%edx e5: 31 c2 xor %eax,%edx e7: 8b 46 0c mov 0xc(%esi),%eax #ip->daddr&ipinfo ea: 23 47 10 and 0x10(%edi),%eax #ipinfo->dmsk.s_addr ed: 3b 46 04 cmp 0x4(%esi),%eax #ipinfo->dst.s_addr f0: 89 d8 mov %ebx,%eax f2: 0f 95 c1 setne %cl f5: c1 e8 04 shr $0x4,%eax f8: 31 c8 xor %ecx,%eax fa: 09 d0 or %edx,%eax fc: a8 01 test $0x1,%al fe: 0f 85 95 00 00 00 jne dest // only one conditional branch
As you can see the whole sequence is rather good : only one conditional branch (No CMOV instructions as you can see, so even on a i486 the code should be roughly the same)
Now here is the code generated for the original code :
/********************/
if (FWINV((ip->saddr&ipinfo->smsk.s_addr) != ipinfo->src.s_addr,
IPT_INV_SRCIP)
|| FWINV((ip->daddr&ipinfo->dmsk.s_addr) != ipinfo->dst.s_addr,
IPT_INV_DSTIP)) {
/********************/
cb: 0f b6 4e 53 movzbl 0x53(%esi),%ecx
cf: f6 c1 08 test $0x8,%cl
d2: 0f 84 af 01 00 00 je 287 <ipt_do_table+0x25d>
d8: 8b 46 08 mov 0x8(%esi),%eax
db: 23 47 0c and 0xc(%edi),%eax
de: 3b 06 cmp (%esi),%eax
e0: 0f 84 b0 01 00 00 je 296 <ipt_do_table+0x26c>
e6: f6 c1 10 test $0x10,%cl
e9: 0f 84 d4 01 00 00 je 2c3 <ipt_do_table+0x299>
ef: 8b 46 0c mov 0xc(%esi),%eax
f2: 23 47 10 and 0x10(%edi),%eax
f5: 3b 46 04 cmp 0x4(%esi),%eax
f8: 0f 84 98 01 00 00 je 296 <ipt_do_table+0x26c>...
287: 8b 46 08 mov 0x8(%esi),%eax
28a: 23 47 0c and 0xc(%edi),%eax
28d: 3b 06 cmp (%esi),%eax
28f: 2e 0f 84 50 fe ff ff je,pn e6 <ipt_do_table+0xbc>
296: 0f b7 46 5a movzwl 0x5a(%esi),%eax
29a: 01 c6 add %eax,%esi
29c: 8b 4d f0 mov 0xfffffff0(%ebp),%ecx
29f: 85 c9 test %ecx,%ecx
2a1: 0f 84 24 fe ff ff je cb <ipt_do_table+0xa1>...
2c3: 8b 46 0c mov 0xc(%esi),%eax
2c6: 23 47 10 and 0x10(%edi),%eax
2c9: 3b 46 04 cmp 0x4(%esi),%eax
2cc: 75 c8 jne 296 <ipt_do_table+0x26c>
2ce: e9 2b fe ff ff jmp fe <ipt_do_table+0xd4>/******************/
As you can see, that a lot of conditional branches, that cannot be predicted
correctly by the cpu, unless consecutives iptables rules generate the same flow.
Eric
Re: [PATCH 2/3] netfilter : 3 patches to boost ip_tables performance
Quoted 8 lines · Eric Dumazet
Even without CMOV support, the suggested patch helps :
Here is the code generated with gcc-3.4.4 on a pentium4 (i686) for :
/********************/
bool1 = ((ip->saddr&ipinfo->smsk.s_addr) !=ipinfo->src.s_addr); bool1 ^= !!(ipinfo->invflags &IPT_INV_SRCIP);
Quoted 6 lines
bool2 = ((ip->daddr&ipinfo->dmsk.s_addr) !=
ipinfo->dst.s_addr); bool2 ^= !!(ipinfo->invflags &IPT_INV_DSTIP);if ((bool1 | bool2) != 0) {
/********************/
(...)
I totally agree with your demonstration. It would be interesting to compare she same code on an architecture with more registers (eg: sparc). One of the reasons of bad optimization of such constructs on x86 seems to be the lack of registers for the number of variables and intermediate results. When you write the code like above, you show the workflow to the compiler (and I often use the same technique).
Regards,
Willy
Re: [PATCH 2/3] netfilter : 3 patches to boost ip_tables performance
In reply to Willy TarreauAFAIK, the usual reason gcc can generate better code when you write
bool = complex conditional
if (bool) ...
is because of C's short-circuit conditional evaluation rules for || and &&. By moving the complex expression to an assignment statement, you are telling gcc that it is okay and safe to evaluate every part of the expression. If gcc was smart about being able to prove that all parts of the complex expression where safe (no possibly null pointer references) and had no side effects, then it could generate the same code with
if (complex conditional) ...
However, there are many situations where even a magical compiler couldn't prove that there were no possible side effects, etc., and would have to generate multiple conditional branches to properly meet the short-circuit conditional evaluation rules for && and ||.
However, in the specific cases in this thread using FWINV without || and && operators, an optimizing compiler "should" be smart enough to generate more linear code for today's heavily pipelined CPUs. For now, I guess it's still the duty of the programmer to use coding style to force the compiler to generate more linear machine code.
Quoted 28 lines · Willy Tarreau
On Thu, Sep 22, 2005 at 03:05:50PM +0200, Eric Dumazet wrote:
(...)Quoted 8 lines
It was necessary to get the best code with gcc-3.4.4 on i386 and gcc-4.0.1 on
x86_64For example :
bool1 =
FWINV(ret != 0, IPT_INV_VIA_OUT);
if (bool1) {Quoted 7 lines
gives a better code than :
if (
FWINV(ret != 0, IPT_INV_VIA_OUT)) {(one less conditional branch)
Dont ask me why, it is shocking but true :(
I also noticed many times that gcc's optimization of "if (complex condition)" is rather poor and it's often better to put it in a variable before. I even remember that if you use an intermediate variable, it can often generate a CMOV instruction on processors which support it, while it produces cond tests and jumps without the variable. Generally speaking, if you want fast code, you have to write it as a long sequence of small instructions, just as if you were writing assembly. As you said, shocking but true.
BTW, cheers for your optimizations !
Quoted 8 lines
Regards,
Willy-
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Tim Mattox - [email protected]
http://homepage.mac.com/tmattox/
I'm a bright... http://www.the-brights.net/
PATCH 3/3 netfilter : 3 patches to boost ip_tables performance
In reply to Eric DumazetPatch 3/3 (please apply after Patch 2/3)
3) NUMA allocation.
Part of the performance problem we have with netfilter is memory allocation is not NUMA aware, but 'only' SMP aware (ie each CPU normally touch separate cache lines)
Even with small iptables rules, the cost of this misplacement can be high
on common workloads.
Instead of using one vmalloc() area (located in the node of the iptables process), we now allocate an area for each possible CPU, using NUMA policy (MPOL_PREFERRED) so that memory should be allocated in the CPU's node if possible.
If the size of ipt_table is small enough (less than one page), we use kmalloc_node() instead of vmalloc(), to use less memory and less TLB entries) in small setups.
Please note that this patch doesnt change the number of allocated bytes, only the location of allocated zones.
Note2 : This patch depends on another patch that declares sys_set_mempolicy() in include/linux/syscalls.h
( http://marc.theaimsgroup.com/?l=linux-kernel&m=112725288622984&w=2 )
net/ipv4/netfilter/ip_tables.c +140 -67
@@ -17,6 +17,8 @@#include <linux/skbuff.h> #include <linux/kmod.h> #include <linux/vmalloc.h> +#include <linux/mempolicy.h> +#include <linux/syscalls.h> #include <linux/netdevice.h> #include <linux/module.h> #include <linux/tcp.h>
@@ -82,11 +84,6 @@context stops packets coming through and allows user context to read the counters or update the rules. -To be cache friendly on SMP, we arrange them like so: -[ n-entries ] -... cache-align padding ... -[ n-entries ] -Hence the start of any table is given by get_table() below. */ /* The table itself */
@@ -104,7 +101,7 @@unsigned int underflow[NF_IP_NUMHOOKS ];/* ipt_entry tables: one per CPU */ -char entries[0 ]____cacheline_aligned ; +void *entries[NR_CPUS ]; };static LIST_HEAD (ipt_target);
@@ -113,12 +110,6 @@#define SET_COUNTER (c,b,p) do { (c).bcnt = (b); (c).pcnt = (p); } while(0)#define ADD_COUNTER (c,b,p) do { (c).bcnt += (b); (c).pcnt += (p); } while(0) -#ifdef CONFIG_SMP -#define TABLE_OFFSET (t,p) (SMP_ALIGN((t)->size)*(p)) -#else -#define TABLE_OFFSET (t,p) 0 -#endif -#if 0 #define down (x) do { printk("DOWN:%u:" #x "\n", __LINE__); down(x); } while(0)#define down_interruptible (x) ({ int __r; printk("DOWNi:%u:" #x "\n", __LINE__); __r = down_interruptible(x); if (__r != 0) printk("ABORT-DOWNi:%u\n", __LINE__); __r; })
@@ -331,8 +322,7 @@read_lock_bh (&table->lock);#endif IP_NF_ASSERT (table->valid_hooks & (1 << hook)); - table_base = (void *)table->private->entries - +TABLE_OFFSET (table->private,smp_processor_id ()); + table_base = (void *)table->private->entries[smp_processor_id ()]; e =get_entry (table_base, table->private->hook_entry[hook]);#ifdef CONFIG_NETFILTER_DEBUG
@@ -608,7 +598,7 @@/* Figures out from what hook each rule can be called: returns 0 if there are loops. Puts hook bitmask in comefrom. */ static int -mark_source_chains (struct ipt_table_info *newinfo,unsigned int valid_hooks) +mark_source_chains (struct ipt_table_info *newinfo,unsigned int valid_hooks,void *entry0) {unsigned int hook;
@@ -617,7 +607,7 @@for (hook =0 ; hook <NF_IP_NUMHOOKS ; hook++) {unsigned int pos = newinfo->hook_entry[hook];struct ipt_entry *e - = (struct ipt_entry *)(newinfo->entries + pos); + = (struct ipt_entry *)(entry0 + pos);if (!(valid_hooks & (1 << hook)))continue ;
@@ -667,13 +657,13 @@ goto next; e = (struct ipt_entry *) - (newinfo->entries + pos); + (entry0 + pos); }while (oldpos == pos + e->next_offset);/* Move along one */ size = e->next_offset; e = (struct ipt_entry *) - (newinfo->entries + pos + size); + (entry0 + pos + size); e->counters.pcnt = pos; pos += size; }else {
@@ -690,7 +680,7 @@ newpos = pos + e->next_offset; } e = (struct ipt_entry *) - (newinfo->entries + newpos); + (entry0 + newpos); e->counters.pcnt = pos; pos = newpos; }
@@ -900,6 +890,7 @@ translate_table(const char *name, unsigned int valid_hooks, struct ipt_table_info *newinfo, + void *entry0, unsigned int size, unsigned int number, const unsigned int *hook_entries,
@@ -920,11 +911,11 @@duprintf ("translate_table: size %u\n" , newinfo->size); i =0 ;/* Walk through entries, checking offsets. */ - ret =IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, + ret =IPT_ENTRY_ITERATE (entry0, newinfo->size, check_entry_size_and_hooks, newinfo, - newinfo->entries, - newinfo->entries + size, + entry0, + entry0 + size, hook_entries, underflows, &i);if (ret !=0 )return ret;
@@ -952,25 +943,24 @@ } } -if (!mark_source_chains (newinfo, valid_hooks)) +if (!mark_source_chains (newinfo, valid_hooks, entry0))return -ELOOP ;/* Finally, each sanity check must pass */ i =0 ; - ret =IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, + ret =IPT_ENTRY_ITERATE (entry0, newinfo->size, check_entry, name, size, &i);if (ret !=0 ) { -IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, +IPT_ENTRY_ITERATE (entry0, newinfo->size, cleanup_entry, &i);return ret; }/* And one copy for every other CPU */ -for (i =1 ; i <num_possible_cpus (); i++) { -memcpy (newinfo->entries +SMP_ALIGN (newinfo->size)*i, - newinfo->entries, -SMP_ALIGN (newinfo->size)); +for_each_cpu (i) { +if (newinfo->entries[i] && newinfo->entries[i] != entry0) +memcpy (newinfo->entries[i], entry0, newinfo->size); }return ret;
@@ -1010,15 +1000,12 @@#ifdef CONFIG_NETFILTER_DEBUG { -struct ipt_entry *table_base; -unsigned int i; +int cpu; -for (i =0 ; i <num_possible_cpus (); i++) { - table_base = - (void *)newinfo->entries - +TABLE_OFFSET (newinfo, i); - - table_base->comefrom =0xdead57ac ; +for_each_cpu (cpu) { +struct ipt_entry *table_base = newinfo->entries[cpu]; +if (table_base) + table_base->comefrom =0xdead57ac ; } }#endif
@@ -1083,7 +1070,7 @@if (table)write_lock_bh (per_cpu_ptr (table->lock_p, curcpu)); i =0 ; - IPT_ENTRY_ITERATE(t->entries + TABLE_OFFSET(t, curcpu), + IPT_ENTRY_ITERATE(t->entries[curcpu], t->size, set_entry_to_counter, counters,
@@ -1097,7 +1084,7 @@if (table)write_lock_bh (per_cpu_ptr (table->lock_p, cpu)); i =0 ; - IPT_ENTRY_ITERATE(t->entries + TABLE_OFFSET(t, cpu), + IPT_ENTRY_ITERATE(t->entries[cpu], t->size, add_entry_to_counter, counters,
@@ -1110,7 +1097,7 @@if (table)write_lock_bh (&table->lock); i =0 ; - IPT_ENTRY_ITERATE(t->entries, + IPT_ENTRY_ITERATE(t->entries[0], t->size, set_entry_to_counter, counters,
@@ -1129,6 +1116,7 @@struct ipt_entry *e;struct ipt_counters *counters;int ret =0 ; +void *loc_cpu_entry;/* We need atomic snapshot of counters: rest doesn't change (other than comefrom, which userspace doesn't care
@@ -1142,8 +1130,12 @@/* First, sum counters... */ get_counters (table, table->private, counters); -/* ... then copy entire thing from CPU 0... */ -if (copy_to_user (userptr, table->private->entries, total_size) !=0 ) { +/* +* choose the copy that is on our node/cpu, +*/ + loc_cpu_entry = table->private->entries[get_cpu ()]; +/* ... then copy entire thing ... */ +if (copy_to_user (userptr, loc_cpu_entry, total_size) !=0 ) { ret = -EFAULT ; goto free_counters; }
@@ -1155,7 +1147,7 @@struct ipt_entry_match *m;struct ipt_entry_target *t; - e = (struct ipt_entry *)(table->private->entries + off); + e = (struct ipt_entry *)(loc_cpu_entry + off); if (copy_to_user(userptr + off + offsetof(struct ipt_entry, counters), &counters[num],
@@ -1192,6 +1184,7 @@ } free_counters: +put_cpu ();vfree (counters);return ret; }
@@ -1224,6 +1217,60 @@return ret; } +static void free_table_info (struct ipt_table_info *info) +{ +int cpu; +for_each_cpu (cpu) { +if (info->size <=PAGE_SIZE ) +kfree (info->entries[cpu]); +else +vfree (info->entries[cpu]); + } +kfree (info); +} + +static struct ipt_table_info *alloc_table_info (unsigned int size) +{ +struct ipt_table_info *newinfo; +int cpu; + newinfo =kzalloc (sizeof (struct ipt_table_info ),GFP_KERNEL ); +if (!newinfo) +return NULL ; + newinfo->size = size; +for_each_cpu (cpu) { +if (size <=PAGE_SIZE ) { + newinfo->entries[cpu] =kmalloc_node (size, +GFP_KERNEL , +cpu_to_node (cpu)); + }else { +#ifdef CONFIG_NUMA +struct mempolicy *oldpol; +mm_segment_t oldfs =get_fs (); +DECLARE_BITMAP (mynode,MAX_NUMNODES ); + + oldpol = current->mempolicy; +mpol_get (oldpol); +bitmap_zero (mynode,MAX_NUMNODES ); +set_bit (cpu_to_node (cpu), mynode); +set_fs (KERNEL_DS ); +sys_set_mempolicy (MPOL_PREFERRED , mynode,MAX_NUMNODES ); +set_fs (oldfs); +#endif + newinfo->entries[cpu] =vmalloc (size); +#ifdef CONFIG_NUMA +mpol_free (current->mempolicy); + current->mempolicy = oldpol; +#endif + } +if (newinfo->entries[cpu] ==0 ) { +free_table_info (newinfo); +return NULL ; + } + } +return newinfo; +} + +static int do_replace (void __user *user,unsigned int len) {
@@ -1232,6 +1279,7 @@struct ipt_table *t;struct ipt_table_info *newinfo, *oldinfo;struct ipt_counters *counters; +void *loc_cpu_entry, *loc_cpu_old_entry;if (copy_from_user (&tmp, user,sizeof (tmp)) !=0 )return -EFAULT ;
@@ -1244,12 +1292,14 @@if ((SMP_ALIGN (tmp.size) >>PAGE_SHIFT ) +2 > num_physpages)return -ENOMEM ; - newinfo =vmalloc (sizeof (struct ipt_table_info ) - +SMP_ALIGN (tmp.size) *num_possible_cpus ()); + newinfo =alloc_table_info (tmp.size);if (!newinfo)return -ENOMEM ; - -if (copy_from_user (newinfo->entries, user +sizeof (tmp), +/* +* choose the copy that is on our node/cpu +*/ + loc_cpu_entry = newinfo->entries[get_cpu ()]; +if (copy_from_user (loc_cpu_entry, user +sizeof (tmp), tmp.size) !=0 ) { ret = -EFAULT ; goto free_newinfo;
@@ -1260,10 +1310,9 @@ ret = -ENOMEM ; goto free_newinfo; } -memset (counters,0 , tmp.num_counters *sizeof (struct ipt_counters )); ret =translate_table (tmp.name, tmp.valid_hooks, - newinfo, tmp.size, tmp.num_entries, + newinfo, loc_cpu_entry, tmp.size, tmp.num_entries, tmp.hook_entry, tmp.underflow);if (ret !=0 ) goto free_newinfo_counters;
@@ -1302,8 +1351,10 @@/* Get the old counters. */ get_counters (NULL , oldinfo, counters);/* Decrease module usage counts and free resource */ -IPT_ENTRY_ITERATE (oldinfo->entries, oldinfo->size, cleanup_entry,NULL ); -vfree (oldinfo); + loc_cpu_old_entry = oldinfo->entries[smp_processor_id ()]; +IPT_ENTRY_ITERATE (loc_cpu_old_entry, oldinfo->size, cleanup_entry,NULL ); +put_cpu (); +free_table_info (oldinfo);if (copy_to_user (tmp.counters, counters,sizeof (struct ipt_counters ) * tmp.num_counters) !=0 ) ret = -EFAULT ;
@@ -1315,11 +1366,12 @@module_put (t->me);up (&ipt_mutex); free_newinfo_counters_untrans: -IPT_ENTRY_ITERATE (newinfo->entries, newinfo->size, cleanup_entry,NULL ); +IPT_ENTRY_ITERATE (loc_cpu_entry, newinfo->size, cleanup_entry,NULL ); free_newinfo_counters:vfree (counters); free_newinfo: -vfree (newinfo); +put_cpu (); +free_table_info (newinfo);return ret; }
@@ -1352,6 +1404,7 @@struct ipt_counters_info tmp, *paddc;struct ipt_table *t;int ret =0 ; +void *loc_cpu_entry;#ifdef CONFIG_SMP rwlock_t *lockp;#endif
@@ -1389,7 +1442,11 @@ } i =0 ; - IPT_ENTRY_ITERATE(t->private->entries, +/* +* choose the copy that is on our node, +*/ + loc_cpu_entry = t->private->entries[smp_processor_id ()]; + IPT_ENTRY_ITERATE(loc_cpu_entry, t->private->size, add_counter_to_entry, paddc->counters,
@@ -1584,8 +1641,15 @@ {int ret;struct ipt_table_info *newinfo; -static struct ipt_table_info bootstrap - = {0 ,0 ,0 , {0 }, {0 }, { } }; +static struct ipt_table_info bootstrap = { + .size =0 , + .number =0 , + .initial_entries =0 , + .hook_entry = {0 }, + .underflow = {0 }, + .entries = {NULL } + }; +void *loc_cpu_entry;#ifdef CONFIG_SMP int cpu;if (!table->lock_p) {
@@ -1597,26 +1661,30 @@#else rwlock_init (&table->lock);#endif - newinfo =vmalloc (sizeof (struct ipt_table_info ) - +SMP_ALIGN (repl->size) *num_possible_cpus ()); + + newinfo =alloc_table_info (repl->size);if (!newinfo)return -ENOMEM ; - -memcpy (newinfo->entries, repl->entries, repl->size); +/* +* choose the copy that is on our node/cpu +*/ + loc_cpu_entry = newinfo->entries[get_cpu ()]; +memcpy (loc_cpu_entry, repl->entries, repl->size); ret =translate_table (table->name, table->valid_hooks, - newinfo, repl->size, + newinfo, loc_cpu_entry, repl->size, repl->num_entries, repl->hook_entry, repl->underflow); +put_cpu ();if (ret !=0 ) { -vfree (newinfo); +free_table_info (newinfo);return ret; } ret =down_interruptible (&ipt_mutex);if (ret !=0 ) { -vfree (newinfo); +free_table_info (newinfo);return ret; }
@@ -1644,20 +1712,25 @@return ret; free_unlock: -vfree (newinfo); +free_table_info (newinfo); goto unlock; }void ipt_unregister_table (struct ipt_table *table) { +void *loc_cpu_entry;down (&ipt_mutex);LIST_DELETE (&ipt_tables, table);up (&ipt_mutex); -/* Decrease module usage counts and free resources */ -IPT_ENTRY_ITERATE (table->private->entries, table->private->size, +/* Decrease module usage counts and free resources +* choose the copy that is on our node/cpu +*/ + loc_cpu_entry = table->private->entries[get_cpu ()]; +IPT_ENTRY_ITERATE (loc_cpu_entry, table->private->size, cleanup_entry,NULL ); -vfree (table->private); +put_cpu (); +free_table_info (table->private);#ifdef CONFIG_SMP free_percpu (table->lock_p); table->lock_p =NULL ;
Re: [PATCH 3/3] netfilter : 3 patches to boost ip_tables performance
Quoted 1 line · Eric Dumazet
Patch 3/3 (please apply after Patch 2/3)
Thanks for your patches, again.
Quoted 1 line
3) NUMA allocation.
I'll wait with applying this patch until a decision has been made on merging something like vmalloc_node() to mainline.
Signature · 5 lines
- Harald Welte <[email protected]> http://netfilter.org/ ============================================================================ "Fragmentation is like classful addressing -- an interesting early architectural error that shows how much experimentation was going on while IP was being designed." -- Paul Vixie