[v1] perf tools: Skip BPF sideband event for userspace profiling

[PATCH] perf tools: Skip BPF sideband event for userspace profiling

Posted by Namhyung Kim 11 months, 2 weeks ago

The BPF sideband information is tracked using a separate thread and
evlist.  But it's only useful for profiling kernel and we can skip it
when users profile their application only.

It seems it already fails to open the sideband event in that case.
Let's remove the noise in the verbose output anyway.

Cc: Song Liu <song@kernel.org>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
 tools/perf/builtin-record.c |  3 +++
 tools/perf/builtin-top.c    |  3 +++
 tools/perf/util/evlist.c    | 14 ++++++++++++++
 tools/perf/util/evlist.h    |  1 +
 4 files changed, 21 insertions(+)

diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
index 0e45bd64185403ae..cc61f5d6c599039c 100644
--- a/tools/perf/builtin-record.c
+++ b/tools/perf/builtin-record.c
@@ -2535,6 +2535,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
 		goto out_free_threads;
 	}
 
+	if (!evlist__needs_bpf_sb_event(rec->evlist))
+		opts->no_bpf_event = true;
+
 	err = record__setup_sb_evlist(rec);
 	if (err)
 		goto out_free_threads;
diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
index 6440b5c1757d92ce..c284a384542ff822 100644
--- a/tools/perf/builtin-top.c
+++ b/tools/perf/builtin-top.c
@@ -1833,6 +1833,9 @@ int cmd_top(int argc, const char **argv)
 		goto out_delete_evlist;
 	}
 
+	if (!evlist__needs_bpf_sb_event(top.evlist))
+		top.record_opts.no_bpf_event = true;
+
 #ifdef HAVE_LIBBPF_SUPPORT
 	if (!top.record_opts.no_bpf_event) {
 		top.sb_evlist = evlist__new();
diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index f0dd174e2debdbe8..43adf6b3d855771a 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -2594,3 +2594,17 @@ bool evlist__has_bpf_output(struct evlist *evlist)
 
 	return false;
 }
+
+bool evlist__needs_bpf_sb_event(struct evlist *evlist)
+{
+	struct evsel *evsel;
+
+	evlist__for_each_entry(evlist, evsel) {
+		if (evsel__is_dummy_event(evsel))
+			continue;
+		if (!evsel->core.attr.exclude_kernel)
+			return true;
+	}
+
+	return false;
+}
diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
index adddb1db1ad2b25d..edcbf1c10e92f0c4 100644
--- a/tools/perf/util/evlist.h
+++ b/tools/perf/util/evlist.h
@@ -435,5 +435,6 @@ void evlist__check_mem_load_aux(struct evlist *evlist);
 void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_list);
 void evlist__uniquify_name(struct evlist *evlist);
 bool evlist__has_bpf_output(struct evlist *evlist);
+bool evlist__needs_bpf_sb_event(struct evlist *evlist);
 
 #endif /* __PERF_EVLIST_H */
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH] perf tools: Skip BPF sideband event for userspace profiling

Posted by Namhyung Kim 11 months, 1 week ago

On Wed, 26 Feb 2025 12:30:39 -0800, Namhyung Kim wrote:
> The BPF sideband information is tracked using a separate thread and
> evlist.  But it's only useful for profiling kernel and we can skip it
> when users profile their application only.
> 
> It seems it already fails to open the sideband event in that case.
> Let's remove the noise in the verbose output anyway.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung

Re: [PATCH] perf tools: Skip BPF sideband event for userspace profiling

Posted by Ian Rogers 11 months, 2 weeks ago

On Wed, Feb 26, 2025 at 12:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> The BPF sideband information is tracked using a separate thread and
> evlist.  But it's only useful for profiling kernel and we can skip it
> when users profile their application only.

nit: It may be worth noting that profiling an application implicitly
excludes the kernel samples.

> It seems it already fails to open the sideband event in that case.
> Let's remove the noise in the verbose output anyway.
>
> Cc: Song Liu <song@kernel.org>
> Signed-off-by: Namhyung Kim <namhyung@kernel.org>

Reviewed-by: Ian Rogers <irogers@google.com>

I wonder if the second evlist could be avoided similar to Howard's
off-CPU sample events?
I also wonder if we should make the evlist responsible for BPF and
dummy/sideband events. Having unnecessary events increases the list
size iterated over when creating sideband data, and so has a runtime
cost. Having the logic separated in places like builtin-top and
builtin-record feels suboptimal.

Thanks,
Ian

