diff mbox series

[RESEND,2/2] locking/x86: Wire up sync_try_cmpxchg

Message ID 20230925150905.54842-2-ubizjak@gmail.com
State New
Headers
Series [RESEND,1/2] locking/generic: Add generic support for sync_try_cmpxchg and its fallback |

Commit Message

Uros Bizjak Sept. 25, 2023, 3:08 p.m. UTC 
  Implement target specific support for sync_try_cmpxchg.

Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Dave Hansen <dave.hansen@linux.intel.com>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
---
 arch/x86/include/asm/cmpxchg.h | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Ingo Molnar Sept. 28, 2023, 8:53 p.m. UTC | #1 
* Uros Bizjak <ubizjak@gmail.com> wrote:

> Implement target specific support for sync_try_cmpxchg.

Could you please provide a before/after description of how
this improves things exactly?

Thanks,

	Ingo
Uros Bizjak Sept. 29, 2023, 5:45 a.m. UTC | #2 
On Thu, Sep 28, 2023 at 10:53 PM Ingo Molnar <mingo@kernel.org> wrote:
>
>
> * Uros Bizjak <ubizjak@gmail.com> wrote:
>
> > Implement target specific support for sync_try_cmpxchg.
>
> Could you please provide a before/after description of how
> this improves things exactly?

The improvement [1] was demonstrated in the original patch submission.
The resent part of the patch series is an infrastructure part, which
introduces a new locking primitive, together with its fallback, so it
has to be committed first. The improvement is in the other part of the
kernel, and this part requires the infrastructure part.

To avoid unwanted patch dependencies, I propose to commit the
infrastructure part first, and after that commit the XEN part of the
series. (Please note that the XEN part is already acked by the
maintainer). I will mention the concrete improvement (code
simplification as shown in [1] and better generated assembly for x86
targets) in the formal submission of the XEN part.

Please also note checksums in include/linux/atomic header files. It is
necessary to regenerate the headers every time the checksum changes.
The resend of the infrastructure part was thus necessary due to the
recent fixes in this area.

[1] https://lore.kernel.org/lkml/20230710192440.47140-3-ubizjak@gmail.com/

Thanks,
Uros.
Ingo Molnar Sept. 29, 2023, 9:11 a.m. UTC | #3 
* Uros Bizjak <ubizjak@gmail.com> wrote:

> On Thu, Sep 28, 2023 at 10:53 PM Ingo Molnar <mingo@kernel.org> wrote:
> >
> >
> > * Uros Bizjak <ubizjak@gmail.com> wrote:
> >
> > > Implement target specific support for sync_try_cmpxchg.
> >
> > Could you please provide a before/after description of how
> > this improves things exactly?
> 
> The improvement [1] was demonstrated in the original patch submission.

What I'm saying: please integrate the required context & arguments into the 
changelogs of the patches you submit.

Patches that change code generation should demonstrate what they achieve.

 - If existing code changes, then describe/demonstrate it with disassembly.

 - If existing code generation is unchanged, then *declare that property in 
   the changelog*, and mention that a future patch relies those changes.

You can either include that future patch in this series, or you can 
describe/demonstrate the benefits in the changelog while noting that those 
changes will come in future patches.

Your submission, as-is, provided no context whatsoever, it described only 
the 'how', not the 'why'.

Thanks,

	Ingo
diff mbox series

Patch

diff --git a/arch/x86/include/asm/cmpxchg.h b/arch/x86/include/asm/cmpxchg.h
index d53636506134..5612648b0202 100644
--- a/arch/x86/include/asm/cmpxchg.h
+++ b/arch/x86/include/asm/cmpxchg.h
@@ -221,12 +221,18 @@  extern void __add_wrong_size(void)
 #define __try_cmpxchg(ptr, pold, new, size)				\
 	__raw_try_cmpxchg((ptr), (pold), (new), (size), LOCK_PREFIX)
 
+#define __sync_try_cmpxchg(ptr, pold, new, size)			\
+	__raw_try_cmpxchg((ptr), (pold), (new), (size), "lock; ")
+
 #define __try_cmpxchg_local(ptr, pold, new, size)			\
 	__raw_try_cmpxchg((ptr), (pold), (new), (size), "")
 
 #define arch_try_cmpxchg(ptr, pold, new) 				\
 	__try_cmpxchg((ptr), (pold), (new), sizeof(*(ptr)))
 
+#define arch_sync_try_cmpxchg(ptr, pold, new) 				\
+	__sync_try_cmpxchg((ptr), (pold), (new), sizeof(*(ptr)))
+
 #define arch_try_cmpxchg_local(ptr, pold, new)				\
 	__try_cmpxchg_local((ptr), (pold), (new), sizeof(*(ptr)))