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

Song Liu
2021-06-24T16:20:32Z

> 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