| Message ID | 20240516041948.3546553-2-irogers@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Use BPF filters for a "perf top -u" workaround | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, May 15, 2024 at 9:20 PM Ian Rogers <irogers@google.com> wrote: > > Give the term types their own enum so that additional terms can be > added that don't correspond to a PERF_SAMPLE_xx flag. The term values > are numerically ascending rather than bit field positions, this means > they need translating to a PERF_SAMPLE_xx bit field in certain places > and they are more densely encoded. > > Signed-off-by: Ian Rogers <irogers@google.com> > --- [SNIP] > diff --git a/tools/perf/util/bpf_skel/sample-filter.h b/tools/perf/util/bpf_skel/sample-filter.h > index 2e96e1ab084a..161d5ff49cb6 100644 > --- a/tools/perf/util/bpf_skel/sample-filter.h > +++ b/tools/perf/util/bpf_skel/sample-filter.h > @@ -16,12 +16,32 @@ enum perf_bpf_filter_op { > PBF_OP_GROUP_END, > }; > > +enum perf_bpf_filter_term { > + /* No term is in use. */ > + PBF_TERM_NONE, > + /* Terms that correspond to PERF_SAMPLE_xx values. */ > + PBF_TERM_IP, > + PBF_TERM_ID, > + PBF_TERM_TID, > + PBF_TERM_CPU, > + PBF_TERM_TIME, > + PBF_TERM_ADDR, > + PBF_TERM_PERIOD, > + PBF_TERM_TRANSACTION, > + PBF_TERM_WEIGHT, > + PBF_TERM_PHYS_ADDR, > + PBF_TERM_CODE_PAGE_SIZE, > + PBF_TERM_DATA_PAGE_SIZE, > + PBF_TERM_WEIGHT_STRUCT, > + PBF_TERM_DATA_SRC, > +}; > + > /* BPF map entry for filtering */ > struct perf_bpf_filter_entry { > enum perf_bpf_filter_op op; > __u32 part; /* sub-sample type info when it has multiple values */ > - __u64 flags; /* perf sample type flags */ > + enum perf_bpf_filter_term term; > __u64 value; > }; > > -#endif /* PERF_UTIL_BPF_SKEL_SAMPLE_FILTER_H */ > \ No newline at end of file > +#endif /* PERF_UTIL_BPF_SKEL_SAMPLE_FILTER_H */ > diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/perf/util/bpf_skel/sample_filter.bpf.c > index fb94f5280626..8666c85e9333 100644 > --- a/tools/perf/util/bpf_skel/sample_filter.bpf.c > +++ b/tools/perf/util/bpf_skel/sample_filter.bpf.c > @@ -48,31 +48,50 @@ static inline __u64 perf_get_sample(struct bpf_perf_event_data_kern *kctx, > { > struct perf_sample_data___new *data = (void *)kctx->data; > > - if (!bpf_core_field_exists(data->sample_flags) || > - (data->sample_flags & entry->flags) == 0) > + if (!bpf_core_field_exists(data->sample_flags)) > return 0; > > - switch (entry->flags) { > - case PERF_SAMPLE_IP: > + switch (entry->term) { > + case PBF_TERM_NONE: > + return 0; > + case PBF_TERM_IP: > + if ((data->sample_flags & PERF_SAMPLE_IP) == 0) > + return 0; Can we check this in a single place like in the original code instead of doing it in every case? I think we can keep the entry->flags and check it only if it's non zero. Then uid and gid will have 0 to skip. Thanks, Namhyung > return kctx->data->ip; > - case PERF_SAMPLE_ID: > + case PBF_TERM_ID: > + if ((data->sample_flags & PERF_SAMPLE_ID) == 0) > + return 0; > return kctx->data->id; > - case PERF_SAMPLE_TID: > + case PBF_TERM_TID: > + if ((data->sample_flags & PERF_SAMPLE_TID) == 0) > + return 0; > if (entry->part) > return kctx->data->tid_entry.pid; > else > return kctx->data->tid_entry.tid; > - case PERF_SAMPLE_CPU: > + case PBF_TERM_CPU: > + if ((data->sample_flags & PERF_SAMPLE_CPU) == 0) > + return 0; > return kctx->data->cpu_entry.cpu; > - case PERF_SAMPLE_TIME: > + case PBF_TERM_TIME: > + if ((data->sample_flags & PERF_SAMPLE_TIME) == 0) > + return 0; > return kctx->data->time; > - case PERF_SAMPLE_ADDR: > + case PBF_TERM_ADDR: > + if ((data->sample_flags & PERF_SAMPLE_ADDR) == 0) > + return 0; > return kctx->data->addr; > - case PERF_SAMPLE_PERIOD: > + case PBF_TERM_PERIOD: > + if ((data->sample_flags & PERF_SAMPLE_PERIOD) == 0) > + return 0; > return kctx->data->period; > - case PERF_SAMPLE_TRANSACTION: > + case PBF_TERM_TRANSACTION: > + if ((data->sample_flags & PERF_SAMPLE_TRANSACTION) == 0) > + return 0; > return kctx->data->txn; > - case PERF_SAMPLE_WEIGHT_STRUCT: > + case PBF_TERM_WEIGHT_STRUCT: > + if ((data->sample_flags & PERF_SAMPLE_WEIGHT_STRUCT) == 0) > + return 0; > if (entry->part == 1) > return kctx->data->weight.var1_dw; > if (entry->part == 2) > @@ -80,15 +99,25 @@ static inline __u64 perf_get_sample(struct bpf_perf_event_data_kern *kctx, > if (entry->part == 3) > return kctx->data->weight.var3_w; > /* fall through */ > - case PERF_SAMPLE_WEIGHT: > + case PBF_TERM_WEIGHT: > + if ((data->sample_flags & PERF_SAMPLE_WEIGHT) == 0) > + return 0; > return kctx->data->weight.full; > - case PERF_SAMPLE_PHYS_ADDR: > + case PBF_TERM_PHYS_ADDR: > + if ((data->sample_flags & PERF_SAMPLE_PHYS_ADDR) == 0) > + return 0; > return kctx->data->phys_addr; > - case PERF_SAMPLE_CODE_PAGE_SIZE: > + case PBF_TERM_CODE_PAGE_SIZE: > + if ((data->sample_flags & PERF_SAMPLE_CODE_PAGE_SIZE) == 0) > + return 0; > return kctx->data->code_page_size; > - case PERF_SAMPLE_DATA_PAGE_SIZE: > + case PBF_TERM_DATA_PAGE_SIZE: > + if ((data->sample_flags & PERF_SAMPLE_DATA_PAGE_SIZE) == 0) > + return 0; > return kctx->data->data_page_size; > - case PERF_SAMPLE_DATA_SRC: > + case PBF_TERM_DATA_SRC: > + if ((data->sample_flags & PERF_SAMPLE_DATA_SRC) == 0) > + return 0; > if (entry->part == 1) > return kctx->data->data_src.mem_op; > if (entry->part == 2) > -- > 2.45.0.rc1.225.g2a3ae87e7f-goog >
On Fri, May 17, 2024 at 6:36 PM Namhyung Kim <namhyung@kernel.org> wrote: > > On Wed, May 15, 2024 at 9:20 PM Ian Rogers <irogers@google.com> wrote: > > > > Give the term types their own enum so that additional terms can be > > added that don't correspond to a PERF_SAMPLE_xx flag. The term values > > are numerically ascending rather than bit field positions, this means > > they need translating to a PERF_SAMPLE_xx bit field in certain places > > and they are more densely encoded. > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > --- > [SNIP] > > diff --git a/tools/perf/util/bpf_skel/sample-filter.h b/tools/perf/util/bpf_skel/sample-filter.h > > index 2e96e1ab084a..161d5ff49cb6 100644 > > --- a/tools/perf/util/bpf_skel/sample-filter.h > > +++ b/tools/perf/util/bpf_skel/sample-filter.h > > @@ -16,12 +16,32 @@ enum perf_bpf_filter_op { > > PBF_OP_GROUP_END, > > }; > > > > +enum perf_bpf_filter_term { > > + /* No term is in use. */ > > + PBF_TERM_NONE, > > + /* Terms that correspond to PERF_SAMPLE_xx values. */ > > + PBF_TERM_IP, > > + PBF_TERM_ID, > > + PBF_TERM_TID, > > + PBF_TERM_CPU, > > + PBF_TERM_TIME, > > + PBF_TERM_ADDR, > > + PBF_TERM_PERIOD, > > + PBF_TERM_TRANSACTION, > > + PBF_TERM_WEIGHT, > > + PBF_TERM_PHYS_ADDR, > > + PBF_TERM_CODE_PAGE_SIZE, > > + PBF_TERM_DATA_PAGE_SIZE, > > + PBF_TERM_WEIGHT_STRUCT, > > + PBF_TERM_DATA_SRC, > > +}; > > + > > /* BPF map entry for filtering */ > > struct perf_bpf_filter_entry { > > enum perf_bpf_filter_op op; > > __u32 part; /* sub-sample type info when it has multiple values */ > > - __u64 flags; /* perf sample type flags */ > > + enum perf_bpf_filter_term term; > > __u64 value; > > }; > > > > -#endif /* PERF_UTIL_BPF_SKEL_SAMPLE_FILTER_H */ > > \ No newline at end of file > > +#endif /* PERF_UTIL_BPF_SKEL_SAMPLE_FILTER_H */ > > diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/perf/util/bpf_skel/sample_filter.bpf.c > > index fb94f5280626..8666c85e9333 100644 > > --- a/tools/perf/util/bpf_skel/sample_filter.bpf.c > > +++ b/tools/perf/util/bpf_skel/sample_filter.bpf.c > > @@ -48,31 +48,50 @@ static inline __u64 perf_get_sample(struct bpf_perf_event_data_kern *kctx, > > { > > struct perf_sample_data___new *data = (void *)kctx->data; > > > > - if (!bpf_core_field_exists(data->sample_flags) || > > - (data->sample_flags & entry->flags) == 0) > > + if (!bpf_core_field_exists(data->sample_flags)) > > return 0; > > > > - switch (entry->flags) { > > - case PERF_SAMPLE_IP: > > + switch (entry->term) { > > + case PBF_TERM_NONE: > > + return 0; > > + case PBF_TERM_IP: > > + if ((data->sample_flags & PERF_SAMPLE_IP) == 0) > > + return 0; > > Can we check this in a single place like in the original code > instead of doing it in every case? I think we can keep the > entry->flags and check it only if it's non zero. Then uid and > gid will have 0 to skip. I found the old way confusing. As the flags are a bitmap it looks like more than one can be set, if that were the case then the switch statement would be broken as the case wouldn't exist. Using an enum like this allows warnings to occur when a term is missing in the switch statement - which is good when you are adding new terms. I think it more obviously matches the man page. We could arrange for the enum values to encode the shift position of the flag. Something like: if ((entry->term > PBF_TERM_NONE && entry->term <= PBF_TERM_DATA_SRC) && (data->sample_flags & (1 << entry->term)) == 0) return 0; But the problem there is that not every sample type has an enum value, and I'm not sure it makes sense for things like STREAM_ID. We could do some macro magic to reduce the verbosity like: #define SAMPLE_CASE(x) \ case PBF_TERM_##x: \ if ((data->sample_flags & PERF_SAMPLE_x) == 0) \ return 0 But I thought that made the code harder to read given the relatively small number of sample cases. Thanks, Ian > Thanks, > Namhyung > > > > return kctx->data->ip; > > - case PERF_SAMPLE_ID: > > + case PBF_TERM_ID: > > + if ((data->sample_flags & PERF_SAMPLE_ID) == 0) > > + return 0; > > return kctx->data->id; > > - case PERF_SAMPLE_TID: > > + case PBF_TERM_TID: > > + if ((data->sample_flags & PERF_SAMPLE_TID) == 0) > > + return 0; > > if (entry->part) > > return kctx->data->tid_entry.pid; > > else > > return kctx->data->tid_entry.tid; > > - case PERF_SAMPLE_CPU: > > + case PBF_TERM_CPU: > > + if ((data->sample_flags & PERF_SAMPLE_CPU) == 0) > > + return 0; > > return kctx->data->cpu_entry.cpu; > > - case PERF_SAMPLE_TIME: > > + case PBF_TERM_TIME: > > + if ((data->sample_flags & PERF_SAMPLE_TIME) == 0) > > + return 0; > > return kctx->data->time; > > - case PERF_SAMPLE_ADDR: > > + case PBF_TERM_ADDR: > > + if ((data->sample_flags & PERF_SAMPLE_ADDR) == 0) > > + return 0; > > return kctx->data->addr; > > - case PERF_SAMPLE_PERIOD: > > + case PBF_TERM_PERIOD: > > + if ((data->sample_flags & PERF_SAMPLE_PERIOD) == 0) > > + return 0; > > return kctx->data->period; > > - case PERF_SAMPLE_TRANSACTION: > > + case PBF_TERM_TRANSACTION: > > + if ((data->sample_flags & PERF_SAMPLE_TRANSACTION) == 0) > > + return 0; > > return kctx->data->txn; > > - case PERF_SAMPLE_WEIGHT_STRUCT: > > + case PBF_TERM_WEIGHT_STRUCT: > > + if ((data->sample_flags & PERF_SAMPLE_WEIGHT_STRUCT) == 0) > > + return 0; > > if (entry->part == 1) > > return kctx->data->weight.var1_dw; > > if (entry->part == 2) > > @@ -80,15 +99,25 @@ static inline __u64 perf_get_sample(struct bpf_perf_event_data_kern *kctx, > > if (entry->part == 3) > > return kctx->data->weight.var3_w; > > /* fall through */ > > - case PERF_SAMPLE_WEIGHT: > > + case PBF_TERM_WEIGHT: > > + if ((data->sample_flags & PERF_SAMPLE_WEIGHT) == 0) > > + return 0; > > return kctx->data->weight.full; > > - case PERF_SAMPLE_PHYS_ADDR: > > + case PBF_TERM_PHYS_ADDR: > > + if ((data->sample_flags & PERF_SAMPLE_PHYS_ADDR) == 0) > > + return 0; > > return kctx->data->phys_addr; > > - case PERF_SAMPLE_CODE_PAGE_SIZE: > > + case PBF_TERM_CODE_PAGE_SIZE: > > + if ((data->sample_flags & PERF_SAMPLE_CODE_PAGE_SIZE) == 0) > > + return 0; > > return kctx->data->code_page_size; > > - case PERF_SAMPLE_DATA_PAGE_SIZE: > > + case PBF_TERM_DATA_PAGE_SIZE: > > + if ((data->sample_flags & PERF_SAMPLE_DATA_PAGE_SIZE) == 0) > > + return 0; > > return kctx->data->data_page_size; > > - case PERF_SAMPLE_DATA_SRC: > > + case PBF_TERM_DATA_SRC: > > + if ((data->sample_flags & PERF_SAMPLE_DATA_SRC) == 0) > > + return 0; > > if (entry->part == 1) > > return kctx->data->data_src.mem_op; > > if (entry->part == 2) > > -- > > 2.45.0.rc1.225.g2a3ae87e7f-goog > >
On Fri, May 17, 2024 at 8:30 PM Ian Rogers <irogers@google.com> wrote: > > On Fri, May 17, 2024 at 6:36 PM Namhyung Kim <namhyung@kernel.org> wrote: > > > > On Wed, May 15, 2024 at 9:20 PM Ian Rogers <irogers@google.com> wrote: > > > > > > Give the term types their own enum so that additional terms can be > > > added that don't correspond to a PERF_SAMPLE_xx flag. The term values > > > are numerically ascending rather than bit field positions, this means > > > they need translating to a PERF_SAMPLE_xx bit field in certain places > > > and they are more densely encoded. > > > > > > Signed-off-by: Ian Rogers <irogers@google.com> > > > --- > > [SNIP] > > > diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/perf/util/bpf_skel/sample_filter.bpf.c > > > index fb94f5280626..8666c85e9333 100644 > > > --- a/tools/perf/util/bpf_skel/sample_filter.bpf.c > > > +++ b/tools/perf/util/bpf_skel/sample_filter.bpf.c > > > @@ -48,31 +48,50 @@ static inline __u64 perf_get_sample(struct bpf_perf_event_data_kern *kctx, > > > { > > > struct perf_sample_data___new *data = (void *)kctx->data; > > > > > > - if (!bpf_core_field_exists(data->sample_flags) || > > > - (data->sample_flags & entry->flags) == 0) > > > + if (!bpf_core_field_exists(data->sample_flags)) > > > return 0; > > > > > > - switch (entry->flags) { > > > - case PERF_SAMPLE_IP: > > > + switch (entry->term) { > > > + case PBF_TERM_NONE: > > > + return 0; > > > + case PBF_TERM_IP: > > > + if ((data->sample_flags & PERF_SAMPLE_IP) == 0) > > > + return 0; > > > > Can we check this in a single place like in the original code > > instead of doing it in every case? I think we can keep the > > entry->flags and check it only if it's non zero. Then uid and > > gid will have 0 to skip. > > I found the old way confusing. As the flags are a bitmap it looks like > more than one can be set, if that were the case then the switch > statement would be broken as the case wouldn't exist. Using an enum The entry->flags is set by the bpf-filter code and it's guaranteed to have a single bit. > like this allows warnings to occur when a term is missing in the > switch statement - which is good when you are adding new terms. I > think it more obviously matches the man page. We could arrange for the > enum values to encode the shift position of the flag. Something like: > > if ((entry->term > PBF_TERM_NONE && entry->term <= PBF_TERM_DATA_SRC) && > (data->sample_flags & (1 << entry->term)) == 0) > return 0; Actually I'm ok with enum (and bit shift if needed). I'm just thinking if it'd be nicer if we can have a check in a single place. > > But the problem there is that not every sample type has an enum value, > and I'm not sure it makes sense for things like STREAM_ID. We could do > some macro magic to reduce the verbosity like: > > #define SAMPLE_CASE(x) \ > case PBF_TERM_##x: \ > if ((data->sample_flags & PERF_SAMPLE_x) == 0) \ > return 0 > > But I thought that made the code harder to read given the relatively > small number of sample cases. I also want to avoid the macro if possible. Thanks, Namhyung > > > > > return kctx->data->ip; > > > - case PERF_SAMPLE_ID: > > > + case PBF_TERM_ID: > > > + if ((data->sample_flags & PERF_SAMPLE_ID) == 0) > > > + return 0; > > > return kctx->data->id; > > > - case PERF_SAMPLE_TID: > > > + case PBF_TERM_TID: > > > + if ((data->sample_flags & PERF_SAMPLE_TID) == 0) > > > + return 0; > > > if (entry->part) > > > return kctx->data->tid_entry.pid; > > > else > > > return kctx->data->tid_entry.tid; > > > - case PERF_SAMPLE_CPU: > > > + case PBF_TERM_CPU: > > > + if ((data->sample_flags & PERF_SAMPLE_CPU) == 0) > > > + return 0; > > > return kctx->data->cpu_entry.cpu; > > > - case PERF_SAMPLE_TIME: > > > + case PBF_TERM_TIME: > > > + if ((data->sample_flags & PERF_SAMPLE_TIME) == 0) > > > + return 0; > > > return kctx->data->time; > > > - case PERF_SAMPLE_ADDR: > > > + case PBF_TERM_ADDR: > > > + if ((data->sample_flags & PERF_SAMPLE_ADDR) == 0) > > > + return 0; > > > return kctx->data->addr; > > > - case PERF_SAMPLE_PERIOD: > > > + case PBF_TERM_PERIOD: > > > + if ((data->sample_flags & PERF_SAMPLE_PERIOD) == 0) > > > + return 0; > > > return kctx->data->period; > > > - case PERF_SAMPLE_TRANSACTION: > > > + case PBF_TERM_TRANSACTION: > > > + if ((data->sample_flags & PERF_SAMPLE_TRANSACTION) == 0) > > > + return 0; > > > return kctx->data->txn; > > > - case PERF_SAMPLE_WEIGHT_STRUCT: > > > + case PBF_TERM_WEIGHT_STRUCT: > > > + if ((data->sample_flags & PERF_SAMPLE_WEIGHT_STRUCT) == 0) > > > + return 0; > > > if (entry->part == 1) > > > return kctx->data->weight.var1_dw; > > > if (entry->part == 2) > > > @@ -80,15 +99,25 @@ static inline __u64 perf_get_sample(struct bpf_perf_event_data_kern *kctx, > > > if (entry->part == 3) > > > return kctx->data->weight.var3_w; > > > /* fall through */ > > > - case PERF_SAMPLE_WEIGHT: > > > + case PBF_TERM_WEIGHT: > > > + if ((data->sample_flags & PERF_SAMPLE_WEIGHT) == 0) > > > + return 0; > > > return kctx->data->weight.full; > > > - case PERF_SAMPLE_PHYS_ADDR: > > > + case PBF_TERM_PHYS_ADDR: > > > + if ((data->sample_flags & PERF_SAMPLE_PHYS_ADDR) == 0) > > > + return 0; > > > return kctx->data->phys_addr; > > > - case PERF_SAMPLE_CODE_PAGE_SIZE: > > > + case PBF_TERM_CODE_PAGE_SIZE: > > > + if ((data->sample_flags & PERF_SAMPLE_CODE_PAGE_SIZE) == 0) > > > + return 0; > > > return kctx->data->code_page_size; > > > - case PERF_SAMPLE_DATA_PAGE_SIZE: > > > + case PBF_TERM_DATA_PAGE_SIZE: > > > + if ((data->sample_flags & PERF_SAMPLE_DATA_PAGE_SIZE) == 0) > > > + return 0; > > > return kctx->data->data_page_size; > > > - case PERF_SAMPLE_DATA_SRC: > > > + case PBF_TERM_DATA_SRC: > > > + if ((data->sample_flags & PERF_SAMPLE_DATA_SRC) == 0) > > > + return 0; > > > if (entry->part == 1) > > > return kctx->data->data_src.mem_op; > > > if (entry->part == 2) > > > -- > > > 2.45.0.rc1.225.g2a3ae87e7f-goog > > >
diff --git a/tools/perf/util/bpf-filter.c b/tools/perf/util/bpf-filter.c index b51544996046..7e8d179f03dc 100644 --- a/tools/perf/util/bpf-filter.c +++ b/tools/perf/util/bpf-filter.c @@ -17,11 +17,11 @@ #define FD(e, x, y) (*(int *)xyarray__entry(e->core.fd, x, y)) -#define __PERF_SAMPLE_TYPE(st, opt) { st, #st, opt } -#define PERF_SAMPLE_TYPE(_st, opt) __PERF_SAMPLE_TYPE(PERF_SAMPLE_##_st, opt) +#define __PERF_SAMPLE_TYPE(tt, st, opt) { tt, #st, opt } +#define PERF_SAMPLE_TYPE(_st, opt) __PERF_SAMPLE_TYPE(PBF_TERM_##_st, PERF_SAMPLE_##_st, opt) static const struct perf_sample_info { - u64 type; + enum perf_bpf_filter_term type; const char *name; const char *option; } sample_table[] = { @@ -44,12 +44,12 @@ static const struct perf_sample_info { PERF_SAMPLE_TYPE(DATA_PAGE_SIZE, "--data-page-size"), }; -static const struct perf_sample_info *get_sample_info(u64 flags) +static const struct perf_sample_info *get_sample_info(enum perf_bpf_filter_term type) { size_t i; for (i = 0; i < ARRAY_SIZE(sample_table); i++) { - if (sample_table[i].type == flags) + if (sample_table[i].type == type) return &sample_table[i]; } return NULL; @@ -59,8 +59,32 @@ static int check_sample_flags(struct evsel *evsel, struct perf_bpf_filter_expr * { const struct perf_sample_info *info; - if (evsel->core.attr.sample_type & expr->sample_flags) - return 0; +#define CHECK_TERM(x) \ + case PBF_TERM_##x: \ + if (evsel->core.attr.sample_type & PERF_SAMPLE_##x) \ + return 0; \ + break + + switch (expr->term) { + CHECK_TERM(IP); + CHECK_TERM(ID); + CHECK_TERM(TID); + CHECK_TERM(CPU); + CHECK_TERM(TIME); + CHECK_TERM(ADDR); + CHECK_TERM(PERIOD); + CHECK_TERM(TRANSACTION); + CHECK_TERM(WEIGHT); + CHECK_TERM(PHYS_ADDR); + CHECK_TERM(CODE_PAGE_SIZE); + CHECK_TERM(DATA_PAGE_SIZE); + CHECK_TERM(WEIGHT_STRUCT); + CHECK_TERM(DATA_SRC); + case PBF_TERM_NONE: + default: + break; + } +#undef CHECK_TERM if (expr->op == PBF_OP_GROUP_BEGIN) { struct perf_bpf_filter_expr *group; @@ -72,10 +96,10 @@ static int check_sample_flags(struct evsel *evsel, struct perf_bpf_filter_expr * return 0; } - info = get_sample_info(expr->sample_flags); + info = get_sample_info(expr->term); if (info == NULL) { - pr_err("Error: %s event does not have sample flags %lx\n", - evsel__name(evsel), expr->sample_flags); + pr_err("Error: %s event does not have sample flags %d\n", + evsel__name(evsel), expr->term); return -1; } @@ -105,7 +129,7 @@ int perf_bpf_filter__prepare(struct evsel *evsel) struct perf_bpf_filter_entry entry = { .op = expr->op, .part = expr->part, - .flags = expr->sample_flags, + .term = expr->term, .value = expr->val, }; @@ -122,7 +146,7 @@ int perf_bpf_filter__prepare(struct evsel *evsel) struct perf_bpf_filter_entry group_entry = { .op = group->op, .part = group->part, - .flags = group->sample_flags, + .term = group->term, .value = group->val, }; bpf_map_update_elem(fd, &i, &group_entry, BPF_ANY); @@ -173,7 +197,8 @@ u64 perf_bpf_filter__lost_count(struct evsel *evsel) return skel ? skel->bss->dropped : 0; } -struct perf_bpf_filter_expr *perf_bpf_filter_expr__new(unsigned long sample_flags, int part, +struct perf_bpf_filter_expr *perf_bpf_filter_expr__new(enum perf_bpf_filter_term term, + int part, enum perf_bpf_filter_op op, unsigned long val) { @@ -181,7 +206,7 @@ struct perf_bpf_filter_expr *perf_bpf_filter_expr__new(unsigned long sample_flag expr = malloc(sizeof(*expr)); if (expr != NULL) { - expr->sample_flags = sample_flags; + expr->term = term, expr->part = part; expr->op = op; expr->val = val; diff --git a/tools/perf/util/bpf-filter.h b/tools/perf/util/bpf-filter.h index 7afd159411b8..cd6764442c16 100644 --- a/tools/perf/util/bpf-filter.h +++ b/tools/perf/util/bpf-filter.h @@ -11,14 +11,15 @@ struct perf_bpf_filter_expr { struct list_head groups; enum perf_bpf_filter_op op; int part; - unsigned long sample_flags; + enum perf_bpf_filter_term term; unsigned long val; }; struct evsel; #ifdef HAVE_BPF_SKEL -struct perf_bpf_filter_expr *perf_bpf_filter_expr__new(unsigned long sample_flags, int part, +struct perf_bpf_filter_expr *perf_bpf_filter_expr__new(enum perf_bpf_filter_term term, + int part, enum perf_bpf_filter_op op, unsigned long val); int perf_bpf_filter__parse(struct list_head *expr_head, const char *str); diff --git a/tools/perf/util/bpf-filter.l b/tools/perf/util/bpf-filter.l index d4ff0f1345cd..62c959813466 100644 --- a/tools/perf/util/bpf-filter.l +++ b/tools/perf/util/bpf-filter.l @@ -9,16 +9,16 @@ #include "bpf-filter.h" #include "bpf-filter-bison.h" -static int sample(unsigned long sample_flag) +static int sample(enum perf_bpf_filter_term term) { - perf_bpf_filter_lval.sample.type = sample_flag; + perf_bpf_filter_lval.sample.term = term; perf_bpf_filter_lval.sample.part = 0; return BFT_SAMPLE; } -static int sample_part(unsigned long sample_flag, int part) +static int sample_part(enum perf_bpf_filter_term term, int part) { - perf_bpf_filter_lval.sample.type = sample_flag; + perf_bpf_filter_lval.sample.term = term; perf_bpf_filter_lval.sample.part = part; return BFT_SAMPLE; } @@ -67,34 +67,34 @@ ident [_a-zA-Z][_a-zA-Z0-9]+ {num_hex} { return value(16); } {space} { } -ip { return sample(PERF_SAMPLE_IP); } -id { return sample(PERF_SAMPLE_ID); } -tid { return sample(PERF_SAMPLE_TID); } -pid { return sample_part(PERF_SAMPLE_TID, 1); } -cpu { return sample(PERF_SAMPLE_CPU); } -time { return sample(PERF_SAMPLE_TIME); } -addr { return sample(PERF_SAMPLE_ADDR); } -period { return sample(PERF_SAMPLE_PERIOD); } -txn { return sample(PERF_SAMPLE_TRANSACTION); } -weight { return sample(PERF_SAMPLE_WEIGHT); } -weight1 { return sample_part(PERF_SAMPLE_WEIGHT_STRUCT, 1); } -weight2 { return sample_part(PERF_SAMPLE_WEIGHT_STRUCT, 2); } -weight3 { return sample_part(PERF_SAMPLE_WEIGHT_STRUCT, 3); } -ins_lat { return sample_part(PERF_SAMPLE_WEIGHT_STRUCT, 2); } /* alias for weight2 */ -p_stage_cyc { return sample_part(PERF_SAMPLE_WEIGHT_STRUCT, 3); } /* alias for weight3 */ -retire_lat { return sample_part(PERF_SAMPLE_WEIGHT_STRUCT, 3); } /* alias for weight3 */ -phys_addr { return sample(PERF_SAMPLE_PHYS_ADDR); } -code_pgsz { return sample(PERF_SAMPLE_CODE_PAGE_SIZE); } -data_pgsz { return sample(PERF_SAMPLE_DATA_PAGE_SIZE); } -mem_op { return sample_part(PERF_SAMPLE_DATA_SRC, 1); } -mem_lvlnum { return sample_part(PERF_SAMPLE_DATA_SRC, 2); } -mem_lvl { return sample_part(PERF_SAMPLE_DATA_SRC, 2); } /* alias for mem_lvlnum */ -mem_snoop { return sample_part(PERF_SAMPLE_DATA_SRC, 3); } /* include snoopx */ -mem_remote { return sample_part(PERF_SAMPLE_DATA_SRC, 4); } -mem_lock { return sample_part(PERF_SAMPLE_DATA_SRC, 5); } -mem_dtlb { return sample_part(PERF_SAMPLE_DATA_SRC, 6); } -mem_blk { return sample_part(PERF_SAMPLE_DATA_SRC, 7); } -mem_hops { return sample_part(PERF_SAMPLE_DATA_SRC, 8); } +ip { return sample(PBF_TERM_IP); } +id { return sample(PBF_TERM_ID); } +tid { return sample(PBF_TERM_TID); } +pid { return sample_part(PBF_TERM_TID, 1); } +cpu { return sample(PBF_TERM_CPU); } +time { return sample(PBF_TERM_TIME); } +addr { return sample(PBF_TERM_ADDR); } +period { return sample(PBF_TERM_PERIOD); } +txn { return sample(PBF_TERM_TRANSACTION); } +weight { return sample(PBF_TERM_WEIGHT); } +weight1 { return sample_part(PBF_TERM_WEIGHT_STRUCT, 1); } +weight2 { return sample_part(PBF_TERM_WEIGHT_STRUCT, 2); } +weight3 { return sample_part(PBF_TERM_WEIGHT_STRUCT, 3); } +ins_lat { return sample_part(PBF_TERM_WEIGHT_STRUCT, 2); } /* alias for weight2 */ +p_stage_cyc { return sample_part(PBF_TERM_WEIGHT_STRUCT, 3); } /* alias for weight3 */ +retire_lat { return sample_part(PBF_TERM_WEIGHT_STRUCT, 3); } /* alias for weight3 */ +phys_addr { return sample(PBF_TERM_PHYS_ADDR); } +code_pgsz { return sample(PBF_TERM_CODE_PAGE_SIZE); } +data_pgsz { return sample(PBF_TERM_DATA_PAGE_SIZE); } +mem_op { return sample_part(PBF_TERM_DATA_SRC, 1); } +mem_lvlnum { return sample_part(PBF_TERM_DATA_SRC, 2); } +mem_lvl { return sample_part(PBF_TERM_DATA_SRC, 2); } /* alias for mem_lvlnum */ +mem_snoop { return sample_part(PBF_TERM_DATA_SRC, 3); } /* include snoopx */ +mem_remote { return sample_part(PBF_TERM_DATA_SRC, 4); } +mem_lock { return sample_part(PBF_TERM_DATA_SRC, 5); } +mem_dtlb { return sample_part(PBF_TERM_DATA_SRC, 6); } +mem_blk { return sample_part(PBF_TERM_DATA_SRC, 7); } +mem_hops { return sample_part(PBF_TERM_DATA_SRC, 8); } "==" { return operator(PBF_OP_EQ); } "!=" { return operator(PBF_OP_NEQ); } diff --git a/tools/perf/util/bpf-filter.y b/tools/perf/util/bpf-filter.y index 0e4d6de3c2ad..0c56fccb8874 100644 --- a/tools/perf/util/bpf-filter.y +++ b/tools/perf/util/bpf-filter.y @@ -27,7 +27,7 @@ static void perf_bpf_filter_error(struct list_head *expr __maybe_unused, { unsigned long num; struct { - unsigned long type; + enum perf_bpf_filter_term term; int part; } sample; enum perf_bpf_filter_op op; @@ -62,7 +62,8 @@ filter_term BFT_LOGICAL_OR filter_expr if ($1->op == PBF_OP_GROUP_BEGIN) { expr = $1; } else { - expr = perf_bpf_filter_expr__new(0, 0, PBF_OP_GROUP_BEGIN, 1); + expr = perf_bpf_filter_expr__new(PBF_TERM_NONE, /*part=*/0, + PBF_OP_GROUP_BEGIN, /*val=*/1); list_add_tail(&$1->list, &expr->groups); } expr->val++; @@ -78,7 +79,7 @@ filter_expr filter_expr: BFT_SAMPLE BFT_OP BFT_NUM { - $$ = perf_bpf_filter_expr__new($1.type, $1.part, $2, $3); + $$ = perf_bpf_filter_expr__new($1.term, $1.part, $2, $3); } %% diff --git a/tools/perf/util/bpf_skel/sample-filter.h b/tools/perf/util/bpf_skel/sample-filter.h index 2e96e1ab084a..161d5ff49cb6 100644 --- a/tools/perf/util/bpf_skel/sample-filter.h +++ b/tools/perf/util/bpf_skel/sample-filter.h @@ -16,12 +16,32 @@ enum perf_bpf_filter_op { PBF_OP_GROUP_END, }; +enum perf_bpf_filter_term { + /* No term is in use. */ + PBF_TERM_NONE, + /* Terms that correspond to PERF_SAMPLE_xx values. */ + PBF_TERM_IP, + PBF_TERM_ID, + PBF_TERM_TID, + PBF_TERM_CPU, + PBF_TERM_TIME, + PBF_TERM_ADDR, + PBF_TERM_PERIOD, + PBF_TERM_TRANSACTION, + PBF_TERM_WEIGHT, + PBF_TERM_PHYS_ADDR, + PBF_TERM_CODE_PAGE_SIZE, + PBF_TERM_DATA_PAGE_SIZE, + PBF_TERM_WEIGHT_STRUCT, + PBF_TERM_DATA_SRC, +}; + /* BPF map entry for filtering */ struct perf_bpf_filter_entry { enum perf_bpf_filter_op op; __u32 part; /* sub-sample type info when it has multiple values */ - __u64 flags; /* perf sample type flags */ + enum perf_bpf_filter_term term; __u64 value; }; -#endif /* PERF_UTIL_BPF_SKEL_SAMPLE_FILTER_H */ \ No newline at end of file +#endif /* PERF_UTIL_BPF_SKEL_SAMPLE_FILTER_H */ diff --git a/tools/perf/util/bpf_skel/sample_filter.bpf.c b/tools/perf/util/bpf_skel/sample_filter.bpf.c index fb94f5280626..8666c85e9333 100644 --- a/tools/perf/util/bpf_skel/sample_filter.bpf.c +++ b/tools/perf/util/bpf_skel/sample_filter.bpf.c @@ -48,31 +48,50 @@ static inline __u64 perf_get_sample(struct bpf_perf_event_data_kern *kctx, { struct perf_sample_data___new *data = (void *)kctx->data; - if (!bpf_core_field_exists(data->sample_flags) || - (data->sample_flags & entry->flags) == 0) + if (!bpf_core_field_exists(data->sample_flags)) return 0; - switch (entry->flags) { - case PERF_SAMPLE_IP: + switch (entry->term) { + case PBF_TERM_NONE: + return 0; + case PBF_TERM_IP: + if ((data->sample_flags & PERF_SAMPLE_IP) == 0) + return 0; return kctx->data->ip; - case PERF_SAMPLE_ID: + case PBF_TERM_ID: + if ((data->sample_flags & PERF_SAMPLE_ID) == 0) + return 0; return kctx->data->id; - case PERF_SAMPLE_TID: + case PBF_TERM_TID: + if ((data->sample_flags & PERF_SAMPLE_TID) == 0) + return 0; if (entry->part) return kctx->data->tid_entry.pid; else return kctx->data->tid_entry.tid; - case PERF_SAMPLE_CPU: + case PBF_TERM_CPU: + if ((data->sample_flags & PERF_SAMPLE_CPU) == 0) + return 0; return kctx->data->cpu_entry.cpu; - case PERF_SAMPLE_TIME: + case PBF_TERM_TIME: + if ((data->sample_flags & PERF_SAMPLE_TIME) == 0) + return 0; return kctx->data->time; - case PERF_SAMPLE_ADDR: + case PBF_TERM_ADDR: + if ((data->sample_flags & PERF_SAMPLE_ADDR) == 0) + return 0; return kctx->data->addr; - case PERF_SAMPLE_PERIOD: + case PBF_TERM_PERIOD: + if ((data->sample_flags & PERF_SAMPLE_PERIOD) == 0) + return 0; return kctx->data->period; - case PERF_SAMPLE_TRANSACTION: + case PBF_TERM_TRANSACTION: + if ((data->sample_flags & PERF_SAMPLE_TRANSACTION) == 0) + return 0; return kctx->data->txn; - case PERF_SAMPLE_WEIGHT_STRUCT: + case PBF_TERM_WEIGHT_STRUCT: + if ((data->sample_flags & PERF_SAMPLE_WEIGHT_STRUCT) == 0) + return 0; if (entry->part == 1) return kctx->data->weight.var1_dw; if (entry->part == 2) @@ -80,15 +99,25 @@ static inline __u64 perf_get_sample(struct bpf_perf_event_data_kern *kctx, if (entry->part == 3) return kctx->data->weight.var3_w; /* fall through */ - case PERF_SAMPLE_WEIGHT: + case PBF_TERM_WEIGHT: + if ((data->sample_flags & PERF_SAMPLE_WEIGHT) == 0) + return 0; return kctx->data->weight.full; - case PERF_SAMPLE_PHYS_ADDR: + case PBF_TERM_PHYS_ADDR: + if ((data->sample_flags & PERF_SAMPLE_PHYS_ADDR) == 0) + return 0; return kctx->data->phys_addr; - case PERF_SAMPLE_CODE_PAGE_SIZE: + case PBF_TERM_CODE_PAGE_SIZE: + if ((data->sample_flags & PERF_SAMPLE_CODE_PAGE_SIZE) == 0) + return 0; return kctx->data->code_page_size; - case PERF_SAMPLE_DATA_PAGE_SIZE: + case PBF_TERM_DATA_PAGE_SIZE: + if ((data->sample_flags & PERF_SAMPLE_DATA_PAGE_SIZE) == 0) + return 0; return kctx->data->data_page_size; - case PERF_SAMPLE_DATA_SRC: + case PBF_TERM_DATA_SRC: + if ((data->sample_flags & PERF_SAMPLE_DATA_SRC) == 0) + return 0; if (entry->part == 1) return kctx->data->data_src.mem_op; if (entry->part == 2)
Give the term types their own enum so that additional terms can be added that don't correspond to a PERF_SAMPLE_xx flag. The term values are numerically ascending rather than bit field positions, this means they need translating to a PERF_SAMPLE_xx bit field in certain places and they are more densely encoded. Signed-off-by: Ian Rogers <irogers@google.com> --- tools/perf/util/bpf-filter.c | 53 +++++++++++----- tools/perf/util/bpf-filter.h | 5 +- tools/perf/util/bpf-filter.l | 64 ++++++++++---------- tools/perf/util/bpf-filter.y | 7 ++- tools/perf/util/bpf_skel/sample-filter.h | 24 +++++++- tools/perf/util/bpf_skel/sample_filter.bpf.c | 63 +++++++++++++------ 6 files changed, 146 insertions(+), 70 deletions(-)