> ---
>  tools/perf/builtin-record.c |  3 +++
>  tools/perf/builtin-top.c    |  3 +++
>  tools/perf/util/evlist.c    | 14 ++++++++++++++
>  tools/perf/util/evlist.h    |  1 +
>  4 files changed, 21 insertions(+)
>
> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> index 0e45bd64185403ae..cc61f5d6c599039c 100644
> --- a/tools/perf/builtin-record.c
> +++ b/tools/perf/builtin-record.c
> @@ -2535,6 +2535,9 @@ static int __cmd_record(struct record *rec, int argc, const char **argv)
>                 goto out_free_threads;
>         }
>
> +       if (!evlist__needs_bpf_sb_event(rec->evlist))
> +               opts->no_bpf_event = true;
> +
>         err = record__setup_sb_evlist(rec);
>         if (err)
>                 goto out_free_threads;
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 6440b5c1757d92ce..c284a384542ff822 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -1833,6 +1833,9 @@ int cmd_top(int argc, const char **argv)
>                 goto out_delete_evlist;
>         }
>
> +       if (!evlist__needs_bpf_sb_event(top.evlist))
> +               top.record_opts.no_bpf_event = true;
> +
>  #ifdef HAVE_LIBBPF_SUPPORT
>         if (!top.record_opts.no_bpf_event) {
>                 top.sb_evlist = evlist__new();
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index f0dd174e2debdbe8..43adf6b3d855771a 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -2594,3 +2594,17 @@ bool evlist__has_bpf_output(struct evlist *evlist)
>
>         return false;
>  }
> +
> +bool evlist__needs_bpf_sb_event(struct evlist *evlist)
> +{
> +       struct evsel *evsel;
> +
> +       evlist__for_each_entry(evlist, evsel) {
> +               if (evsel__is_dummy_event(evsel))
> +                       continue;
> +               if (!evsel->core.attr.exclude_kernel)
> +                       return true;
> +       }
> +
> +       return false;
> +}
> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> index adddb1db1ad2b25d..edcbf1c10e92f0c4 100644
> --- a/tools/perf/util/evlist.h
> +++ b/tools/perf/util/evlist.h
> @@ -435,5 +435,6 @@ void evlist__check_mem_load_aux(struct evlist *evlist);
>  void evlist__warn_user_requested_cpus(struct evlist *evlist, const char *cpu_list);
>  void evlist__uniquify_name(struct evlist *evlist);
>  bool evlist__has_bpf_output(struct evlist *evlist);
> +bool evlist__needs_bpf_sb_event(struct evlist *evlist);
>
>  #endif /* __PERF_EVLIST_H */
> --
> 2.48.1.658.g4767266eb4-goog
>

Re: [PATCH] perf tools: Skip BPF sideband event for userspace profiling

Posted by Song Liu 11 months, 2 weeks ago

On Wed, Feb 26, 2025 at 12:59 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Feb 26, 2025 at 12:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > The BPF sideband information is tracked using a separate thread and
> > evlist.  But it's only useful for profiling kernel and we can skip it
> > when users profile their application only.
>
> nit: It may be worth noting that profiling an application implicitly
> excludes the kernel samples.
>
> > It seems it already fails to open the sideband event in that case.
> > Let's remove the noise in the verbose output anyway.
> >
> > Cc: Song Liu <song@kernel.org>
> > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
>
> Reviewed-by: Ian Rogers <irogers@google.com>
>
> I wonder if the second evlist could be avoided similar to Howard's
> off-CPU sample events?
> I also wonder if we should make the evlist responsible for BPF and
> dummy/sideband events. Having unnecessary events increases the list
> size iterated over when creating sideband data, and so has a runtime
> cost. Having the logic separated in places like builtin-top and
> builtin-record feels suboptimal.

It appears we can indeed put this logic in evlist.

Thanks,
Song

> Thanks,
> Ian
>

Re: [PATCH] perf tools: Skip BPF sideband event for userspace profiling

Posted by Namhyung Kim 11 months, 2 weeks ago

Hello,

On Wed, Feb 26, 2025 at 02:00:36PM -0800, Song Liu wrote:
> On Wed, Feb 26, 2025 at 12:59 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Wed, Feb 26, 2025 at 12:30 PM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > The BPF sideband information is tracked using a separate thread and
> > > evlist.  But it's only useful for profiling kernel and we can skip it
> > > when users profile their application only.
> >
> > nit: It may be worth noting that profiling an application implicitly
> > excludes the kernel samples.
> >
> > > It seems it already fails to open the sideband event in that case.
> > > Let's remove the noise in the verbose output anyway.
> > >
> > > Cc: Song Liu <song@kernel.org>
> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org>
> >
> > Reviewed-by: Ian Rogers <irogers@google.com>

Thanks for your review!

> >
> > I wonder if the second evlist could be avoided similar to Howard's
> > off-CPU sample events?
> > I also wonder if we should make the evlist responsible for BPF and
> > dummy/sideband events. Having unnecessary events increases the list
> > size iterated over when creating sideband data, and so has a runtime
> > cost. Having the logic separated in places like builtin-top and
> > builtin-record feels suboptimal.
> 
> It appears we can indeed put this logic in evlist.

Ok, I'll give it a spin.

Thanks,
Namhyung