Messages in this thread | | | From | Namhyung Kim <> | Date | Fri, 20 May 2022 11:59:35 -0700 | Subject | Re: [PATCH 2/7] libperf: Introduce perf_{evsel, evlist}__open_opt with extensible struct opts |
| |
Hello,
On Fri, Apr 22, 2022 at 2:44 AM Shunsuke Nakamura <nakamura.shun@fujitsu.com> wrote: > > Introduce perf_{evsel, evlist}__open_opt with extensible structure opts. > The mechanism of the extensible structure opts imitates > tools/lib/bpf/libbpf.h. Currently, only open_flags is supported for the > opts structure. > > Signed-off-by: Shunsuke Nakamura <nakamura.shun@fujitsu.com> > --- [SNIP] > diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c > index 210ea7c06ce8..00c0cea43b52 100644 > --- a/tools/lib/perf/evsel.c > +++ b/tools/lib/perf/evsel.c > @@ -16,8 +16,12 @@ > #include <internal/lib.h> > #include <linux/string.h> > #include <sys/ioctl.h> > +#include <signal.h> > +#include <fcntl.h> > +#include <sys/types.h> > #include <sys/mman.h> > #include <asm/bug.h> > +#include "internal.h" > > void perf_evsel__init(struct perf_evsel *evsel, struct perf_event_attr *attr, > int idx) > @@ -26,6 +30,7 @@ void perf_evsel__init(struct perf_evsel *evsel, struct perf_event_attr *attr, > evsel->attr = *attr; > evsel->idx = idx; > evsel->leader = evsel; > + evsel->open_flags = 0;
In general, you don't need to reset it to zero as it's allocated with zalloc().
> } > [SNIP] > diff --git a/tools/lib/perf/internal.h b/tools/lib/perf/internal.h > index 2c27e158de6b..4b91a087ed62 100644 > --- a/tools/lib/perf/internal.h > +++ b/tools/lib/perf/internal.h > @@ -20,4 +20,48 @@ do { \ > #define pr_debug2(fmt, ...) __pr(LIBPERF_DEBUG2, fmt, ##__VA_ARGS__) > #define pr_debug3(fmt, ...) __pr(LIBPERF_DEBUG3, fmt, ##__VA_ARGS__) > > +static inline bool libperf_is_mem_zeroed(const char *p, ssize_t len) > +{ > + while (len > 0) { > + if (*p) > + return false; > + p++; > + len--; > + } > + return true; > +} > + > +static inline bool libperf_validate_opts(const char *opts, > + size_t opts_sz, size_t user_sz, > + const char *type_name) > +{ > + if (user_sz < sizeof(size_t)) { > + pr_warning("%s size (%zu) is too small\n", type_name, user_sz); > + return false; > + } > + if (!libperf_is_mem_zeroed(opts + opts_sz, (ssize_t)user_sz - opts_sz)) {
I don't think the cast is necessary since it'll be promoted again to size_t due to opts_sz. Instead, we can check if user_sz is greater than opts_sz explicitly and call the function.
> + pr_warning("%s has non-zero extra bytes\n", type_name); > + return false; > + } > + return true; > +} > + > +#define offsetofend(TYPE, FIELD) \ > + (offsetof(TYPE, FIELD) + sizeof(((TYPE *)0)->FIELD)) > + > +#define OPTS_VALID(opts, type) \ > + (!(opts) || libperf_validate_opts((const char *)opts, \ > + offsetofend(struct type, \ > + type##__last_field), \ > + (opts)->sz, #type)) > +#define OPTS_HAS(opts, field) \ > + ((opts) && opts->sz >= offsetofend(typeof(*(opts)), field)) > +#define OPTS_GET(opts, field, fallback_value) \ > + (OPTS_HAS(opts, field) ? (opts)->field : fallback_value)
It'd be nice if you add a blank line between the macros.
Thanks, Namhyung
> +#define OPTS_SET(opts, field, value) \ > + do { \ > + if (OPTS_HAS(opts, field)) \ > + (opts)->field = value; \ > + } while (0) > + > #endif /* __LIBPERF_INTERNAL_H */ > diff --git a/tools/lib/perf/libperf.map b/tools/lib/perf/libperf.map > index 190b56ae923a..eeeb3075e092 100644 > --- a/tools/lib/perf/libperf.map > +++ b/tools/lib/perf/libperf.map > @@ -25,6 +25,7 @@ LIBPERF_0.0.1 { > perf_evsel__enable; > perf_evsel__disable; > perf_evsel__open; > + perf_evsel__open_opts; > perf_evsel__close; > perf_evsel__mmap; > perf_evsel__munmap; > @@ -36,6 +37,7 @@ LIBPERF_0.0.1 { > perf_evlist__new; > perf_evlist__delete; > perf_evlist__open; > + perf_evlist__open_opts; > perf_evlist__close; > perf_evlist__enable; > perf_evlist__disable; > -- > 2.25.1 >
| |