* Re: [PATCH] perf/x86/amd: Use try_cmpxchg() in events/amd/{un,}core.c
2024-04-25 10:16 [PATCH] perf/x86/amd: Use try_cmpxchg() in events/amd/{un,}core.c Uros Bizjak
@ 2024-05-18 9:15 ` Ingo Molnar
0 siblings, 0 replies; 2+ messages in thread
From: Ingo Molnar @ 2024-05-18 9:15 UTC (permalink / raw)
To: Uros Bizjak
Cc: x86, linux-perf-users, linux-kernel, Peter Zijlstra,
Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland,
Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter,
Thomas Gleixner, Borislav Petkov, Dave Hansen, H. Peter Anvin
* Uros Bizjak <ubizjak@gmail.com> wrote:
> Replace this pattern in events/amd/{un,}core.c:
>
> cmpxchg(*ptr, old, new) == old
>
> ... with the simpler and faster:
>
> try_cmpxchg(*ptr, &old, new)
>
> The x86 CMPXCHG instruction returns success in the ZF flag, so this change
> saves a compare after the CMPXCHG.
>
> No functional change intended.
>
> Signed-off-by: Uros Bizjak <ubizjak@gmail.com>
> Cc: Peter Zijlstra <peterz@infradead.org>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: Ian Rogers <irogers@google.com>
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> ---
> arch/x86/events/amd/core.c | 3 ++-
> arch/x86/events/amd/uncore.c | 6 ++++--
> 2 files changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
> index 1fc4ce44e743..c1b8aeff7bc0 100644
> --- a/arch/x86/events/amd/core.c
> +++ b/arch/x86/events/amd/core.c
> @@ -433,7 +433,8 @@ static void __amd_put_nb_event_constraints(struct cpu_hw_events *cpuc,
> * when we come here
> */
> for (i = 0; i < x86_pmu.num_counters; i++) {
> - if (cmpxchg(nb->owners + i, event, NULL) == event)
> + struct perf_event *tmp = event;
> + if (try_cmpxchg(nb->owners + i, &tmp, NULL))
> break;
> }
> }
> diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
> index 4ccb8fa483e6..cac7c20d1aee 100644
> --- a/arch/x86/events/amd/uncore.c
> +++ b/arch/x86/events/amd/uncore.c
> @@ -162,7 +162,8 @@ static int amd_uncore_add(struct perf_event *event, int flags)
> /* if not, take the first available counter */
> hwc->idx = -1;
> for (i = 0; i < pmu->num_counters; i++) {
> - if (cmpxchg(&ctx->events[i], NULL, event) == NULL) {
> + struct perf_event *tmp = NULL;
> + if (try_cmpxchg(&ctx->events[i], &tmp, event)) {
> hwc->idx = i;
> break;
> }
> @@ -196,7 +197,8 @@ static void amd_uncore_del(struct perf_event *event, int flags)
> event->pmu->stop(event, PERF_EF_UPDATE);
>
> for (i = 0; i < pmu->num_counters; i++) {
> - if (cmpxchg(&ctx->events[i], event, NULL) == event)
> + struct perf_event *tmp = event;
> + if (try_cmpxchg(&ctx->events[i], &tmp, NULL))
> break;
> }
Note that's a newline missing between local variable declaration block and
the first statement after that. I've added all 3 to this patch (see below),
but please keep an eye on this in future patches.
Thanks,
Ingo
================>
arch/x86/events/amd/core.c | 1 +
arch/x86/events/amd/uncore.c | 2 ++
2 files changed, 3 insertions(+)
diff --git a/arch/x86/events/amd/core.c b/arch/x86/events/amd/core.c
index c1b8aeff7bc0..18bfe3451f3a 100644
--- a/arch/x86/events/amd/core.c
+++ b/arch/x86/events/amd/core.c
@@ -434,6 +434,7 @@ static void __amd_put_nb_event_constraints(struct cpu_hw_events *cpuc,
*/
for (i = 0; i < x86_pmu.num_counters; i++) {
struct perf_event *tmp = event;
+
if (try_cmpxchg(nb->owners + i, &tmp, NULL))
break;
}
diff --git a/arch/x86/events/amd/uncore.c b/arch/x86/events/amd/uncore.c
index cac7c20d1aee..0fafe233bba4 100644
--- a/arch/x86/events/amd/uncore.c
+++ b/arch/x86/events/amd/uncore.c
@@ -163,6 +163,7 @@ static int amd_uncore_add(struct perf_event *event, int flags)
hwc->idx = -1;
for (i = 0; i < pmu->num_counters; i++) {
struct perf_event *tmp = NULL;
+
if (try_cmpxchg(&ctx->events[i], &tmp, event)) {
hwc->idx = i;
break;
@@ -198,6 +199,7 @@ static void amd_uncore_del(struct perf_event *event, int flags)
for (i = 0; i < pmu->num_counters; i++) {
struct perf_event *tmp = event;
+
if (try_cmpxchg(&ctx->events[i], &tmp, NULL))
break;
}
^ permalink raw reply related [flat|nested] 2+ messages in thread