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@xxxxxxxxxxxxxxx>
> >>>> ---
> >>>> 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
