[PATCH 0/5] iptables lockless receive (v0.3)

[PATCH 0/5] iptables lockless receive (v0.3)

[Stephen Hemminger]

Another experimental version of iptables locking that removes
reader/writer lock overhead. This version incorprates suggestion
of moving counter to table_info.  I haven't done ebtables, and don't
know if it is worth bothering.

-- 

[PATCH 1/5] netfilter: change elements in x_tables

[Stephen Hemminger]

[-- Attachment #1: x_tables.patch --]
[-- Type: text/plain, Size: 1062 bytes --]

Change to proper type on private pointer rather than anonymous void.
Keep active elements on same cache line.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
 include/linux/netfilter/x_tables.h |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

--- a/include/linux/netfilter/x_tables.h	2009-01-26 17:24:43.251543415 -0800
+++ b/include/linux/netfilter/x_tables.h	2009-01-26 17:29:12.510649107 -0800
@@ -349,9 +349,6 @@ struct xt_table
 {
 	struct list_head list;
 
-	/* A unique name... */
-	const char name[XT_TABLE_MAXNAMELEN];
-
 	/* What hooks you will enter on */
 	unsigned int valid_hooks;
 
@@ -359,13 +356,15 @@ struct xt_table
 	rwlock_t lock;
 
 	/* Man behind the curtain... */
-	//struct ip6t_table_info *private;
-	void *private;
+	struct xt_table_info *private;
 
 	/* Set this to THIS_MODULE if you are a module, otherwise NULL */
 	struct module *me;
 
 	u_int8_t af;		/* address/protocol family */
+
+	/* A unique name... */
+	const char name[XT_TABLE_MAXNAMELEN];
 };
 
 #include <linux/netfilter_ipv4.h>

-- 

[PATCH 2/5] netfilter: remove unneeded initializations

[Stephen Hemminger]

[-- Attachment #1: iptables-lock-init.patch --]
[-- Type: text/plain, Size: 4593 bytes --]

Later patches change the locking on xt_table and the initialization of
the lock element is not needed since the lock is always initialized in
xt_table_register anyway.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
 net/ipv4/netfilter/arptable_filter.c   |    2 --
 net/ipv4/netfilter/iptable_filter.c    |    1 -
 net/ipv4/netfilter/iptable_mangle.c    |    1 -
 net/ipv4/netfilter/iptable_raw.c       |    1 -
 net/ipv4/netfilter/iptable_security.c  |    1 -
 net/ipv4/netfilter/nf_nat_rule.c       |    1 -
 net/ipv6/netfilter/ip6table_filter.c   |    1 -
 net/ipv6/netfilter/ip6table_mangle.c   |    1 -
 net/ipv6/netfilter/ip6table_raw.c      |    1 -
 net/ipv6/netfilter/ip6table_security.c |    1 -
 10 files changed, 11 deletions(-)

--- a/net/ipv4/netfilter/arptable_filter.c	2009-01-26 17:24:43.687542005 -0800
+++ b/net/ipv4/netfilter/arptable_filter.c	2009-01-26 19:50:37.891042244 -0800
@@ -48,8 +48,6 @@ static struct
 static struct xt_table packet_filter = {
 	.name		= "filter",
 	.valid_hooks	= FILTER_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(packet_filter.lock),
-	.private	= NULL,
 	.me		= THIS_MODULE,
 	.af		= NFPROTO_ARP,
 };
--- a/net/ipv4/netfilter/iptable_filter.c	2009-01-26 17:24:43.691541994 -0800
+++ b/net/ipv4/netfilter/iptable_filter.c	2009-01-26 19:50:37.891042244 -0800
@@ -56,7 +56,6 @@ static struct
 static struct xt_table packet_filter = {
 	.name		= "filter",
 	.valid_hooks	= FILTER_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(packet_filter.lock),
 	.me		= THIS_MODULE,
 	.af		= AF_INET,
 };
--- a/net/ipv4/netfilter/iptable_raw.c	2009-01-26 17:24:43.691541994 -0800
+++ b/net/ipv4/netfilter/iptable_raw.c	2009-01-26 19:50:37.891042244 -0800
@@ -39,7 +39,6 @@ static struct
 static struct xt_table packet_raw = {
 	.name = "raw",
 	.valid_hooks =  RAW_VALID_HOOKS,
-	.lock = __RW_LOCK_UNLOCKED(packet_raw.lock),
 	.me = THIS_MODULE,
 	.af = AF_INET,
 };
--- a/net/ipv4/netfilter/iptable_security.c	2009-01-26 17:24:43.691541994 -0800
+++ b/net/ipv4/netfilter/iptable_security.c	2009-01-26 19:50:37.891042244 -0800
@@ -60,7 +60,6 @@ static struct
 static struct xt_table security_table = {
 	.name		= "security",
 	.valid_hooks	= SECURITY_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(security_table.lock),
 	.me		= THIS_MODULE,
 	.af		= AF_INET,
 };
--- a/net/ipv4/netfilter/nf_nat_rule.c	2009-01-26 17:24:43.695541481 -0800
+++ b/net/ipv4/netfilter/nf_nat_rule.c	2009-01-26 19:51:20.338030618 -0800
@@ -61,7 +61,6 @@ static struct
 static struct xt_table nat_table = {
 	.name		= "nat",
 	.valid_hooks	= NAT_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(nat_table.lock),
 	.me		= THIS_MODULE,
 	.af		= AF_INET,
 };
--- a/net/ipv6/netfilter/ip6table_filter.c	2009-01-26 17:24:43.735541493 -0800
+++ b/net/ipv6/netfilter/ip6table_filter.c	2009-01-26 19:50:37.895044361 -0800
@@ -54,7 +54,6 @@ static struct
 static struct xt_table packet_filter = {
 	.name		= "filter",
 	.valid_hooks	= FILTER_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(packet_filter.lock),
 	.me		= THIS_MODULE,
 	.af		= AF_INET6,
 };
--- a/net/ipv6/netfilter/ip6table_mangle.c	2009-01-26 17:24:43.735541493 -0800
+++ b/net/ipv6/netfilter/ip6table_mangle.c	2009-01-26 19:50:37.895044361 -0800
@@ -60,7 +60,6 @@ static struct
 static struct xt_table packet_mangler = {
 	.name		= "mangle",
 	.valid_hooks	= MANGLE_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(packet_mangler.lock),
 	.me		= THIS_MODULE,
 	.af		= AF_INET6,
 };
--- a/net/ipv6/netfilter/ip6table_raw.c	2009-01-26 17:24:43.735541493 -0800
+++ b/net/ipv6/netfilter/ip6table_raw.c	2009-01-26 19:50:37.895044361 -0800
@@ -38,7 +38,6 @@ static struct
 static struct xt_table packet_raw = {
 	.name = "raw",
 	.valid_hooks = RAW_VALID_HOOKS,
-	.lock = __RW_LOCK_UNLOCKED(packet_raw.lock),
 	.me = THIS_MODULE,
 	.af = AF_INET6,
 };
--- a/net/ipv6/netfilter/ip6table_security.c	2009-01-26 17:24:43.735541493 -0800
+++ b/net/ipv6/netfilter/ip6table_security.c	2009-01-26 19:50:37.895044361 -0800
@@ -59,7 +59,6 @@ static struct
 static struct xt_table security_table = {
 	.name		= "security",
 	.valid_hooks	= SECURITY_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(security_table.lock),
 	.me		= THIS_MODULE,
 	.af		= AF_INET6,
 };
--- a/net/ipv4/netfilter/iptable_mangle.c	2009-01-26 17:24:43.691541994 -0800
+++ b/net/ipv4/netfilter/iptable_mangle.c	2009-01-26 19:50:37.895044361 -0800
@@ -67,7 +67,6 @@ static struct
 static struct xt_table packet_mangler = {
 	.name		= "mangle",
 	.valid_hooks	= MANGLE_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(packet_mangler.lock),
 	.me		= THIS_MODULE,
 	.af		= AF_INET,
 };

-- 

[PATCH 3/5] ebtables: remove unneeded initializations

[Stephen Hemminger]

[-- Attachment #1: ebtables-lock-init.patch --]
[-- Type: text/plain, Size: 1537 bytes --]

The initialization of the lock element is not needed
since the lock is always initialized in ebt_register_table.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
 net/bridge/netfilter/ebtable_broute.c |    1 -
 net/bridge/netfilter/ebtable_filter.c |    1 -
 net/bridge/netfilter/ebtable_nat.c    |    1 -
 3 files changed, 3 deletions(-)

--- a/net/bridge/netfilter/ebtable_broute.c	2009-01-27 17:09:10.313100854 -0800
+++ b/net/bridge/netfilter/ebtable_broute.c	2009-01-27 17:09:15.862142852 -0800
@@ -46,7 +46,6 @@ static struct ebt_table broute_table =
 	.name		= "broute",
 	.table		= &initial_table,
 	.valid_hooks	= 1 << NF_BR_BROUTING,
-	.lock		= __RW_LOCK_UNLOCKED(broute_table.lock),
 	.check		= check,
 	.me		= THIS_MODULE,
 };
--- a/net/bridge/netfilter/ebtable_filter.c	2009-01-27 17:08:50.725100955 -0800
+++ b/net/bridge/netfilter/ebtable_filter.c	2009-01-27 17:08:53.828611768 -0800
@@ -55,7 +55,6 @@ static struct ebt_table frame_filter =
 	.name		= "filter",
 	.table		= &initial_table,
 	.valid_hooks	= FILTER_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(frame_filter.lock),
 	.check		= check,
 	.me		= THIS_MODULE,
 };
--- a/net/bridge/netfilter/ebtable_nat.c	2009-01-27 17:09:22.896602465 -0800
+++ b/net/bridge/netfilter/ebtable_nat.c	2009-01-27 17:09:31.589085328 -0800
@@ -55,7 +55,6 @@ static struct ebt_table frame_nat =
 	.name		= "nat",
 	.table		= &initial_table,
 	.valid_hooks	= NAT_VALID_HOOKS,
-	.lock		= __RW_LOCK_UNLOCKED(frame_nat.lock),
 	.check		= check,
 	.me		= THIS_MODULE,
 };

