lkml.org 
[lkml]   [2021]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v2 1/9] atomic: Prepare scripts for macro ops
On Fri, Dec 10, 2021 at 05:16:19PM +0100, Peter Zijlstra wrote:
> Due to the limited usability of the GCC 'Labels as Values' extention,
> specifically it refuses to have a goto in an __always_inline function,
> even with a compile time constant label pointer, some new atomic ops
> will need to be introduced as macros.
>
> The xchg/cmpxchg family is already macros so extend that code to the
> regular atomic ops.
>
> Specifically introduce meta-'M', meta-'m' and meta-'n' to signify the
> op is a macro and add arg-'L', arg-'V' and arg-'P' for Label, VarArg
> and Pair arguments respectively.

Looks like `M` got factored out into the next patch, so I think that can be
deleted from this commit message.

> This unconvered some latent bugs in the instrumentation wrappery,
> specifically the try_cmpxchg() @oldp argument was instrumented as an
> atomic_read_write, while it's a regular read_write and many of the
> gen_xchg() ops had atomic_write instrumentation while
> atomic_read_write seems more appropriate.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> include/linux/atomic/atomic-instrumented.h | 78 +++++++--------
> scripts/atomic/atomic-tbl.sh | 8 +
> scripts/atomic/atomics.tbl | 5 +
> scripts/atomic/gen-atomic-fallback.sh | 4
> scripts/atomic/gen-atomic-instrumented.sh | 142 +++++++++++++++++++----------
> scripts/atomic/gen-atomic-long.sh | 32 +++++-
> 6 files changed, 177 insertions(+), 92 deletions(-)
>

[...]

> --- a/scripts/atomic/atomics.tbl
> +++ b/scripts/atomic/atomics.tbl
> @@ -10,12 +10,17 @@
> # * F/f - fetch: returns base type (has fetch_ variants)
> # * l - load: returns base type (has _acquire order variant)
> # * s - store: returns void (has _release order variant)
> +# * m - macro: with return value
> +# * n - macro: with No return value
> #
> # Where args contains list of type[:name], where type is:
> # * cv - const pointer to atomic base type (atomic_t/atomic64_t/atomic_long_t)
> # * v - pointer to atomic base type (atomic_t/atomic64_t/atomic_long_t)
> +# * P - pointer to pair of atomic base type
> # * i - base type (int/s64/long)
> # * p - pointer to base type (int/s64/long)
> +# * L - label for exception case
> +# * V:... - vararg

Nit: should've been a tab here?

[...]

> @@ -51,57 +112,49 @@ gen_proto_order_variant()
> local order="$1"; shift
> local atomic="$1"; shift
> local int="$1"; shift
> + local atomicname;
>
> - local atomicname="${atomic}_${pfx}${name}${sfx}${order}"
> + if [ -n "${atomic}" ]; then
> + atomicname="${atomic}_${pfx}${name}${sfx}${order}"
> + else
> + atomicname="${pfx}${name}${sfx}${order}"
> + fi

How about:

local atomicname="${pfx}${name}${sfx}${order}"
if [ -n "${atomic}" ]; then
atomicname="${atomic}_${atomicname}"
fi

... so that it's clear we're just adding the atomic type as a prefix, and the
rest of the name should be the same either way?

Regardless of that specifically:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

Thanks,
Mark.

\
 
 \ /
  Last update: 2021-12-10 18:27    [W:0.255 / U:0.352 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site