linux-kernel - Re: [PATCH 1/2] perf util: move bperf definitions to a libperf header

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: <FAF2CD39-4FDB-4E6D-96AB-E246030DF8CC@fb.com>
Date:   Tue, 6 Apr 2021 16:26:03 +0000
From:   Song Liu <songliubraving@...com>
To:     Jiri Olsa <jolsa@...hat.com>
CC:     Song Liu <song@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Kernel Team <Kernel-team@...com>,
        "acme@...nel.org" <acme@...nel.org>,
        "acme@...hat.com" <acme@...hat.com>,
        "namhyung@...nel.org" <namhyung@...nel.org>,
        "jolsa@...nel.org" <jolsa@...nel.org>
Subject: Re: [PATCH 1/2] perf util: move bperf definitions to a libperf header



> On Apr 6, 2021, at 7:21 AM, Jiri Olsa <jolsa@...hat.com> wrote:
> 
> On Fri, Apr 02, 2021 at 05:29:37PM -0700, Song Liu wrote:
>> By following the same protocol, other tools can share hardware PMCs with
>> perf. Move perf_event_attr_map_entry and BPERF_DEFAULT_ATTR_MAP_PATH to
>> bperf.h for other tools to use.
> 
> hi,
> so is this necessary for some other tool now?

We have monitoring tools do perf_event_open(). I would like to migrate these
to bperf. 

> 
>> 
>> Also add bperf_attr_map_compatible() to check whether existing attr_map
>> is compatible with current perf binary.
> 
> please separate this change

Will do. 

> 
>> 
>> Signed-off-by: Song Liu <song@...nel.org>
>> ---
>> tools/lib/perf/include/perf/bperf.h | 29 +++++++++++++++++++
>> tools/perf/util/bpf_counter.c       | 44 ++++++++++++++---------------
>> 2 files changed, 50 insertions(+), 23 deletions(-)
>> create mode 100644 tools/lib/perf/include/perf/bperf.h
>> 
>> diff --git a/tools/lib/perf/include/perf/bperf.h b/tools/lib/perf/include/perf/bperf.h
>> new file mode 100644
>> index 0000000000000..02b2fd5e50c75
>> --- /dev/null
>> +++ b/tools/lib/perf/include/perf/bperf.h
> 
> I wonder we want to call it bpf_perf.h to be more generic?
> or best just bpf.h ... but that might give us some conflict
> headache in future ;-)

I would rather avoid bpf.h... I am ok with bpf_perf.h or bperf.h

> 
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */
>> +#ifndef __LIBPERF_BPERF_H
>> +#define __LIBPERF_BPERF_H
>> +
>> +/*
>> + * bperf uses a hashmap, the attr_map, to track all the leader programs.
>> + * The hashmap is pinned in bpffs. flock() on this file is used to ensure
>> + * no concurrent access to the attr_map.  The key of attr_map is struct
>> + * perf_event_attr, and the value is struct perf_event_attr_map_entry.
>> + *
>> + * struct perf_event_attr_map_entry contains two __u32 IDs, bpf_link of the
>> + * leader prog, and the diff_map. Each perf-stat session holds a reference
>> + * to the bpf_link to make sure the leader prog is attached to sched_switch
>> + * tracepoint.
>> + *
>> + * Since the hashmap only contains IDs of the bpf_link and diff_map, it
>> + * does not hold any references to the leader program. Once all perf-stat
>> + * sessions of these events exit, the leader prog, its maps, and the
>> + * perf_events will be freed.
>> + */
>> +struct perf_event_attr_map_entry {
>> +	__u32 link_id;
>> +	__u32 diff_map_id;
>> +};
> 
> this header file should be self contained,
> so you need __u32 definitions

Will add. 

> 
> 
>> +
>> +/* pin the map at sysfs__mountpoint()/BPERF_DEFAULT_ATTR_MAP_PATH */
>> +#define BPERF_DEFAULT_ATTR_MAP_PATH "fs/bpf/perf_attr_map"
> 
> if we are going to expose this, I think we should expose just
> "perf_attr_map" ... without the 'fs/bpf' part, because that
> could be mounted anywhere

Will fix this. 

Thanks,
Song