Received-SPF: pass (google.com: domain of linux-kernel+bounces-176263-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240509060541.1928390-1-irogers@google.com> <Zj4tVDdY3CzU7KZM@x1>
 <CAP-5=fWhSfZyWh3ye3WYmPBTEKsqRAd-ZrZjaT68=RVqb-m4kA@mail.gmail.com> <CAM9d7cgos57-56NOevzFb+KwgSHZxjNPHddnf2fK-ypF6Ao7bg@mail.gmail.com>
In-Reply-To: <CAM9d7cgos57-56NOevzFb+KwgSHZxjNPHddnf2fK-ypF6Ao7bg@mail.gmail.com>
From: Ian Rogers <irogers@google.com>
Date: Fri, 10 May 2024 15:11:45 -0700
Message-ID: <CAP-5=fVf0G9pnHGL2DNFmBdzm_kW2gHzz-D9ToTyy51BQ_W1RA@mail.gmail.com>
Subject: Re: [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if
 one exists
To: Namhyung Kim <namhyung@kernel.org>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>, Peter Zijlstra <peterz@infradead.org>, 
	Ingo Molnar <mingo@redhat.com>, Mark Rutland <mark.rutland@arm.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, Jiri Olsa <jolsa@kernel.org>, 
	Adrian Hunter <adrian.hunter@intel.com>, Kan Liang <kan.liang@linux.intel.com>, 
	linux-perf-users <linux-perf-users@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, May 10, 2024 at 2:54=E2=80=AFPM Namhyung Kim <namhyung@kernel.org> =
wrote:
>
> Hi Ian and Arnaldo,
>
> On Fri, May 10, 2024 at 7:29=E2=80=AFAM Ian Rogers <irogers@google.com> w=
rote:
> >
> >
> >
> > On Fri, May 10, 2024, 7:21=E2=80=AFAM Arnaldo Carvalho de Melo <acme@ke=
rnel.org> wrote:
> >>
> >> On Wed, May 08, 2024 at 11:05:41PM -0700, Ian Rogers wrote:
> >> > In is_valid_tracepoint, rather than scanning
> >> > "/sys/kernel/tracing/events/*/*" skipping any path where
> >> > "/sys/kernel/tracing/events/*/*/id" doesn't exist, and then testing =
if
> >> > "*:*" matches the tracepoint name, just use the given tracepoint nam=
e
> >> > replace the ':' with '/' and see if the id file exists. This turns a
> >> > nested directory search into a single file available test.
> >> >
> >> > Rather than return 1 for valid and 0 for invalid, return true and
> >> > false.
> >> >
> >> > Signed-off-by: Ian Rogers <irogers@google.com>
> >> > ---
> >> >  tools/perf/util/tracepoint.c | 53 ++++++++++++++-------------------=
---
> >> >  tools/perf/util/tracepoint.h |  3 +-
> >> >  2 files changed, 23 insertions(+), 33 deletions(-)
> >> >
> >> > diff --git a/tools/perf/util/tracepoint.c b/tools/perf/util/tracepoi=
nt.c
> >> > index 92dd8b455b90..cca6808f7951 100644
> >> > --- a/tools/perf/util/tracepoint.c
> >> > +++ b/tools/perf/util/tracepoint.c
> >> > @@ -4,10 +4,12 @@
> >> >  #include <errno.h>
> >> >  #include <fcntl.h>
> >> >  #include <stdio.h>
> >> > +#include <stdlib.h>
> >> >  #include <sys/param.h>
> >> >  #include <unistd.h>
> >> >
> >> >  #include <api/fs/tracing_path.h>
> >> > +#include "fncache.h"
> >> >
> >> >  int tp_event_has_id(const char *dir_path, struct dirent *evt_dir)
> >> >  {
> >> > @@ -26,39 +28,26 @@ int tp_event_has_id(const char *dir_path, struct=
 dirent *evt_dir)
> >> >  /*
> >> >   * Check whether event is in <debugfs_mount_point>/tracing/events
> >> >   */
> >> > -int is_valid_tracepoint(const char *event_string)
> >> > +bool is_valid_tracepoint(const char *event_string)
> >> >  {
> >> > -     DIR *sys_dir, *evt_dir;
> >> > -     struct dirent *sys_dirent, *evt_dirent;
> >> > -     char evt_path[MAXPATHLEN];
> >> > -     char *dir_path;
> >> > -
> >> > -     sys_dir =3D tracing_events__opendir();
> >> > -     if (!sys_dir)
> >> > -             return 0;
> >> > +     char *dst, *path =3D malloc(strlen(event_string) + 4); /* Spac=
e for "/id\0". */
> >> > +     const char *src;
> >> > +     bool have_file;
> >> >
> >> > -     for_each_subsystem(sys_dir, sys_dirent) {
> >> > -             dir_path =3D get_events_file(sys_dirent->d_name);
> >> > -             if (!dir_path)
> >> > -                     continue;
> >> > -             evt_dir =3D opendir(dir_path);
> >> > -             if (!evt_dir)
> >> > -                     goto next;
> >> > -
> >> > -             for_each_event(dir_path, evt_dir, evt_dirent) {
> >> > -                     snprintf(evt_path, MAXPATHLEN, "%s:%s",
> >> > -                              sys_dirent->d_name, evt_dirent->d_nam=
e);
> >> > -                     if (!strcmp(evt_path, event_string)) {
> >> > -                             closedir(evt_dir);
> >> > -                             put_events_file(dir_path);
> >> > -                             closedir(sys_dir);
> >> > -                             return 1;
> >> > -                     }
> >> > -             }
> >> > -             closedir(evt_dir);
> >> > -next:
> >> > -             put_events_file(dir_path);
> >> > +     if (!path) {
> >> > +             /* Conservatively return false if memory allocation fa=
iled. */
> >> > +             return false;
> >> >       }
> >> > -     closedir(sys_dir);
> >> > -     return 0;
> >> > +     /* Copy event_string replacing the ':' with '/'. */
> >> > +     for (src =3D event_string, dst =3D path; *src; src++, dst++)
> >> > +             *dst =3D (*src =3D=3D ':') ? '/' : *src;
> >> > +     /* Add "/id\0". */
> >> > +     memcpy(dst, "/id", 4);
> >> > +
> >> > +     dst =3D get_events_file(path);
> >> > +     if (dst)
> >> > +             have_file =3D file_available(dst);
>
> Off-topic.  But it seems we don't release fncache entries.

Agreed it is weird. Going further off-topic it seems silly that we can
tell whether a tracepoint is present via its id, but we don't load the
id so that we can do things like pretty printing unless libtraceevent
is present:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tr=
ee/tools/perf/util/perf_event_attr_fprintf.c?h=3Dperf-tools-next#n242
I think it would make sense to make the tracepoint PMU have
events/aliases and load them as a special case from
`/sys/kernel/tracing/events/`. Currently all the tracepoint event
logic is special because the only event name containing a colon is a
tracepoint - we don't allow `tracepoint/lock:contention_begin/` as an
event, which seems out of keeping. I think we could go pretty far with
what perf needs by supporting tracepoint events through the PMU and
removing as much "#ifdef HAVE_LIBTRACEEVENT" as possible.
I mention this as it would make sense to have the name fncache be
related to the PMU in case PMUs come and go, as could happen with the
perf daemon.

Thanks,
Ian