Re: [PATCH 3/3] perf stat: Enable BPF counter with --for-each-cgroup

[Song Liu <songliubraving@fb.com>]

> On Jun 23, 2021, at 10:49 PM, Namhyung Kim <namhyung@kernel.org> wrote:
[...]

>>> +
>>> +     skel->rodata->num_cpus = nr_cpus;
>>> +     skel->rodata->num_events = evlist->core.nr_entries / nr_cgroups;
>>> +
>>> +     BUG_ON(evlist->core.nr_entries % nr_cgroups != 0);
>>> +
>>> +     /* we need one copy of events per cpu for reading */
>>> +     map_size = nr_cpus * evlist->core.nr_entries / nr_cgroups;
>>> +     bpf_map__resize(skel->maps.events, map_size);
>>> +     bpf_map__resize(skel->maps.cpu_idx, nr_cpus);
>>> +     bpf_map__resize(skel->maps.cgrp_idx, nr_cgroups);
>>> +     /* previous result is saved in a per-cpu array */
>>> +     map_size = evlist->core.nr_entries / nr_cgroups;
>>> +     bpf_map__resize(skel->maps.prev_readings, map_size);
>>> +     /* cgroup result needs all events */
>>> +     map_size = nr_cpus * evlist->core.nr_entries;
>>> +     bpf_map__resize(skel->maps.cgrp_readings, map_size);
>> 
>> We are setting map_size back and forth here.
> 
> But they are all different sizes.

Right. I misread the code. 

> 
>> 
>> [...]
>> 
>> 
>>> diff --git a/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c b/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c
>>> new file mode 100644
>>> index 000000000000..6d74e93dd1f5
>>> --- /dev/null
>>> +++ b/tools/perf/util/bpf_skel/bperf_cgroup.bpf.c
>>> @@ -0,0 +1,207 @@
>>> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +// Copyright (c) 2021 Facebook
>>> +// Copyright (c) 2021 Google
>>> +#include "vmlinux.h"
>>> +#include <bpf/bpf_helpers.h>
>>> +#include <bpf/bpf_tracing.h>
>>> +#include <bpf/bpf_core_read.h>
>>> +
>>> +#define MAX_LEVELS  10  // max cgroup hierarchy level: arbitrary
>>> +#define MAX_EVENTS  32  // max events per cgroup: arbitrary
>>> +
>>> +// NOTE: many of map and global data will be modified before loading
>>> +//       from the userspace (perf tool) using the skeleton helpers.
>>> +
>>> +// single set of global perf events to measure
>>> +struct {
>>> +     __uint(type, BPF_MAP_TYPE_PERF_EVENT_ARRAY);
>>> +     __uint(key_size, sizeof(__u32));
>>> +     __uint(value_size, sizeof(int));
>>> +     __uint(max_entries, 1);
>>> +} events SEC(".maps");
>>> +
>>> +// from logical cpu number to event index
>>> +// useful when user wants to count subset of cpus
>>> +struct {
>>> +     __uint(type, BPF_MAP_TYPE_HASH);
>>> +     __uint(key_size, sizeof(__u32));
>>> +     __uint(value_size, sizeof(__u32));
>>> +     __uint(max_entries, 1);
>>> +} cpu_idx SEC(".maps");
>> 
>> How about we make cpu_idx a percpu array and use 0,1 for
>> disable/enable profiling on this cpu?
> 
> No, it's to calculate an index to the cgrp_readings map which
> has the event x cpu x cgroup number of elements.
> 
> It controls enabling events with a global (bss) variable.

If we make cgrp_idx a per cpu array, we probably don't need the
cpu_idx map? 

> 
>> 
>>> +
>>> +// from cgroup id to event index
>>> +struct {
>>> +     __uint(type, BPF_MAP_TYPE_HASH);
>>> +     __uint(key_size, sizeof(__u64));
>>> +     __uint(value_size, sizeof(__u32));
>>> +     __uint(max_entries, 1);
>>> +} cgrp_idx SEC(".maps");
>>> +
>>> +// per-cpu event snapshots to calculate delta
>>> +struct {
>>> +     __uint(type, BPF_MAP_TYPE_PERCPU_ARRAY);
>>> +     __uint(key_size, sizeof(__u32));
>>> +     __uint(value_size, sizeof(struct bpf_perf_event_value));
>>> +} prev_readings SEC(".maps");
>>> +
>>> +// aggregated event values for each cgroup
>>> +// will be read from the user-space
>>> +struct {
>>> +     __uint(type, BPF_MAP_TYPE_ARRAY);
>>> +     __uint(key_size, sizeof(__u32));
>>> +     __uint(value_size, sizeof(struct bpf_perf_event_value));
>>> +} cgrp_readings SEC(".maps");
>> 
>> Maybe also make this a percpu array? This should make the BPF program
>> faster.
> 
> Maybe.  But I don't know how to access the elements
> in a per-cpu map from userspace.

Please refer to bperf__read() reading accum_readings. Basically, we read
one index of all CPUs with one bpf_map_lookup_elem(). 

Thanks,
Song