Re: [PATCH] drivers/net: Use RCU_INIT_POINTER(x, NULL) in tun.c

[Paul E. McKenney]

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@xxxxxxxxxxxxxxx
> > 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 linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/