Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230715032915.97146-1-yangjihong1@huawei.com>
 <20230715032915.97146-3-yangjihong1@huawei.com> <CAP-5=fVysKhUn1YsUr0NBU2kVBDgkoczO861XwK5VCtkeYSRJA@mail.gmail.com>
 <085942cc-f6e7-a81b-243f-a739c91ecd72@intel.com>
In-Reply-To: <085942cc-f6e7-a81b-243f-a739c91ecd72@intel.com>
From:   Ian Rogers <irogers@google.com>
Date:   Wed, 19 Jul 2023 10:12:09 -0700
Message-ID: <CAP-5=fVmKmgsABpnsngT9L1QeaWaUxakHc1078LahGFqK4-riA@mail.gmail.com>
Subject: Re: [PATCH v2 2/7] perf evlist: Add evlist__findnew_tracking_event() helper
To:     Adrian Hunter <adrian.hunter@intel.com>
Cc:     Yang Jihong <yangjihong1@huawei.com>, peterz@infradead.org,
        mingo@redhat.com, acme@kernel.org, mark.rutland@arm.com,
        alexander.shishkin@linux.intel.com, jolsa@kernel.org,
        namhyung@kernel.org, kan.liang@linux.intel.com,
        james.clark@arm.com, tmricht@linux.ibm.com, ak@linux.intel.com,
        anshuman.khandual@arm.com, linux-kernel@vger.kernel.org,
        linux-perf-users@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Jul 19, 2023 at 9:59=E2=80=AFAM Adrian Hunter <adrian.hunter@intel.=
com> wrote:
>
> On 19/07/23 19:44, Ian Rogers wrote:
> > On Fri, Jul 14, 2023 at 8:31=E2=80=AFPM Yang Jihong <yangjihong1@huawei=
.com> wrote:
> >>
> >> Currently, intel-bts, intel-pt, and arm-spe may add a dummy event for
> >> tracking to the evlist. We may need to search for the dummy event for
> >> some settings. Therefore, add evlist__findnew_tracking_event() helper.
> >>
> >> evlist__findnew_tracking_event() also deal with system_wide maps if
> >> system_wide is true.
> >
> > I'm wondering if we can simplify the naming in the API, we have "dummy
> > event" which makes sense as we literally call the event "dummy",
> > "sideband" which refers to the kind of samples/events the dummy event
> > will record but "tracking" I think tends to get used as a verb rather
> > than a noun. So I think evlist__findnew_tracking_event should be
> > evlist__findnew_dummy_event.
>
> Except the tracking event has "tracking" set but there can be other dummy
> events that do not.

Thanks! I'm wondering what a dummy event without tracking would be
used for - do you have a reference? I don't see anything in
perf_event.h calling things like mmap2/comm in perf_event_attr
tracking. I'm not a fan of dummy due to it not being intention
revealing. Perhaps if we could go back in time  we could call the
event "sideband", maybe we should migrate to this. We have other
non-obvious uses of the term dummy like in cpumap:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tr=
ee/tools/lib/perf/include/perf/cpumap.h?h=3Dperf-tools-next#n24
If tracking can be on any event then does that make the functions
behavior more ambiguous if it means dummy+tracking not <any
event>+tracking?

Thanks,
Ian

> >
> > Thanks,
> > Ian
> >
> >> Signed-off-by: Yang Jihong <yangjihong1@huawei.com>
> >> ---
> >>  tools/perf/builtin-record.c | 11 +++--------
> >>  tools/perf/util/evlist.c    | 18 ++++++++++++++++++
> >>  tools/perf/util/evlist.h    |  1 +
> >>  3 files changed, 22 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/tools/perf/builtin-record.c b/tools/perf/builtin-record.c
> >> index aec18db7ff23..ca83599cc50c 100644
> >> --- a/tools/perf/builtin-record.c
> >> +++ b/tools/perf/builtin-record.c
> >> @@ -1295,14 +1295,9 @@ static int record__open(struct record *rec)
> >>          */
> >>         if (opts->target.initial_delay || target__has_cpu(&opts->targe=
t) ||
> >>             perf_pmus__num_core_pmus() > 1) {
> >> -               pos =3D evlist__get_tracking_event(evlist);
> >> -               if (!evsel__is_dummy_event(pos)) {
> >> -                       /* Set up dummy event. */
> >> -                       if (evlist__add_dummy(evlist))
> >> -                               return -ENOMEM;
> >> -                       pos =3D evlist__last(evlist);
> >> -                       evlist__set_tracking_event(evlist, pos);
> >> -               }
> >> +               pos =3D evlist__findnew_tracking_event(evlist, false);
> >> +               if (!pos)
> >> +                       return -ENOMEM;
> >>
> >>                 /*
> >>                  * Enable the dummy event when the process is forked f=
or
> >> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> >> index 7ef43f72098e..25c3ebe2c2f5 100644
> >> --- a/tools/perf/util/evlist.c
> >> +++ b/tools/perf/util/evlist.c
> >> @@ -1694,6 +1694,24 @@ void evlist__set_tracking_event(struct evlist *=
evlist, struct evsel *tracking_ev
> >>         tracking_evsel->tracking =3D true;
> >>  }
> >>
> >> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, b=
ool system_wide)
> >> +{
> >> +       struct evsel *evsel;
> >> +
> >> +       evsel =3D evlist__get_tracking_event(evlist);
> >> +       if (!evsel__is_dummy_event(evsel)) {
> >> +               evsel =3D evlist__add_aux_dummy(evlist, system_wide);
> >> +               if (!evsel)
> >> +                       return NULL;
> >> +
> >> +               evlist__set_tracking_event(evlist, evsel);
> >> +       } else if (system_wide) {
> >> +               perf_evlist__go_system_wide(&evlist->core, &evsel->cor=
e);
> >> +       }
> >> +
> >> +       return evsel;
> >> +}
> >> +
> >>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const =
char *str)
> >>  {
> >>         struct evsel *evsel;
> >> diff --git a/tools/perf/util/evlist.h b/tools/perf/util/evlist.h
> >> index 664c6bf7b3e0..98e7ddb2bd30 100644
> >> --- a/tools/perf/util/evlist.h
> >> +++ b/tools/perf/util/evlist.h
> >> @@ -387,6 +387,7 @@ bool evlist_cpu_iterator__end(const struct evlist_=
cpu_iterator *evlist_cpu_itr);
> >>
> >>  struct evsel *evlist__get_tracking_event(struct evlist *evlist);
> >>  void evlist__set_tracking_event(struct evlist *evlist, struct evsel *=
tracking_evsel);
> >> +struct evsel *evlist__findnew_tracking_event(struct evlist *evlist, b=
ool system_wide);
> >>
> >>  struct evsel *evlist__find_evsel_by_str(struct evlist *evlist, const =
char *str);
> >>
> >> --
> >> 2.30.GIT
> >>
>