Messages in this thread | | | Date | Tue, 6 Apr 2021 16:21:36 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH 1/2] perf util: move bperf definitions to a libperf header |
| |
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?
> > Also add bperf_attr_map_compatible() to check whether existing attr_map > is compatible with current perf binary.
please separate this change
> > Signed-off-by: Song Liu <song@kernel.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 ;-)
> @@ -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
> + > +/* 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
thanks, jirka
> + > +#endif /* __LIBPERF_BPERF_H */ > diff --git a/tools/perf/util/bpf_counter.c b/tools/perf/util/bpf_counter.c > index 81d1df3c4ec0e..df70c8dcf7850 100644 > --- a/tools/perf/util/bpf_counter.c > +++ b/tools/perf/util/bpf_counter.c > @@ -14,6 +14,7 @@ > #include <bpf/btf.h> > #include <bpf/libbpf.h> > #include <api/fs/fs.h> > +#include <perf/bperf.h> > > #include "bpf_counter.h" > #include "counts.h" > @@ -29,28 +30,6 @@ > #include "bpf_skel/bperf_leader.skel.h" > #include "bpf_skel/bperf_follower.skel.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; > -}; > - > -#define DEFAULT_ATTR_MAP_PATH "fs/bpf/perf_attr_map" > #define ATTR_MAP_SIZE 16 > > static inline void *u64_to_ptr(__u64 ptr) > @@ -333,6 +312,20 @@ static __u32 bpf_map_get_id(int fd) > return map_info.id; > } > > +static bool bperf_attr_map_compatible(int attr_map_fd) > +{ > + struct bpf_map_info map_info = {0}; > + __u32 map_info_len = sizeof(map_info); > + int err; > + > + err = bpf_obj_get_info_by_fd(attr_map_fd, &map_info, &map_info_len); > + > + if (err) > + return false; > + return (map_info.key_size == sizeof(struct perf_event_attr)) && > + (map_info.value_size == sizeof(struct perf_event_attr_map_entry)); > +} > + > static int bperf_lock_attr_map(struct target *target) > { > char path[PATH_MAX]; > @@ -342,7 +335,7 @@ static int bperf_lock_attr_map(struct target *target) > scnprintf(path, PATH_MAX, "%s", target->attr_map); > } else { > scnprintf(path, PATH_MAX, "%s/%s", sysfs__mountpoint(), > - DEFAULT_ATTR_MAP_PATH); > + BPERF_DEFAULT_ATTR_MAP_PATH); > } > > if (access(path, F_OK)) { > @@ -367,6 +360,11 @@ static int bperf_lock_attr_map(struct target *target) > return -1; > } > > + if (!bperf_attr_map_compatible(map_fd)) { > + close(map_fd); > + return -1; > + > + } > err = flock(map_fd, LOCK_EX); > if (err) { > close(map_fd); > -- > 2.30.2 >
| |