Messages in this thread | | | From | Nick Desaulniers <> | Date | Mon, 18 May 2020 15:28:50 -0700 | Subject | Re: [PATCH 7/7] x86/percpu: Clean up percpu_cmpxchg_op() |
| |
On Sun, May 17, 2020 at 8:29 AM Brian Gerst <brgerst@gmail.com> wrote: > > The core percpu macros already have a switch on the data size, so the switch > in the x86 code is redundant and produces more dead code. > > Also use appropriate types for the width of the instructions. This avoids > errors when compiling with Clang. > > Signed-off-by: Brian Gerst <brgerst@gmail.com> > --- > arch/x86/include/asm/percpu.h | 58 +++++++++++------------------------ > 1 file changed, 18 insertions(+), 40 deletions(-) > > diff --git a/arch/x86/include/asm/percpu.h b/arch/x86/include/asm/percpu.h > index 3c95ab3c99cd..b61d4fc5568e 100644 > --- a/arch/x86/include/asm/percpu.h > +++ b/arch/x86/include/asm/percpu.h > @@ -236,39 +236,17 @@ do { \ > * cmpxchg has no such implied lock semantics as a result it is much > * more efficient for cpu local operations. > */ > -#define percpu_cmpxchg_op(qual, var, oval, nval) \ > +#define percpu_cmpxchg_op(size, qual, _var, _oval, _nval) \ > ({ \ > - typeof(var) pco_ret__; \ > - typeof(var) pco_old__ = (oval); \ > - typeof(var) pco_new__ = (nval); \ > - switch (sizeof(var)) { \ > - case 1: \ > - asm qual ("cmpxchgb %2, "__percpu_arg(1) \ > - : "=a" (pco_ret__), "+m" (var) \ > - : "q" (pco_new__), "0" (pco_old__) \ > - : "memory"); \ > - break; \ > - case 2: \ > - asm qual ("cmpxchgw %2, "__percpu_arg(1) \ > - : "=a" (pco_ret__), "+m" (var) \ > - : "r" (pco_new__), "0" (pco_old__) \ > - : "memory"); \ > - break; \ > - case 4: \ > - asm qual ("cmpxchgl %2, "__percpu_arg(1) \ > - : "=a" (pco_ret__), "+m" (var) \ > - : "r" (pco_new__), "0" (pco_old__) \ > - : "memory"); \ > - break; \ > - case 8: \ > - asm qual ("cmpxchgq %2, "__percpu_arg(1) \ > - : "=a" (pco_ret__), "+m" (var) \ > - : "r" (pco_new__), "0" (pco_old__) \ > - : "memory"); \ > - break; \ > - default: __bad_percpu_size(); \ > - } \ > - pco_ret__; \ > + __pcpu_type_##size pco_old__ = __pcpu_cast_##size(_oval); \ > + __pcpu_type_##size pco_new__ = __pcpu_cast_##size(_nval); \ > + asm qual (__pcpu_op2_##size("cmpxchg", "%[nval]", \ > + __percpu_arg([var])) \ > + : [oval] "+a" (pco_old__), \ > + [var] "+m" (_var) \ > + : [nval] __pcpu_reg_##size(, pco_new__) \
Looks like we're no longer using "=a" and "0" constraints. Looking these up for reference for other reviewers:
"0" [0]: Input constraints can also be digits (for example, "0"). This indicates that the specified input must be in the same place as the output constraint at the (zero-based) index in the output constraint list. When using asmSymbolicName syntax for the output operands, you may use these names (enclosed in brackets ‘[]’) instead of digits. [0] https://gcc.gnu.org/onlinedocs/gcc/Extended-Asm.html#Extended-Asm
"+" [1]: Means that this operand is both read and written by the instruction. [1] https://gcc.gnu.org/onlinedocs/gcc/Modifiers.html#Modifiers
"=" [1]: Means that this operand is written to by this instruction: the previous value is discarded and replaced by new data.
So this looks like a nice simplification. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> + : "memory"); \ > + (typeof(_var))(unsigned long) pco_old__; \ > }) > > /* > @@ -336,16 +314,16 @@ do { \ > #define raw_cpu_add_return_1(pcp, val) percpu_add_return_op(1, , pcp, val) > #define raw_cpu_add_return_2(pcp, val) percpu_add_return_op(2, , pcp, val) > #define raw_cpu_add_return_4(pcp, val) percpu_add_return_op(4, , pcp, val) > -#define raw_cpu_cmpxchg_1(pcp, oval, nval) percpu_cmpxchg_op(, pcp, oval, nval) > -#define raw_cpu_cmpxchg_2(pcp, oval, nval) percpu_cmpxchg_op(, pcp, oval, nval) > -#define raw_cpu_cmpxchg_4(pcp, oval, nval) percpu_cmpxchg_op(, pcp, oval, nval) > +#define raw_cpu_cmpxchg_1(pcp, oval, nval) percpu_cmpxchg_op(1, , pcp, oval, nval) > +#define raw_cpu_cmpxchg_2(pcp, oval, nval) percpu_cmpxchg_op(2, , pcp, oval, nval) > +#define raw_cpu_cmpxchg_4(pcp, oval, nval) percpu_cmpxchg_op(4, , pcp, oval, nval) > > #define this_cpu_add_return_1(pcp, val) percpu_add_return_op(1, volatile, pcp, val) > #define this_cpu_add_return_2(pcp, val) percpu_add_return_op(2, volatile, pcp, val) > #define this_cpu_add_return_4(pcp, val) percpu_add_return_op(4, volatile, pcp, val) > -#define this_cpu_cmpxchg_1(pcp, oval, nval) percpu_cmpxchg_op(volatile, pcp, oval, nval) > -#define this_cpu_cmpxchg_2(pcp, oval, nval) percpu_cmpxchg_op(volatile, pcp, oval, nval) > -#define this_cpu_cmpxchg_4(pcp, oval, nval) percpu_cmpxchg_op(volatile, pcp, oval, nval) > +#define this_cpu_cmpxchg_1(pcp, oval, nval) percpu_cmpxchg_op(1, volatile, pcp, oval, nval) > +#define this_cpu_cmpxchg_2(pcp, oval, nval) percpu_cmpxchg_op(2, volatile, pcp, oval, nval) > +#define this_cpu_cmpxchg_4(pcp, oval, nval) percpu_cmpxchg_op(4, volatile, pcp, oval, nval) > > #ifdef CONFIG_X86_CMPXCHG64 > #define percpu_cmpxchg8b_double(pcp1, pcp2, o1, o2, n1, n2) \ > @@ -376,7 +354,7 @@ do { \ > #define raw_cpu_or_8(pcp, val) percpu_to_op(8, , "or", (pcp), val) > #define raw_cpu_add_return_8(pcp, val) percpu_add_return_op(8, , pcp, val) > #define raw_cpu_xchg_8(pcp, nval) raw_percpu_xchg_op(pcp, nval) > -#define raw_cpu_cmpxchg_8(pcp, oval, nval) percpu_cmpxchg_op(, pcp, oval, nval) > +#define raw_cpu_cmpxchg_8(pcp, oval, nval) percpu_cmpxchg_op(8, , pcp, oval, nval) > > #define this_cpu_read_8(pcp) percpu_from_op(8, volatile, "mov", pcp) > #define this_cpu_write_8(pcp, val) percpu_to_op(8, volatile, "mov", (pcp), val) > @@ -385,7 +363,7 @@ do { \ > #define this_cpu_or_8(pcp, val) percpu_to_op(8, volatile, "or", (pcp), val) > #define this_cpu_add_return_8(pcp, val) percpu_add_return_op(8, volatile, pcp, val) > #define this_cpu_xchg_8(pcp, nval) percpu_xchg_op(8, volatile, pcp, nval) > -#define this_cpu_cmpxchg_8(pcp, oval, nval) percpu_cmpxchg_op(volatile, pcp, oval, nval) > +#define this_cpu_cmpxchg_8(pcp, oval, nval) percpu_cmpxchg_op(8, volatile, pcp, oval, nval) > > /* > * Pretty complex macro to generate cmpxchg16 instruction. The instruction > -- > 2.25.4 >
-- Thanks, ~Nick Desaulniers
| |