Received-SPF: pass (google.com: domain of linux-kernel+bounces-179909-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <de92101c-f9c4-4af4-95f4-19a6f59b636f@paulmck-laptop>
 <d037f37a-4722-4a1d-a282-63355a97a1a1@acm.org> <c83d9c25-b839-4e31-8dd4-85f3cb938653@paulmck-laptop>
 <4d230bac-bdb0-4a01-8006-e95156965aa8@acm.org> <447ad732-3ff8-40bf-bd82-f7be66899cee@paulmck-laptop>
 <ca7c2ef0-7e21-4fb3-ac6b-3dae652a7a0e@acm.org> <59ec96c2-52ce-4da1-92c3-9fe38053cd3d@paulmck-laptop>
 <CANpmjNMj9r1V6Z63fcJxrFC1v4i2vUCEhm1HT77ikxhx0Rghdw@mail.gmail.com>
 <dd251dba-0a63-4b57-a05b-bfa02615fae5@paulmck-laptop> <CANpmjNMqRUNUs1mZEhrOSyK0Hk+PdGOi+VAs22qYD+1zTkwfhg@mail.gmail.com>
 <ZkSvKbWj1lhGZvLE@gmail.com>
In-Reply-To: <ZkSvKbWj1lhGZvLE@gmail.com>
From: Marco Elver <elver@google.com>
Date: Wed, 15 May 2024 15:20:41 +0200
Message-ID: <CANpmjNP1ajhz5kSuJDv+go=UUvoSaQM2BVphX9LcDA9iGR5A1Q@mail.gmail.com>
Subject: Re: [PATCH] block: Annotate a racy read in blk_do_io_stat()
To: Breno Leitao <leitao@debian.org>
Cc: paulmck@kernel.org, Bart Van Assche <bvanassche@acm.org>, Jens Axboe <axboe@kernel.dk>, 
	"open list:BLOCK LAYER" <linux-block@vger.kernel.org>, open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"

On Wed, 15 May 2024 at 14:48, Breno Leitao <leitao@debian.org> wrote:
>
> On Wed, May 15, 2024 at 09:58:35AM +0200, Marco Elver wrote:
> > On Wed, 15 May 2024 at 01:47, Paul E. McKenney <paulmck@kernel.org> wrote:
> > > On Mon, May 13, 2024 at 10:13:49AM +0200, Marco Elver wrote:
> > > +Use of volatile and __data_racy
> > > +-------------------------------
> > > +
> > > +Adding the volatile keyword to the declaration of a variable causes both
> > > +the compiler and KCSAN to treat all reads from that variable as if they
> > > +were protected by READ_ONCE() and all writes to that variable as if they
> > > +were protected by WRITE_ONCE().
>
> > "volatile" isn't something we encourage, right? In which case, I think
> > to avoid confusion we should not mention volatile. After all we have
> > this: Documentation/process/volatile-considered-harmful.rst
>
> Since you mentioned this document, the other day I was reading
> volatile-considered-harmful.rst document, and I was surprised that there
> is no reference for READ_ONCE() primitive at all (same for WRITE_ONCE).
>
>         # grep -c READ_ONCE Documentation/process/volatile-considered-harmful.rst
>         0
>
> From my perspective, READ_ONCE() is another way of doing real memory
> read (volatile) when you really need, at the same time keeping the
> compiler free to optimize and reuse the value that was read.

The Linux kernel memory model [1] defines the semantics of READ_ONCE()
/ WRITE_ONCE(). You could say that a READ_ONCE() is something like a
"relaxed (but sometimes consume) atomic load" (in C11 lingo), and a
WRITE_ONCE() is a "relaxed atomic store". But again, not exactly
because the kernel wants to do things that are outside the C standard
and no compiler fully supports. This has fun consequences, such as
[2].

[1] https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p0124r6.html
[2] https://lpc.events/event/16/contributions/1174/attachments/1108/2121/Status%20Report%20-%20Broken%20Dependency%20Orderings%20in%20the%20Linux%20Kernel.pdf

To get what the kernel wants, implementing *ONCE in terms of
"volatile" works in all important cases. But we know that it can go
wrong - [2] discusses this.

The big hope is that one day, the kernel can just switch the *ONCE()
implementation to use something better but as a programmer we
shouldn't care that there's volatile underneath.

> Should volatile-considered-harmful.rst be also expanded to cover
> READ_ONCE()?

No, on most architectures READ_ONCE/WRITE_ONCE are implemented with
volatile, but that's merely an implementation detail. Once upon a
time, READ_ONCE on alpha actually implied a real barrier. On arm64
with LTO, READ_ONCE translates into an acquire-load [3] to mitigate
the risk of "volatile" actually resulting in "miscompilation"
(according to our desired semantics of READ_ONCE).

[3] https://elixir.bootlin.com/linux/latest/source/arch/arm64/include/asm/rwonce.h#L26

Thanks,
-- Marco