-- 

[PATCH 4/5] netfilter: use sequence number synchronization for counters

[Stephen Hemminger]

[-- Attachment #1: counters-seqcount.patch --]
[-- Type: text/plain, Size: 9055 bytes --]

Change how synchronization is done on the iptables counters. Use seqcount
wrapper instead of depending on reader/writer lock.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
 include/linux/netfilter/x_tables.h |    3 +++
 net/ipv4/netfilter/arp_tables.c    |   24 +++++++++++++++++++-----
 net/ipv4/netfilter/ip_tables.c     |   24 +++++++++++++++++++-----
 net/ipv6/netfilter/ip6_tables.c    |   32 +++++++++++++++++++++++---------
 net/netfilter/x_tables.c           |   11 +++++++++++
 5 files changed, 75 insertions(+), 19 deletions(-)
4
--- a/net/ipv4/netfilter/arp_tables.c	2009-01-28 21:24:39.223991934 -0800
+++ b/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:13:16.423490077 -0800
@@ -230,6 +230,7 @@ unsigned int arpt_do_table(struct sk_buf
 	void *table_base;
 	const struct xt_table_info *private;
 	struct xt_target_param tgpar;
+	seqcount_t *seq;
 
 	if (!pskb_may_pull(skb, arp_hdr_len(skb->dev)))
 		return NF_DROP;
@@ -240,6 +241,7 @@ unsigned int arpt_do_table(struct sk_buf
 	read_lock_bh(&table->lock);
 	private = table->private;
 	table_base = (void *)private->entries[smp_processor_id()];
+	seq = per_cpu_ptr(private->seq, smp_processor_id());
 	e = get_entry(table_base, private->hook_entry[hook]);
 	back = get_entry(table_base, private->underflow[hook]);
 
@@ -256,7 +258,9 @@ unsigned int arpt_do_table(struct sk_buf
 
 			hdr_len = sizeof(*arp) + (2 * sizeof(struct in_addr)) +
 				(2 * skb->dev->addr_len);
+			write_seqcount_begin(seq);
 			ADD_COUNTER(e->counters, hdr_len, 1);
+			write_seqcount_end(seq);
 
 			t = arpt_get_target(e);
 
@@ -662,10 +666,20 @@ static int translate_table(const char *n
 
 /* Gets counters. */
 static inline int add_entry_to_counter(const struct arpt_entry *e,
+				       seqcount_t *seq,
 				       struct xt_counters total[],
 				       unsigned int *i)
 {
-	ADD_COUNTER(total[*i], e->counters.bcnt, e->counters.pcnt);
+	struct xt_counters count;
+	unsigned int start;
+
+	/* Atomic fetch */
+	do {
+		start = read_seqcount_begin(seq);
+		count = e->counters;
+	} while (read_seqcount_retry(seq, start));
+
+	ADD_COUNTER(total[*i], count.bcnt, count.pcnt);
 
 	(*i)++;
 	return 0;
@@ -709,6 +723,7 @@ static void get_counters(const struct xt
 		ARPT_ENTRY_ITERATE(t->entries[cpu],
 				   t->size,
 				   add_entry_to_counter,
+				   per_cpu_ptr(t->seq, cpu),
 				   counters,
 				   &i);
 	}
@@ -731,9 +746,9 @@ static inline struct xt_counters *alloc_
 		return ERR_PTR(-ENOMEM);
 
 	/* First, sum counters... */
-	write_lock_bh(&table->lock);
+	local_bh_disable();
 	get_counters(private, counters);
-	write_unlock_bh(&table->lock);
+	local_bh_enable();
 
 	return counters;
 }
@@ -1736,8 +1751,7 @@ struct xt_table *arpt_register_table(str
 {
 	int ret;
 	struct xt_table_info *newinfo;
-	struct xt_table_info bootstrap
-		= { 0, 0, 0, { 0 }, { 0 }, { } };
+	struct xt_table_info bootstrap = { 0 };
 	void *loc_cpu_entry;
 	struct xt_table *new_table;
 
--- a/net/ipv4/netfilter/ip_tables.c	2009-01-28 21:24:39.211990658 -0800
+++ b/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:06:10.596739805 -0800
@@ -327,6 +327,7 @@ ipt_do_table(struct sk_buff *skb,
 	struct xt_table_info *private;
 	struct xt_match_param mtpar;
 	struct xt_target_param tgpar;
+	seqcount_t *seq;
 
 	/* Initialization */
 	ip = ip_hdr(skb);
@@ -351,6 +352,7 @@ ipt_do_table(struct sk_buff *skb,
 	IP_NF_ASSERT(table->valid_hooks & (1 << hook));
 	private = table->private;
 	table_base = (void *)private->entries[smp_processor_id()];
+	seq = per_cpu_ptr(private->seq, smp_processor_id());
 	e = get_entry(table_base, private->hook_entry[hook]);
 
 	/* For return from builtin chain */
@@ -366,7 +368,9 @@ ipt_do_table(struct sk_buff *skb,
 			if (IPT_MATCH_ITERATE(e, do_match, skb, &mtpar) != 0)
 				goto no_match;
 
+			write_seqcount_begin(seq);
 			ADD_COUNTER(e->counters, ntohs(ip->tot_len), 1);
+			write_seqcount_end(seq);
 
 			t = ipt_get_target(e);
 			IP_NF_ASSERT(t->u.kernel.target);
@@ -872,10 +876,20 @@ translate_table(const char *name,
 /* Gets counters. */
 static inline int
 add_entry_to_counter(const struct ipt_entry *e,
+		     seqcount_t *seq,
 		     struct xt_counters total[],
 		     unsigned int *i)
 {
-	ADD_COUNTER(total[*i], e->counters.bcnt, e->counters.pcnt);
+	struct xt_counters count;
+	unsigned int start;
+
+	/* Atomic fetch */
+	do {
+		start = read_seqcount_begin(seq);
+		count = e->counters;
+	} while (read_seqcount_retry(seq, start));
+
+	ADD_COUNTER(total[*i], count.bcnt, count.pcnt);
 
 	(*i)++;
 	return 0;
@@ -921,6 +935,7 @@ get_counters(const struct xt_table_info 
 		IPT_ENTRY_ITERATE(t->entries[cpu],
 				  t->size,
 				  add_entry_to_counter,
+				  per_cpu_ptr(t->seq, cpu),
 				  counters,
 				  &i);
 	}
@@ -942,9 +957,9 @@ static struct xt_counters * alloc_counte
 		return ERR_PTR(-ENOMEM);
 
 	/* First, sum counters... */
-	write_lock_bh(&table->lock);
+	local_bh_disable();
 	get_counters(private, counters);
-	write_unlock_bh(&table->lock);
+	local_bh_enable();
 
 	return counters;
 }
@@ -2064,8 +2079,7 @@ struct xt_table *ipt_register_table(stru
 {
 	int ret;
 	struct xt_table_info *newinfo;
-	struct xt_table_info bootstrap
-		= { 0, 0, 0, { 0 }, { 0 }, { } };
+	struct xt_table_info bootstrap = { 0 };
 	void *loc_cpu_entry;
 	struct xt_table *new_table;
 
--- a/net/ipv6/netfilter/ip6_tables.c	2009-01-28 21:24:39.243992135 -0800
+++ b/net/ipv6/netfilter/ip6_tables.c	2009-01-28 22:13:16.419490741 -0800
@@ -357,6 +357,7 @@ ip6t_do_table(struct sk_buff *skb,
 	struct xt_table_info *private;
 	struct xt_match_param mtpar;
 	struct xt_target_param tgpar;
+	seqcount_t *seq;
 
 	/* Initialization */
 	indev = in ? in->name : nulldevname;
@@ -377,6 +378,7 @@ ip6t_do_table(struct sk_buff *skb,
 	IP_NF_ASSERT(table->valid_hooks & (1 << hook));
 	private = table->private;
 	table_base = (void *)private->entries[smp_processor_id()];
+	seq = per_cpu_ptr(private->seq, smp_processor_id());
 	e = get_entry(table_base, private->hook_entry[hook]);
 
 	/* For return from builtin chain */
@@ -392,9 +394,11 @@ ip6t_do_table(struct sk_buff *skb,
 			if (IP6T_MATCH_ITERATE(e, do_match, skb, &mtpar) != 0)
 				goto no_match;
 
+			write_seqcount_begin(seq);
 			ADD_COUNTER(e->counters,
 				    ntohs(ipv6_hdr(skb)->payload_len) +
 				    sizeof(struct ipv6hdr), 1);
+			write_seqcount_end(seq);
 
 			t = ip6t_get_target(e);
 			IP_NF_ASSERT(t->u.kernel.target);
@@ -901,11 +905,21 @@ translate_table(const char *name,
 /* Gets counters. */
 static inline int
 add_entry_to_counter(const struct ip6t_entry *e,
+		     seqcount_t *seq,
 		     struct xt_counters total[],
 		     unsigned int *i)
 {
-	ADD_COUNTER(total[*i], e->counters.bcnt, e->counters.pcnt);
 
+	struct xt_counters count;
+	unsigned int start;
+
+	/* Atomic fetch */
+	do {
+		start = read_seqcount_begin(seq);
+		count = e->counters;
+	} while (read_seqcount_retry(seq, start));
+
+	ADD_COUNTER(total[*i], count.bcnt, count.pcnt);
 	(*i)++;
 	return 0;
 }
@@ -948,10 +962,11 @@ get_counters(const struct xt_table_info 
 			continue;
 		i = 0;
 		IP6T_ENTRY_ITERATE(t->entries[cpu],
-				  t->size,
-				  add_entry_to_counter,
-				  counters,
-				  &i);
+				   t->size,
+				   add_entry_to_counter,
+				   per_cpu_ptr(t->seq, cpu),
+				   counters,
+				   &i);
 	}
 }
 
@@ -971,9 +986,9 @@ static struct xt_counters *alloc_counter
 		return ERR_PTR(-ENOMEM);
 
 	/* First, sum counters... */
-	write_lock_bh(&table->lock);
+	local_bh_disable();
 	get_counters(private, counters);
-	write_unlock_bh(&table->lock);
+	local_bh_enable();
 
 	return counters;
 }
@@ -2094,8 +2109,7 @@ struct xt_table *ip6t_register_table(str
 {
 	int ret;
 	struct xt_table_info *newinfo;
-	struct xt_table_info bootstrap
-		= { 0, 0, 0, { 0 }, { 0 }, { } };
+	struct xt_table_info bootstrap =  { 0 };
 	void *loc_cpu_entry;
 	struct xt_table *new_table;
 
--- a/net/netfilter/x_tables.c	2009-01-28 21:39:17.644495623 -0800
+++ b/net/netfilter/x_tables.c	2009-01-28 22:14:33.143990681 -0800
@@ -591,8 +591,18 @@ struct xt_table_info *xt_alloc_table_inf
 		return NULL;
 
 	newinfo->size = size;
+	newinfo->seq = alloc_percpu(seqcount_t);
+	if (!newinfo->seq) {
+		kfree(newinfo);
+		return NULL;
+	}
+
 
 	for_each_possible_cpu(cpu) {
+		seqcount_t *cnt = per_cpu_ptr(newinfo->seq, cpu);
+
+		seqcount_init(cnt);
+
 		if (size <= PAGE_SIZE)
 			newinfo->entries[cpu] = kmalloc_node(size,
 							GFP_KERNEL,
@@ -621,6 +631,7 @@ void xt_free_table_info(struct xt_table_
 		else
 			vfree(info->entries[cpu]);
 	}
+	free_percpu(info->seq);
 	kfree(info);
 }
 EXPORT_SYMBOL(xt_free_table_info);
--- a/include/linux/netfilter/x_tables.h	2009-01-28 21:35:12.044240843 -0800
+++ b/include/linux/netfilter/x_tables.h	2009-01-28 22:04:39.316517913 -0800
@@ -383,6 +383,9 @@ struct xt_table_info
 	unsigned int hook_entry[NF_INET_NUMHOOKS];
 	unsigned int underflow[NF_INET_NUMHOOKS];
 
+	/* Secret compartment */
+	seqcount_t *seq;
+
 	/* ipt_entry tables: one per CPU */
 	/* Note : this field MUST be the last one, see XT_TABLE_INFO_SZ */
 	char *entries[1];

-- 

[PATCH 5/5] netfilter: convert x_tables to use RCU

[Stephen Hemminger]

[-- Attachment #1: iptables-rcu.patch --]
[-- Type: text/plain, Size: 8238 bytes --]

Replace existing reader/writer lock with Read-Copy-Update to
elminate the overhead of a read lock on each incoming packet.
This should reduce the overhead of iptables especially on SMP
systems.

The previous code used a reader-writer lock for two purposes.
The first was to ensure that the xt_table_info reference was not in
process of being changed. Since xt_table_info is only freed via one
routine, it was a direct conversion to RCU.

The other use of the reader-writer lock was to to block changes
to counters while they were being read. This synchronization was
fixed by the previous patch.  But still need to make sure table info
isn't going away.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>


---
 include/linux/netfilter/x_tables.h |   10 ++++++-
 net/ipv4/netfilter/arp_tables.c    |   12 ++++-----
 net/ipv4/netfilter/ip_tables.c     |   12 ++++-----
 net/ipv6/netfilter/ip6_tables.c    |   12 ++++-----
 net/netfilter/x_tables.c           |   48 ++++++++++++++++++++++++++-----------
 5 files changed, 60 insertions(+), 34 deletions(-)

--- a/include/linux/netfilter/x_tables.h	2009-01-28 22:04:39.316517913 -0800
+++ b/include/linux/netfilter/x_tables.h	2009-01-28 22:14:54.648490491 -0800
@@ -352,8 +352,8 @@ struct xt_table
 	/* What hooks you will enter on */
 	unsigned int valid_hooks;
 
-	/* Lock for the curtain */
-	rwlock_t lock;
+	/* Lock for curtain */
+	spinlock_t lock;
 
 	/* Man behind the curtain... */
 	struct xt_table_info *private;
@@ -386,6 +386,12 @@ struct xt_table_info
 	/* Secret compartment */
 	seqcount_t *seq;
 
+	/* For the dustman... */
+	union {
+		struct rcu_head rcu;
+		struct work_struct work;
+	};
+
 	/* ipt_entry tables: one per CPU */
 	/* Note : this field MUST be the last one, see XT_TABLE_INFO_SZ */
 	char *entries[1];
--- a/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:13:16.423490077 -0800
+++ b/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:14:54.648490491 -0800
@@ -238,8 +238,8 @@ unsigned int arpt_do_table(struct sk_buf
 	indev = in ? in->name : nulldevname;
 	outdev = out ? out->name : nulldevname;
 
-	read_lock_bh(&table->lock);
-	private = table->private;
+	rcu_read_lock_bh();
+	private = rcu_dereference(table->private);
 	table_base = (void *)private->entries[smp_processor_id()];
 	seq = per_cpu_ptr(private->seq, smp_processor_id());
 	e = get_entry(table_base, private->hook_entry[hook]);
@@ -315,7 +315,7 @@ unsigned int arpt_do_table(struct sk_buf
 			e = (void *)e + e->next_offset;
 		}
 	} while (!hotdrop);
-	read_unlock_bh(&table->lock);
+	rcu_read_unlock_bh();
 
 	if (hotdrop)
 		return NF_DROP;
@@ -1163,8 +1163,8 @@ static int do_add_counters(struct net *n
 		goto free;
 	}
 
-	write_lock_bh(&t->lock);
-	private = t->private;
+	rcu_read_lock_bh();
+	private = rcu_dereference(t->private);
 	if (private->number != num_counters) {
 		ret = -EINVAL;
 		goto unlock_up_free;
@@ -1179,7 +1179,7 @@ static int do_add_counters(struct net *n
 			   paddc,
 			   &i);
  unlock_up_free:
-	write_unlock_bh(&t->lock);
+	rcu_read_unlock_bh();
 	xt_table_unlock(t);
 	module_put(t->me);
  free:
--- a/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:06:10.596739805 -0800
+++ b/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:14:54.648490491 -0800
@@ -348,9 +348,9 @@ ipt_do_table(struct sk_buff *skb,
 	mtpar.family  = tgpar.family = NFPROTO_IPV4;
 	tgpar.hooknum = hook;
 
-	read_lock_bh(&table->lock);
+	rcu_read_lock_bh();
 	IP_NF_ASSERT(table->valid_hooks & (1 << hook));
-	private = table->private;
+	private = rcu_dereference(table->private);
 	table_base = (void *)private->entries[smp_processor_id()];
 	seq = per_cpu_ptr(private->seq, smp_processor_id());
 	e = get_entry(table_base, private->hook_entry[hook]);
@@ -449,7 +449,7 @@ ipt_do_table(struct sk_buff *skb,
 		}
 	} while (!hotdrop);
 
-	read_unlock_bh(&table->lock);
+	rcu_read_unlock_bh();
 
 #ifdef DEBUG_ALLOW_ALL
 	return NF_ACCEPT;
@@ -1408,8 +1408,8 @@ do_add_counters(struct net *net, void __
 		goto free;
 	}
 
-	write_lock_bh(&t->lock);
-	private = t->private;
+	rcu_read_lock_bh();
+	private = rcu_dereference(t->private);
 	if (private->number != num_counters) {
 		ret = -EINVAL;
 		goto unlock_up_free;
@@ -1424,7 +1424,7 @@ do_add_counters(struct net *net, void __
 			  paddc,
 			  &i);
  unlock_up_free:
-	write_unlock_bh(&t->lock);
+	rcu_read_unlock_bh();
 	xt_table_unlock(t);
 	module_put(t->me);
  free:
--- a/net/ipv6/netfilter/ip6_tables.c	2009-01-28 22:13:16.419490741 -0800
+++ b/net/ipv6/netfilter/ip6_tables.c	2009-01-28 22:14:54.652490133 -0800
@@ -374,9 +374,9 @@ ip6t_do_table(struct sk_buff *skb,
 	mtpar.family  = tgpar.family = NFPROTO_IPV6;
 	tgpar.hooknum = hook;
 
-	read_lock_bh(&table->lock);
+	rcu_read_lock_bh();
 	IP_NF_ASSERT(table->valid_hooks & (1 << hook));
-	private = table->private;
+	private = rcu_dereference(table->private);
 	table_base = (void *)private->entries[smp_processor_id()];
 	seq = per_cpu_ptr(private->seq, smp_processor_id());
 	e = get_entry(table_base, private->hook_entry[hook]);
@@ -478,7 +478,7 @@ ip6t_do_table(struct sk_buff *skb,
 #ifdef CONFIG_NETFILTER_DEBUG
 	((struct ip6t_entry *)table_base)->comefrom = NETFILTER_LINK_POISON;
 #endif
-	read_unlock_bh(&table->lock);
+	rcu_read_unlock_bh();
 
 #ifdef DEBUG_ALLOW_ALL
 	return NF_ACCEPT;
@@ -1439,8 +1439,8 @@ do_add_counters(struct net *net, void __
 		goto free;
 	}
 
-	write_lock_bh(&t->lock);
-	private = t->private;
+	rcu_read_lock_bh();
+	private = rcu_dereference(t->private);
 	if (private->number != num_counters) {
 		ret = -EINVAL;
 		goto unlock_up_free;
@@ -1455,7 +1455,7 @@ do_add_counters(struct net *net, void __
 			  paddc,
 			  &i);
  unlock_up_free:
-	write_unlock_bh(&t->lock);
+	rcu_read_unlock_bh();
 	xt_table_unlock(t);
 	module_put(t->me);
  free:
--- a/net/netfilter/x_tables.c	2009-01-28 22:14:33.143990681 -0800
+++ b/net/netfilter/x_tables.c	2009-01-28 22:17:40.183990832 -0800
@@ -621,19 +621,39 @@ struct xt_table_info *xt_alloc_table_inf
 }
 EXPORT_SYMBOL(xt_alloc_table_info);
 
-void xt_free_table_info(struct xt_table_info *info)
+/* callback to do free for vmalloc'd case */
+static void xt_free_table_info_work(struct work_struct *arg)
 {
-	int cpu;
+	struct xt_table_info *info = container_of(arg, struct xt_table_info, work);
+	unsigned int cpu;
 
-	for_each_possible_cpu(cpu) {
-		if (info->size <= PAGE_SIZE)
-			kfree(info->entries[cpu]);
-		else
-			vfree(info->entries[cpu]);
-	}
-	free_percpu(info->seq);
+	for_each_possible_cpu(cpu)
+		vfree(info->entries[cpu]);
 	kfree(info);
 }
+
+static void xt_free_table_info_rcu(struct rcu_head *arg)
+{
+ 	struct xt_table_info *info = container_of(arg, struct xt_table_info, rcu);
+
+	free_percpu(info->seq);
+
+ 	if (info->size <= PAGE_SIZE) {
+		unsigned int cpu;
+ 		for_each_possible_cpu(cpu)
+ 			kfree(info->entries[cpu]);
+ 		kfree(info);
+ 	} else {
+ 		/* can't safely call vfree in current context */
+ 		INIT_WORK(&info->work, xt_free_table_info_work);
+ 		schedule_work(&info->work);
+  	}
+}
+
+void xt_free_table_info(struct xt_table_info *info)
+{
+ 	call_rcu(&info->rcu, xt_free_table_info_rcu);
+}
 EXPORT_SYMBOL(xt_free_table_info);
 
 /* Find table by name, grabs mutex & ref.  Returns ERR_PTR() on error. */
@@ -682,20 +702,20 @@ xt_replace_table(struct xt_table *table,
 	struct xt_table_info *oldinfo, *private;
 
 	/* Do the substitution. */
-	write_lock_bh(&table->lock);
+	spin_lock_bh(&table->lock);
 	private = table->private;
 	/* Check inside lock: is the old number correct? */
 	if (num_counters != private->number) {
 		duprintf("num_counters != table->private->number (%u/%u)\n",
 			 num_counters, private->number);
-		write_unlock_bh(&table->lock);
+		spin_unlock_bh(&table->lock);
 		*error = -EAGAIN;
 		return NULL;
 	}
 	oldinfo = private;
-	table->private = newinfo;
+	rcu_assign_pointer(table->private, newinfo);
 	newinfo->initial_entries = oldinfo->initial_entries;
-	write_unlock_bh(&table->lock);
+	spin_unlock_bh(&table->lock);
 
 	return oldinfo;
 }
@@ -730,7 +750,7 @@ struct xt_table *xt_register_table(struc
 
 	/* Simplifies replace_table code. */
 	table->private = bootstrap;
-	rwlock_init(&table->lock);
+	spin_lock_init(&table->lock);
 	if (!xt_replace_table(table, 0, newinfo, &ret))
 		goto unlock;
 

-- 

Re: [PATCH 0/5] iptables lockless receive (v0.3)

[Eric Dumazet]

Stephen Hemminger a écrit :
> Another experimental version of iptables locking that removes
> reader/writer lock overhead. This version incorprates suggestion
> of moving counter to table_info.  I haven't done ebtables, and don't
> know if it is worth bothering.
> 

A fast review reveals no problems, this looks good.

I will do some testings today and give a feedback.

By the way, you forgot to CC Patrick McHardy and netfilter-devel

Thanks a lot Stephen

Re: [PATCH 4/5] netfilter: use sequence number synchronization for counters

[Eric Dumazet]

Stephen Hemminger a écrit :
> Change how synchronization is done on the iptables counters. Use seqcount
> wrapper instead of depending on reader/writer lock.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> ---
>  include/linux/netfilter/x_tables.h |    3 +++
>  net/ipv4/netfilter/arp_tables.c    |   24 +++++++++++++++++++-----
>  net/ipv4/netfilter/ip_tables.c     |   24 +++++++++++++++++++-----
>  net/ipv6/netfilter/ip6_tables.c    |   32 +++++++++++++++++++++++---------
>  net/netfilter/x_tables.c           |   11 +++++++++++
>  5 files changed, 75 insertions(+), 19 deletions(-)
> 4
> --- a/net/ipv4/netfilter/arp_tables.c	2009-01-28 21:24:39.223991934 -0800
> +++ b/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:13:16.423490077 -0800
> @@ -230,6 +230,7 @@ unsigned int arpt_do_table(struct sk_buf
>  	void *table_base;
>  	const struct xt_table_info *private;
>  	struct xt_target_param tgpar;
> +	seqcount_t *seq;
>  
>  	if (!pskb_may_pull(skb, arp_hdr_len(skb->dev)))
>  		return NF_DROP;
> @@ -240,6 +241,7 @@ unsigned int arpt_do_table(struct sk_buf
>  	read_lock_bh(&table->lock);
>  	private = table->private;
>  	table_base = (void *)private->entries[smp_processor_id()];
> +	seq = per_cpu_ptr(private->seq, smp_processor_id());

But, why not using a global seqcount_t, shared by all tables, no matter they
are arp_tables, ip_tables, ip6_tables ?

A global PER_CPU variable, not dynamically allocated, so that its
access can be faster (no indirection), and uses exactly 4 bytes per
possible cpu.

DEFINE_PER_CPU(seqcount_t,  nf_seqcount);

Re: [PATCH 5/5] netfilter: convert x_tables to use RCU

[Eric Dumazet]

Stephen Hemminger a écrit :
> Replace existing reader/writer lock with Read-Copy-Update to
> elminate the overhead of a read lock on each incoming packet.
> This should reduce the overhead of iptables especially on SMP
> systems.
> 
> The previous code used a reader-writer lock for two purposes.
> The first was to ensure that the xt_table_info reference was not in
> process of being changed. Since xt_table_info is only freed via one
> routine, it was a direct conversion to RCU.
> 
> The other use of the reader-writer lock was to to block changes
> to counters while they were being read. This synchronization was
> fixed by the previous patch.  But still need to make sure table info
> isn't going away.
> 
> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> 
> 
> ---
>  include/linux/netfilter/x_tables.h |   10 ++++++-
>  net/ipv4/netfilter/arp_tables.c    |   12 ++++-----
>  net/ipv4/netfilter/ip_tables.c     |   12 ++++-----
>  net/ipv6/netfilter/ip6_tables.c    |   12 ++++-----
>  net/netfilter/x_tables.c           |   48 ++++++++++++++++++++++++++-----------
>  5 files changed, 60 insertions(+), 34 deletions(-)
> 
> --- a/include/linux/netfilter/x_tables.h	2009-01-28 22:04:39.316517913 -0800
> +++ b/include/linux/netfilter/x_tables.h	2009-01-28 22:14:54.648490491 -0800
> @@ -352,8 +352,8 @@ struct xt_table
>  	/* What hooks you will enter on */
>  	unsigned int valid_hooks;
>  
> -	/* Lock for the curtain */
> -	rwlock_t lock;
> +	/* Lock for curtain */
> +	spinlock_t lock;
>  
>  	/* Man behind the curtain... */
>  	struct xt_table_info *private;
> @@ -386,6 +386,12 @@ struct xt_table_info
>  	/* Secret compartment */
>  	seqcount_t *seq;
>  
> +	/* For the dustman... */
> +	union {
> +		struct rcu_head rcu;
> +		struct work_struct work;
> +	};
> +
>  	/* ipt_entry tables: one per CPU */
>  	/* Note : this field MUST be the last one, see XT_TABLE_INFO_SZ */
>  	char *entries[1];
> --- a/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:13:16.423490077 -0800
> +++ b/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:14:54.648490491 -0800
> @@ -238,8 +238,8 @@ unsigned int arpt_do_table(struct sk_buf
>  	indev = in ? in->name : nulldevname;
>  	outdev = out ? out->name : nulldevname;
>  
> -	read_lock_bh(&table->lock);
> -	private = table->private;
> +	rcu_read_lock_bh();
> +	private = rcu_dereference(table->private);
>  	table_base = (void *)private->entries[smp_processor_id()];
>  	seq = per_cpu_ptr(private->seq, smp_processor_id());
>  	e = get_entry(table_base, private->hook_entry[hook]);
> @@ -315,7 +315,7 @@ unsigned int arpt_do_table(struct sk_buf
>  			e = (void *)e + e->next_offset;
>  		}
>  	} while (!hotdrop);
> -	read_unlock_bh(&table->lock);
> +	rcu_read_unlock_bh();
>  
>  	if (hotdrop)
>  		return NF_DROP;
> @@ -1163,8 +1163,8 @@ static int do_add_counters(struct net *n
>  		goto free;
>  	}
>  
> -	write_lock_bh(&t->lock);
> -	private = t->private;
> +	rcu_read_lock_bh();
> +	private = rcu_dereference(t->private);
>  	if (private->number != num_counters) {
>  		ret = -EINVAL;
>  		goto unlock_up_free;
> @@ -1179,7 +1179,7 @@ static int do_add_counters(struct net *n
>  			   paddc,
>  			   &i);
>   unlock_up_free:
> -	write_unlock_bh(&t->lock);
> +	rcu_read_unlock_bh();
>  	xt_table_unlock(t);
>  	module_put(t->me);
>   free:
> --- a/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:06:10.596739805 -0800
> +++ b/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:14:54.648490491 -0800
> @@ -348,9 +348,9 @@ ipt_do_table(struct sk_buff *skb,
>  	mtpar.family  = tgpar.family = NFPROTO_IPV4;
>  	tgpar.hooknum = hook;
>  
> -	read_lock_bh(&table->lock);
> +	rcu_read_lock_bh();
>  	IP_NF_ASSERT(table->valid_hooks & (1 << hook));
> -	private = table->private;
> +	private = rcu_dereference(table->private);
>  	table_base = (void *)private->entries[smp_processor_id()];
>  	seq = per_cpu_ptr(private->seq, smp_processor_id());
>  	e = get_entry(table_base, private->hook_entry[hook]);
> @@ -449,7 +449,7 @@ ipt_do_table(struct sk_buff *skb,
>  		}
>  	} while (!hotdrop);
>  
> -	read_unlock_bh(&table->lock);
> +	rcu_read_unlock_bh();
>  
>  #ifdef DEBUG_ALLOW_ALL
>  	return NF_ACCEPT;
> @@ -1408,8 +1408,8 @@ do_add_counters(struct net *net, void __
>  		goto free;
>  	}
>  
> -	write_lock_bh(&t->lock);
> -	private = t->private;
> +	rcu_read_lock_bh();
> +	private = rcu_dereference(t->private);

I feel litle bit nervous seeing a write_lock_bh() changed to a rcu_read_lock()

Also, add_counter_to_entry() is not using seqcount protection, so another thread
doing an iptables -L in parallel with this thread will possibly get corrupted counters.


(With write_lock_bh(), this corruption could not occur)

Re: [PATCH 5/5] netfilter: convert x_tables to use RCU

[Stephen Hemminger]

On Fri, 30 Jan 2009 00:04:16 +0100
Eric Dumazet <dada1@cosmosbay.com> wrote:

> Stephen Hemminger a écrit :
> > Replace existing reader/writer lock with Read-Copy-Update to
> > elminate the overhead of a read lock on each incoming packet.
> > This should reduce the overhead of iptables especially on SMP
> > systems.
> > 
> > The previous code used a reader-writer lock for two purposes.
> > The first was to ensure that the xt_table_info reference was not in
> > process of being changed. Since xt_table_info is only freed via one
> > routine, it was a direct conversion to RCU.
> > 
> > The other use of the reader-writer lock was to to block changes
> > to counters while they were being read. This synchronization was
> > fixed by the previous patch.  But still need to make sure table info
> > isn't going away.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
> > 
> > 
> > ---
> >  include/linux/netfilter/x_tables.h |   10 ++++++-
> >  net/ipv4/netfilter/arp_tables.c    |   12 ++++-----
> >  net/ipv4/netfilter/ip_tables.c     |   12 ++++-----
> >  net/ipv6/netfilter/ip6_tables.c    |   12 ++++-----
> >  net/netfilter/x_tables.c           |   48 ++++++++++++++++++++++++++-----------
> >  5 files changed, 60 insertions(+), 34 deletions(-)
> > 
> > --- a/include/linux/netfilter/x_tables.h	2009-01-28 22:04:39.316517913 -0800
> > +++ b/include/linux/netfilter/x_tables.h	2009-01-28 22:14:54.648490491 -0800
> > @@ -352,8 +352,8 @@ struct xt_table
> >  	/* What hooks you will enter on */
> >  	unsigned int valid_hooks;
> >  
> > -	/* Lock for the curtain */
> > -	rwlock_t lock;
> > +	/* Lock for curtain */
> > +	spinlock_t lock;
> >  
> >  	/* Man behind the curtain... */
> >  	struct xt_table_info *private;
> > @@ -386,6 +386,12 @@ struct xt_table_info
> >  	/* Secret compartment */
> >  	seqcount_t *seq;
> >  
> > +	/* For the dustman... */
> > +	union {
> > +		struct rcu_head rcu;
> > +		struct work_struct work;
> > +	};
> > +
> >  	/* ipt_entry tables: one per CPU */
> >  	/* Note : this field MUST be the last one, see XT_TABLE_INFO_SZ */
> >  	char *entries[1];
> > --- a/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:13:16.423490077 -0800
> > +++ b/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:14:54.648490491 -0800
> > @@ -238,8 +238,8 @@ unsigned int arpt_do_table(struct sk_buf
> >  	indev = in ? in->name : nulldevname;
> >  	outdev = out ? out->name : nulldevname;
> >  
> > -	read_lock_bh(&table->lock);
> > -	private = table->private;
> > +	rcu_read_lock_bh();
> > +	private = rcu_dereference(table->private);
> >  	table_base = (void *)private->entries[smp_processor_id()];
> >  	seq = per_cpu_ptr(private->seq, smp_processor_id());
> >  	e = get_entry(table_base, private->hook_entry[hook]);
> > @@ -315,7 +315,7 @@ unsigned int arpt_do_table(struct sk_buf
> >  			e = (void *)e + e->next_offset;
> >  		}
> >  	} while (!hotdrop);
> > -	read_unlock_bh(&table->lock);
> > +	rcu_read_unlock_bh();
> >  
> >  	if (hotdrop)
> >  		return NF_DROP;
> > @@ -1163,8 +1163,8 @@ static int do_add_counters(struct net *n
> >  		goto free;
> >  	}
> >  
> > -	write_lock_bh(&t->lock);
> > -	private = t->private;
> > +	rcu_read_lock_bh();
> > +	private = rcu_dereference(t->private);
> >  	if (private->number != num_counters) {
> >  		ret = -EINVAL;
> >  		goto unlock_up_free;
> > @@ -1179,7 +1179,7 @@ static int do_add_counters(struct net *n
> >  			   paddc,
> >  			   &i);
> >   unlock_up_free:
> > -	write_unlock_bh(&t->lock);
> > +	rcu_read_unlock_bh();
> >  	xt_table_unlock(t);
> >  	module_put(t->me);
> >   free:
> > --- a/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:06:10.596739805 -0800
> > +++ b/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:14:54.648490491 -0800
> > @@ -348,9 +348,9 @@ ipt_do_table(struct sk_buff *skb,
> >  	mtpar.family  = tgpar.family = NFPROTO_IPV4;
> >  	tgpar.hooknum = hook;
> >  
> > -	read_lock_bh(&table->lock);
> > +	rcu_read_lock_bh();
> >  	IP_NF_ASSERT(table->valid_hooks & (1 << hook));
> > -	private = table->private;
> > +	private = rcu_dereference(table->private);
> >  	table_base = (void *)private->entries[smp_processor_id()];
> >  	seq = per_cpu_ptr(private->seq, smp_processor_id());
> >  	e = get_entry(table_base, private->hook_entry[hook]);
> > @@ -449,7 +449,7 @@ ipt_do_table(struct sk_buff *skb,
> >  		}
> >  	} while (!hotdrop);
> >  
> > -	read_unlock_bh(&table->lock);
> > +	rcu_read_unlock_bh();
> >  
> >  #ifdef DEBUG_ALLOW_ALL
> >  	return NF_ACCEPT;
> > @@ -1408,8 +1408,8 @@ do_add_counters(struct net *net, void __
> >  		goto free;
> >  	}
> >  
> > -	write_lock_bh(&t->lock);
> > -	private = t->private;
> > +	rcu_read_lock_bh();
> > +	private = rcu_dereference(t->private);
> 
> I feel litle bit nervous seeing a write_lock_bh() changed to a rcu_read_lock()

Facts, it is only updating entries on current cpu

> Also, add_counter_to_entry() is not using seqcount protection, so another thread
> doing an iptables -L in parallel with this thread will possibly get corrupted counters.
add_counter_to_entry is local to current CPU.


> (With write_lock_bh(), this corruption could not occur)
> 
> 

Re: [PATCH 5/5] netfilter: convert x_tables to use RCU

[Eric Dumazet]

Stephen Hemminger a écrit :
> On Fri, 30 Jan 2009 00:04:16 +0100
> Eric Dumazet <dada1@cosmosbay.com> wrote:
> 
>> Stephen Hemminger a écrit :
>>> Replace existing reader/writer lock with Read-Copy-Update to
>>> elminate the overhead of a read lock on each incoming packet.
>>> This should reduce the overhead of iptables especially on SMP
>>> systems.
>>>
>>> The previous code used a reader-writer lock for two purposes.
>>> The first was to ensure that the xt_table_info reference was not in
>>> process of being changed. Since xt_table_info is only freed via one
>>> routine, it was a direct conversion to RCU.
>>>
>>> The other use of the reader-writer lock was to to block changes
>>> to counters while they were being read. This synchronization was
>>> fixed by the previous patch.  But still need to make sure table info
>>> isn't going away.
>>>
>>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>>>
>>>
>>> ---
>>>  include/linux/netfilter/x_tables.h |   10 ++++++-
>>>  net/ipv4/netfilter/arp_tables.c    |   12 ++++-----
>>>  net/ipv4/netfilter/ip_tables.c     |   12 ++++-----
>>>  net/ipv6/netfilter/ip6_tables.c    |   12 ++++-----
>>>  net/netfilter/x_tables.c           |   48 ++++++++++++++++++++++++++-----------
>>>  5 files changed, 60 insertions(+), 34 deletions(-)
>>>
>>> --- a/include/linux/netfilter/x_tables.h	2009-01-28 22:04:39.316517913 -0800
>>> +++ b/include/linux/netfilter/x_tables.h	2009-01-28 22:14:54.648490491 -0800
>>> @@ -352,8 +352,8 @@ struct xt_table
>>>  	/* What hooks you will enter on */
>>>  	unsigned int valid_hooks;
>>>  
>>> -	/* Lock for the curtain */
>>> -	rwlock_t lock;
>>> +	/* Lock for curtain */
>>> +	spinlock_t lock;
>>>  
>>>  	/* Man behind the curtain... */
>>>  	struct xt_table_info *private;
>>> @@ -386,6 +386,12 @@ struct xt_table_info
>>>  	/* Secret compartment */
>>>  	seqcount_t *seq;
>>>  
>>> +	/* For the dustman... */
>>> +	union {
>>> +		struct rcu_head rcu;
>>> +		struct work_struct work;
>>> +	};
>>> +
>>>  	/* ipt_entry tables: one per CPU */
>>>  	/* Note : this field MUST be the last one, see XT_TABLE_INFO_SZ */
>>>  	char *entries[1];
>>> --- a/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:13:16.423490077 -0800
>>> +++ b/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:14:54.648490491 -0800
>>> @@ -238,8 +238,8 @@ unsigned int arpt_do_table(struct sk_buf
>>>  	indev = in ? in->name : nulldevname;
>>>  	outdev = out ? out->name : nulldevname;
>>>  
>>> -	read_lock_bh(&table->lock);
>>> -	private = table->private;
>>> +	rcu_read_lock_bh();
>>> +	private = rcu_dereference(table->private);
>>>  	table_base = (void *)private->entries[smp_processor_id()];
>>>  	seq = per_cpu_ptr(private->seq, smp_processor_id());
>>>  	e = get_entry(table_base, private->hook_entry[hook]);
>>> @@ -315,7 +315,7 @@ unsigned int arpt_do_table(struct sk_buf
>>>  			e = (void *)e + e->next_offset;
>>>  		}
>>>  	} while (!hotdrop);
>>> -	read_unlock_bh(&table->lock);
>>> +	rcu_read_unlock_bh();
>>>  
>>>  	if (hotdrop)
>>>  		return NF_DROP;
>>> @@ -1163,8 +1163,8 @@ static int do_add_counters(struct net *n
>>>  		goto free;
>>>  	}
>>>  
>>> -	write_lock_bh(&t->lock);
>>> -	private = t->private;
>>> +	rcu_read_lock_bh();
>>> +	private = rcu_dereference(t->private);
>>>  	if (private->number != num_counters) {
>>>  		ret = -EINVAL;
>>>  		goto unlock_up_free;
>>> @@ -1179,7 +1179,7 @@ static int do_add_counters(struct net *n
>>>  			   paddc,
>>>  			   &i);
>>>   unlock_up_free:
>>> -	write_unlock_bh(&t->lock);
>>> +	rcu_read_unlock_bh();
>>>  	xt_table_unlock(t);
>>>  	module_put(t->me);
>>>   free:
>>> --- a/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:06:10.596739805 -0800
>>> +++ b/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:14:54.648490491 -0800
>>> @@ -348,9 +348,9 @@ ipt_do_table(struct sk_buff *skb,
>>>  	mtpar.family  = tgpar.family = NFPROTO_IPV4;
>>>  	tgpar.hooknum = hook;
>>>  
>>> -	read_lock_bh(&table->lock);
>>> +	rcu_read_lock_bh();
>>>  	IP_NF_ASSERT(table->valid_hooks & (1 << hook));
>>> -	private = table->private;
>>> +	private = rcu_dereference(table->private);
>>>  	table_base = (void *)private->entries[smp_processor_id()];
>>>  	seq = per_cpu_ptr(private->seq, smp_processor_id());
>>>  	e = get_entry(table_base, private->hook_entry[hook]);
>>> @@ -449,7 +449,7 @@ ipt_do_table(struct sk_buff *skb,
>>>  		}
>>>  	} while (!hotdrop);
>>>  
>>> -	read_unlock_bh(&table->lock);
>>> +	rcu_read_unlock_bh();
>>>  
>>>  #ifdef DEBUG_ALLOW_ALL
>>>  	return NF_ACCEPT;
>>> @@ -1408,8 +1408,8 @@ do_add_counters(struct net *net, void __
>>>  		goto free;
>>>  	}
>>>  
>>> -	write_lock_bh(&t->lock);
>>> -	private = t->private;
>>> +	rcu_read_lock_bh();
>>> +	private = rcu_dereference(t->private);
>> I feel litle bit nervous seeing a write_lock_bh() changed to a rcu_read_lock()
> 
> Facts, it is only updating entries on current cpu

Yes, like done in ipt_do_table() ;)

Fact is we need to tell other threads, running on other cpus, that an update
 of our entries is running.

Let me check if your v4 and xt_counters abstraction already solved this problem.

> 
>> Also, add_counter_to_entry() is not using seqcount protection, so another thread
>> doing an iptables -L in parallel with this thread will possibly get corrupted counters.
> add_counter_to_entry is local to current CPU.
> 
> 
>> (With write_lock_bh(), this corruption could not occur)
>>
>>
> --

Re: [PATCH 5/5] netfilter: convert x_tables to use RCU

[Eric Dumazet]

Eric Dumazet a écrit :
> Stephen Hemminger a écrit :
>> On Fri, 30 Jan 2009 00:04:16 +0100
>> Eric Dumazet <dada1@cosmosbay.com> wrote:
>>
>>> Stephen Hemminger a écrit :
>>>> Replace existing reader/writer lock with Read-Copy-Update to
>>>> elminate the overhead of a read lock on each incoming packet.
>>>> This should reduce the overhead of iptables especially on SMP
>>>> systems.
>>>>
>>>> The previous code used a reader-writer lock for two purposes.
>>>> The first was to ensure that the xt_table_info reference was not in
>>>> process of being changed. Since xt_table_info is only freed via one
>>>> routine, it was a direct conversion to RCU.
>>>>
>>>> The other use of the reader-writer lock was to to block changes
>>>> to counters while they were being read. This synchronization was
>>>> fixed by the previous patch.  But still need to make sure table info
>>>> isn't going away.
>>>>
>>>> Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>
>>>>
>>>>
>>>> ---
>>>>  include/linux/netfilter/x_tables.h |   10 ++++++-
>>>>  net/ipv4/netfilter/arp_tables.c    |   12 ++++-----
>>>>  net/ipv4/netfilter/ip_tables.c     |   12 ++++-----
>>>>  net/ipv6/netfilter/ip6_tables.c    |   12 ++++-----
>>>>  net/netfilter/x_tables.c           |   48 ++++++++++++++++++++++++++-----------
>>>>  5 files changed, 60 insertions(+), 34 deletions(-)
>>>>
>>>> --- a/include/linux/netfilter/x_tables.h	2009-01-28 22:04:39.316517913 -0800
>>>> +++ b/include/linux/netfilter/x_tables.h	2009-01-28 22:14:54.648490491 -0800
>>>> @@ -352,8 +352,8 @@ struct xt_table
>>>>  	/* What hooks you will enter on */
>>>>  	unsigned int valid_hooks;
>>>>  
>>>> -	/* Lock for the curtain */
>>>> -	rwlock_t lock;
>>>> +	/* Lock for curtain */
>>>> +	spinlock_t lock;
>>>>  
>>>>  	/* Man behind the curtain... */
>>>>  	struct xt_table_info *private;
>>>> @@ -386,6 +386,12 @@ struct xt_table_info
>>>>  	/* Secret compartment */
>>>>  	seqcount_t *seq;
>>>>  
>>>> +	/* For the dustman... */
>>>> +	union {
>>>> +		struct rcu_head rcu;
>>>> +		struct work_struct work;
>>>> +	};
>>>> +
>>>>  	/* ipt_entry tables: one per CPU */
>>>>  	/* Note : this field MUST be the last one, see XT_TABLE_INFO_SZ */
>>>>  	char *entries[1];
>>>> --- a/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:13:16.423490077 -0800
>>>> +++ b/net/ipv4/netfilter/arp_tables.c	2009-01-28 22:14:54.648490491 -0800
>>>> @@ -238,8 +238,8 @@ unsigned int arpt_do_table(struct sk_buf
>>>>  	indev = in ? in->name : nulldevname;
>>>>  	outdev = out ? out->name : nulldevname;
>>>>  
>>>> -	read_lock_bh(&table->lock);
>>>> -	private = table->private;
>>>> +	rcu_read_lock_bh();
>>>> +	private = rcu_dereference(table->private);
>>>>  	table_base = (void *)private->entries[smp_processor_id()];
>>>>  	seq = per_cpu_ptr(private->seq, smp_processor_id());
>>>>  	e = get_entry(table_base, private->hook_entry[hook]);
>>>> @@ -315,7 +315,7 @@ unsigned int arpt_do_table(struct sk_buf
>>>>  			e = (void *)e + e->next_offset;
>>>>  		}
>>>>  	} while (!hotdrop);
>>>> -	read_unlock_bh(&table->lock);
>>>> +	rcu_read_unlock_bh();
>>>>  
>>>>  	if (hotdrop)
>>>>  		return NF_DROP;
>>>> @@ -1163,8 +1163,8 @@ static int do_add_counters(struct net *n
>>>>  		goto free;
>>>>  	}
>>>>  
>>>> -	write_lock_bh(&t->lock);
>>>> -	private = t->private;
>>>> +	rcu_read_lock_bh();
>>>> +	private = rcu_dereference(t->private);
>>>>  	if (private->number != num_counters) {
>>>>  		ret = -EINVAL;
>>>>  		goto unlock_up_free;
>>>> @@ -1179,7 +1179,7 @@ static int do_add_counters(struct net *n
>>>>  			   paddc,
>>>>  			   &i);
>>>>   unlock_up_free:
>>>> -	write_unlock_bh(&t->lock);
>>>> +	rcu_read_unlock_bh();
>>>>  	xt_table_unlock(t);
>>>>  	module_put(t->me);
>>>>   free:
>>>> --- a/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:06:10.596739805 -0800
>>>> +++ b/net/ipv4/netfilter/ip_tables.c	2009-01-28 22:14:54.648490491 -0800
>>>> @@ -348,9 +348,9 @@ ipt_do_table(struct sk_buff *skb,
>>>>  	mtpar.family  = tgpar.family = NFPROTO_IPV4;
>>>>  	tgpar.hooknum = hook;
>>>>  
>>>> -	read_lock_bh(&table->lock);
>>>> +	rcu_read_lock_bh();
>>>>  	IP_NF_ASSERT(table->valid_hooks & (1 << hook));
>>>> -	private = table->private;
>>>> +	private = rcu_dereference(table->private);
>>>>  	table_base = (void *)private->entries[smp_processor_id()];
>>>>  	seq = per_cpu_ptr(private->seq, smp_processor_id());
>>>>  	e = get_entry(table_base, private->hook_entry[hook]);
>>>> @@ -449,7 +449,7 @@ ipt_do_table(struct sk_buff *skb,
>>>>  		}
>>>>  	} while (!hotdrop);
>>>>  
>>>> -	read_unlock_bh(&table->lock);
>>>> +	rcu_read_unlock_bh();
>>>>  
>>>>  #ifdef DEBUG_ALLOW_ALL
>>>>  	return NF_ACCEPT;
>>>> @@ -1408,8 +1408,8 @@ do_add_counters(struct net *net, void __
>>>>  		goto free;
>>>>  	}
>>>>  
>>>> -	write_lock_bh(&t->lock);
>>>> -	private = t->private;
>>>> +	rcu_read_lock_bh();
>>>> +	private = rcu_dereference(t->private);
>>> I feel litle bit nervous seeing a write_lock_bh() changed to a rcu_read_lock()
>> Facts, it is only updating entries on current cpu
> 
> Yes, like done in ipt_do_table() ;)
> 
> Fact is we need to tell other threads, running on other cpus, that an update
>  of our entries is running.
> 
> Let me check if your v4 and xt_counters abstraction already solved this problem.

Hum, I just checked and indeed there is a problem...

#define SUM_COUNTER(s,c)  do { (s).bcnt += (c).bcnt; (s).pcnt += (c).pcnt; } while(0)

need to be changed to use 

#define SUM_COUNTER(s, c)  do { xt_incr_counter(s, (c).cnt, (c).pcnt);} while (0)

Re: [PATCH 5/5] netfilter: convert x_tables to use RCU

[Eric Dumazet]

Eric Dumazet a écrit :
> 
> Hum, I just checked and indeed there is a problem...
> 
> #define SUM_COUNTER(s,c)  do { (s).bcnt += (c).bcnt; (s).pcnt += (c).pcnt; } while(0)
> 
> need to be changed to use 
> 
> #define SUM_COUNTER(s, c)  do { xt_incr_counter(s, (c).cnt, (c).pcnt);} while (0)
> 

Oops

#define SUM_COUNTER(s, c)  xt_incr_counter(s, (c).bcnt, (c).pcnt)