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

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

[Joerg Roedel]

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

> 
> diff --git a/include/asm-generic/rwonce.h b/include/asm-generic/rwonce.h
> index 8d0a6280e982..8bf942ad5ef3 100644
> --- a/include/asm-generic/rwonce.h
> +++ 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().")
>  
>  /*
> diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
> index 1a957ea2f4fe..54b56ae25db7 100644
> --- a/include/linux/compiler_types.h
> +++ b/include/linux/compiler_types.h
> @@ -469,6 +469,12 @@ struct ftrace_likely_data {
>  		unsigned type:	(unsigned type)0,			\
>  		signed type:	(signed type)0
>  
> +#ifdef __SIZEOF_INT128__
> +#define __dword_type __int128
> +#else
> +#define __dword_type long long
> +#endif
> +
>  #define __unqual_scalar_typeof(x) typeof(				\
>  		_Generic((x),						\
>  			 char:	(char)0,				\
> @@ -476,7 +482,7 @@ struct ftrace_likely_data {
>  			 __scalar_type_to_expr_cases(short),		\
>  			 __scalar_type_to_expr_cases(int),		\
>  			 __scalar_type_to_expr_cases(long),		\
> -			 __scalar_type_to_expr_cases(long long),	\
> +			 __scalar_type_to_expr_cases(__dword_type),	\
>  			 default: (x)))
>  
>  /* Is this type a native word size -- useful for atomic operations */
> -- 
> 2.34.1
> 

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

[Arnd Bergmann]

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.

       Arnd

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

[Uros Bizjak]

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/

FYI, Processors with AVX guarantee 128bit atomic access with SSE
128bit move instructions, see e.g. [2].

[2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688

Uros.

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

[Jason Gunthorpe]

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

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

[Arnd Bergmann]

On Wed, Nov 6, 2024, at 14:40, Jason Gunthorpe wrote:
> 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/

I understand the special case for ARMv7VE. I think the more important
comment in that file is

  * Use __READ_ONCE() instead of READ_ONCE() if you do not require any
  * atomicity. Note that this may result in tears!

The entire point of compiletime_assert_rwonce_type() is to ensure
that these are accesses fit the stricter definition, and I would
prefer to not extend that to 64-bit architecture. If there are users
that need the "once" behavior but not require atomicity of the
access, can't that just use __READ_ONCE() instead?

> 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..

Right, at the minimum, we'd need to separate READ_ONCE()/WRITE_ONCE()
from the smp_load_acquire()/smp_store_release() definitions in
asm/barrier.h. Those certainly don't work beyond word size aside
from a few special cases.

>> FYI, Processors with AVX guarantee 128bit atomic access with SSE
>> 128bit move instructions, see e.g. [2].
>> 
>> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688

AVX instructions are not used in the kernel. If you want atomic
loads, that has to rely on architecture specific instructions
like cmpxchg16b on x86-64 or ldp on arm64. Actually using these
requires checking the system_has_cmpxchg128() macro.

   Arnd

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

[Uros Bizjak]

[-- Attachment #1: Type: text/plain, Size: 3213 bytes --]

On Thu, Nov 7, 2024 at 11:02 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
>
>
> On Wed, Nov 6, 2024, at 14:40, Jason Gunthorpe wrote:
> > 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/
>
> I understand the special case for ARMv7VE. I think the more important
> comment in that file is
>
>   * Use __READ_ONCE() instead of READ_ONCE() if you do not require any
>   * atomicity. Note that this may result in tears!
>
> The entire point of compiletime_assert_rwonce_type() is to ensure
> that these are accesses fit the stricter definition, and I would
> prefer to not extend that to 64-bit architecture. If there are users
> that need the "once" behavior but not require atomicity of the
> access, can't that just use __READ_ONCE() instead?

If this is the case, then the patch could be simply something like the
attached (untested) patch.

Thanks,
Uros.

[-- Attachment #2: p.diff.txt --]
[-- Type: text/plain, Size: 1274 bytes --]

diff --git a/include/linux/compiler_types.h b/include/linux/compiler_types.h
index 1a957ea2f4fe..94a18e8ccf7e 100644
--- a/include/linux/compiler_types.h
+++ b/include/linux/compiler_types.h
@@ -469,7 +469,8 @@ struct ftrace_likely_data {
 		unsigned type:	(unsigned type)0,			\
 		signed type:	(signed type)0
 
-#define __unqual_scalar_typeof(x) typeof(				\
+#if defined(CONFIG_ARCH_SUPPORTS_INT128) && defined(__SIZEOF_INT128__)
+ #define __unqual_scalar_typeof(x) typeof(				\
 		_Generic((x),						\
 			 char:	(char)0,				\
 			 __scalar_type_to_expr_cases(char),		\
@@ -477,7 +478,19 @@ struct ftrace_likely_data {
 			 __scalar_type_to_expr_cases(int),		\
 			 __scalar_type_to_expr_cases(long),		\
 			 __scalar_type_to_expr_cases(long long),	\
+			 __scalar_type_to_expr_cases(__int128),		\
 			 default: (x)))
+#else
+ #define __unqual_scalar_typeof(x) typeof(				\
+		_Generic((x),						\
+			 char:	(char)0,				\
+			 __scalar_type_to_expr_cases(char),		\
+			 __scalar_type_to_expr_cases(short),		\
+			 __scalar_type_to_expr_cases(int),		\
+			 __scalar_type_to_expr_cases(long),		\
+			 __scalar_type_to_expr_cases(long long),	\
+			 default: (x)))
+#endif
 
 /* Is this type a native word size -- useful for atomic operations */
 #define __native_word(t) \

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

[Jason Gunthorpe]

On Thu, Nov 07, 2024 at 11:01:58AM +0100, Arnd Bergmann wrote:

> >> 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/
> 
> I understand the special case for ARMv7VE. I think the more important
> comment in that file is
> 
>   * Use __READ_ONCE() instead of READ_ONCE() if you do not require any
>   * atomicity. Note that this may result in tears!

That makes sense, let's just use that and there is no need to change
anything here?

Uros?

> >> FYI, Processors with AVX guarantee 128bit atomic access with SSE
> >> 128bit move instructions, see e.g. [2].
> >> 
> >> [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=104688
> 
> AVX instructions are not used in the kernel. If you want atomic
> loads, that has to rely on architecture specific instructions
> like cmpxchg16b on x86-64 or ldp on arm64. Actually using these
> requires checking the system_has_cmpxchg128() macro.

Yeah, that is what this series is doing..

Jason

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

[Uros Bizjak]

On Thu, Nov 7, 2024 at 2:37 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Thu, Nov 07, 2024 at 11:01:58AM +0100, Arnd Bergmann wrote:
>
> > >> 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/
> >
> > I understand the special case for ARMv7VE. I think the more important
> > comment in that file is
> >
> >   * Use __READ_ONCE() instead of READ_ONCE() if you do not require any
> >   * atomicity. Note that this may result in tears!
>
> That makes sense, let's just use that and there is no need to change
> anything here?
>
> Uros?

Yes, preloading "old" value for try_cmpxchg loop does not need to be
atomic (cmpxchg will fail in case teared value is preloaded and loop
will be retried). So, __READ_ONCE() is perfectly OK to be used in this
series.

Please note that __READ_ONCE() uses  __unqual_scalar_typeof()
operator, so at least patch at [1] to teach __uqual_scalar_typeof()
about __int128 is needed.

[1] https://lore.kernel.org/lkml/CAFULd4Z86uiH+w+1N36kOuhYZ5_ZkQkaEN6nyPh8VNJth3WNhg@mail.gmail.com/

Uros.