Re: [PATCH v2 9/9] netfilter: Replace smp_read_barrier_depends() with lockless_dereference()
From: Eric Dumazet <hidden>
Date: 2014-11-21 16:12:50
Also in:
lkml, netfilter-devel
On Fri, 2014-11-21 at 10:06 -0500, Pranith Kumar wrote:
quoted hunk ↗ jump to hunk
Recently lockless_dereference() was added which can be used in place of hard-coding smp_read_barrier_depends(). The following PATCH makes the change. Signed-off-by: Pranith Kumar <redacted> --- net/ipv4/netfilter/arp_tables.c | 3 +-- net/ipv4/netfilter/ip_tables.c | 3 +-- net/ipv6/netfilter/ip6_tables.c | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-)diff --git a/net/ipv4/netfilter/arp_tables.c b/net/ipv4/netfilter/arp_tables.c index f95b6f9..fc7533d 100644 --- a/net/ipv4/netfilter/arp_tables.c +++ b/net/ipv4/netfilter/arp_tables.c@@ -270,12 +270,11 @@ unsigned int arpt_do_table(struct sk_buff *skb, local_bh_disable(); addend = xt_write_recseq_begin(); - private = table->private; /* * Ensure we load private-> members after we've fetched the base * pointer. */ - smp_read_barrier_depends(); + private = lockless_dereference(table->private); table_base = private->entries[smp_processor_id()];
Please carefully read the code, before and after your change, then you'll see this change broke the code. Problem is that a bug like that can be really hard to diagnose and fix later, so really you have to be very careful doing these mechanical changes. IMO, current code+comment is better than with this lockless_dereference() which in this particular case obfuscates the code. more than anything. In this case we do have a lock (sort of), so lockless_dereference() is quite misleading.