| Message ID | 20240828052953.1445862-1-namhyung@kernel.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | perf lock contention: Fix spinlock and rwlock accounting | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, Aug 27, 2024 at 10:29:53PM -0700, Namhyung Kim wrote: > The spinlock and rwlock use a single-element per-cpu array to track > current locks due to performance reason. But this means the key is > always available and it cannot simply account lock stats in the array > because some of them are invalid. > > In fact, the contention_end() program in the BPF invalidates the entry > by setting the 'lock' value to 0 instead of deleting the entry for the > hashmap. So it should skip entries with the lock value of 0 in the > account_end_timestamp(). Thanks, applied to perf-tools-next, - Arnaldo
On Fri, Aug 30, 2024 at 08:51:43AM -0700, Namhyung Kim wrote: > Hi Arnaldo, > > On Fri, Aug 30, 2024 at 6:23 AM Arnaldo Carvalho de Melo > <acme@kernel.org> wrote: > > > > On Tue, Aug 27, 2024 at 10:29:53PM -0700, Namhyung Kim wrote: > > > The spinlock and rwlock use a single-element per-cpu array to track > > > current locks due to performance reason. But this means the key is > > > always available and it cannot simply account lock stats in the array > > > because some of them are invalid. > > > > > > In fact, the contention_end() program in the BPF invalidates the entry > > > by setting the 'lock' value to 0 instead of deleting the entry for the > > > hashmap. So it should skip entries with the lock value of 0 in the > > > account_end_timestamp(). > > > > Thanks, applied to perf-tools-next, > > I think this can go to perf-tools instead. I think I published it already, don't think this is a major problem tho, we can make a note when submitting for v6.12 that there are a few patches that are already mainline. For the future, its interesting that when posting patches we inform the intended branch where it should be applied, something like: [PATCH perf-tools] ... Or I can add something to my scripts to check if the patch is a regression introduced in the current merge window... - Arnaldo
diff --git a/tools/perf/util/bpf_lock_contention.c b/tools/perf/util/bpf_lock_contention.c index 4ee54538aba2..a3d40940fb23 100644 --- a/tools/perf/util/bpf_lock_contention.c +++ b/tools/perf/util/bpf_lock_contention.c @@ -296,6 +296,9 @@ static void account_end_timestamp(struct lock_contention *con) goto next; for (int i = 0; i < total_cpus; i++) { + if (cpu_data[i].lock == 0) + continue; + update_lock_stat(stat_fd, -1, end_ts, aggr_mode, &cpu_data[i]); }
The spinlock and rwlock use a single-element per-cpu array to track current locks due to performance reason. But this means the key is always available and it cannot simply account lock stats in the array because some of them are invalid. In fact, the contention_end() program in the BPF invalidates the entry by setting the 'lock' value to 0 instead of deleting the entry for the hashmap. So it should skip entries with the lock value of 0 in the account_end_timestamp(). Otherwise, it'd have spurious high contention on an idle machine: $ sudo perf lock con -ab -Y spinlock sleep 3 contended total wait max wait avg wait type caller 8 4.72 s 1.84 s 590.46 ms spinlock rcu_core+0xc7 8 1.87 s 1.87 s 233.48 ms spinlock process_one_work+0x1b5 2 1.87 s 1.87 s 933.92 ms spinlock worker_thread+0x1a2 3 1.81 s 1.81 s 603.93 ms spinlock tmigr_update_events+0x13c 2 1.72 s 1.72 s 861.98 ms spinlock tick_do_update_jiffies64+0x25 6 42.48 us 13.02 us 7.08 us spinlock futex_q_lock+0x2a 1 13.03 us 13.03 us 13.03 us spinlock futex_wake+0xce 1 11.61 us 11.61 us 11.61 us spinlock rcu_core+0xc7 I don't believe it has contention on a spinlock longer than 1 second. After this change, it only reports some small contentions. $ sudo perf lock con -ab -Y spinlock sleep 3 contended total wait max wait avg wait type caller 4 133.51 us 43.29 us 33.38 us spinlock tick_do_update_jiffies64+0x25 4 69.06 us 31.82 us 17.27 us spinlock process_one_work+0x1b5 2 50.66 us 25.77 us 25.33 us spinlock rcu_core+0xc7 1 28.45 us 28.45 us 28.45 us spinlock rcu_core+0xc7 1 24.77 us 24.77 us 24.77 us spinlock tmigr_update_events+0x13c 1 23.34 us 23.34 us 23.34 us spinlock raw_spin_rq_lock_nested+0x15 Fixes: b5711042a1c8 ("perf lock contention: Use per-cpu array map for spinlocks") Reported-by: Xi Wang <xii@google.com> Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- tools/perf/util/bpf_lock_contention.c | 3 +++ 1 file changed, 3 insertions(+)