Messages in this thread | | | Date | Mon, 24 Mar 2014 06:38:32 -0700 | From | "Paul E. McKenney" <> | Subject | Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c |
| |
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/ > > >
| |