* [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if one exists @ 2024-05-09 6:05 Ian Rogers 2024-05-10 14:21 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 4+ messages in thread From: Ian Rogers @ 2024-05-09 6:05 UTC (permalink / raw) To: Peter Zijlstra, Ingo Molnar, Arnaldo Carvalho de Melo, Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa, Ian Rogers, Adrian Hunter, Kan Liang, linux-perf-users, linux-kernel 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 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); + 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 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if one exists 2024-05-09 6:05 [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if one exists Ian Rogers @ 2024-05-10 14:21 ` Arnaldo Carvalho de Melo [not found] ` <CAP-5=fWhSfZyWh3ye3WYmPBTEKsqRAd-ZrZjaT68=RVqb-m4kA@mail.gmail.com> 0 siblings, 1 reply; 4+ messages in thread From: Arnaldo Carvalho de Melo @ 2024-05-10 14:21 UTC (permalink / raw) To: Ian Rogers Cc: Peter Zijlstra, Ingo Molnar, Namhyung Kim, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang, linux-perf-users, linux-kernel 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 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); If dst == NULL have_file isn't initialized? > + 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 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
[parent not found: <CAP-5=fWhSfZyWh3ye3WYmPBTEKsqRAd-ZrZjaT68=RVqb-m4kA@mail.gmail.com>]
* Re: [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if one exists [not found] ` <CAP-5=fWhSfZyWh3ye3WYmPBTEKsqRAd-ZrZjaT68=RVqb-m4kA@mail.gmail.com> @ 2024-05-10 21:53 ` Namhyung Kim 2024-05-10 22:11 ` Ian Rogers 0 siblings, 1 reply; 4+ messages in thread From: Namhyung Kim @ 2024-05-10 21:53 UTC (permalink / raw) To: Ian Rogers Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang, linux-perf-users, LKML Hi Ian and Arnaldo, On Fri, May 10, 2024 at 7:29 AM Ian Rogers <irogers@google.com> wrote: > > > > On Fri, May 10, 2024, 7:21 AM Arnaldo Carvalho de Melo <acme@kernel.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 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/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 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if one exists 2024-05-10 21:53 ` Namhyung Kim @ 2024-05-10 22:11 ` Ian Rogers 0 siblings, 0 replies; 4+ messages in thread From: Ian Rogers @ 2024-05-10 22:11 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Peter Zijlstra, Ingo Molnar, Mark Rutland, Alexander Shishkin, Jiri Olsa, Adrian Hunter, Kan Liang, linux-perf-users, LKML On Fri, May 10, 2024 at 2:54 PM Namhyung Kim <namhyung@kernel.org> wrote: > > Hi Ian and Arnaldo, > > On Fri, May 10, 2024 at 7:29 AM Ian Rogers <irogers@google.com> wrote: > > > > > > > > On Fri, May 10, 2024, 7:21 AM Arnaldo Carvalho de Melo <acme@kernel.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 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. 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/tree/tools/perf/util/perf_event_attr_fprintf.c?h=perf-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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2024-05-10 22:11 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-09 6:05 [PATCH v1] perf tracepoint: Don't scan all tracepoints to test if one exists Ian Rogers
2024-05-10 14:21 ` Arnaldo Carvalho de Melo
[not found] ` <CAP-5=fWhSfZyWh3ye3WYmPBTEKsqRAd-ZrZjaT68=RVqb-m4kA@mail.gmail.com>
2024-05-10 21:53 ` Namhyung Kim
2024-05-10 22:11 ` Ian Rogers
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).