Re: [PATCH v9 03/10] asm/rwonce: Introduce [READ|WRITE]_ONCE() support for __int128

[Jason Gunthorpe <jgg@nvidia.com>]

On Wed, Nov 06, 2024 at 11:01:20AM +0100, Uros Bizjak wrote:
> On Wed, Nov 6, 2024 at 9:55 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Nov 5, 2024, at 13:30, Joerg Roedel wrote:
> > > On Fri, Nov 01, 2024 at 04:22:57PM +0000, Suravee Suthikulpanit wrote:
> > >>  include/asm-generic/rwonce.h   | 2 +-
> > >>  include/linux/compiler_types.h | 8 +++++++-
> > >>  2 files changed, 8 insertions(+), 2 deletions(-)
> > >
> > > This patch needs Cc:
> > >
> > >       Arnd Bergmann <arnd@arndb.de>
> > >       linux-arch@vger.kernel.org
> > >
> >
> > It also needs an update to the comment about why this is safe:
> >
> > >> +++ b/include/asm-generic/rwonce.h
> > >> @@ -33,7 +33,7 @@
> > >>   * (e.g. a virtual address) and a strong prevailing wind.
> > >>   */
> > >>  #define compiletime_assert_rwonce_type(t)                                   \
> > >> -    compiletime_assert(__native_word(t) || sizeof(t) == sizeof(long long),  \
> > >> +    compiletime_assert(__native_word(t) || sizeof(t) == sizeof(__dword_type), \
> > >>              "Unsupported access size for {READ,WRITE}_ONCE().")
> >
> > As far as I can tell, 128-but words don't get stored atomically on
> > any architecture, so this seems wrong, because it would remove
> > the assertion on someone incorrectly using WRITE_ONCE() on a
> > 128-bit variable.
> 
> READ_ONCE() and WRITE_ONCE() do not guarantee atomicity for double
> word types. They only guarantee (c.f. include/asm/generic/rwonce.h):
> 
>  * Prevent the compiler from merging or refetching reads or writes. The
>  * compiler is also forbidden from reordering successive instances of
>  * READ_ONCE and WRITE_ONCE, but only when the compiler is aware of some
>  * particular ordering. ...
> 
> and later:
> 
>  * Yes, this permits 64-bit accesses on 32-bit architectures. These will
>  * actually be atomic in some cases (namely Armv7 + LPAE), but for others we
>  * rely on the access being split into 2x32-bit accesses for a 32-bit quantity
>  * (e.g. a virtual address) and a strong prevailing wind.
> 
> This is the "strong prevailing wind", mentioned in the patch review at [1].
> 
> [1] https://lore.kernel.org/lkml/20241016130819.GJ3559746@nvidia.com/

Yes, there are two common uses for READ_ONCE, actually "read once" and
prevent compiler double read "optimizations". It doesn't matter if
things tear in this case because it would be used with cmpxchg or
something like that.

The other is an atomic relaxed memory order read, which would
have to guarentee non-tearing.

It is unfortunate the kernel has combined these two things, and we
probably have exciting bugs on 32 bit from places using the atomic
read variation on a u64..

Jason