Re: __{read,write}_once

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

Le 08/11/2019 à 13:40, Mindaugas Rasiukevicius a écrit :
> Maxime Villard <max%m00nbsd.net@localhost> wrote:
> > > 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.
>
> Let me try to clarify:
>
> - The main purpose of READ_ONCE()/WRITE_ONCE() is to provide a way to
> perform atomic loads/stores (in a sense of preventing from the said
> behaviours), even though they help to get the memory ordering right too.
> Currently, 'volatile' is a key instrument in achieving that.  However,
> as stated before, terms and conditions apply: 'volatile' just in itself
> does not provide the guarantee; the loads/stores also have to be properly
> aligned and word-sized (these are the pre-C11 assumptions we always had).
> Note: C11 introduces atomic _types_, so that the compiler could leverage
> the type system and thus provide the necessary guarantees.
>
> - Having re-read Marco's emails in this thread, I think we are very much
> in agreement.  I think he merely points out that 'volatile' in itself is
> not sufficient; it does not mean it's not necessary.
>
> - There is quite a bit of confusion regarding 'volatile' amongst the
> developers.  This is partly because 'volatile' is arguably underspecified
> in the C standard.  AFAIK, some people in the C standardization committee
> have a view that it provides weaker guarantees; however, others maintain
> that the intent has always been clear and the wording is sufficient.
> Without going into the details (somewhat philosophical anyway), at least
> for now, 'volatile' is a de facto ingredient (one of a few, but absolutely
> necessary) in achieving atomic loads/stores.
>
> Sorry if this is a bit repetitive, but I hope it gets the point across.

Alright, thanks.

Do you think there is a performance degradation with using explicitly atomic
operations (with the "lock" prefix on x86), compared to just using an aligned
volatile which may not be exactly atomic in that sense (even if it happens to
be on x86)?

Typically in sys/uvm/uvm_fault.c there are several lockless stat increments
like the following:

	/* Increment the counters.*/
	uvmexp.fltanget++;

In your (not just rmind@, but tech-kern@) opinion, is it better to switch to:
	atomic_inc_uint(&uvmexp.fltanget);
or to
	__add_once(uvmexp.fltanget, 1);
as I did in my patch? Considering that the latter may possibly not be atomic
on certain arches, which could possibly result in garbage when the value is
read.

> > 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
> > ?
>
> Feel free to write a patch and I'll have a look at it once I have a
> little bit more free time.

I've committed it right away, because this one seemed clear enough, feel free
to comment or improve if you have a better idea:

	https://mail-index.netbsd.org/source-changes/2019/11/11/msg110726.html

Maxime