Received-SPF: pass (google.com: domain of linux-kernel+bounces-176262-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20240509060541.1928390-1-irogers@google.com> <Zj4tVDdY3CzU7KZM@x1>
 <CAP-5=fWhSfZyWh3ye3WYmPBTEKsqRAd-ZrZjaT68=RVqb-m4kA@mail.gmail.com>
In-Reply-To: <CAP-5=fWhSfZyWh3ye3WYmPBTEKsqRAd-ZrZjaT68=RVqb-m4kA@mail.gmail.com>
From: Namhyung Kim <namhyung@kernel.org>
Date: Fri, 10 May 2024 14:53:51 -0700
Message-ID: <CAM9d7cgos57-56NOevzFb+KwgSHZxjNPHddnf2fK-ypF6Ao7bg@mail.gmail.com>
Subject: Re: [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if
 one exists
To: Ian Rogers <irogers@google.com>
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

Hi Ian and Arnaldo,

On Fri, May 10, 2024 at 7:29=E2=80=AFAM Ian Rogers <irogers@google.com> wro=
te:
>
>
>
> On Fri, May 10, 2024, 7:21=E2=80=AFAM Arnaldo Carvalho de Melo <acme@kern=
el.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 name
>> > 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/tracepoint=
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 d=
irent *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); /* Space =
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_name)=
;
>> > -                     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 fail=
ed. */
>> > +             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.

Thanks,
Namhyung

>
>> > +     free(dst);
>> > +     free(path);
>> > +     return have_file;
>> >  }
>> > diff --git a/tools/perf/util/tracepoint.h b/tools/perf/util/tracepoint=
h
>> > index c4a110fe87d7..65ccb01fc312 100644
>> > --- a/tools/perf/util/tracepoint.h
>> > +++ b/tools/perf/util/tracepoint.h
>> > @@ -4,6 +4,7 @@
>> >
>> >  #include <dirent.h>
>> >  #include <string.h>
>> > +#include <stdbool.h>
>> >
>> >  int tp_event_has_id(const char *dir_path, struct dirent *evt_dir);
>> >
>> > @@ -20,6 +21,6 @@ int tp_event_has_id(const char *dir_path, struct dir=
ent *evt_dir);
>> >                   (strcmp(sys_dirent->d_name, ".")) &&        \
>> >                   (strcmp(sys_dirent->d_name, "..")))
>> >
>> > -int is_valid_tracepoint(const char *event_string);
>> > +bool is_valid_tracepoint(const char *event_string);
>> >
>> >  #endif /* __PERF_TRACEPOINT_H */
>> > --
>> > 2.45.0.rc1.225.g2a3ae87e7f-goog