| Message ID | 1416582363-20661-10-git-send-email-bobby.prani@gmail.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Fri, 2014-11-21 at 10:06 -0500, Pranith Kumar wrote: > 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 <bobby.prani@gmail.com> > --- > 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 21, 2014 at 11:12 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Fri, 2014-11-21 at 10:06 -0500, Pranith Kumar wrote: >> 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 <bobby.prani@gmail.com> >> --- >> 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. > Hi Eric, Thanks for looking at this patch. I've been scratching my head since morning trying to find out what was so obviously wrong with this patch. Alas, I don't see what you do. Could you point it out and show me how incompetent I am, please? Thanks!
On Fri, 2014-11-21 at 16:57 -0500, Pranith Kumar wrote: > Hi Eric, > > Thanks for looking at this patch. > > I've been scratching my head since morning trying to find out what was > so obviously wrong with this patch. Alas, I don't see what you do. > > Could you point it out and show me how incompetent I am, please? > > Thanks! Well, even it the code is _not_ broken, I don't see any value with this patch. If I use git blame on current code, line containing smp_read_barrier_depends() exactly points to the relevant commit [1] After your change, it will point to some cleanup, which makes little sense to me, considering you did not change the smp_wmb() in xt_replace_table(). I, as a netfilter contributor would like to keep current code as is, because it is how I feel safe with it. We have a proliferation of interfaces, but this does not help to understand the issues and code maintenance. smp_read_barrier_depends() better documents the read barrier than lockless_dereference(). The point of having a lock or not is irrelevant here. [1] http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=b416c144f46af1a30ddfa4e4319a8f077381ad63 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 2014-11-21 16:57:00 -0500, Pranith Kumar wrote: > On Fri, Nov 21, 2014 at 11:12 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > On Fri, 2014-11-21 at 10:06 -0500, Pranith Kumar wrote: > >> 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 <bobby.prani@gmail.com> > >> --- > >> 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. > > > > Hi Eric, > > Thanks for looking at this patch. > > I've been scratching my head since morning trying to find out what was > so obviously wrong with this patch. Alas, I don't see what you do. Afaics the read_barrier_depends protected the load from private->entries[x] earlier, not the load of table->private itself. Greetings, Andres Freund -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 21, 2014 at 7:05 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > > On Fri, 2014-11-21 at 16:57 -0500, Pranith Kumar wrote: > >> Hi Eric, >> >> Thanks for looking at this patch. >> >> I've been scratching my head since morning trying to find out what was >> so obviously wrong with this patch. Alas, I don't see what you do. >> >> Could you point it out and show me how incompetent I am, please? >> >> Thanks! > > Well, even it the code is _not_ broken, I don't see any value with this > patch. Phew. Not being broken itself is a win :) > > If I use git blame on current code, line containing > smp_read_barrier_depends() exactly points to the relevant commit [1] And that is an opinion I will respect. I don't want to muck the git history where it is significant. This effort is to eventually replace the uses of smp_read_barrier_depends() and to use either rcu or lockless_dereference() as documented in memory-barriers.txt. > > After your change, it will point to some cleanup, which makes little > sense to me, considering you did not change the smp_wmb() in > xt_replace_table(). That does not need to change as it is fine as it is. It still pairs with the smp_read_barrier_depends() in lockless_dereference(). > > I, as a netfilter contributor would like to keep current code as is, > because it is how I feel safe with it. > > We have a proliferation of interfaces, but this does not help to > understand the issues and code maintenance. > > smp_read_barrier_depends() better documents the read barrier than > lockless_dereference(). I think this is a matter of opinion. But in the current effort I've seen cases where it is not clear what the barrier is actually guaranteeing. I am glad that the current code is not one of those and it has reasonable comments. lockless_dereference() on the other hand makes the dependency explicit. > > The point of having a lock or not is irrelevant here. > > [1] > http://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=b416c144f46af1a30ddfa4e4319a8f077381ad63 > > > > Thanks!
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()]; e = get_entry(table_base, private->hook_entry[hook]); diff --git a/net/ipv4/netfilter/ip_tables.c b/net/ipv4/netfilter/ip_tables.c index 99e810f..e0fd044 100644 --- a/net/ipv4/netfilter/ip_tables.c +++ b/net/ipv4/netfilter/ip_tables.c @@ -325,13 +325,12 @@ ipt_do_table(struct sk_buff *skb, IP_NF_ASSERT(table->valid_hooks & (1 << hook)); local_bh_disable(); addend = xt_write_recseq_begin(); - private = table->private; cpu = smp_processor_id(); /* * 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[cpu]; jumpstack = (struct ipt_entry **)private->jumpstack[cpu]; stackptr = per_cpu_ptr(private->stackptr, cpu); diff --git a/net/ipv6/netfilter/ip6_tables.c b/net/ipv6/netfilter/ip6_tables.c index e080fbb..0459d6a 100644 --- a/net/ipv6/netfilter/ip6_tables.c +++ b/net/ipv6/netfilter/ip6_tables.c @@ -348,12 +348,11 @@ ip6t_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); cpu = smp_processor_id(); table_base = private->entries[cpu]; jumpstack = (struct ip6t_entry **)private->jumpstack[cpu];
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 <bobby.prani@gmail.com> --- 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(-)