| Message ID | 20231020204741.1869520-2-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | BPF |
| Headers | show |
| Series | [v3,1/3] perf lock contention: Clear lock addr after use | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Fri, Oct 20, 2023 at 1:47 PM Namhyung Kim <namhyung@kernel.org> wrote: > > When pelem is NULL, it'd create a new entry with zero data. But it > might be preempted by IRQ/NMI just before calling bpf_map_update_elem() > then there's a chance to call it twice for the same pid. So it'd be > better to use BPF_NOEXIST flag and check the return value to prevent > the race. > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> Acked-by: Ian Rogers <irogers@google.com> Thanks, Ian > --- > tools/perf/util/bpf_skel/lock_contention.bpf.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c > index b11179452e19..69d31fd77cd0 100644 > --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c > +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c > @@ -328,7 +328,11 @@ int contention_begin(u64 *ctx) > if (pelem == NULL) { > struct tstamp_data zero = {}; > > - bpf_map_update_elem(&tstamp, &pid, &zero, BPF_ANY); > + if (bpf_map_update_elem(&tstamp, &pid, &zero, BPF_NOEXIST) < 0) { > + __sync_fetch_and_add(&task_fail, 1); > + return 0; > + } > + > pelem = bpf_map_lookup_elem(&tstamp, &pid); > if (pelem == NULL) { > __sync_fetch_and_add(&task_fail, 1); > -- > 2.42.0.655.g421f12c284-goog >
diff --git a/tools/perf/util/bpf_skel/lock_contention.bpf.c b/tools/perf/util/bpf_skel/lock_contention.bpf.c index b11179452e19..69d31fd77cd0 100644 --- a/tools/perf/util/bpf_skel/lock_contention.bpf.c +++ b/tools/perf/util/bpf_skel/lock_contention.bpf.c @@ -328,7 +328,11 @@ int contention_begin(u64 *ctx) if (pelem == NULL) { struct tstamp_data zero = {}; - bpf_map_update_elem(&tstamp, &pid, &zero, BPF_ANY); + if (bpf_map_update_elem(&tstamp, &pid, &zero, BPF_NOEXIST) < 0) { + __sync_fetch_and_add(&task_fail, 1); + return 0; + } + pelem = bpf_map_lookup_elem(&tstamp, &pid); if (pelem == NULL) { __sync_fetch_and_add(&task_fail, 1);
When pelem is NULL, it'd create a new entry with zero data. But it might be preempted by IRQ/NMI just before calling bpf_map_update_elem() then there's a chance to call it twice for the same pid. So it'd be better to use BPF_NOEXIST flag and check the return value to prevent the race. Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- tools/perf/util/bpf_skel/lock_contention.bpf.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)