linux-kernel - Re: [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if one exists

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAM9d7cgos57-56NOevzFb+KwgSHZxjNPHddnf2fK-ypF6Ao7bg@mail.gmail.com>
Date: Fri, 10 May 2024 14:53:51 -0700
From: Namhyung Kim <namhyung@...nel.org>
To: Ian Rogers <irogers@...gle.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Ingo Molnar <mingo@...hat.com>, Mark Rutland <mark.rutland@....com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, Jiri Olsa <jolsa@...nel.org>, 
	Adrian Hunter <adrian.hunter@...el.com>, Kan Liang <kan.liang@...ux.intel.com>, 
	linux-perf-users <linux-perf-users@...r.kernel.org>, LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if
 one exists

Hi Ian and Arnaldo,

On Fri, May 10, 2024 at 7:29 AM Ian Rogers <irogers@...gle.com> wrote:
>
>
>
> On Fri, May 10, 2024, 7:21 AM Arnaldo Carvalho de Melo <acme@...nel.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@...gle.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/tracepointc
>> > 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 = tracing_events__opendir();
>> > -     if (!sys_dir)
>> > -             return 0;
>> > +     char *dst, *path = malloc(strlen(event_string) + 4); /* Space for "/id\0". */
>> > +     const char *src;
>> > +     bool have_file;
>> >
>> > -     for_each_subsystem(sys_dir, sys_dirent) {
>> > -             dir_path = get_events_file(sys_dirent->d_name);
>> > -             if (!dir_path)
>> > -                     continue;
>> > -             evt_dir = 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 failed. */
>> > +             return false;
>> >       }
>> > -     closedir(sys_dir);
>> > -     return 0;
>> > +     /* Copy event_string replacing the ':' with '/'. */
>> > +     for (src = event_string, dst = path; *src; src++, dst++)
>> > +             *dst = (*src == ':') ? '/' : *src;
>> > +     /* Add "/id\0". */
>> > +     memcpy(dst, "/id", 4);
>> > +
>> > +     dst = get_events_file(path);
>> > +     if (dst)
>> > +             have_file = 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/tracepointh
>> > 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 dirent *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