tech-kern archive

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index][Old Index]

Re: __{read,write}_once

Le 06/11/2019 à 23:41, Mindaugas Rasiukevicius a écrit :
Maxime Villard <> 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

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?


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.

Home | Main Index | Thread Index | Old Index