Re: Confused about hlist_unhashed_lockless()

[Will Deacon <will@kernel.org>]

Hi Paul,

On Mon, Feb 03, 2020 at 09:29:47AM -0800, Paul E. McKenney wrote:
> On Mon, Feb 03, 2020 at 04:02:28PM +0000, Will Deacon wrote:
> > On Mon, Feb 03, 2020 at 07:58:39AM -0800, Paul E. McKenney wrote:
> > > On Mon, Feb 03, 2020 at 03:45:54PM +0000, David Laight wrote:
> > > > From: Eric Dumazet
> > > > > Sent: 31 January 2020 18:53
> > > > > 
> > > > > On Fri, Jan 31, 2020 at 10:48 AM Eric Dumazet <edumazet@google.com> wrote:
> > > > > >
> > > > > 
> > > > > > This is nice, now with have data_race()
> > > > > >
> > > > > > Remember these patches were sent 2 months ago, at a time we were
> > > > > > trying to sort out things.
> > > > > >
> > > > > > data_race() was merged a few days ago.
> > > > > 
> > > > > Well, actually data_race() is not there yet anyway.
> > > > 
> > > > Shouldn't it be NO_DATA_RACE() ??
> > > 
> > > No, because you use data_race() when there really are data races, but you
> > > want KCSAN to ignore them.  For example, diagnostic code that doesn't
> > > participate in the actual concurrency design and that doesn't run all
> > > that often might use data_race().  For another example, if a developer
> > > knew that data races existed, but that the compiler could not reasonably
> > > do anything untoward with those data races, that developer might well
> > > choose to use data_race() instead of READ_ONCE().  Especially if the
> > > access in question was on a fastpath where helpful compiler optimizations
> > > would be prohibited by use of READ_ONCE().
> > 
> > Yes, and in this particular case I think we can remove some WRITE_ONCE()s
> > from the non-RCU hlist code too (similarly for hlist_nulls).
> 
> Quite possibly, but we should take them case by case.  READ_ONCE()
> really does protect against some optimizations, while data_race() does
> not at all.

Agreed, and I plan to send patches for review so we can discuss them in
more detail then.

> But yes, in some cases you want to -avoid- using READ_ONCE() and
> WRITE_ONCE() so that KCSAN can do its job.  For example, given a per-CPU
> variable that is only supposed to be accessed from the corresponding CPU
> except for reads by diagnostic code, you should have the main algorithm
> use plain C-language reads and writes, and have the diagnostic code
> use data_race().  This allows KCSAN to correctly flag bugs that access
> this per-CPU variable off-CPU while leaving the diagnostic code alone.

Yes, and in a similar vein I think the WRITE_ONCE() additions to the hlist
code may hide unintentional racy access to the hlist where I would argue
that the correct behaviour is either to acknowledge the data race (like the
timer code) or to use the RCU variant. The problem with what's currently in
mainline is that it reads a bit like the non-RCU hlist is directly usable as
a lock-free list implementation, which really isn't the case.

> Seem reasonable?

It does to me, but we should probably try to apply this a bit more
consistently in patch review. Adding {READ,WRITE}_ONCE() until the
sanitiser shuts up is easy, but picking that apart later on is a real
challenge.

Will