Re: [PATCH -next v4 1/2] perf stat: Support inherit events during fork() for bperf

[Namhyung Kim <namhyung@kernel.org>]

On Sat, Oct 19, 2024 at 04:27:38PM +0800, Tengda Wu wrote:
> 
> 
> On 2024/10/19 1:12, Namhyung Kim wrote:
> > On Thu, Oct 17, 2024 at 10:53:46AM +0800, Tengda Wu wrote:
> >> Hi,
> >>
> >> On 2024/10/17 5:16, Namhyung Kim wrote:
> >>> Hello,
> >>>
> >>> On Sat, Oct 12, 2024 at 02:32:24AM +0000, Tengda Wu wrote:
> >>>> bperf has a nice ability to share PMUs, but it still does not support
> >>>> inherit events during fork(), resulting in some deviations in its stat
> >>>> results compared with perf.
> >>>>
> >>>> perf stat result:
> >>>> $ ./perf stat -e cycles,instructions -- ./perf test -w sqrtloop
> >>>>
> >>>>    Performance counter stats for './perf test -w sqrtloop':
> >>>>
> >>>>        2,316,038,116      cycles
> >>>>        2,859,350,725      instructions
> >>>>
> >>>>          1.009603637 seconds time elapsed
> >>>>
> >>>>          1.004196000 seconds user
> >>>>          0.003950000 seconds sys
> >>>>
> >>>> bperf stat result:
> >>>> $ ./perf stat --bpf-counters -e cycles,instructions -- \
> >>>>       ./perf test -w sqrtloop
> >>>>
> >>>>    Performance counter stats for './perf test -w sqrtloop':
> >>>>
> >>>>           18,762,093      cycles
> >>>>           23,487,766      instructions
> >>>>
> >>>>          1.008913769 seconds time elapsed
> >>>>
> >>>>          1.003248000 seconds user
> >>>>          0.004069000 seconds sys
> >>>>
> >>>> In order to support event inheritance, two new bpf programs are added
> >>>> to monitor the fork and exit of tasks respectively. When a task is
> >>>> created, add it to the filter map to enable counting, and reuse the
> >>>> `accum_key` of its parent task to count together with the parent task.
> >>>> When a task exits, remove it from the filter map to disable counting.
> >>>>
> >>>> After support:
> >>>> $ ./perf stat --bpf-counters -e cycles,instructions -- \
> >>>>       ./perf test -w sqrtloop
> >>>>
> >>>>  Performance counter stats for './perf test -w sqrtloop':
> >>>>
> >>>>      2,316,252,189      cycles
> >>>>      2,859,946,547      instructions
> >>>>
> >>>>        1.009422314 seconds time elapsed
> >>>>
> >>>>        1.003597000 seconds user
> >>>>        0.004270000 seconds sys
> >>>>
> >>>> Signed-off-by: Tengda Wu <wutengda@huaweicloud.com>
> >>>> ---
> >>>>  tools/perf/builtin-stat.c                     |  4 +-
> >>>>  tools/perf/util/bpf_counter.c                 | 57 +++++++++---
> >>>>  tools/perf/util/bpf_counter.h                 | 13 ++-
> >>>>  tools/perf/util/bpf_counter_cgroup.c          |  3 +-
> >>>>  tools/perf/util/bpf_skel/bperf_follower.bpf.c | 87 +++++++++++++++++--
> >>>>  tools/perf/util/bpf_skel/bperf_u.h            |  5 ++
> >>>>  6 files changed, 145 insertions(+), 24 deletions(-)
> >>>>
> >>>> diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
> >>>> index 3e6b9f216e80..c27b107c1985 100644
> >>>> --- a/tools/perf/builtin-stat.c
> >>>> +++ b/tools/perf/builtin-stat.c
> >>>> @@ -698,6 +698,7 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >>>>  	char msg[BUFSIZ];
> >>>>  	unsigned long long t0, t1;
> >>>>  	struct evsel *counter;
> >>>> +	struct bpf_stat_opts opts;
> >>>>  	size_t l;
> >>>>  	int status = 0;
> >>>>  	const bool forks = (argc > 0);
> >>>> @@ -725,7 +726,8 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
> >>>>  
> >>>>  	evlist__for_each_entry(evsel_list, counter) {
> >>>>  		counter->reset_group = false;
> >>>> -		if (bpf_counter__load(counter, &target)) {
> >>>> +		opts.inherit = !stat_config.no_inherit;
> >>>> +		if (bpf_counter__load(counter, &target, &opts)) {
> >>>
> >>> Maybe you can just add a boolean member in the struct target.
> >>
> >> Yes，this approach would be more straightforward. 
> >>
> >> I had considered it before, but, as you see, considering that `inherit` does not
> >> align well with the `target` semantics, I chose the another one.
> > 
> > Well, I think 'inherit' is well aligned with the target semantics.
> > We want some processes as the targets of the event and we want to
> > profile their children or not.
> > 
> 
> Ok.
> 
> >>
> >> Anyway, I'll try it. Code changes would be more clean. Thanks.
> >>
> >>>
> >>>
> >>>>  			err = -1;
> >>>>  			goto err_out;
> >>>>  		}
> >>>> diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c
> >>>> index 7a8af60e0f51..00afea6bde63 100644
> >>>> --- a/tools/perf/util/bpf_counter.c
> >>>> +++ b/tools/perf/util/bpf_counter.c
> >>>> @@ -166,7 +166,9 @@ static int bpf_program_profiler_load_one(struct evsel *evsel, u32 prog_id)
> >>>>  	return -1;
> >>>>  }
> >>>>  
> >>>> -static int bpf_program_profiler__load(struct evsel *evsel, struct target *target)
> >>>> +static int bpf_program_profiler__load(struct evsel *evsel,
> >>>> +				      struct target *target,
> >>>> +				      struct bpf_stat_opts *opts __maybe_unused)
> >>>>  {
> >>>>  	char *bpf_str, *bpf_str_, *tok, *saveptr = NULL, *p;
> >>>>  	u32 prog_id;
> >>>> @@ -364,6 +366,7 @@ static int bperf_lock_attr_map(struct target *target)
> >>>>  
> >>>>  static int bperf_check_target(struct evsel *evsel,
> >>>>  			      struct target *target,
> >>>> +			      struct bpf_stat_opts *opts,
> >>>>  			      enum bperf_filter_type *filter_type,
> >>>>  			      __u32 *filter_entry_cnt)
> >>>>  {
> >>>> @@ -383,7 +386,12 @@ static int bperf_check_target(struct evsel *evsel,
> >>>>  		*filter_type = BPERF_FILTER_PID;
> >>>>  		*filter_entry_cnt = perf_thread_map__nr(evsel->core.threads);
> >>>>  	} else if (target->pid || evsel->evlist->workload.pid != -1) {
> >>>> -		*filter_type = BPERF_FILTER_TGID;
> >>>> +		/*
> >>>> +		 * unlike the PID type, the TGID type implicitly enables
> >>>> +		 * event inheritance within a single process.
> >>>> +		 */
> >>>> +		*filter_type = opts->inherit ?
> >>>> +				BPERF_FILTER_TGID : BPERF_FILTER_PID;
> >>>
> >>> I'm not sure if it's right.  You should be able to use PID type with
> >>> inheritance.  In this case child processes or threads from the selected
> >>> thread would be counted only.
> >>
> >> Sorry, don't quite understand. TGID type counts together with all sub-threads within
> >> the same process, which is what inheritance needs to do; while PID type only counts
> >> for a single thread and should be used when inheritance is turned off. This is equivalent
> >> to the code above.
> > 
> > Let me be clear:
> > 
> >  * PID w/o inherit : specified threads only
> >  * PID w/ inherit  : specified threads + all threads or child process from the threads
> >  * TGID w/o inherit: specified process (all threads in the process) only
> >  * TGID w/ inherit : specified process + all children from the processes
> > 
> > For the TGID w/o inherit case, it's ok not to track new threads in the
> > process because they will have the same tgid anyway.
> > 
> > So you cannot change the filter type using inherit value.  It should be
> > used to control whether it tracks new task only.
> > 
> 
> So changing 'TGID w/o inherit' to 'PID w/o inherit' will lose counts of all
> threads in the process, right?

Yep.

> 
> It's clear now. Thanks for the explanation.
 
No problem, looking forward to v5.

Thanks,
Namhyung