perf lock: Fix non-atomic max/time and min_time updates in contention_data

The update_contention_data() had a FIXME noting that max_time and
min_time updates lacked atomicity. Two CPUs could simultaneously read a
stale value, pass the comparison check and race on the write-back, with
the smaller value potentially overwriting the larger one and silently
corrupting the statistics.

Fix this by replacing the bare conditional assignments with a
bpf_loop()-based CAS retry loop. Each field tracks its own convergence
independently via max_done/min_done flags in cas_ctx, so a successful
CAS on one field is never retried even if the other field needs more
attempts.

Signed-off-by: Suchit Karunakaran <suchitkarunakaran@gmail.com>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>

diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c
index 96e7d853b9edf3b559f2d706b3c35cf53b96adcd..5c8431be674acd94f8bf6e0d3688f055b17ed1b8 100644 (file)
--- a/tools/perf/util/bpf_skel/lock_contention.bpf.c
+++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c
@@ -175,6 +175,13 @@ struct mm_struct___new {
        struct rw_semaphore mmap_lock;
 } __attribute__((preserve_access_index));
 
+struct cas_ctx {
+       struct contention_data *data;
+       u64 duration;
+       int max_done;
+       int min_done;
+};
+
 extern struct kmem_cache *bpf_get_kmem_cache(u64 addr) __ksym __weak;
 
 /* control flags */
@@ -486,16 +493,49 @@ static inline s32 get_owner_stack_id(u64 *stacktrace)
        return -1;
 }
 
+static long cas_min_max_cb(u64 idx, void *arg)
+{
+       struct cas_ctx *ctx = arg;
+
+       if (!ctx->max_done) {
+               u64 old_max = ctx->data->max_time;
+               if (old_max >= ctx->duration) {
+                       ctx->max_done = 1;
+               } else {
+                       u64 r = __sync_val_compare_and_swap(
+                               &ctx->data->max_time, old_max, ctx->duration);
+                       if (r == old_max)
+                               ctx->max_done = 1;
+               }
+       }
+
+       if (!ctx->min_done) {
+               u64 old_min = ctx->data->min_time;
+               if (old_min <= ctx->duration) {
+                       ctx->min_done = 1;
+               } else {
+                       u64 r = __sync_val_compare_and_swap(
+                               &ctx->data->min_time, old_min, ctx->duration);
+                       if (r == old_min)
+                               ctx->min_done = 1;
+               }
+       }
+
+       return (ctx->max_done && ctx->min_done) ? 1 : 0;
+}
+
 static inline void update_contention_data(struct contention_data *data, u64 duration, u32 count)
 {
        __sync_fetch_and_add(&data->total_time, duration);
        __sync_fetch_and_add(&data->count, count);
 
-       /* FIXME: need atomic operations */
-       if (data->max_time < duration)
-               data->max_time = duration;
-       if (data->min_time > duration)
-               data->min_time = duration;
+       struct cas_ctx ctx = {
+               .data     = data,
+               .duration = duration,
+               .max_done = 0,
+               .min_done = 0,
+       };
+       bpf_loop(64, cas_min_max_cb, &ctx, 0);
 }
 
 static inline void update_owner_stat(u32 id, u64 duration, u32 flags)