linux-perf-users.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] perf bpf_counter: Fix opening of "any"(-1) CPU events
@ 2025-10-07 18:58 Ian Rogers
  2025-10-08  7:35 ` Namhyung Kim
  0 siblings, 1 reply; 3+ messages in thread
From: Ian Rogers @ 2025-10-07 18:58 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Namhyung Kim, Alexander Shishkin, Jiri Olsa, Ian Rogers,
	Adrian Hunter, Tengda Wu, linux-perf-users, linux-kernel

The bperf BPF counter code doesn't handle "any"(-1) CPU events, always
wanting to aggregate a count against a CPU, which avoids the need for
atomics. Force evsels used for BPF counters to require a CPU when not
in system-wide mode so that the "any"(-1) value isn't used during map
propagation and evsel's CPU map matches that of the PMU.

Fixes: b91917c0c6fa ("perf bpf_counter: Fix handling of cpumap fixing hybrid")
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/builtin-stat.c     | 12 ++++++++++++
 tools/perf/util/bpf_counter.c |  1 +
 2 files changed, 13 insertions(+)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 7006f848f87a..7fdc7f273a48 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -2797,6 +2797,18 @@ int cmd_stat(int argc, const char **argv)
 
 	evlist__warn_user_requested_cpus(evsel_list, target.cpu_list);
 
+	if (target.use_bpf && !target.system_wide) {
+		/*
+		 * Setup BPF counters to require CPUs as any(-1) isn't
+		 * supported. evlist__create_maps below will propagate this
+		 * information to the evsels.
+		 */
+		struct evsel *counter;
+
+		evlist__for_each_entry(evsel_list, counter)
+			counter->core.requires_cpu = true;
+	}
+
 	if (evlist__create_maps(evsel_list, &target) < 0) {
 		if (target__has_task(&target)) {
 			pr_err("Problems finding threads of monitor\n");
diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
index ca5d01b9017d..d3e5933b171b 100644
--- a/tools/perf/util/bpf_counter.c
+++ b/tools/perf/util/bpf_counter.c
@@ -495,6 +495,7 @@ static int bperf_reload_leader_program(struct evsel *evsel, int attr_map_fd,
 	 * following evsel__open_per_cpu call
 	 */
 	evsel->leader_skel = skel;
+	assert(!perf_cpu_map__has_any_cpu_or_is_empty(evsel->core.cpus));
 	evsel__open(evsel, evsel->core.cpus, evsel->core.threads);
 
 out:
-- 
2.51.0.710.ga91ca5db03-goog


^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH v1] perf bpf_counter: Fix opening of "any"(-1) CPU events
  2025-10-07 18:58 [PATCH v1] perf bpf_counter: Fix opening of "any"(-1) CPU events Ian Rogers
@ 2025-10-08  7:35 ` Namhyung Kim
  2025-10-08 15:08   ` Ian Rogers
  0 siblings, 1 reply; 3+ messages in thread
From: Namhyung Kim @ 2025-10-08  7:35 UTC (permalink / raw)
  To: Ian Rogers
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Jiri Olsa, Adrian Hunter, Tengda Wu,
	linux-perf-users, linux-kernel

Hi Ian,

On Tue, Oct 07, 2025 at 11:58:26AM -0700, Ian Rogers wrote:
> The bperf BPF counter code doesn't handle "any"(-1) CPU events, always
> wanting to aggregate a count against a CPU, which avoids the need for
> atomics. Force evsels used for BPF counters to require a CPU when not
> in system-wide mode so that the "any"(-1) value isn't used during map
> propagation and evsel's CPU map matches that of the PMU.
> 
> Fixes: b91917c0c6fa ("perf bpf_counter: Fix handling of cpumap fixing hybrid")
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/builtin-stat.c     | 12 ++++++++++++
>  tools/perf/util/bpf_counter.c |  1 +
>  2 files changed, 13 insertions(+)
> 
> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> index 7006f848f87a..7fdc7f273a48 100644
> --- a/tools/perf/builtin-stat.c
> +++ b/tools/perf/builtin-stat.c
> @@ -2797,6 +2797,18 @@ int cmd_stat(int argc, const char **argv)
>  
>  	evlist__warn_user_requested_cpus(evsel_list, target.cpu_list);
>  
> +	if (target.use_bpf && !target.system_wide) {

The target.use_bpf only checks the --bpf-counters option.  But IIUC it's
possible to use BPF only for selected events with ':b' modifier.  I
think you need to check each evsel with evsel__is_bperf().

Also system_wide might not be set for -C/--cpu option.  Probably you
want target__has_cpu() instead of target.system_wide.

Thanks,
Namhyung


> +		/*
> +		 * Setup BPF counters to require CPUs as any(-1) isn't
> +		 * supported. evlist__create_maps below will propagate this
> +		 * information to the evsels.
> +		 */
> +		struct evsel *counter;
> +
> +		evlist__for_each_entry(evsel_list, counter)
> +			counter->core.requires_cpu = true;
> +	}
> +
>  	if (evlist__create_maps(evsel_list, &target) < 0) {
>  		if (target__has_task(&target)) {
>  			pr_err("Problems finding threads of monitor\n");
> diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
> index ca5d01b9017d..d3e5933b171b 100644
> --- a/tools/perf/util/bpf_counter.c
> +++ b/tools/perf/util/bpf_counter.c
> @@ -495,6 +495,7 @@ static int bperf_reload_leader_program(struct evsel *evsel, int attr_map_fd,
>  	 * following evsel__open_per_cpu call
>  	 */
>  	evsel->leader_skel = skel;
> +	assert(!perf_cpu_map__has_any_cpu_or_is_empty(evsel->core.cpus));
>  	evsel__open(evsel, evsel->core.cpus, evsel->core.threads);
>  
>  out:
> -- 
> 2.51.0.710.ga91ca5db03-goog
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH v1] perf bpf_counter: Fix opening of "any"(-1) CPU events
  2025-10-08  7:35 ` Namhyung Kim
