netdev - Re: [PATCH] drivers/net: Use RCU_INIT

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140324133831.GK4405@linux.vnet.ibm.com>
Date:	Mon, 24 Mar 2014 06:38:32 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Lai Jiangshan <laijs@...fujitsu.com>
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	"Michael S. Tsirkin" <mst@...hat.com>,
	Monam Agarwal <monamagarwal123@...il.com>, davem@...emloft.net,
	jasowang@...hat.com, xemul@...allels.com, wuzhy@...ux.vnet.ibm.com,
	therbert@...gle.com, yamato@...hat.com, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org
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@...r.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html