| Message ID | 20240103163203.72768-3-leobras@redhat.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 4bfa185fe3f0b20ebb6e7224dae771fcb657f260 |
| Headers | show |
| Series | Rework & improve riscv cmpxchg.h and atomic.h | expand |
On Wed, Jan 03, 2024 at 01:31:59PM -0300, Leonardo Bras wrote: > In this header every xchg define (_relaxed, _acquire, _release, vanilla) > contain it's own asm file, both for 4-byte variables an 8-byte variables, > on a total of 8 versions of mostly the same asm. > > This is usually bad, as it means any change may be done in up to 8 > different places. > > Unify those versions by creating a new define with enough parameters to > generate any version of the previous 8. > > Then unify the result under a more general define, and simplify > arch_xchg* generation. > > (This did not cause any change in generated asm) > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > Reviewed-by: Guo Ren <guoren@kernel.org> > Reviewed-by: Andrea Parri <parri.andrea@gmail.com> > Tested-by: Guo Ren <guoren@kernel.org> > --- > arch/riscv/include/asm/cmpxchg.h | 138 ++++++------------------------- > 1 file changed, 23 insertions(+), 115 deletions(-) > > diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h > index 2f4726d3cfcc2..48478a8eecee7 100644 > --- a/arch/riscv/include/asm/cmpxchg.h > +++ b/arch/riscv/include/asm/cmpxchg.h > @@ -11,140 +11,48 @@ > #include <asm/barrier.h> > #include <asm/fence.h> > > -#define __xchg_relaxed(ptr, new, size) \ > +#define __arch_xchg(sfx, prepend, append, r, p, n) \ > ({ \ > - __typeof__(ptr) __ptr = (ptr); \ > - __typeof__(new) __new = (new); \ > - __typeof__(*(ptr)) __ret; \ > - switch (size) { \ > - case 4: \ > - __asm__ __volatile__ ( \ > - " amoswap.w %0, %2, %1\n" \ > - : "=r" (__ret), "+A" (*__ptr) \ > - : "r" (__new) \ > - : "memory"); \ Hmm... actually xchg_relaxed() doesn't need to be a barrier(), so the "memory" clobber here is not needed here. Of course, it's out of the scope of this series, but I'm curious to see what would happen if we remove the "memory" clobber _relaxed() ;-) Regards, Boqun > - break; \ > - case 8: \ > - __asm__ __volatile__ ( \ > - " amoswap.d %0, %2, %1\n" \ > - : "=r" (__ret), "+A" (*__ptr) \ > - : "r" (__new) \ > - : "memory"); \ > - break; \ > - default: \ > - BUILD_BUG(); \ > - } \ > - __ret; \ > -}) > - > -#define arch_xchg_relaxed(ptr, x) \ > -({ \ > - __typeof__(*(ptr)) _x_ = (x); \ > - (__typeof__(*(ptr))) __xchg_relaxed((ptr), \ > - _x_, sizeof(*(ptr))); \ > + __asm__ __volatile__ ( \ > + prepend \ > + " amoswap" sfx " %0, %2, %1\n" \ > + append \ > + : "=r" (r), "+A" (*(p)) \ > + : "r" (n) \ > + : "memory"); \ > }) > > -#define __xchg_acquire(ptr, new, size) \ > +#define _arch_xchg(ptr, new, sfx, prepend, append) \ > ({ \ > __typeof__(ptr) __ptr = (ptr); \ > - __typeof__(new) __new = (new); \ > - __typeof__(*(ptr)) __ret; \ > - switch (size) { \ > + __typeof__(*(__ptr)) __new = (new); \ > + __typeof__(*(__ptr)) __ret; \ > + switch (sizeof(*__ptr)) { \ > case 4: \ > - __asm__ __volatile__ ( \ > - " amoswap.w %0, %2, %1\n" \ > - RISCV_ACQUIRE_BARRIER \ > - : "=r" (__ret), "+A" (*__ptr) \ > - : "r" (__new) \ > - : "memory"); \ > + __arch_xchg(".w" sfx, prepend, append, \ > + __ret, __ptr, __new); \ > break; \ > case 8: \ > - __asm__ __volatile__ ( \ > - " amoswap.d %0, %2, %1\n" \ > - RISCV_ACQUIRE_BARRIER \ > - : "=r" (__ret), "+A" (*__ptr) \ > - : "r" (__new) \ > - : "memory"); \ > + __arch_xchg(".d" sfx, prepend, append, \ > + __ret, __ptr, __new); \ > break; \ > default: \ > BUILD_BUG(); \ > } \ > - __ret; \ > + (__typeof__(*(__ptr)))__ret; \ > }) > > -#define arch_xchg_acquire(ptr, x) \ > -({ \ > - __typeof__(*(ptr)) _x_ = (x); \ > - (__typeof__(*(ptr))) __xchg_acquire((ptr), \ > - _x_, sizeof(*(ptr))); \ > -}) > +#define arch_xchg_relaxed(ptr, x) \ > + _arch_xchg(ptr, x, "", "", "") > > -#define __xchg_release(ptr, new, size) \ > -({ \ > - __typeof__(ptr) __ptr = (ptr); \ > - __typeof__(new) __new = (new); \ > - __typeof__(*(ptr)) __ret; \ > - switch (size) { \ > - case 4: \ > - __asm__ __volatile__ ( \ > - RISCV_RELEASE_BARRIER \ > - " amoswap.w %0, %2, %1\n" \ > - : "=r" (__ret), "+A" (*__ptr) \ > - : "r" (__new) \ > - : "memory"); \ > - break; \ > - case 8: \ > - __asm__ __volatile__ ( \ > - RISCV_RELEASE_BARRIER \ > - " amoswap.d %0, %2, %1\n" \ > - : "=r" (__ret), "+A" (*__ptr) \ > - : "r" (__new) \ > - : "memory"); \ > - break; \ > - default: \ > - BUILD_BUG(); \ > - } \ > - __ret; \ > -}) > +#define arch_xchg_acquire(ptr, x) \ > + _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER) > > #define arch_xchg_release(ptr, x) \ > -({ \ > - __typeof__(*(ptr)) _x_ = (x); \ > - (__typeof__(*(ptr))) __xchg_release((ptr), \ > - _x_, sizeof(*(ptr))); \ > -}) > - > -#define __arch_xchg(ptr, new, size) \ > -({ \ > - __typeof__(ptr) __ptr = (ptr); \ > - __typeof__(new) __new = (new); \ > - __typeof__(*(ptr)) __ret; \ > - switch (size) { \ > - case 4: \ > - __asm__ __volatile__ ( \ > - " amoswap.w.aqrl %0, %2, %1\n" \ > - : "=r" (__ret), "+A" (*__ptr) \ > - : "r" (__new) \ > - : "memory"); \ > - break; \ > - case 8: \ > - __asm__ __volatile__ ( \ > - " amoswap.d.aqrl %0, %2, %1\n" \ > - : "=r" (__ret), "+A" (*__ptr) \ > - : "r" (__new) \ > - : "memory"); \ > - break; \ > - default: \ > - BUILD_BUG(); \ > - } \ > - __ret; \ > -}) > + _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "") > > #define arch_xchg(ptr, x) \ > -({ \ > - __typeof__(*(ptr)) _x_ = (x); \ > - (__typeof__(*(ptr))) __arch_xchg((ptr), _x_, sizeof(*(ptr))); \ > -}) > + _arch_xchg(ptr, x, ".aqrl", "", "") > > #define xchg32(ptr, x) \ > ({ \ > -- > 2.43.0 >
On Thu, Jan 04, 2024 at 11:53:45AM -0800, Boqun Feng wrote: > On Wed, Jan 03, 2024 at 01:31:59PM -0300, Leonardo Bras wrote: > > In this header every xchg define (_relaxed, _acquire, _release, vanilla) > > contain it's own asm file, both for 4-byte variables an 8-byte variables, > > on a total of 8 versions of mostly the same asm. > > > > This is usually bad, as it means any change may be done in up to 8 > > different places. > > > > Unify those versions by creating a new define with enough parameters to > > generate any version of the previous 8. > > > > Then unify the result under a more general define, and simplify > > arch_xchg* generation. > > > > (This did not cause any change in generated asm) > > > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > Reviewed-by: Guo Ren <guoren@kernel.org> > > Reviewed-by: Andrea Parri <parri.andrea@gmail.com> > > Tested-by: Guo Ren <guoren@kernel.org> > > --- > > arch/riscv/include/asm/cmpxchg.h | 138 ++++++------------------------- > > 1 file changed, 23 insertions(+), 115 deletions(-) > > > > diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h > > index 2f4726d3cfcc2..48478a8eecee7 100644 > > --- a/arch/riscv/include/asm/cmpxchg.h > > +++ b/arch/riscv/include/asm/cmpxchg.h > > @@ -11,140 +11,48 @@ > > #include <asm/barrier.h> > > #include <asm/fence.h> > > > > -#define __xchg_relaxed(ptr, new, size) \ > > +#define __arch_xchg(sfx, prepend, append, r, p, n) \ > > ({ \ > > - __typeof__(ptr) __ptr = (ptr); \ > > - __typeof__(new) __new = (new); \ > > - __typeof__(*(ptr)) __ret; \ > > - switch (size) { \ > > - case 4: \ > > - __asm__ __volatile__ ( \ > > - " amoswap.w %0, %2, %1\n" \ > > - : "=r" (__ret), "+A" (*__ptr) \ > > - : "r" (__new) \ > > - : "memory"); \ Hello Boqun, thanks for reviewing! > > Hmm... actually xchg_relaxed() doesn't need to be a barrier(), so the > "memory" clobber here is not needed here. Of course, it's out of the > scope of this series, but I'm curious to see what would happen if we > remove the "memory" clobber _relaxed() ;-) Nice question :) I am happy my patch can help bring up those ideas :) According to gcc.gnu.org: --- "memory" [clobber]: The "memory" clobber tells the compiler that the assembly code performs memory reads or writes to items other than those listed in the input and output operands (for example, accessing the memory pointed to by one of the input parameters). To ensure memory contains correct values, GCC may need to flush specific register values to memory before executing the asm. Further, the compiler does not assume that any values read from memory before an asm remain unchanged after that asm ; it reloads them as needed. Using the "memory" clobber effectively forms a read/write memory barrier for the compiler. Note that this clobber does not prevent the processor from doing speculative reads past the asm statement. To prevent that, you need processor-specific fence instructions. --- IIUC above text says that having memory accesses to *__ptr would require above asm to have the "memory" clobber, so memory accesses don't get reordered by the compiler. By above affirmation, all asm in this file should have the "memory" clobber, since all atomic operations will change memory pointed by an input ptr. Is that correct? Thanks! Leo > > Regards, > Boqun > > > - break; \ > > - case 8: \ > > - __asm__ __volatile__ ( \ > > - " amoswap.d %0, %2, %1\n" \ > > - : "=r" (__ret), "+A" (*__ptr) \ > > - : "r" (__new) \ > > - : "memory"); \ > > - break; \ > > - default: \ > > - BUILD_BUG(); \ > > - } \ > > - __ret; \ > > -}) > > - > > -#define arch_xchg_relaxed(ptr, x) \ > > -({ \ > > - __typeof__(*(ptr)) _x_ = (x); \ > > - (__typeof__(*(ptr))) __xchg_relaxed((ptr), \ > > - _x_, sizeof(*(ptr))); \ > > + __asm__ __volatile__ ( \ > > + prepend \ > > + " amoswap" sfx " %0, %2, %1\n" \ > > + append \ > > + : "=r" (r), "+A" (*(p)) \ > > + : "r" (n) \ > > + : "memory"); \ > > }) > > > > -#define __xchg_acquire(ptr, new, size) \ > > +#define _arch_xchg(ptr, new, sfx, prepend, append) \ > > ({ \ > > __typeof__(ptr) __ptr = (ptr); \ > > - __typeof__(new) __new = (new); \ > > - __typeof__(*(ptr)) __ret; \ > > - switch (size) { \ > > + __typeof__(*(__ptr)) __new = (new); \ > > + __typeof__(*(__ptr)) __ret; \ > > + switch (sizeof(*__ptr)) { \ > > case 4: \ > > - __asm__ __volatile__ ( \ > > - " amoswap.w %0, %2, %1\n" \ > > - RISCV_ACQUIRE_BARRIER \ > > - : "=r" (__ret), "+A" (*__ptr) \ > > - : "r" (__new) \ > > - : "memory"); \ > > + __arch_xchg(".w" sfx, prepend, append, \ > > + __ret, __ptr, __new); \ > > break; \ > > case 8: \ > > - __asm__ __volatile__ ( \ > > - " amoswap.d %0, %2, %1\n" \ > > - RISCV_ACQUIRE_BARRIER \ > > - : "=r" (__ret), "+A" (*__ptr) \ > > - : "r" (__new) \ > > - : "memory"); \ > > + __arch_xchg(".d" sfx, prepend, append, \ > > + __ret, __ptr, __new); \ > > break; \ > > default: \ > > BUILD_BUG(); \ > > } \ > > - __ret; \ > > + (__typeof__(*(__ptr)))__ret; \ > > }) > > > > -#define arch_xchg_acquire(ptr, x) \ > > -({ \ > > - __typeof__(*(ptr)) _x_ = (x); \ > > - (__typeof__(*(ptr))) __xchg_acquire((ptr), \ > > - _x_, sizeof(*(ptr))); \ > > -}) > > +#define arch_xchg_relaxed(ptr, x) \ > > + _arch_xchg(ptr, x, "", "", "") > > > > -#define __xchg_release(ptr, new, size) \ > > -({ \ > > - __typeof__(ptr) __ptr = (ptr); \ > > - __typeof__(new) __new = (new); \ > > - __typeof__(*(ptr)) __ret; \ > > - switch (size) { \ > > - case 4: \ > > - __asm__ __volatile__ ( \ > > - RISCV_RELEASE_BARRIER \ > > - " amoswap.w %0, %2, %1\n" \ > > - : "=r" (__ret), "+A" (*__ptr) \ > > - : "r" (__new) \ > > - : "memory"); \ > > - break; \ > > - case 8: \ > > - __asm__ __volatile__ ( \ > > - RISCV_RELEASE_BARRIER \ > > - " amoswap.d %0, %2, %1\n" \ > > - : "=r" (__ret), "+A" (*__ptr) \ > > - : "r" (__new) \ > > - : "memory"); \ > > - break; \ > > - default: \ > > - BUILD_BUG(); \ > > - } \ > > - __ret; \ > > -}) > > +#define arch_xchg_acquire(ptr, x) \ > > + _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER) > > > > #define arch_xchg_release(ptr, x) \ > > -({ \ > > - __typeof__(*(ptr)) _x_ = (x); \ > > - (__typeof__(*(ptr))) __xchg_release((ptr), \ > > - _x_, sizeof(*(ptr))); \ > > -}) > > - > > -#define __arch_xchg(ptr, new, size) \ > > -({ \ > > - __typeof__(ptr) __ptr = (ptr); \ > > - __typeof__(new) __new = (new); \ > > - __typeof__(*(ptr)) __ret; \ > > - switch (size) { \ > > - case 4: \ > > - __asm__ __volatile__ ( \ > > - " amoswap.w.aqrl %0, %2, %1\n" \ > > - : "=r" (__ret), "+A" (*__ptr) \ > > - : "r" (__new) \ > > - : "memory"); \ > > - break; \ > > - case 8: \ > > - __asm__ __volatile__ ( \ > > - " amoswap.d.aqrl %0, %2, %1\n" \ > > - : "=r" (__ret), "+A" (*__ptr) \ > > - : "r" (__new) \ > > - : "memory"); \ > > - break; \ > > - default: \ > > - BUILD_BUG(); \ > > - } \ > > - __ret; \ > > -}) > > + _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "") > > > > #define arch_xchg(ptr, x) \ > > -({ \ > > - __typeof__(*(ptr)) _x_ = (x); \ > > - (__typeof__(*(ptr))) __arch_xchg((ptr), _x_, sizeof(*(ptr))); \ > > -}) > > + _arch_xchg(ptr, x, ".aqrl", "", "") > > > > #define xchg32(ptr, x) \ > > ({ \ > > -- > > 2.43.0 > > >
On Thu, Jan 04, 2024 at 05:41:26PM -0300, Leonardo Bras wrote: > On Thu, Jan 04, 2024 at 11:53:45AM -0800, Boqun Feng wrote: > > On Wed, Jan 03, 2024 at 01:31:59PM -0300, Leonardo Bras wrote: > > > In this header every xchg define (_relaxed, _acquire, _release, vanilla) > > > contain it's own asm file, both for 4-byte variables an 8-byte variables, > > > on a total of 8 versions of mostly the same asm. > > > > > > This is usually bad, as it means any change may be done in up to 8 > > > different places. > > > > > > Unify those versions by creating a new define with enough parameters to > > > generate any version of the previous 8. > > > > > > Then unify the result under a more general define, and simplify > > > arch_xchg* generation. > > > > > > (This did not cause any change in generated asm) > > > > > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > > Reviewed-by: Guo Ren <guoren@kernel.org> > > > Reviewed-by: Andrea Parri <parri.andrea@gmail.com> > > > Tested-by: Guo Ren <guoren@kernel.org> > > > --- > > > arch/riscv/include/asm/cmpxchg.h | 138 ++++++------------------------- > > > 1 file changed, 23 insertions(+), 115 deletions(-) > > > > > > diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h > > > index 2f4726d3cfcc2..48478a8eecee7 100644 > > > --- a/arch/riscv/include/asm/cmpxchg.h > > > +++ b/arch/riscv/include/asm/cmpxchg.h > > > @@ -11,140 +11,48 @@ > > > #include <asm/barrier.h> > > > #include <asm/fence.h> > > > > > > -#define __xchg_relaxed(ptr, new, size) \ > > > +#define __arch_xchg(sfx, prepend, append, r, p, n) \ > > > ({ \ > > > - __typeof__(ptr) __ptr = (ptr); \ > > > - __typeof__(new) __new = (new); \ > > > - __typeof__(*(ptr)) __ret; \ > > > - switch (size) { \ > > > - case 4: \ > > > - __asm__ __volatile__ ( \ > > > - " amoswap.w %0, %2, %1\n" \ > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > - : "r" (__new) \ > > > - : "memory"); \ > > Hello Boqun, thanks for reviewing! > > > > > Hmm... actually xchg_relaxed() doesn't need to be a barrier(), so the > > "memory" clobber here is not needed here. Of course, it's out of the > > scope of this series, but I'm curious to see what would happen if we > > remove the "memory" clobber _relaxed() ;-) > > Nice question :) > I am happy my patch can help bring up those ideas :) > > > According to gcc.gnu.org: > > --- > "memory" [clobber]: > > The "memory" clobber tells the compiler that the assembly code > performs memory reads or writes to items other than those listed in > the input and output operands (for example, accessing the memory > pointed to by one of the input parameters). To ensure memory contains Note here it says "other than those listed in the input and output operands", and in the above asm block, the memory pointed by "__ptr" is already marked as read-and-write by the asm block via "+A" (*__ptr), so the compiler knows the asm block may modify the memory pointed by "__ptr", therefore in _relaxed() case, "memory" clobber can be avoided. Here is an example showing the difference, considering the follow case: this_val = *this; that_val = *that; xchg_relaxed(this, 1); reread_this = *this; by the semantics of _relaxed, compilers can optimize the above into this_val = *this; xchg_relaxed(this, 1); that_val = *that; reread_this = *this; but the "memory" clobber in the xchg_relexed() will provide this. Needless to say the '"+A" (*__ptr)' prevents compiler from the following optimization: this_val = *this; that_val = *that; xchg_relaxed(this, 1); reread_this = this_val; since the compiler knows the asm block will read and write *this. Regards, Boqun > correct values, GCC may need to flush specific register values to > memory before executing the asm. Further, the compiler does not assume > that any values read from memory before an asm remain unchanged after > that asm ; it reloads them as needed. Using the "memory" clobber > effectively forms a read/write memory barrier for the compiler. > > Note that this clobber does not prevent the processor from doing > speculative reads past the asm statement. To prevent that, you need > processor-specific fence instructions. > --- > > IIUC above text says that having memory accesses to *__ptr would require > above asm to have the "memory" clobber, so memory accesses don't get > reordered by the compiler. > > By above affirmation, all asm in this file should have the "memory" > clobber, since all atomic operations will change memory pointed by an input > ptr. Is that correct? > > Thanks! > Leo > > > > > > Regards, > > Boqun > > > > > - break; \ > > > - case 8: \ > > > - __asm__ __volatile__ ( \ > > > - " amoswap.d %0, %2, %1\n" \ > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > - : "r" (__new) \ > > > - : "memory"); \ > > > - break; \ > > > - default: \ > > > - BUILD_BUG(); \ > > > - } \ > > > - __ret; \ > > > -}) > > > - > > > -#define arch_xchg_relaxed(ptr, x) \ > > > -({ \ > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > - (__typeof__(*(ptr))) __xchg_relaxed((ptr), \ > > > - _x_, sizeof(*(ptr))); \ > > > + __asm__ __volatile__ ( \ > > > + prepend \ > > > + " amoswap" sfx " %0, %2, %1\n" \ > > > + append \ > > > + : "=r" (r), "+A" (*(p)) \ > > > + : "r" (n) \ > > > + : "memory"); \ > > > }) > > > > > > -#define __xchg_acquire(ptr, new, size) \ > > > +#define _arch_xchg(ptr, new, sfx, prepend, append) \ > > > ({ \ > > > __typeof__(ptr) __ptr = (ptr); \ > > > - __typeof__(new) __new = (new); \ > > > - __typeof__(*(ptr)) __ret; \ > > > - switch (size) { \ > > > + __typeof__(*(__ptr)) __new = (new); \ > > > + __typeof__(*(__ptr)) __ret; \ > > > + switch (sizeof(*__ptr)) { \ > > > case 4: \ > > > - __asm__ __volatile__ ( \ > > > - " amoswap.w %0, %2, %1\n" \ > > > - RISCV_ACQUIRE_BARRIER \ > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > - : "r" (__new) \ > > > - : "memory"); \ > > > + __arch_xchg(".w" sfx, prepend, append, \ > > > + __ret, __ptr, __new); \ > > > break; \ > > > case 8: \ > > > - __asm__ __volatile__ ( \ > > > - " amoswap.d %0, %2, %1\n" \ > > > - RISCV_ACQUIRE_BARRIER \ > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > - : "r" (__new) \ > > > - : "memory"); \ > > > + __arch_xchg(".d" sfx, prepend, append, \ > > > + __ret, __ptr, __new); \ > > > break; \ > > > default: \ > > > BUILD_BUG(); \ > > > } \ > > > - __ret; \ > > > + (__typeof__(*(__ptr)))__ret; \ > > > }) > > > > > > -#define arch_xchg_acquire(ptr, x) \ > > > -({ \ > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > - (__typeof__(*(ptr))) __xchg_acquire((ptr), \ > > > - _x_, sizeof(*(ptr))); \ > > > -}) > > > +#define arch_xchg_relaxed(ptr, x) \ > > > + _arch_xchg(ptr, x, "", "", "") > > > > > > -#define __xchg_release(ptr, new, size) \ > > > -({ \ > > > - __typeof__(ptr) __ptr = (ptr); \ > > > - __typeof__(new) __new = (new); \ > > > - __typeof__(*(ptr)) __ret; \ > > > - switch (size) { \ > > > - case 4: \ > > > - __asm__ __volatile__ ( \ > > > - RISCV_RELEASE_BARRIER \ > > > - " amoswap.w %0, %2, %1\n" \ > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > - : "r" (__new) \ > > > - : "memory"); \ > > > - break; \ > > > - case 8: \ > > > - __asm__ __volatile__ ( \ > > > - RISCV_RELEASE_BARRIER \ > > > - " amoswap.d %0, %2, %1\n" \ > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > - : "r" (__new) \ > > > - : "memory"); \ > > > - break; \ > > > - default: \ > > > - BUILD_BUG(); \ > > > - } \ > > > - __ret; \ > > > -}) > > > +#define arch_xchg_acquire(ptr, x) \ > > > + _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER) > > > > > > #define arch_xchg_release(ptr, x) \ > > > -({ \ > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > - (__typeof__(*(ptr))) __xchg_release((ptr), \ > > > - _x_, sizeof(*(ptr))); \ > > > -}) > > > - > > > -#define __arch_xchg(ptr, new, size) \ > > > -({ \ > > > - __typeof__(ptr) __ptr = (ptr); \ > > > - __typeof__(new) __new = (new); \ > > > - __typeof__(*(ptr)) __ret; \ > > > - switch (size) { \ > > > - case 4: \ > > > - __asm__ __volatile__ ( \ > > > - " amoswap.w.aqrl %0, %2, %1\n" \ > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > - : "r" (__new) \ > > > - : "memory"); \ > > > - break; \ > > > - case 8: \ > > > - __asm__ __volatile__ ( \ > > > - " amoswap.d.aqrl %0, %2, %1\n" \ > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > - : "r" (__new) \ > > > - : "memory"); \ > > > - break; \ > > > - default: \ > > > - BUILD_BUG(); \ > > > - } \ > > > - __ret; \ > > > -}) > > > + _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "") > > > > > > #define arch_xchg(ptr, x) \ > > > -({ \ > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > - (__typeof__(*(ptr))) __arch_xchg((ptr), _x_, sizeof(*(ptr))); \ > > > -}) > > > + _arch_xchg(ptr, x, ".aqrl", "", "") > > > > > > #define xchg32(ptr, x) \ > > > ({ \ > > > -- > > > 2.43.0 > > > > > >
On Thu, Jan 04, 2024 at 01:51:20PM -0800, Boqun Feng wrote: > On Thu, Jan 04, 2024 at 05:41:26PM -0300, Leonardo Bras wrote: > > On Thu, Jan 04, 2024 at 11:53:45AM -0800, Boqun Feng wrote: > > > On Wed, Jan 03, 2024 at 01:31:59PM -0300, Leonardo Bras wrote: > > > > In this header every xchg define (_relaxed, _acquire, _release, vanilla) > > > > contain it's own asm file, both for 4-byte variables an 8-byte variables, > > > > on a total of 8 versions of mostly the same asm. > > > > > > > > This is usually bad, as it means any change may be done in up to 8 > > > > different places. > > > > > > > > Unify those versions by creating a new define with enough parameters to > > > > generate any version of the previous 8. > > > > > > > > Then unify the result under a more general define, and simplify > > > > arch_xchg* generation. > > > > > > > > (This did not cause any change in generated asm) > > > > > > > > Signed-off-by: Leonardo Bras <leobras@redhat.com> > > > > Reviewed-by: Guo Ren <guoren@kernel.org> > > > > Reviewed-by: Andrea Parri <parri.andrea@gmail.com> > > > > Tested-by: Guo Ren <guoren@kernel.org> > > > > --- > > > > arch/riscv/include/asm/cmpxchg.h | 138 ++++++------------------------- > > > > 1 file changed, 23 insertions(+), 115 deletions(-) > > > > > > > > diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h > > > > index 2f4726d3cfcc2..48478a8eecee7 100644 > > > > --- a/arch/riscv/include/asm/cmpxchg.h > > > > +++ b/arch/riscv/include/asm/cmpxchg.h > > > > @@ -11,140 +11,48 @@ > > > > #include <asm/barrier.h> > > > > #include <asm/fence.h> > > > > > > > > -#define __xchg_relaxed(ptr, new, size) \ > > > > +#define __arch_xchg(sfx, prepend, append, r, p, n) \ > > > > ({ \ > > > > - __typeof__(ptr) __ptr = (ptr); \ > > > > - __typeof__(new) __new = (new); \ > > > > - __typeof__(*(ptr)) __ret; \ > > > > - switch (size) { \ > > > > - case 4: \ > > > > - __asm__ __volatile__ ( \ > > > > - " amoswap.w %0, %2, %1\n" \ > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > - : "r" (__new) \ > > > > - : "memory"); \ > > > > Hello Boqun, thanks for reviewing! > > > > > > > > Hmm... actually xchg_relaxed() doesn't need to be a barrier(), so the > > > "memory" clobber here is not needed here. Of course, it's out of the > > > scope of this series, but I'm curious to see what would happen if we > > > remove the "memory" clobber _relaxed() ;-) > > > > Nice question :) > > I am happy my patch can help bring up those ideas :) > > > > > > According to gcc.gnu.org: > > > > --- > > "memory" [clobber]: > > > > The "memory" clobber tells the compiler that the assembly code > > performs memory reads or writes to items other than those listed in > > the input and output operands (for example, accessing the memory > > pointed to by one of the input parameters). To ensure memory contains > > Note here it says "other than those listed in the input and output > operands", and in the above asm block, the memory pointed by "__ptr" is > already marked as read-and-write by the asm block via "+A" (*__ptr), so > the compiler knows the asm block may modify the memory pointed by > "__ptr", therefore in _relaxed() case, "memory" clobber can be avoided. Thanks for pointing that out! That helped me improve my understanding on constraints for asm operands :) (I ended up getting even more info from the gcc manual) So "+A" constraints means the operand will get read/write and it's an address stored into a register. > > Here is an example showing the difference, considering the follow case: > > this_val = *this; > that_val = *that; > xchg_relaxed(this, 1); > reread_this = *this; > > by the semantics of _relaxed, compilers can optimize the above into > > this_val = *this; > xchg_relaxed(this, 1); > that_val = *that; > reread_this = *this; > Seems correct, since there is no barrier(). > but the "memory" clobber in the xchg_relexed() will provide this. By 'this' here you mean the barrier? I mean, IIUC "memory" clobber will avoid the above optimization, right? > Needless to say the '"+A" (*__ptr)' prevents compiler from the following > optimization: > > this_val = *this; > that_val = *that; > xchg_relaxed(this, 1); > reread_this = this_val; > > since the compiler knows the asm block will read and write *this. Right, the compiler knows that address will be wrote by the asm block, and so it reloads the value instead of re-using the old one. A question, though: Do we need the "memory" clobber in any other xchg / cmpxchg asm? I mean, usually the only write to memory will happen in the *__ptr, which should be safe by "+A". I understand that since the others are not "relaxed" they will need to have a barrier, but is not the compiler supposed to understand the barrier instruction and avoid compiler reordering / optimizations across given instruction ? Thanks! Leo > Regards, > Boqun > > > correct values, GCC may need to flush specific register values to > > memory before executing the asm. Further, the compiler does not assume > > that any values read from memory before an asm remain unchanged after > > that asm ; it reloads them as needed. Using the "memory" clobber > > effectively forms a read/write memory barrier for the compiler. > > > > Note that this clobber does not prevent the processor from doing > > speculative reads past the asm statement. To prevent that, you need > > processor-specific fence instructions. > > --- > > > > IIUC above text says that having memory accesses to *__ptr would require > > above asm to have the "memory" clobber, so memory accesses don't get > > reordered by the compiler. > > > > By above affirmation, all asm in this file should have the "memory" > > clobber, since all atomic operations will change memory pointed by an input > > ptr. Is that correct? > > > > Thanks! > > Leo > > > > > > > > > > Regards, > > > Boqun > > > > > > > - break; \ > > > > - case 8: \ > > > > - __asm__ __volatile__ ( \ > > > > - " amoswap.d %0, %2, %1\n" \ > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > - : "r" (__new) \ > > > > - : "memory"); \ > > > > - break; \ > > > > - default: \ > > > > - BUILD_BUG(); \ > > > > - } \ > > > > - __ret; \ > > > > -}) > > > > - > > > > -#define arch_xchg_relaxed(ptr, x) \ > > > > -({ \ > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > - (__typeof__(*(ptr))) __xchg_relaxed((ptr), \ > > > > - _x_, sizeof(*(ptr))); \ > > > > + __asm__ __volatile__ ( \ > > > > + prepend \ > > > > + " amoswap" sfx " %0, %2, %1\n" \ > > > > + append \ > > > > + : "=r" (r), "+A" (*(p)) \ > > > > + : "r" (n) \ > > > > + : "memory"); \ > > > > }) > > > > > > > > -#define __xchg_acquire(ptr, new, size) \ > > > > +#define _arch_xchg(ptr, new, sfx, prepend, append) \ > > > > ({ \ > > > > __typeof__(ptr) __ptr = (ptr); \ > > > > - __typeof__(new) __new = (new); \ > > > > - __typeof__(*(ptr)) __ret; \ > > > > - switch (size) { \ > > > > + __typeof__(*(__ptr)) __new = (new); \ > > > > + __typeof__(*(__ptr)) __ret; \ > > > > + switch (sizeof(*__ptr)) { \ > > > > case 4: \ > > > > - __asm__ __volatile__ ( \ > > > > - " amoswap.w %0, %2, %1\n" \ > > > > - RISCV_ACQUIRE_BARRIER \ > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > - : "r" (__new) \ > > > > - : "memory"); \ > > > > + __arch_xchg(".w" sfx, prepend, append, \ > > > > + __ret, __ptr, __new); \ > > > > break; \ > > > > case 8: \ > > > > - __asm__ __volatile__ ( \ > > > > - " amoswap.d %0, %2, %1\n" \ > > > > - RISCV_ACQUIRE_BARRIER \ > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > - : "r" (__new) \ > > > > - : "memory"); \ > > > > + __arch_xchg(".d" sfx, prepend, append, \ > > > > + __ret, __ptr, __new); \ > > > > break; \ > > > > default: \ > > > > BUILD_BUG(); \ > > > > } \ > > > > - __ret; \ > > > > + (__typeof__(*(__ptr)))__ret; \ > > > > }) > > > > > > > > -#define arch_xchg_acquire(ptr, x) \ > > > > -({ \ > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > - (__typeof__(*(ptr))) __xchg_acquire((ptr), \ > > > > - _x_, sizeof(*(ptr))); \ > > > > -}) > > > > +#define arch_xchg_relaxed(ptr, x) \ > > > > + _arch_xchg(ptr, x, "", "", "") > > > > > > > > -#define __xchg_release(ptr, new, size) \ > > > > -({ \ > > > > - __typeof__(ptr) __ptr = (ptr); \ > > > > - __typeof__(new) __new = (new); \ > > > > - __typeof__(*(ptr)) __ret; \ > > > > - switch (size) { \ > > > > - case 4: \ > > > > - __asm__ __volatile__ ( \ > > > > - RISCV_RELEASE_BARRIER \ > > > > - " amoswap.w %0, %2, %1\n" \ > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > - : "r" (__new) \ > > > > - : "memory"); \ > > > > - break; \ > > > > - case 8: \ > > > > - __asm__ __volatile__ ( \ > > > > - RISCV_RELEASE_BARRIER \ > > > > - " amoswap.d %0, %2, %1\n" \ > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > - : "r" (__new) \ > > > > - : "memory"); \ > > > > - break; \ > > > > - default: \ > > > > - BUILD_BUG(); \ > > > > - } \ > > > > - __ret; \ > > > > -}) > > > > +#define arch_xchg_acquire(ptr, x) \ > > > > + _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER) > > > > > > > > #define arch_xchg_release(ptr, x) \ > > > > -({ \ > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > - (__typeof__(*(ptr))) __xchg_release((ptr), \ > > > > - _x_, sizeof(*(ptr))); \ > > > > -}) > > > > - > > > > -#define __arch_xchg(ptr, new, size) \ > > > > -({ \ > > > > - __typeof__(ptr) __ptr = (ptr); \ > > > > - __typeof__(new) __new = (new); \ > > > > - __typeof__(*(ptr)) __ret; \ > > > > - switch (size) { \ > > > > - case 4: \ > > > > - __asm__ __volatile__ ( \ > > > > - " amoswap.w.aqrl %0, %2, %1\n" \ > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > - : "r" (__new) \ > > > > - : "memory"); \ > > > > - break; \ > > > > - case 8: \ > > > > - __asm__ __volatile__ ( \ > > > > - " amoswap.d.aqrl %0, %2, %1\n" \ > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > - : "r" (__new) \ > > > > - : "memory"); \ > > > > - break; \ > > > > - default: \ > > > > - BUILD_BUG(); \ > > > > - } \ > > > > - __ret; \ > > > > -}) > > > > + _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "") > > > > > > > > #define arch_xchg(ptr, x) \ > > > > -({ \ > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > - (__typeof__(*(ptr))) __arch_xchg((ptr), _x_, sizeof(*(ptr))); \ > > > > -}) > > > > + _arch_xchg(ptr, x, ".aqrl", "", "") > > > > > > > > #define xchg32(ptr, x) \ > > > > ({ \ > > > > -- > > > > 2.43.0 > > > > > > > > > >
On Fri, Jan 05, 2024 at 01:45:42AM -0300, Leonardo Bras wrote: [...] > > > According to gcc.gnu.org: > > > > > > --- > > > "memory" [clobber]: > > > > > > The "memory" clobber tells the compiler that the assembly code > > > performs memory reads or writes to items other than those listed in > > > the input and output operands (for example, accessing the memory > > > pointed to by one of the input parameters). To ensure memory contains > > > > Note here it says "other than those listed in the input and output > > operands", and in the above asm block, the memory pointed by "__ptr" is > > already marked as read-and-write by the asm block via "+A" (*__ptr), so > > the compiler knows the asm block may modify the memory pointed by > > "__ptr", therefore in _relaxed() case, "memory" clobber can be avoided. > > Thanks for pointing that out! > That helped me improve my understanding on constraints for asm operands :) > (I ended up getting even more info from the gcc manual) > > So "+A" constraints means the operand will get read/write and it's an > address stored into a register. > > > > > Here is an example showing the difference, considering the follow case: > > > > this_val = *this; > > that_val = *that; > > xchg_relaxed(this, 1); > > reread_this = *this; > > > > by the semantics of _relaxed, compilers can optimize the above into > > > > this_val = *this; > > xchg_relaxed(this, 1); > > that_val = *that; > > reread_this = *this; > > > > Seems correct, since there is no barrier(). > > > but the "memory" clobber in the xchg_relexed() will provide this. > > By 'this' here you mean the barrier? I mean, IIUC "memory" clobber will > avoid the above optimization, right? > Right, seems I mis-typed "provide" (I meant "prevent") > > Needless to say the '"+A" (*__ptr)' prevents compiler from the following > > optimization: > > > > this_val = *this; > > that_val = *that; > > xchg_relaxed(this, 1); > > reread_this = this_val; > > > > since the compiler knows the asm block will read and write *this. > > Right, the compiler knows that address will be wrote by the asm block, and > so it reloads the value instead of re-using the old one. > Correct. > > A question, though: Good question ;-) > Do we need the "memory" clobber in any other xchg / cmpxchg asm? The "memory" clobber is needed for others, see below: > I mean, usually the only write to memory will happen in the *__ptr, which > should be safe by "+A". > > I understand that since the others are not "relaxed" they will need to > have a barrier, but is not the compiler supposed to understand the barrier > instruction and avoid compiler reordering / optimizations across given > instruction ? > The barrier semantics (ACQUIRE/RELEASE/FULL) is provided by the combined effort of both 1) preventing compiler optimization by "memory" clobber and 2) preventing CPU/memory reordering by arch-specific instructions. In other words, an asm block contains a hardware barrier instruction should always have the "memory" clobber, otherwise, there are possiblities that compilers reorder the asm block therefore break the ordering provided by the hardware instructions. Regards, Boqun > > Thanks! > Leo > > > Regards, > > Boqun > > > > > correct values, GCC may need to flush specific register values to > > > memory before executing the asm. Further, the compiler does not assume > > > that any values read from memory before an asm remain unchanged after > > > that asm ; it reloads them as needed. Using the "memory" clobber > > > effectively forms a read/write memory barrier for the compiler. > > > > > > Note that this clobber does not prevent the processor from doing > > > speculative reads past the asm statement. To prevent that, you need > > > processor-specific fence instructions. > > > --- > > > > > > IIUC above text says that having memory accesses to *__ptr would require > > > above asm to have the "memory" clobber, so memory accesses don't get > > > reordered by the compiler. > > > > > > By above affirmation, all asm in this file should have the "memory" > > > clobber, since all atomic operations will change memory pointed by an input > > > ptr. Is that correct? > > > > > > Thanks! > > > Leo > > > > > > > > > > > > > > Regards, > > > > Boqun > > > > > > > > > - break; \ > > > > > - case 8: \ > > > > > - __asm__ __volatile__ ( \ > > > > > - " amoswap.d %0, %2, %1\n" \ > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > - : "r" (__new) \ > > > > > - : "memory"); \ > > > > > - break; \ > > > > > - default: \ > > > > > - BUILD_BUG(); \ > > > > > - } \ > > > > > - __ret; \ > > > > > -}) > > > > > - > > > > > -#define arch_xchg_relaxed(ptr, x) \ > > > > > -({ \ > > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > > - (__typeof__(*(ptr))) __xchg_relaxed((ptr), \ > > > > > - _x_, sizeof(*(ptr))); \ > > > > > + __asm__ __volatile__ ( \ > > > > > + prepend \ > > > > > + " amoswap" sfx " %0, %2, %1\n" \ > > > > > + append \ > > > > > + : "=r" (r), "+A" (*(p)) \ > > > > > + : "r" (n) \ > > > > > + : "memory"); \ > > > > > }) > > > > > > > > > > -#define __xchg_acquire(ptr, new, size) \ > > > > > +#define _arch_xchg(ptr, new, sfx, prepend, append) \ > > > > > ({ \ > > > > > __typeof__(ptr) __ptr = (ptr); \ > > > > > - __typeof__(new) __new = (new); \ > > > > > - __typeof__(*(ptr)) __ret; \ > > > > > - switch (size) { \ > > > > > + __typeof__(*(__ptr)) __new = (new); \ > > > > > + __typeof__(*(__ptr)) __ret; \ > > > > > + switch (sizeof(*__ptr)) { \ > > > > > case 4: \ > > > > > - __asm__ __volatile__ ( \ > > > > > - " amoswap.w %0, %2, %1\n" \ > > > > > - RISCV_ACQUIRE_BARRIER \ > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > - : "r" (__new) \ > > > > > - : "memory"); \ > > > > > + __arch_xchg(".w" sfx, prepend, append, \ > > > > > + __ret, __ptr, __new); \ > > > > > break; \ > > > > > case 8: \ > > > > > - __asm__ __volatile__ ( \ > > > > > - " amoswap.d %0, %2, %1\n" \ > > > > > - RISCV_ACQUIRE_BARRIER \ > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > - : "r" (__new) \ > > > > > - : "memory"); \ > > > > > + __arch_xchg(".d" sfx, prepend, append, \ > > > > > + __ret, __ptr, __new); \ > > > > > break; \ > > > > > default: \ > > > > > BUILD_BUG(); \ > > > > > } \ > > > > > - __ret; \ > > > > > + (__typeof__(*(__ptr)))__ret; \ > > > > > }) > > > > > > > > > > -#define arch_xchg_acquire(ptr, x) \ > > > > > -({ \ > > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > > - (__typeof__(*(ptr))) __xchg_acquire((ptr), \ > > > > > - _x_, sizeof(*(ptr))); \ > > > > > -}) > > > > > +#define arch_xchg_relaxed(ptr, x) \ > > > > > + _arch_xchg(ptr, x, "", "", "") > > > > > > > > > > -#define __xchg_release(ptr, new, size) \ > > > > > -({ \ > > > > > - __typeof__(ptr) __ptr = (ptr); \ > > > > > - __typeof__(new) __new = (new); \ > > > > > - __typeof__(*(ptr)) __ret; \ > > > > > - switch (size) { \ > > > > > - case 4: \ > > > > > - __asm__ __volatile__ ( \ > > > > > - RISCV_RELEASE_BARRIER \ > > > > > - " amoswap.w %0, %2, %1\n" \ > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > - : "r" (__new) \ > > > > > - : "memory"); \ > > > > > - break; \ > > > > > - case 8: \ > > > > > - __asm__ __volatile__ ( \ > > > > > - RISCV_RELEASE_BARRIER \ > > > > > - " amoswap.d %0, %2, %1\n" \ > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > - : "r" (__new) \ > > > > > - : "memory"); \ > > > > > - break; \ > > > > > - default: \ > > > > > - BUILD_BUG(); \ > > > > > - } \ > > > > > - __ret; \ > > > > > -}) > > > > > +#define arch_xchg_acquire(ptr, x) \ > > > > > + _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER) > > > > > > > > > > #define arch_xchg_release(ptr, x) \ > > > > > -({ \ > > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > > - (__typeof__(*(ptr))) __xchg_release((ptr), \ > > > > > - _x_, sizeof(*(ptr))); \ > > > > > -}) > > > > > - > > > > > -#define __arch_xchg(ptr, new, size) \ > > > > > -({ \ > > > > > - __typeof__(ptr) __ptr = (ptr); \ > > > > > - __typeof__(new) __new = (new); \ > > > > > - __typeof__(*(ptr)) __ret; \ > > > > > - switch (size) { \ > > > > > - case 4: \ > > > > > - __asm__ __volatile__ ( \ > > > > > - " amoswap.w.aqrl %0, %2, %1\n" \ > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > - : "r" (__new) \ > > > > > - : "memory"); \ > > > > > - break; \ > > > > > - case 8: \ > > > > > - __asm__ __volatile__ ( \ > > > > > - " amoswap.d.aqrl %0, %2, %1\n" \ > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > - : "r" (__new) \ > > > > > - : "memory"); \ > > > > > - break; \ > > > > > - default: \ > > > > > - BUILD_BUG(); \ > > > > > - } \ > > > > > - __ret; \ > > > > > -}) > > > > > + _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "") > > > > > > > > > > #define arch_xchg(ptr, x) \ > > > > > -({ \ > > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > > - (__typeof__(*(ptr))) __arch_xchg((ptr), _x_, sizeof(*(ptr))); \ > > > > > -}) > > > > > + _arch_xchg(ptr, x, ".aqrl", "", "") > > > > > > > > > > #define xchg32(ptr, x) \ > > > > > ({ \ > > > > > -- > > > > > 2.43.0 > > > > > > > > > > > > > > >
On Thu, Jan 04, 2024 at 09:18:15PM -0800, Boqun Feng wrote: > On Fri, Jan 05, 2024 at 01:45:42AM -0300, Leonardo Bras wrote: > [...] > > > > According to gcc.gnu.org: > > > > > > > > --- > > > > "memory" [clobber]: > > > > > > > > The "memory" clobber tells the compiler that the assembly code > > > > performs memory reads or writes to items other than those listed in > > > > the input and output operands (for example, accessing the memory > > > > pointed to by one of the input parameters). To ensure memory contains > > > > > > Note here it says "other than those listed in the input and output > > > operands", and in the above asm block, the memory pointed by "__ptr" is > > > already marked as read-and-write by the asm block via "+A" (*__ptr), so > > > the compiler knows the asm block may modify the memory pointed by > > > "__ptr", therefore in _relaxed() case, "memory" clobber can be avoided. > > > > Thanks for pointing that out! > > That helped me improve my understanding on constraints for asm operands :) > > (I ended up getting even more info from the gcc manual) > > > > So "+A" constraints means the operand will get read/write and it's an > > address stored into a register. > > > > > > > > Here is an example showing the difference, considering the follow case: > > > > > > this_val = *this; > > > that_val = *that; > > > xchg_relaxed(this, 1); > > > reread_this = *this; > > > > > > by the semantics of _relaxed, compilers can optimize the above into > > > > > > this_val = *this; > > > xchg_relaxed(this, 1); > > > that_val = *that; > > > reread_this = *this; > > > > > > > Seems correct, since there is no barrier(). > > > > > but the "memory" clobber in the xchg_relexed() will provide this. > > > > By 'this' here you mean the barrier? I mean, IIUC "memory" clobber will > > avoid the above optimization, right? > > > > Right, seems I mis-typed "provide" (I meant "prevent") > > > > Needless to say the '"+A" (*__ptr)' prevents compiler from the following > > > optimization: > > > > > > this_val = *this; > > > that_val = *that; > > > xchg_relaxed(this, 1); > > > reread_this = this_val; > > > > > > since the compiler knows the asm block will read and write *this. > > > > Right, the compiler knows that address will be wrote by the asm block, and > > so it reloads the value instead of re-using the old one. > > > > Correct. > > > > > A question, though: > > Good question ;-) > > > Do we need the "memory" clobber in any other xchg / cmpxchg asm? > > The "memory" clobber is needed for others, see below: > > > I mean, usually the only write to memory will happen in the *__ptr, which > > should be safe by "+A". > > > > I understand that since the others are not "relaxed" they will need to > > have a barrier, but is not the compiler supposed to understand the barrier > > instruction and avoid compiler reordering / optimizations across given > > instruction ? > > > > The barrier semantics (ACQUIRE/RELEASE/FULL) is provided by the combined > effort of both 1) preventing compiler optimization by "memory" clobber > and 2) preventing CPU/memory reordering by arch-specific instructions. > > In other words, an asm block contains a hardware barrier instruction > should always have the "memory" clobber, otherwise, there are > possiblities that compilers reorder the asm block therefore break the > ordering provided by the hardware instructions. Oh, I see. So this means the compiler does not check for memory barrier instructions before reordering loads/stores. Right? Meaning it needs a way to signal a compiler barrier, on top of the barrier instructions. Thanks for helping me improve my understanding of this! Leo > > Regards, > Boqun > > > > > Thanks! > > Leo > > > > > Regards, > > > Boqun > > > > > > > correct values, GCC may need to flush specific register values to > > > > memory before executing the asm. Further, the compiler does not assume > > > > that any values read from memory before an asm remain unchanged after > > > > that asm ; it reloads them as needed. Using the "memory" clobber > > > > effectively forms a read/write memory barrier for the compiler. > > > > > > > > Note that this clobber does not prevent the processor from doing > > > > speculative reads past the asm statement. To prevent that, you need > > > > processor-specific fence instructions. > > > > --- > > > > > > > > IIUC above text says that having memory accesses to *__ptr would require > > > > above asm to have the "memory" clobber, so memory accesses don't get > > > > reordered by the compiler. > > > > > > > > By above affirmation, all asm in this file should have the "memory" > > > > clobber, since all atomic operations will change memory pointed by an input > > > > ptr. Is that correct? > > > > > > > > Thanks! > > > > Leo > > > > > > > > > > > > > > > > > > Regards, > > > > > Boqun > > > > > > > > > > > - break; \ > > > > > > - case 8: \ > > > > > > - __asm__ __volatile__ ( \ > > > > > > - " amoswap.d %0, %2, %1\n" \ > > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > > - : "r" (__new) \ > > > > > > - : "memory"); \ > > > > > > - break; \ > > > > > > - default: \ > > > > > > - BUILD_BUG(); \ > > > > > > - } \ > > > > > > - __ret; \ > > > > > > -}) > > > > > > - > > > > > > -#define arch_xchg_relaxed(ptr, x) \ > > > > > > -({ \ > > > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > > > - (__typeof__(*(ptr))) __xchg_relaxed((ptr), \ > > > > > > - _x_, sizeof(*(ptr))); \ > > > > > > + __asm__ __volatile__ ( \ > > > > > > + prepend \ > > > > > > + " amoswap" sfx " %0, %2, %1\n" \ > > > > > > + append \ > > > > > > + : "=r" (r), "+A" (*(p)) \ > > > > > > + : "r" (n) \ > > > > > > + : "memory"); \ > > > > > > }) > > > > > > > > > > > > -#define __xchg_acquire(ptr, new, size) \ > > > > > > +#define _arch_xchg(ptr, new, sfx, prepend, append) \ > > > > > > ({ \ > > > > > > __typeof__(ptr) __ptr = (ptr); \ > > > > > > - __typeof__(new) __new = (new); \ > > > > > > - __typeof__(*(ptr)) __ret; \ > > > > > > - switch (size) { \ > > > > > > + __typeof__(*(__ptr)) __new = (new); \ > > > > > > + __typeof__(*(__ptr)) __ret; \ > > > > > > + switch (sizeof(*__ptr)) { \ > > > > > > case 4: \ > > > > > > - __asm__ __volatile__ ( \ > > > > > > - " amoswap.w %0, %2, %1\n" \ > > > > > > - RISCV_ACQUIRE_BARRIER \ > > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > > - : "r" (__new) \ > > > > > > - : "memory"); \ > > > > > > + __arch_xchg(".w" sfx, prepend, append, \ > > > > > > + __ret, __ptr, __new); \ > > > > > > break; \ > > > > > > case 8: \ > > > > > > - __asm__ __volatile__ ( \ > > > > > > - " amoswap.d %0, %2, %1\n" \ > > > > > > - RISCV_ACQUIRE_BARRIER \ > > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > > - : "r" (__new) \ > > > > > > - : "memory"); \ > > > > > > + __arch_xchg(".d" sfx, prepend, append, \ > > > > > > + __ret, __ptr, __new); \ > > > > > > break; \ > > > > > > default: \ > > > > > > BUILD_BUG(); \ > > > > > > } \ > > > > > > - __ret; \ > > > > > > + (__typeof__(*(__ptr)))__ret; \ > > > > > > }) > > > > > > > > > > > > -#define arch_xchg_acquire(ptr, x) \ > > > > > > -({ \ > > > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > > > - (__typeof__(*(ptr))) __xchg_acquire((ptr), \ > > > > > > - _x_, sizeof(*(ptr))); \ > > > > > > -}) > > > > > > +#define arch_xchg_relaxed(ptr, x) \ > > > > > > + _arch_xchg(ptr, x, "", "", "") > > > > > > > > > > > > -#define __xchg_release(ptr, new, size) \ > > > > > > -({ \ > > > > > > - __typeof__(ptr) __ptr = (ptr); \ > > > > > > - __typeof__(new) __new = (new); \ > > > > > > - __typeof__(*(ptr)) __ret; \ > > > > > > - switch (size) { \ > > > > > > - case 4: \ > > > > > > - __asm__ __volatile__ ( \ > > > > > > - RISCV_RELEASE_BARRIER \ > > > > > > - " amoswap.w %0, %2, %1\n" \ > > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > > - : "r" (__new) \ > > > > > > - : "memory"); \ > > > > > > - break; \ > > > > > > - case 8: \ > > > > > > - __asm__ __volatile__ ( \ > > > > > > - RISCV_RELEASE_BARRIER \ > > > > > > - " amoswap.d %0, %2, %1\n" \ > > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > > - : "r" (__new) \ > > > > > > - : "memory"); \ > > > > > > - break; \ > > > > > > - default: \ > > > > > > - BUILD_BUG(); \ > > > > > > - } \ > > > > > > - __ret; \ > > > > > > -}) > > > > > > +#define arch_xchg_acquire(ptr, x) \ > > > > > > + _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER) > > > > > > > > > > > > #define arch_xchg_release(ptr, x) \ > > > > > > -({ \ > > > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > > > - (__typeof__(*(ptr))) __xchg_release((ptr), \ > > > > > > - _x_, sizeof(*(ptr))); \ > > > > > > -}) > > > > > > - > > > > > > -#define __arch_xchg(ptr, new, size) \ > > > > > > -({ \ > > > > > > - __typeof__(ptr) __ptr = (ptr); \ > > > > > > - __typeof__(new) __new = (new); \ > > > > > > - __typeof__(*(ptr)) __ret; \ > > > > > > - switch (size) { \ > > > > > > - case 4: \ > > > > > > - __asm__ __volatile__ ( \ > > > > > > - " amoswap.w.aqrl %0, %2, %1\n" \ > > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > > - : "r" (__new) \ > > > > > > - : "memory"); \ > > > > > > - break; \ > > > > > > - case 8: \ > > > > > > - __asm__ __volatile__ ( \ > > > > > > - " amoswap.d.aqrl %0, %2, %1\n" \ > > > > > > - : "=r" (__ret), "+A" (*__ptr) \ > > > > > > - : "r" (__new) \ > > > > > > - : "memory"); \ > > > > > > - break; \ > > > > > > - default: \ > > > > > > - BUILD_BUG(); \ > > > > > > - } \ > > > > > > - __ret; \ > > > > > > -}) > > > > > > + _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "") > > > > > > > > > > > > #define arch_xchg(ptr, x) \ > > > > > > -({ \ > > > > > > - __typeof__(*(ptr)) _x_ = (x); \ > > > > > > - (__typeof__(*(ptr))) __arch_xchg((ptr), _x_, sizeof(*(ptr))); \ > > > > > > -}) > > > > > > + _arch_xchg(ptr, x, ".aqrl", "", "") > > > > > > > > > > > > #define xchg32(ptr, x) \ > > > > > > ({ \ > > > > > > -- > > > > > > 2.43.0 > > > > > > > > > > > > > > > > > > > > >
Hi Leonardo,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 610a9b8f49fbcf1100716370d3b5f6f884a2835a]
url: https://github.com/intel-lab-lkp/linux/commits/Leonardo-Bras/riscv-cmpxchg-Deduplicate-xchg-asm-functions/20240104-003501
base: 610a9b8f49fbcf1100716370d3b5f6f884a2835a
patch link: https://lore.kernel.org/r/20240103163203.72768-3-leobras%40redhat.com
patch subject: [PATCH v1 1/5] riscv/cmpxchg: Deduplicate xchg() asm functions
config: riscv-randconfig-r111-20240112 (https://download.01.org/0day-ci/archive/20240113/202401131438.f8SELM0W-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project 9bde5becb44ea071f5e1fa1f5d4071dc8788b18c)
reproduce: (https://download.01.org/0day-ci/archive/20240113/202401131438.f8SELM0W-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401131438.f8SELM0W-lkp@intel.com/
sparse warnings: (new ones prefixed by >>)
>> net/ipv4/tcp_cong.c:300:24: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected struct tcp_congestion_ops const [noderef] __rcu *__new @@ got struct tcp_congestion_ops *[assigned] ca @@
net/ipv4/tcp_cong.c:300:24: sparse: expected struct tcp_congestion_ops const [noderef] __rcu *__new
net/ipv4/tcp_cong.c:300:24: sparse: got struct tcp_congestion_ops *[assigned] ca
net/ipv4/tcp_cong.c:300:22: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct tcp_congestion_ops const *prev @@ got struct tcp_congestion_ops const [noderef] __rcu * @@
net/ipv4/tcp_cong.c:300:22: sparse: expected struct tcp_congestion_ops const *prev
net/ipv4/tcp_cong.c:300:22: sparse: got struct tcp_congestion_ops const [noderef] __rcu *
net/ipv4/tcp_cong.c: note: in included file (through include/linux/module.h):
include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true
vim +300 net/ipv4/tcp_cong.c
317a76f9a44b43 Stephen Hemminger 2005-06-23 281
317a76f9a44b43 Stephen Hemminger 2005-06-23 282 /* Used by sysctl to change default congestion control */
6670e152447732 Stephen Hemminger 2017-11-14 283 int tcp_set_default_congestion_control(struct net *net, const char *name)
317a76f9a44b43 Stephen Hemminger 2005-06-23 284 {
317a76f9a44b43 Stephen Hemminger 2005-06-23 285 struct tcp_congestion_ops *ca;
6670e152447732 Stephen Hemminger 2017-11-14 286 const struct tcp_congestion_ops *prev;
6670e152447732 Stephen Hemminger 2017-11-14 287 int ret;
317a76f9a44b43 Stephen Hemminger 2005-06-23 288
6670e152447732 Stephen Hemminger 2017-11-14 289 rcu_read_lock();
6670e152447732 Stephen Hemminger 2017-11-14 290 ca = tcp_ca_find_autoload(net, name);
6670e152447732 Stephen Hemminger 2017-11-14 291 if (!ca) {
6670e152447732 Stephen Hemminger 2017-11-14 292 ret = -ENOENT;
0baf26b0fcd74b Martin KaFai Lau 2020-01-08 293 } else if (!bpf_try_module_get(ca, ca->owner)) {
6670e152447732 Stephen Hemminger 2017-11-14 294 ret = -EBUSY;
8d432592f30fcc Jonathon Reinhart 2021-05-01 295 } else if (!net_eq(net, &init_net) &&
8d432592f30fcc Jonathon Reinhart 2021-05-01 296 !(ca->flags & TCP_CONG_NON_RESTRICTED)) {
8d432592f30fcc Jonathon Reinhart 2021-05-01 297 /* Only init netns can set default to a restricted algorithm */
8d432592f30fcc Jonathon Reinhart 2021-05-01 298 ret = -EPERM;
6670e152447732 Stephen Hemminger 2017-11-14 299 } else {
6670e152447732 Stephen Hemminger 2017-11-14 @300 prev = xchg(&net->ipv4.tcp_congestion_control, ca);
6670e152447732 Stephen Hemminger 2017-11-14 301 if (prev)
0baf26b0fcd74b Martin KaFai Lau 2020-01-08 302 bpf_module_put(prev, prev->owner);
317a76f9a44b43 Stephen Hemminger 2005-06-23 303
6670e152447732 Stephen Hemminger 2017-11-14 304 ca->flags |= TCP_CONG_NON_RESTRICTED;
317a76f9a44b43 Stephen Hemminger 2005-06-23 305 ret = 0;
317a76f9a44b43 Stephen Hemminger 2005-06-23 306 }
6670e152447732 Stephen Hemminger 2017-11-14 307 rcu_read_unlock();
317a76f9a44b43 Stephen Hemminger 2005-06-23 308
317a76f9a44b43 Stephen Hemminger 2005-06-23 309 return ret;
317a76f9a44b43 Stephen Hemminger 2005-06-23 310 }
317a76f9a44b43 Stephen Hemminger 2005-06-23 311
On Sat, Jan 13, 2024 at 02:54:17PM +0800, kernel test robot wrote: > Hi Leonardo, > > kernel test robot noticed the following build warnings: > > [auto build test WARNING on 610a9b8f49fbcf1100716370d3b5f6f884a2835a] > > url: https://github.com/intel-lab-lkp/linux/commits/Leonardo-Bras/riscv-cmpxchg-Deduplicate-xchg-asm-functions/20240104-003501 Cloned this repo > base: 610a9b8f49fbcf1100716370d3b5f6f884a2835a > patch link: https://lore.kernel.org/r/20240103163203.72768-3-leobras%40redhat.com > patch subject: [PATCH v1 1/5] riscv/cmpxchg: Deduplicate xchg() asm functions > config: riscv-randconfig-r111-20240112 (https://download.01.org/0day-ci/archive/20240113/202401131438.f8SELM0W-lkp@intel.com/config) > compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project 9bde5becb44ea071f5e1fa1f5d4071dc8788b18c) > reproduce: (https://download.01.org/0day-ci/archive/20240113/202401131438.f8SELM0W-lkp@intel.com/reproduce) And followed those instructions, while using sparse v0.6.4-52-g1cf3d98c. > > If you fix the issue in a separate patch/commit (i.e. not just a new version of > the same patch/commit), kindly add following tags > | Reported-by: kernel test robot <lkp@intel.com> > | Closes: https://lore.kernel.org/oe-kbuild-all/202401131438.f8SELM0W-lkp@intel.com/ > > sparse warnings: (new ones prefixed by >>) > >> net/ipv4/tcp_cong.c:300:24: sparse: sparse: incorrect type in initializer (different address spaces) @@ expected struct tcp_congestion_ops const [noderef] __rcu *__new @@ got struct tcp_congestion_ops *[assigned] ca @@ > net/ipv4/tcp_cong.c:300:24: sparse: expected struct tcp_congestion_ops const [noderef] __rcu *__new > net/ipv4/tcp_cong.c:300:24: sparse: got struct tcp_congestion_ops *[assigned] ca > net/ipv4/tcp_cong.c:300:22: sparse: sparse: incorrect type in assignment (different address spaces) @@ expected struct tcp_congestion_ops const *prev @@ got struct tcp_congestion_ops const [noderef] __rcu * @@ > net/ipv4/tcp_cong.c:300:22: sparse: expected struct tcp_congestion_ops const *prev > net/ipv4/tcp_cong.c:300:22: sparse: got struct tcp_congestion_ops const [noderef] __rcu * > net/ipv4/tcp_cong.c: note: in included file (through include/linux/module.h): > include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true > include/linux/list.h:83:21: sparse: sparse: self-comparison always evaluates to true > > vim +300 net/ipv4/tcp_cong.c > > 317a76f9a44b43 Stephen Hemminger 2005-06-23 281 > 317a76f9a44b43 Stephen Hemminger 2005-06-23 282 /* Used by sysctl to change default congestion control */ > 6670e152447732 Stephen Hemminger 2017-11-14 283 int tcp_set_default_congestion_control(struct net *net, const char *name) > 317a76f9a44b43 Stephen Hemminger 2005-06-23 284 { > 317a76f9a44b43 Stephen Hemminger 2005-06-23 285 struct tcp_congestion_ops *ca; > 6670e152447732 Stephen Hemminger 2017-11-14 286 const struct tcp_congestion_ops *prev; > 6670e152447732 Stephen Hemminger 2017-11-14 287 int ret; > 317a76f9a44b43 Stephen Hemminger 2005-06-23 288 > 6670e152447732 Stephen Hemminger 2017-11-14 289 rcu_read_lock(); > 6670e152447732 Stephen Hemminger 2017-11-14 290 ca = tcp_ca_find_autoload(net, name); > 6670e152447732 Stephen Hemminger 2017-11-14 291 if (!ca) { > 6670e152447732 Stephen Hemminger 2017-11-14 292 ret = -ENOENT; > 0baf26b0fcd74b Martin KaFai Lau 2020-01-08 293 } else if (!bpf_try_module_get(ca, ca->owner)) { > 6670e152447732 Stephen Hemminger 2017-11-14 294 ret = -EBUSY; > 8d432592f30fcc Jonathon Reinhart 2021-05-01 295 } else if (!net_eq(net, &init_net) && > 8d432592f30fcc Jonathon Reinhart 2021-05-01 296 !(ca->flags & TCP_CONG_NON_RESTRICTED)) { > 8d432592f30fcc Jonathon Reinhart 2021-05-01 297 /* Only init netns can set default to a restricted algorithm */ > 8d432592f30fcc Jonathon Reinhart 2021-05-01 298 ret = -EPERM; > 6670e152447732 Stephen Hemminger 2017-11-14 299 } else { > 6670e152447732 Stephen Hemminger 2017-11-14 @300 prev = xchg(&net->ipv4.tcp_congestion_control, ca); > 6670e152447732 Stephen Hemminger 2017-11-14 301 if (prev) > 0baf26b0fcd74b Martin KaFai Lau 2020-01-08 302 bpf_module_put(prev, prev->owner); > 317a76f9a44b43 Stephen Hemminger 2005-06-23 303 > 6670e152447732 Stephen Hemminger 2017-11-14 304 ca->flags |= TCP_CONG_NON_RESTRICTED; > 317a76f9a44b43 Stephen Hemminger 2005-06-23 305 ret = 0; > 317a76f9a44b43 Stephen Hemminger 2005-06-23 306 } > 6670e152447732 Stephen Hemminger 2017-11-14 307 rcu_read_unlock(); > 317a76f9a44b43 Stephen Hemminger 2005-06-23 308 > 317a76f9a44b43 Stephen Hemminger 2005-06-23 309 return ret; > 317a76f9a44b43 Stephen Hemminger 2005-06-23 310 } > 317a76f9a44b43 Stephen Hemminger 2005-06-23 311 > > -- > 0-DAY CI Kernel Test Service > https://github.com/intel/lkp-tests/wiki > I did some testing using the instructions above on above file, and patch 1/5 haven't introduced anything new. Command for gathering sparse warnings: COMPILER_INSTALL_PATH=$HOME/0day ~/lkp-tests/kbuild/make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__ -fmax-errors=unlimited -fmax-warnings=unlimited' O=build_dir ARCH=riscv SHELL=/bin/bash net/ipv4/tcp_cong.o 2> sparse I ran this for the commit mentioned in the reproduction instructions (7931dc023 : riscv/cmpxchg: Deduplicate xchg() asm functions ) and for it's parent (610a9b8f49 : Linux 6.7-rc8). The diff -u on the output was: # diff -u sparse_vanilla sparse_p1_5 --- sparse_vanilla 2024-01-16 14:16:36.217965076 -0500 +++ sparse_p1_5 2024-01-16 14:15:29.942712160 -0500 @@ -1,5 +1,5 @@ ../net/ipv4/tcp_cong.c:300:24: sparse: warning: incorrect type in initializer (different address spaces) -../net/ipv4/tcp_cong.c:300:24: sparse: expected struct tcp_congestion_ops const [noderef] __rcu *_x_ +../net/ipv4/tcp_cong.c:300:24: sparse: expected struct tcp_congestion_ops const [noderef] __rcu *__new ../net/ipv4/tcp_cong.c:300:24: sparse: got struct tcp_congestion_ops *[assigned] ca ../net/ipv4/tcp_cong.c:300:22: sparse: warning: incorrect type in assignment (different address spaces) ../net/ipv4/tcp_cong.c:300:22: sparse: expected struct tcp_congestion_ops const *prev So I did not introduce anything new, as per sparse v0.6.4-52-g1cf3d98c . I noticed the output is slightly different, and that in the reproduction steps this used: # sparse version: v0.6.4-52-g1cf3d98c-dirty Since there is no indicator on what the -dirty stands for, it's hard for me to get the same reproduction, but as far as I could test there is not any new error. Thanks! Leo
diff --git a/arch/riscv/include/asm/cmpxchg.h b/arch/riscv/include/asm/cmpxchg.h index 2f4726d3cfcc2..48478a8eecee7 100644 --- a/arch/riscv/include/asm/cmpxchg.h +++ b/arch/riscv/include/asm/cmpxchg.h @@ -11,140 +11,48 @@ #include <asm/barrier.h> #include <asm/fence.h> -#define __xchg_relaxed(ptr, new, size) \ +#define __arch_xchg(sfx, prepend, append, r, p, n) \ ({ \ - __typeof__(ptr) __ptr = (ptr); \ - __typeof__(new) __new = (new); \ - __typeof__(*(ptr)) __ret; \ - switch (size) { \ - case 4: \ - __asm__ __volatile__ ( \ - " amoswap.w %0, %2, %1\n" \ - : "=r" (__ret), "+A" (*__ptr) \ - : "r" (__new) \ - : "memory"); \ - break; \ - case 8: \ - __asm__ __volatile__ ( \ - " amoswap.d %0, %2, %1\n" \ - : "=r" (__ret), "+A" (*__ptr) \ - : "r" (__new) \ - : "memory"); \ - break; \ - default: \ - BUILD_BUG(); \ - } \ - __ret; \ -}) - -#define arch_xchg_relaxed(ptr, x) \ -({ \ - __typeof__(*(ptr)) _x_ = (x); \ - (__typeof__(*(ptr))) __xchg_relaxed((ptr), \ - _x_, sizeof(*(ptr))); \ + __asm__ __volatile__ ( \ + prepend \ + " amoswap" sfx " %0, %2, %1\n" \ + append \ + : "=r" (r), "+A" (*(p)) \ + : "r" (n) \ + : "memory"); \ }) -#define __xchg_acquire(ptr, new, size) \ +#define _arch_xchg(ptr, new, sfx, prepend, append) \ ({ \ __typeof__(ptr) __ptr = (ptr); \ - __typeof__(new) __new = (new); \ - __typeof__(*(ptr)) __ret; \ - switch (size) { \ + __typeof__(*(__ptr)) __new = (new); \ + __typeof__(*(__ptr)) __ret; \ + switch (sizeof(*__ptr)) { \ case 4: \ - __asm__ __volatile__ ( \ - " amoswap.w %0, %2, %1\n" \ - RISCV_ACQUIRE_BARRIER \ - : "=r" (__ret), "+A" (*__ptr) \ - : "r" (__new) \ - : "memory"); \ + __arch_xchg(".w" sfx, prepend, append, \ + __ret, __ptr, __new); \ break; \ case 8: \ - __asm__ __volatile__ ( \ - " amoswap.d %0, %2, %1\n" \ - RISCV_ACQUIRE_BARRIER \ - : "=r" (__ret), "+A" (*__ptr) \ - : "r" (__new) \ - : "memory"); \ + __arch_xchg(".d" sfx, prepend, append, \ + __ret, __ptr, __new); \ break; \ default: \ BUILD_BUG(); \ } \ - __ret; \ + (__typeof__(*(__ptr)))__ret; \ }) -#define arch_xchg_acquire(ptr, x) \ -({ \ - __typeof__(*(ptr)) _x_ = (x); \ - (__typeof__(*(ptr))) __xchg_acquire((ptr), \ - _x_, sizeof(*(ptr))); \ -}) +#define arch_xchg_relaxed(ptr, x) \ + _arch_xchg(ptr, x, "", "", "") -#define __xchg_release(ptr, new, size) \ -({ \ - __typeof__(ptr) __ptr = (ptr); \ - __typeof__(new) __new = (new); \ - __typeof__(*(ptr)) __ret; \ - switch (size) { \ - case 4: \ - __asm__ __volatile__ ( \ - RISCV_RELEASE_BARRIER \ - " amoswap.w %0, %2, %1\n" \ - : "=r" (__ret), "+A" (*__ptr) \ - : "r" (__new) \ - : "memory"); \ - break; \ - case 8: \ - __asm__ __volatile__ ( \ - RISCV_RELEASE_BARRIER \ - " amoswap.d %0, %2, %1\n" \ - : "=r" (__ret), "+A" (*__ptr) \ - : "r" (__new) \ - : "memory"); \ - break; \ - default: \ - BUILD_BUG(); \ - } \ - __ret; \ -}) +#define arch_xchg_acquire(ptr, x) \ + _arch_xchg(ptr, x, "", "", RISCV_ACQUIRE_BARRIER) #define arch_xchg_release(ptr, x) \ -({ \ - __typeof__(*(ptr)) _x_ = (x); \ - (__typeof__(*(ptr))) __xchg_release((ptr), \ - _x_, sizeof(*(ptr))); \ -}) - -#define __arch_xchg(ptr, new, size) \ -({ \ - __typeof__(ptr) __ptr = (ptr); \ - __typeof__(new) __new = (new); \ - __typeof__(*(ptr)) __ret; \ - switch (size) { \ - case 4: \ - __asm__ __volatile__ ( \ - " amoswap.w.aqrl %0, %2, %1\n" \ - : "=r" (__ret), "+A" (*__ptr) \ - : "r" (__new) \ - : "memory"); \ - break; \ - case 8: \ - __asm__ __volatile__ ( \ - " amoswap.d.aqrl %0, %2, %1\n" \ - : "=r" (__ret), "+A" (*__ptr) \ - : "r" (__new) \ - : "memory"); \ - break; \ - default: \ - BUILD_BUG(); \ - } \ - __ret; \ -}) + _arch_xchg(ptr, x, "", RISCV_RELEASE_BARRIER, "") #define arch_xchg(ptr, x) \ -({ \ - __typeof__(*(ptr)) _x_ = (x); \ - (__typeof__(*(ptr))) __arch_xchg((ptr), _x_, sizeof(*(ptr))); \ -}) + _arch_xchg(ptr, x, ".aqrl", "", "") #define xchg32(ptr, x) \ ({ \