@ 2025-10-08 15:08   ` Ian Rogers
  0 siblings, 0 replies; 3+ messages in thread
From: Ian Rogers @ 2025-10-08 15:08 UTC (permalink / raw)
  To: Namhyung Kim
  Cc: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo,
	Alexander Shishkin, Jiri Olsa, Adrian Hunter, Tengda Wu,
	linux-perf-users, linux-kernel

On Wed, Oct 8, 2025 at 12:35 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> Hi Ian,
>
> On Tue, Oct 07, 2025 at 11:58:26AM -0700, Ian Rogers wrote:
> > The bperf BPF counter code doesn't handle "any"(-1) CPU events, always
> > wanting to aggregate a count against a CPU, which avoids the need for
> > atomics. Force evsels used for BPF counters to require a CPU when not
> > in system-wide mode so that the "any"(-1) value isn't used during map
> > propagation and evsel's CPU map matches that of the PMU.
> >
> > Fixes: b91917c0c6fa ("perf bpf_counter: Fix handling of cpumap fixing hybrid")
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/builtin-stat.c     | 12 ++++++++++++
> >  tools/perf/util/bpf_counter.c |  1 +
> >  2 files changed, 13 insertions(+)
> >
> > diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> > index 7006f848f87a..7fdc7f273a48 100644
> > --- a/tools/perf/builtin-stat.c
> > +++ b/tools/perf/builtin-stat.c
> > @@ -2797,6 +2797,18 @@ int cmd_stat(int argc, const char **argv)
> >
> >       evlist__warn_user_requested_cpus(evsel_list, target.cpu_list);
> >
> > +     if (target.use_bpf && !target.system_wide) {
>
> The target.use_bpf only checks the --bpf-counters option.  But IIUC it's
> possible to use BPF only for selected events with ':b' modifier.  I
> think you need to check each evsel with evsel__is_bperf().
>
> Also system_wide might not be set for -C/--cpu option.  Probably you
> want target__has_cpu() instead of target.system_wide.

Thanks Namhyung, I'll fix in v2.

Ian

> Thanks,
> Namhyung
>
>
> > +             /*
> > +              * Setup BPF counters to require CPUs as any(-1) isn't
> > +              * supported. evlist__create_maps below will propagate this
> > +              * information to the evsels.
> > +              */
> > +             struct evsel *counter;
> > +
> > +             evlist__for_each_entry(evsel_list, counter)
> > +                     counter->core.requires_cpu = true;
> > +     }
> > +
> >       if (evlist__create_maps(evsel_list, &target) < 0) {
> >               if (target__has_task(&target)) {
> >                       pr_err("Problems finding threads of monitor\n");
> > diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
> > index ca5d01b9017d..d3e5933b171b 100644
> > --- a/tools/perf/util/bpf_counter.c
> > +++ b/tools/perf/util/bpf_counter.c
> > @@ -495,6 +495,7 @@ static int bperf_reload_leader_program(struct evsel *evsel, int attr_map_fd,
> >        * following evsel__open_per_cpu call
> >        */
> >       evsel->leader_skel = skel;
> > +     assert(!perf_cpu_map__has_any_cpu_or_is_empty(evsel->core.cpus));
> >       evsel__open(evsel, evsel->core.cpus, evsel->core.threads);
> >
> >  out:
> > --
> > 2.51.0.710.ga91ca5db03-goog
> >

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2025-10-08 15:08 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-07 18:58 [PATCH v1] perf bpf_counter: Fix opening of "any"(-1) CPU events Ian Rogers
2025-10-08  7:35 ` Namhyung Kim
2025-10-08 15:08   ` Ian Rogers

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).