| Message ID | 20140323213349.GA8832@redhat.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sun, Mar 23, 2014 at 11:33:49PM +0200, Michael S. Tsirkin wrote: > On Sun, Mar 23, 2014 at 12:54:17PM -0700, Eric Dumazet wrote: > > On Sun, 2014-03-23 at 21:41 +0200, Michael S. Tsirkin wrote: > > > > > The rcu_assign_pointer() ensures that the initialization of a structure > > > is carried out before storing a pointer to that structure. > > > In the case of the NULL pointer, there is no structure to initialize, > > > so we can safely drop smp_wmb in this case. > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > -- > > > > > > Lightly tested. > > > v is evaluated twice here but that should be ok since this > > > only happens when v is a constant, so evaluating it should > > > have no side effects. > > > Paul, what do you think? > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > > index 72bf3a0..d33c9ec 100644 > > > --- a/include/linux/rcupdate.h > > > +++ b/include/linux/rcupdate.h > > > @@ -587,7 +587,8 @@ static inline void rcu_preempt_sleep_check(void) > > > */ > > > #define rcu_assign_pointer(p, v) \ > > > do { \ > > > - smp_wmb(); \ > > > + if (!__builtin_constant_p(v) || (v)) \ > > > + smp_wmb(); \ > > > ACCESS_ONCE(p) = RCU_INITIALIZER(v); \ > > > } while (0) > > > > > > > Yeah, I suggest you read d322f45ceed525daa changelog ;) > > > > Oh I see. It does not seem hard to silence that warning though. > See below. This would at the very least need to be tested under a wide variety of compilers. And we need to keep > Alternatively apply these patches everywhere though it does > look like too much work for too little gain to me. > > --> > > rcu: optimize rcu_assign_pointer with NULL > > The rcu_assign_pointer() dropped __builtin_constant_p check to > avoid a compiler warning, but we can actually work around it without > adding code. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > Untested, too late here, sorry. > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > index 72bf3a0..9111d40 100644 > --- a/include/linux/rcupdate.h > +++ b/include/linux/rcupdate.h > @@ -585,9 +585,14 @@ static inline void rcu_preempt_sleep_check(void) > * please be careful when making changes to rcu_assign_pointer() and the > * other macros that it invokes. > */ > +/* The convoluted __builtin_constant_p logic is here to prevent > + * gcc from emitting a warning when passed a pointer to a variable. > + */ > #define rcu_assign_pointer(p, v) \ > do { \ > - smp_wmb(); \ > + if (!__builtin_constant_p(v) || \ > + (__builtin_constant_p(v) ? (v) : NULL)) \ You lost me on this one. If "v" is not a built-in constant, we want the smp_wmb(), right? Thanx, Paul > + smp_wmb(); \ > ACCESS_ONCE(p) = RCU_INITIALIZER(v); \ > } while (0) > > -- 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 Sun, Mar 23, 2014 at 03:12:56PM -0700, Paul E. McKenney wrote: > On Sun, Mar 23, 2014 at 11:33:49PM +0200, Michael S. Tsirkin wrote: > > On Sun, Mar 23, 2014 at 12:54:17PM -0700, Eric Dumazet wrote: > > > On Sun, 2014-03-23 at 21:41 +0200, Michael S. Tsirkin wrote: > > > > > > > The rcu_assign_pointer() ensures that the initialization of a structure > > > > is carried out before storing a pointer to that structure. > > > > In the case of the NULL pointer, there is no structure to initialize, > > > > so we can safely drop smp_wmb in this case. > > > > > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > > > > > -- > > > > > > > > Lightly tested. > > > > v is evaluated twice here but that should be ok since this > > > > only happens when v is a constant, so evaluating it should > > > > have no side effects. > > > > Paul, what do you think? > > > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > > > index 72bf3a0..d33c9ec 100644 > > > > --- a/include/linux/rcupdate.h > > > > +++ b/include/linux/rcupdate.h > > > > @@ -587,7 +587,8 @@ static inline void rcu_preempt_sleep_check(void) > > > > */ > > > > #define rcu_assign_pointer(p, v) \ > > > > do { \ > > > > - smp_wmb(); \ > > > > + if (!__builtin_constant_p(v) || (v)) \ > > > > + smp_wmb(); \ > > > > ACCESS_ONCE(p) = RCU_INITIALIZER(v); \ > > > > } while (0) > > > > > > > > > > Yeah, I suggest you read d322f45ceed525daa changelog ;) > > > > > > > Oh I see. It does not seem hard to silence that warning though. > > See below. > > This would at the very least need to be tested under a wide variety > of compilers. Seems an incredibly strict requirement for something that just silences a warning. What exactly should I test? I intended to just verify this produces same code as before d322f45ceed525daa under a recent gcc. > And we need to keep > > > Alternatively apply these patches everywhere though it does > > look like too much work for too little gain to me. > > > > --> > > > > rcu: optimize rcu_assign_pointer with NULL > > > > The rcu_assign_pointer() dropped __builtin_constant_p check to > > avoid a compiler warning, but we can actually work around it without > > adding code. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > --- > > > > Untested, too late here, sorry. > > > > > > diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h > > index 72bf3a0..9111d40 100644 > > --- a/include/linux/rcupdate.h > > +++ b/include/linux/rcupdate.h > > @@ -585,9 +585,14 @@ static inline void rcu_preempt_sleep_check(void) > > * please be careful when making changes to rcu_assign_pointer() and the > > * other macros that it invokes. > > */ > > +/* The convoluted __builtin_constant_p logic is here to prevent > > + * gcc from emitting a warning when passed a pointer to a variable. > > + */ > > #define rcu_assign_pointer(p, v) \ > > do { \ > > - smp_wmb(); \ > > + if (!__builtin_constant_p(v) || \ > > + (__builtin_constant_p(v) ? (v) : NULL)) \ > > You lost me on this one. If "v" is not a built-in constant, we want > the smp_wmb(), right? If "v" is not a built-in constant, then !__builtin_constant_p(v) is true so (__builtin_constant_p(v) ? (v) : NULL)) is never evaluated. Basically if ( 1. !A || 2. A ? B : C If A is false, only 1 is evaluated and the expression evaluates to true If A is true, then 2 evaluates to B. C is never evaluated. Makes sense? Did I miss anything? But the effect as far as I can tell is that instead of converting v to integer type we convert an expression involving v, so even though it's able to figure out the value, gcc understands it's not a typo and does not warn. > Thanx, Paul > > > + smp_wmb(); \ > > ACCESS_ONCE(p) = RCU_INITIALIZER(v); \ > > } while (0) > > > > -- 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 Mon, 2014-03-24 at 07:09 +0200, Michael S. Tsirkin wrote: > Seems an incredibly strict requirement for something that just > silences a warning. > What exactly should I test? > I intended to just verify this produces same code as before > d322f45ceed525daa under a recent gcc. Thats because many rcu_assign_pointer(X, NULL) were already converted to RCU_INIT_POINTER(X, NULL) Quite frankly I don't know why you bother at all. Adding back the lazy test in rcu_assign_pointer() doesn't help to make the API cleaner and easier to understand. People are usually using RCU API without really understanding all the issues. They tend to add superfluous barriers because they feel better. Having separate RCU_INIT_POINTER() and rcu_assign_pointer() serve as better documentation of the code, I find it more easier to immediately check what is going on while reviewing stuff. Presumably, checkpatch.pl could be augmented to suggest to use RCU_INIT_POINTER(X, NULL) instead of rcu_assign_pointer(X, NULL) -- 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 03/24/2014 01:25 PM, Eric Dumazet wrote: > On Mon, 2014-03-24 at 07:09 +0200, Michael S. Tsirkin wrote: > >> Seems an incredibly strict requirement for something that just >> silences a warning. >> What exactly should I test? >> I intended to just verify this produces same code as before >> d322f45ceed525daa under a recent gcc. > > Thats because many rcu_assign_pointer(X, NULL) were already converted to > RCU_INIT_POINTER(X, NULL) > > Quite frankly I don't know why you bother at all. > > Adding back the lazy test in rcu_assign_pointer() doesn't help to make > the API cleaner and easier to understand. > > People are usually using RCU API without really understanding > all the issues. They tend to add superfluous barriers because they feel > better. > > Having separate RCU_INIT_POINTER() and rcu_assign_pointer() serve as > better documentation of the code, I find it more easier to immediately > check what is going on while reviewing stuff. > > Presumably, checkpatch.pl could be augmented to suggest to use > RCU_INIT_POINTER(X, NULL) instead of rcu_assign_pointer(X, NULL) I prefer rcu_assign_pointer(X, NULL) than RCU_INIT_POINTER(X, NULL), NULL should not be a special pointer value to the users of RCU. the RCU implements should hide the difference if RCU implements differentiate the values for optimization. RCU_INIT_POINTER() sounds as an initialization-stage API. If we need something different for NULL pointer, I prefer rcu_assign_*null*_pointer(). rcu_assign_pointer(X, NULL) implies compiler barrier(), but RCU_INIT_POINTER(X, NULL) doesn't. > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > -- 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 Mon, Mar 24, 2014 at 04:47:32PM +0800, Lai Jiangshan wrote: > On 03/24/2014 01:25 PM, Eric Dumazet wrote: > > On Mon, 2014-03-24 at 07:09 +0200, Michael S. Tsirkin wrote: > > > >> Seems an incredibly strict requirement for something that just > >> silences a warning. > >> What exactly should I test? > >> I intended to just verify this produces same code as before > >> d322f45ceed525daa under a recent gcc. > > > > Thats because many rcu_assign_pointer(X, NULL) were already converted to > > RCU_INIT_POINTER(X, NULL) > > > > Quite frankly I don't know why you bother at all. > > > > Adding back the lazy test in rcu_assign_pointer() doesn't help to make > > the API cleaner and easier to understand. > > > > People are usually using RCU API without really understanding > > all the issues. They tend to add superfluous barriers because they feel > > better. > > > > Having separate RCU_INIT_POINTER() and rcu_assign_pointer() serve as > > better documentation of the code, I find it more easier to immediately > > check what is going on while reviewing stuff. > > > > Presumably, checkpatch.pl could be augmented to suggest to use > > RCU_INIT_POINTER(X, NULL) instead of rcu_assign_pointer(X, NULL) > > > I prefer rcu_assign_pointer(X, NULL) than RCU_INIT_POINTER(X, NULL), > NULL should not be a special pointer value to the users of RCU. > > the RCU implements should hide the difference if RCU implements > differentiate the values for optimization. > > RCU_INIT_POINTER() sounds as an initialization-stage API. If we need > something different for NULL pointer, I prefer > rcu_assign_*null*_pointer(). Let's keep what we have for a year or so, and then see how things look at that point. A really easy Coccinelle script will make the needed changes, so we aren't losing anything by waiting. And who knows, perhaps someone will come up with a clever idea in that time. > rcu_assign_pointer(X, NULL) implies compiler barrier(), but > RCU_INIT_POINTER(X, NULL) doesn't. Good point! I don't believe that the current docbook mentions this, will fix. And you are right, this is a good argument for maintaining a separate API for NULL-pointer assignment rather than making rcu_assign_pointer() sometimes do the smp_wmb() and sometimes not. With the current approach, you can count on rcu_assign_pointer() always implying a memory barrier. Also, one thing I forgot earlier, rcu_assign_pointer() now uses smp_store_release() rather than smp_wmb(). Thanx, Paul > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ > > > -- 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/include/linux/rcupdate.h b/include/linux/rcupdate.h index 72bf3a0..9111d40 100644 --- a/include/linux/rcupdate.h +++ b/include/linux/rcupdate.h @@ -585,9 +585,14 @@ static inline void rcu_preempt_sleep_check(void) * please be careful when making changes to rcu_assign_pointer() and the * other macros that it invokes. */ +/* The convoluted __builtin_constant_p logic is here to prevent + * gcc from emitting a warning when passed a pointer to a variable. + */ #define rcu_assign_pointer(p, v) \ do { \ - smp_wmb(); \ + if (!__builtin_constant_p(v) || \ + (__builtin_constant_p(v) ? (v) : NULL)) \ + smp_wmb(); \ ACCESS_ONCE(p) = RCU_INITIALIZER(v); \ } while (0)