| Message ID | 1381359077.4971.37.camel@edumazet-glaptop.roam.corp.google.com |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, Oct 09, 2013 at 03:51:17PM -0700, Eric Dumazet wrote: > On Wed, 2013-10-09 at 15:36 -0700, Paul E. McKenney wrote: > > > That would work, though it would probably give sparse complaints. > > No sparse error here, as I said types are correct and SPARSE_RCU ready : > > diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c > index 583b77e..28f8495 100644 > --- a/net/ipv6/ip6_tunnel.c > +++ b/net/ipv6/ip6_tunnel.c > @@ -245,7 +245,7 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t) > (iter = rtnl_dereference(*tp)) != NULL; > tp = &iter->next) { > if (t == iter) { > - rcu_assign_pointer(*tp, t->next); > + ACCESS_ONCE(*tp) = t->next; > break; > } > } I'd be really hesitant to introduce that type of direct assignment to an __rcu pointer without wrapping it in some appropriately named macro, or at the very least adding a comment. - Josh Triplett -- 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 Wed, 2013-10-09 at 15:56 -0700, Josh Triplett wrote: > I'd be really hesitant to introduce that type of direct assignment to an > __rcu pointer without wrapping it in some appropriately named macro, or > at the very least adding a comment. Well, there is no special magic here, in this specific case : - deleting an item in an rcu list Check list_del_rcu(), and you'll notice there is no _barrier_ Adding correct barriers is good, but please do not add them when not needed. It makes code hard to understand. ACCESS(*ptr) = value; is clear and autodocumented, because it highlights the potential problem, that is *ptr can be read without any barrier from another cpu. So we ask the compiler to not write temporary garbage in it. rcu_assign_pointer(*ptr, rcu_access_pointer(value)) is very confusing, because it hides the _real_ problem and add defensive programming tricks. -- 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 Wed, Oct 09, 2013 at 04:17:55PM -0700, Eric Dumazet wrote: > On Wed, 2013-10-09 at 15:56 -0700, Josh Triplett wrote: > > > I'd be really hesitant to introduce that type of direct assignment to an > > __rcu pointer without wrapping it in some appropriately named macro, or > > at the very least adding a comment. > > Well, there is no special magic here, in this specific case : > > - deleting an item in an rcu list > > Check list_del_rcu(), and you'll notice there is no _barrier_ > > Adding correct barriers is good, but please do not add them when not > needed. > > It makes code hard to understand. I'm not arguing for the inclusion of an unnecessary barrier. I'm arguing for something more self-documenting than: > ACCESS(*ptr) = value; that. Constructs like list_del_rcu are much clearer, and not open-coded. Open-coding synchronization code is almost always a Bad Idea. - Josh Triplett -- 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 Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote: > that. Constructs like list_del_rcu are much clearer, and not > open-coded. Open-coding synchronization code is almost always a Bad > Idea. OK, so you think there is synchronization code. I will shut up then, no need to waste time. -- 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 Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote: > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote: > > > that. Constructs like list_del_rcu are much clearer, and not > > open-coded. Open-coding synchronization code is almost always a Bad > > Idea. > > OK, so you think there is synchronization code. > > I will shut up then, no need to waste time. As you said earlier, we should at least get rid of the memory barrier as long as we are changing the code. Josh, what would you suggest as the best way to avoid the memory barrier, keep sparse happy, and not be too ugly? Thanx, Paul -- 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 Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote: > On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote: > > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote: > > > > > that. Constructs like list_del_rcu are much clearer, and not > > > open-coded. Open-coding synchronization code is almost always a Bad > > > Idea. > > > > OK, so you think there is synchronization code. > > > > I will shut up then, no need to waste time. > > As you said earlier, we should at least get rid of the memory barrier > as long as we are changing the code. Interesting thread! Sorry to chime in and asking a question: Why do we need an ACCESS_ONCE here if rcu_assign_pointer can do without one? In other words I wonder why rcu_assign_pointer is not a static inline function to use the sequence point in argument evaluation (if I remember correctly this also holds for inline functions) to not allow something like this: E.g. we want to publish which lock to take first to prevent an ABBA problem (extreme example): rcu_assign_pointer(lockptr, min(lptr1, lptr2)); Couldn't a compiler spill the lockptr memory location as a temporary buffer if the compiler is under register pressure? (yes, this seems unlikely if we flushed out most registers to memory because of the barrier, but still... ;) ) This seems to be also the case if we publish a multi-dereferencing pointers e.g. ptr->ptr->ptr. Thanks, Hannes -- 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 Thu, Oct 10, 2013 at 04:04:22AM +0200, Hannes Frederic Sowa wrote: > On Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote: > > On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote: > > > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote: > > > > > > > that. Constructs like list_del_rcu are much clearer, and not > > > > open-coded. Open-coding synchronization code is almost always a Bad > > > > Idea. > > > > > > OK, so you think there is synchronization code. > > > > > > I will shut up then, no need to waste time. > > > > As you said earlier, we should at least get rid of the memory barrier > > as long as we are changing the code. > > Interesting thread! > > Sorry to chime in and asking a question: > > Why do we need an ACCESS_ONCE here if rcu_assign_pointer can do without one? > In other words I wonder why rcu_assign_pointer is not a static inline function > to use the sequence point in argument evaluation (if I remember correctly this > also holds for inline functions) to not allow something like this: > > E.g. we want to publish which lock to take first to prevent an ABBA problem > (extreme example): > > rcu_assign_pointer(lockptr, min(lptr1, lptr2)); > > Couldn't a compiler spill the lockptr memory location as a temporary buffer > if the compiler is under register pressure? (yes, this seems unlikely if we > flushed out most registers to memory because of the barrier, but still... ;) ) > > This seems to be also the case if we publish a multi-dereferencing pointers > e.g. ptr->ptr->ptr. IIRC, sequence points only confine volatile accesses. For non-volatile accesses, the so-called "as-if rule" allows compiler writers to do some surprisingly global reordering. The reason that rcu_assign_pointer() isn't an inline function is because it needs to be type-generic, in other words, it needs to be OK to use it on any type of pointers as long as the C types of the two pointers match (the sparse types can vary a bit). One of the reasons for wanting a volatile cast in rcu_assign_pointer() is to prevent compiler mischief such as you described in your last two paragraphs. That said, it would take a very brave compiler to pull a pointer-referenced memory location into a register and keep it there. Unfortunately, increasing compiler bravery seems to be a solid long-term trend. Thanx, Paul -- 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 Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote: > On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote: > > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote: > > > > > that. Constructs like list_del_rcu are much clearer, and not > > > open-coded. Open-coding synchronization code is almost always a Bad > > > Idea. > > > > OK, so you think there is synchronization code. > > > > I will shut up then, no need to waste time. > > As you said earlier, we should at least get rid of the memory barrier > as long as we are changing the code. > > Josh, what would you suggest as the best way to avoid the memory barrier, > keep sparse happy, and not be too ugly? The more I think about it, the more I realize that assigning an __rcu pointer to an __rcu pointer *without* a memory barrier is a sufficiently uncommon case that you probably *should* just write an open-coded assignment. Just please put a very clear comment right before it. I'd originally thought it might make sense to have a macro similar to rcu_assign_pointer, but I just don't think this is a common enough case, and we don't want people thinking they can use this in general for __rcu to __rcu assignments (most of which still need a memory barrier). - Josh Triplett -- 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 Thu, Oct 10, 2013 at 05:20:44PM -0700, Josh Triplett wrote: > On Wed, Oct 09, 2013 at 05:28:33PM -0700, Paul E. McKenney wrote: > > On Wed, Oct 09, 2013 at 05:12:40PM -0700, Eric Dumazet wrote: > > > On Wed, 2013-10-09 at 16:40 -0700, Josh Triplett wrote: > > > > > > > that. Constructs like list_del_rcu are much clearer, and not > > > > open-coded. Open-coding synchronization code is almost always a Bad > > > > Idea. > > > > > > OK, so you think there is synchronization code. > > > > > > I will shut up then, no need to waste time. > > > > As you said earlier, we should at least get rid of the memory barrier > > as long as we are changing the code. > > > > Josh, what would you suggest as the best way to avoid the memory barrier, > > keep sparse happy, and not be too ugly? > > The more I think about it, the more I realize that assigning an __rcu > pointer to an __rcu pointer *without* a memory barrier is a sufficiently > uncommon case that you probably *should* just write an open-coded > assignment. Just please put a very clear comment right before it. Fair enough, will do! Given earlier email, I believe that Eric is fine with this, and if he isn't I am sure he will let us know. ;-) > I'd originally thought it might make sense to have a macro similar to > rcu_assign_pointer, but I just don't think this is a common enough case, > and we don't want people thinking they can use this in general for __rcu > to __rcu assignments (most of which still need a memory barrier). Yep, it is a rather small fraction of rcu_assign_pointer() instances. Thanx, Paul -- 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
diff --git a/net/ipv6/ip6_tunnel.c b/net/ipv6/ip6_tunnel.c index 583b77e..28f8495 100644 --- a/net/ipv6/ip6_tunnel.c +++ b/net/ipv6/ip6_tunnel.c @@ -245,7 +245,7 @@ ip6_tnl_unlink(struct ip6_tnl_net *ip6n, struct ip6_tnl *t) (iter = rtnl_dereference(*tp)) != NULL; tp = &iter->next) { if (t == iter) { - rcu_assign_pointer(*tp, t->next); + ACCESS_ONCE(*tp) = t->next; break; } }
On Wed, 2013-10-09 at 15:36 -0700, Paul E. McKenney wrote: > That would work, though it would probably give sparse complaints. No sparse error here, as I said types are correct and SPARSE_RCU ready : root@edumazet-glaptop:/usr/src/net-next# grep CONFIG_SPARSE_RCU_POINTER .config CONFIG_SPARSE_RCU_POINTER=y root@edumazet-glaptop:/usr/src/net-next# make C=2 net/ipv6/ip6_tunnel.o make[1]: Nothing to be done for `all'. make[1]: Nothing to be done for `relocs'. CHK include/config/kernel.release CHK include/generated/uapi/linux/version.h CHK include/generated/utsrelease.h CALL scripts/checksyscalls.sh CHECK scripts/mod/empty.c CHECK net/ipv6/ip6_tunnel.c -- 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