Re: [PATCH 13/59] libperf: Move exclude_GH to perf_evsel

[Jiri Olsa <jolsa@redhat.com>]

On Mon, Nov 08, 2021 at 09:53:44AM -0800, Ian Rogers wrote:
> On Mon, Nov 8, 2021 at 5:38 AM Jiri Olsa <jolsa@redhat.com> wrote:
> >
> > Moving exclude_GH to perf_evsel struct.
> >
> > Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> > ---
> >  tools/lib/perf/include/internal/evsel.h | 1 +
> >  tools/perf/util/evsel.c                 | 2 +-
> >  tools/perf/util/evsel.h                 | 1 -
> >  tools/perf/util/parse-events.c          | 4 ++--
> >  4 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/tools/lib/perf/include/internal/evsel.h b/tools/lib/perf/include/internal/evsel.h
> > index b96c9f4808ca..7f3ee73c5aa5 100644
> > --- a/tools/lib/perf/include/internal/evsel.h
> > +++ b/tools/lib/perf/include/internal/evsel.h
> > @@ -69,6 +69,7 @@ struct perf_evsel {
> >                 const char              *metric_id;
> >                 enum perf_tool_event    tool_event;
> >                 const char              *unit;
> > +               int                     exclude_GH;
> 
> I think it'd be pretty awesome if when these things get moved around
> we added a comment here to explain what the variable means. Perhaps
> even put an example :-) In this case it could explain why this is an
> int and not a bool. This is a while you are here request rather than a
> criticism of the changes which I find fine.

ok, I noticed you added several comments like that already,
I'll put some in here in next version

thanks,
jirka

> 
> Thanks,
> Ian
> 
> >         };
> >  };
> >
> > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> > index 133b571e1190..802c7de9e8e3 100644
> > --- a/tools/perf/util/evsel.c
> > +++ b/tools/perf/util/evsel.c
> > @@ -431,7 +431,7 @@ struct evsel *evsel__clone(struct evsel *orig)
> >         evsel->use_uncore_alias = orig->use_uncore_alias;
> >         evsel->is_libpfm_event = orig->is_libpfm_event;
> >
> > -       evsel->exclude_GH = orig->exclude_GH;
> > +       evsel->core.exclude_GH = orig->core.exclude_GH;
> >         evsel->sample_read = orig->sample_read;
> >         evsel->core.auto_merge_stats = orig->core.auto_merge_stats;
> >         evsel->collect_stat = orig->collect_stat;
> > diff --git a/tools/perf/util/evsel.h b/tools/perf/util/evsel.h
> > index f6cc1e78903f..6c3634ed169a 100644
> > --- a/tools/perf/util/evsel.h
> > +++ b/tools/perf/util/evsel.h
> > @@ -62,7 +62,6 @@ struct evsel {
> >                 double                  scale;
> >                 struct cgroup           *cgrp;
> >                 /* parse modifier helper */
> > -               int                     exclude_GH;
> >                 int                     sample_read;
> >                 bool                    snapshot;
> >                 bool                    per_pkg;
> > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> > index 1878ea1310d3..89787ebc23ef 100644
> > --- a/tools/perf/util/parse-events.c
> > +++ b/tools/perf/util/parse-events.c
> > @@ -1918,7 +1918,7 @@ static int get_event_modifier(struct event_modifier *mod, char *str,
> >         int exclusive = evsel ? evsel->core.attr.exclusive : 0;
> >
> >         int exclude = eu | ek | eh;
> > -       int exclude_GH = evsel ? evsel->exclude_GH : 0;
> > +       int exclude_GH = evsel ? evsel->core.exclude_GH : 0;
> >         int weak = 0;
> >         int bpf_counter = 0;
> >
> > @@ -2049,7 +2049,7 @@ int parse_events__modifier_event(struct list_head *list, char *str, bool add)
> >                 evsel->core.attr.exclude_host   = mod.eH;
> >                 evsel->core.attr.exclude_guest  = mod.eG;
> >                 evsel->core.attr.exclude_idle   = mod.eI;
> > -               evsel->exclude_GH          = mod.exclude_GH;
> > +               evsel->core.exclude_GH          = mod.exclude_GH;
> >                 evsel->sample_read         = mod.sample_read;
> >                 evsel->precise_max         = mod.precise_max;
> >                 evsel->weak_group          = mod.weak;
> > --
> > 2.31.1
> >
>