[PATCH] perf lock contention: Fix spinlock and rwlock accounting

[PATCH] perf lock contention: Fix spinlock and rwlock accounting

[Namhyung Kim]

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(+)

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]);
 		}
-- 
2.46.0.295.g3b9ea8a38a-goog

Re: [PATCH] perf lock contention: Fix spinlock and rwlock accounting

[Arnaldo Carvalho de Melo]

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

Re: [PATCH] perf lock contention: Fix spinlock and rwlock accounting

[Arnaldo Carvalho de Melo]

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