Re: __{read,write}_once

[Maxime Villard <max%m00nbsd.net@localhost>]

Le 06/11/2019 à 23:41, Mindaugas Rasiukevicius a écrit :
> Maxime Villard <max%m00nbsd.net@localhost> wrote:
> > > - If we do not want to stick with the C11 API (its emulation), then I
> > > would suggest to use the similar terminology, e.g. atomic_load_relaxed()
> > > and atomic_store_relaxed(), or Linux READ_ONCE()/WRITE_ONCE().  There is
> > > no reason invent new terminology; it might just add more confusion.
> >
> > But... Linux's READ_ONCE/WRITE_ONCE are not actually atomic, is that
> > correct? So there is a significant semantic difference.
>
> They are "atomic" in a sense that they prevent from tearing, fusing and
> invented loads/stores.  Terms and conditions apply (e.g. they assume properly
> aligned and word-sized accesses).  Additionally, READ_ONCE() provides a
> data-dependency barrier, applicable only to DEC Alpha.  I think it was
> the right decision on the Linux side (even though trying to get all the
> data-dependency barriers right these days is kind of a lost cause, IMO).
>
> So, READ_ONCE()/WRITE_ONCE() is more or less equivalent to the C11 atomic
> load/stores routines with memory_order_relaxed (or memory_order_consume,
> if you care about DEC Alpha).

But... Didn't Marco just say that 'volatile' accesses do not actually prevent
tearing/fusing/invented loads/stores? READ_ONCE/WRITE_ONCE only do volatile
accesses.

> > Also, in my original patch I marked two branches of subr_xcall.c, but it
> > seems to me now they are actually races: ie xc_wait(), the read of
> > 'xc_donep' could be made by two 4-byte fetches on 32bit arches (at
> > least). Between the two, an xc thread could have updated the value. So
> > there is an actual race which could possibly result in returning early
> > while we shouldn't have. Does that look correct to you?
>
> Correct.

Alright, so we have the first bug found by KCSAN :)

To fix that, do you agree that I should
 - Remove the first branch (because no lockless fastpath possible)
 - Move down the second branch (KASSERT) right after the mutex_enter
?

> There is more code in the NetBSD kernel which needs fixing.  I would say
> pretty much all lock-free code should be audited.

I believe KCSAN can greatly help with that, since it automatically reports
concurrent accesses. Up to us then to switch to atomic, or other kinds of
markers like READ_ONCE.