Re: [PATCH 46/59] perf tools: Add add_numeric callback to struct parse_events_ops

[Jiri Olsa <jolsa@redhat.com>]

On Mon, Nov 08, 2021 at 10:27:10AM -0800, Ian Rogers wrote:
> On Mon, Nov 8, 2021 at 5:41 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > Adding add_numeric callback to struct parse_events_ops,
> > to allow custom numeric parsing code.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/perf/include/internal/parse-events.h |  5 +++++
> >  tools/perf/util/parse-events.c                 |  2 ++
> >  tools/perf/util/parse-events.h                 |  4 ----
> >  tools/perf/util/parse-events.y                 | 12 ++++++++----
> >  4 files changed, 15 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/lib/perf/include/internal/parse-events.h b/tools/lib/perf/include/internal/parse-events.h
> > index 78dd3f989346..cbcf799f8969 100644
> > --- a/tools/lib/perf/include/internal/parse-events.h
> > +++ b/tools/lib/perf/include/internal/parse-events.h
> > @@ -102,6 +102,11 @@ struct parse_events_ops {
> >         int (*add_pmu_multi)(struct parse_events_state *parse_state,
> >                              char *str, struct list_head *head,
> >                              struct list_head **listp);
> > +
> > +       int (*add_numeric)(struct parse_events_state *parse_state,
> > +                          struct list_head *list,
> > +                          u32 type, u64 config,
> > +                          struct list_head *head_config);
> >  };
> >
> >  struct parse_events_state {
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index c67634c9de0d..a05e1bdb4e60 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1306,6 +1306,7 @@ int parse_events_add_tracepoint(struct parse_events_state *parse_state,
> >                                             err, head_config);
> >  }
> >
> > +static
> >  int parse_events_add_numeric(struct parse_events_state *parse_state,
> >                              struct list_head *list,
> >                              u32 type, u64 config,
> > @@ -2904,4 +2905,5 @@ static struct parse_events_ops parse_state_ops = {
> >         .perf_evsel__delete = perf_evsel__delete_helper,
> >         .add_pmu            = parse_events_add_pmu,
> >         .add_pmu_multi      = parse_events_multi_pmu_add,
> > +       .add_numeric        = parse_events_add_numeric,
> 
> Some documentation on the ops would be awesome :-) Especially as this
> is allowing configuration and it is not immediately clear what that
> could mean when changes happen.

so the ops are to help with perf specific code like 'evsel allocation
and one chicken and egg problem we have in event parsing

to move flex/bison code to libperf I need to move all functions called
from that code to libperf.. and some of those functions end up calling
parser again - the pmu_loopup for terms parsing, which is sharing
parser/flexer code with event parsing

having the ops allows us to keep this 'so far unmovable code' in perf
and move just the flexer/bison, which is the base for everything

I expect some of the callbacks will go away at the end, because the
code for them will be completely in libperf, so there would be no
need for special perf implementation

but yes, I'll put some docs on them

thanks,
jirka

> 
> Thanks,
> Ian
> 
> >  };
> > diff --git a/tools/perf/util/parse-events.h b/tools/perf/util/parse-events.h
> > index fa03f8f70f33..40d192cace03 100644
> > --- a/tools/perf/util/parse-events.h
> > +++ b/tools/perf/util/parse-events.h
> > @@ -79,10 +79,6 @@ int parse_events_load_bpf_obj(struct parse_events_state *parse_state,
> >                               struct list_head *list,
> >                               struct bpf_object *obj,
> >                               struct list_head *head_config);
> > -int parse_events_add_numeric(struct parse_events_state *parse_state,
> > -                            struct list_head *list,
> > -                            u32 type, u64 config,
> > -                            struct list_head *head_config);
> >  int parse_events_add_tool(struct parse_events_state *parse_state,
> >                           struct list_head *list,
> >                           int tool_event);
> > diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
> > index f903690b3c8a..b23691397374 100644
> > --- a/tools/perf/util/parse-events.y
> > +++ b/tools/perf/util/parse-events.y
> > @@ -436,6 +436,7 @@ PE_VALUE_SYM_SW
> >  event_legacy_symbol:
> >  value_sym '/' event_config '/'
> >  {
> > +       struct parse_events_state *parse_state = _parse_state;
> >         struct list_head *list;
> >         int type = $1 >> 16;
> >         int config = $1 & 255;
> > @@ -443,7 +444,7 @@ value_sym '/' event_config '/'
> >
> >         list = alloc_list();
> >         ABORT_ON(!list);
> > -       err = parse_events_add_numeric(_parse_state, list, type, config, $3);
> > +       err = parse_state->ops->add_numeric(parse_state, list, type, config, $3);
> >         parse_events_terms__delete($3);
> >         if (err) {
> >                 free_list_evsel(_parse_state, list);
> > @@ -454,13 +455,14 @@ value_sym '/' event_config '/'
> >  |
> >  value_sym sep_slash_slash_dc
> >  {
> > +       struct parse_events_state *parse_state = _parse_state;
> >         struct list_head *list;
> >         int type = $1 >> 16;
> >         int config = $1 & 255;
> >
> >         list = alloc_list();
> >         ABORT_ON(!list);
> > -       ABORT_ON(parse_events_add_numeric(_parse_state, list, type, config, NULL));
> > +       ABORT_ON(parse_state->ops->add_numeric(parse_state, list, type, config, NULL));
> >         $$ = list;
> >  }
> >  |
> > @@ -646,12 +648,13 @@ PE_NAME ':' PE_NAME
> >  event_legacy_numeric:
> >  PE_VALUE ':' PE_VALUE opt_event_config
> >  {
> > +       struct parse_events_state *parse_state = _parse_state;
> >         struct list_head *list;
> >         int err;
> >
> >         list = alloc_list();
> >         ABORT_ON(!list);
> > -       err = parse_events_add_numeric(_parse_state, list, (u32)$1, $3, $4);
> > +       err = parse_state->ops->add_numeric(_parse_state, list, (u32)$1, $3, $4);
> >         parse_events_terms__delete($4);
> >         if (err) {
> >                 free(list);
> > @@ -663,12 +666,13 @@ PE_VALUE ':' PE_VALUE opt_event_config
> >  event_legacy_raw:
> >  PE_RAW opt_event_config
> >  {
> > +       struct parse_events_state *parse_state = _parse_state;
> >         struct list_head *list;
> >         int err;
> >
> >         list = alloc_list();
> >         ABORT_ON(!list);
> > -       err = parse_events_add_numeric(_parse_state, list, PERF_TYPE_RAW, $1, $2);
> > +       err = parse_state->ops->add_numeric(_parse_state, list, PERF_TYPE_RAW, $1, $2);
> >         parse_events_terms__delete($2);
> >         if (err) {
> >                 free(list);
> > --
> > 2.31.1
> >
>