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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <D5859F92-8AB6-4327-9D5F-A67DB14CB493@fb.com>
Date:   Thu, 24 Jun 2021 16:20:07 +0000
From:   Song Liu <songliubraving@...com>
To:     Namhyung Kim <namhyung@...nel.org>
CC:     Arnaldo Carvalho de Melo <acme@...nel.org>,
        Jiri Olsa <jolsa@...hat.com>, Ingo Molnar <mingo@...nel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Andi Kleen <ak@...ux.intel.com>,
        Ian Rogers <irogers@...gle.com>,
        Stephane Eranian <eranian@...gle.com>
Subject: Re: [PATCH 3/3] perf stat: Enable BPF counter with --for-each-cgroup



> On Jun 23, 2021, at 10:49 PM, Namhyung Kim <namhyung@...nel.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