From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id C32751DE8B5; Wed, 23 Jul 2025 16:57:37 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1753289857; cv=none; b=q7VLhXzZor9zTZCX83dDNLceVKiN3Wo+bSp6CqP2/hVqy9/Yv6esmd2F9jm87NP7cGZcYXhOh7kOMfMIWautKaOO7wQXIM+30fAo2DhiAyQ/4eYUYn+KF4GjDyQ/Y7IGSI9V/QtQj7r3rwpaKStJMeRDZeNPkq+n/9btD2PGcYs= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1753289857; c=relaxed/simple; bh=oag4FLsFSar3NOF8cvreDiurpThmOQqT2qFDb4pj/XY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=sjhbxmF8WIHAkYGCC3DqGRumR5wiqks7U7zI1i91wUjLVngzaLvyGLBWEnkFmAaXolMWUHS28zi1D8ul5YnG3HZInIMpbScIq9ohA8+RR521x8lq03m32pNBNwDzZSwp7dY8yNU88clVS9URxeUGbMY0ZIWbBqPzsyKjhyK6HGk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=gJvq83WT; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="gJvq83WT" Received: by smtp.kernel.org (Postfix) with ESMTPSA id AA926C4CEE7; Wed, 23 Jul 2025 16:57:36 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1753289857; bh=oag4FLsFSar3NOF8cvreDiurpThmOQqT2qFDb4pj/XY=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gJvq83WT5radbNWhJt/WCZGXVN6koKi+ayxy5rJDIaofoVs52tneG92ETtcJc853E Ww7rhiQakkV/HQLbaWY4mnyC6244jwsRJtAVRRB6P8f7HezFMPLAXYPlLNcPIWam/Z LJoyqm0dLgvtTemOFeyQVpcFqcrGCnU6xyrUMQqL2xgscWrn+EUI2KZcKIxb9Otfer pbK3btNp6cJrWE5LqTLWwjTuxEIwOYduIS9xuUAVDg4cceBQtyo4HLkDbBDlvnvXzb svFC77MW+O2ha9BULbQKASQuTZMTKRHDwvWWFoLhOsg3r6ksJL6jiZPYfwSSsg2/QU 2tjsmXYWz7ebA== Date: Wed, 23 Jul 2025 13:57:34 -0300 From: Arnaldo Carvalho de Melo To: Ian Rogers Cc: Peter Zijlstra , Ingo Molnar , Namhyung Kim , Mark Rutland , Alexander Shishkin , Jiri Olsa , Adrian Hunter , Kan Liang , James Clark , Xu Yang , "Masami Hiramatsu (Google)" , Collin Funk , Howard Chu , Weilin Wang , Andi Kleen , "Dr. David Alan Gilbert" , Thomas Richter , Tiezhu Yang , Gautam Menghani , Thomas Falcon , Chun-Tse Shao , linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org Subject: Re: [PATCH v7 04/16] perf tp_pmu: Factor existing tracepoint logic to new file Message-ID: References: <20250714164405.111477-1-irogers@google.com> <20250714164405.111477-5-irogers@google.com> Precedence: bulk X-Mailing-List: linux-perf-users@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20250714164405.111477-5-irogers@google.com> On Mon, Jul 14, 2025 at 09:43:52AM -0700, Ian Rogers wrote: > Start the creation of a tracepoint PMU abstraction. Tracepoint events > don't follow the regular sysfs perf conventions. Eventually the new > PMU abstraction will bridge the gap so tracepoint events look more > like regular perf ones. > > Signed-off-by: Ian Rogers > --- > tools/perf/util/Build | 1 + > tools/perf/util/evsel.c | 21 +---- > tools/perf/util/parse-events.c | 147 ++++++++++++++------------------- > tools/perf/util/tp_pmu.c | 95 +++++++++++++++++++++ > tools/perf/util/tp_pmu.h | 12 +++ > 5 files changed, 170 insertions(+), 106 deletions(-) > create mode 100644 tools/perf/util/tp_pmu.c > create mode 100644 tools/perf/util/tp_pmu.h > > diff --git a/tools/perf/util/Build b/tools/perf/util/Build > index 12bc01c843b2..4959e7a990e4 100644 > --- a/tools/perf/util/Build > +++ b/tools/perf/util/Build > @@ -88,6 +88,7 @@ perf-util-y += pmu-bison.o > perf-util-y += drm_pmu.o > perf-util-y += hwmon_pmu.o > perf-util-y += tool_pmu.o > +perf-util-y += tp_pmu.o > perf-util-y += svghelper.o > perf-util-y += trace-event-info.o > perf-util-y += trace-event-scripting.o > diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c > index 3896a04d90af..5a1d19b4e5cd 100644 > --- a/tools/perf/util/evsel.c > +++ b/tools/perf/util/evsel.c > @@ -59,6 +59,7 @@ > #include "drm_pmu.h" > #include "hwmon_pmu.h" > #include "tool_pmu.h" > +#include "tp_pmu.h" > #include "rlimit.h" > #include "../perf-sys.h" > #include "util/parse-branch-options.h" > @@ -571,24 +572,6 @@ struct evsel *evsel__clone(struct evsel *dest, struct evsel *orig) > return NULL; > } > > -static int trace_event__id(const char *sys, const char *name) > -{ > - char *tp_dir = get_events_file(sys); > - char path[PATH_MAX]; > - int id, err; > - > - if (!tp_dir) > - return -1; > - > - scnprintf(path, PATH_MAX, "%s/%s/id", tp_dir, name); > - put_events_file(tp_dir); > - err = filename__read_int(path, &id); > - if (err) > - return err; > - > - return id; > -} > - > /* > * Returns pointer with encoded error via interface. > */ > @@ -622,7 +605,7 @@ struct evsel *evsel__newtp_idx(const char *sys, const char *name, int idx, bool > event_attr_init(&attr); > > if (format) { > - id = trace_event__id(sys, name); > + id = tp_pmu__id(sys, name); > if (id < 0) { > err = id; > goto out_free; > diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c > index 1ae481c9802b..f19541ca5268 100644 > --- a/tools/perf/util/parse-events.c > +++ b/tools/perf/util/parse-events.c > @@ -17,13 +17,12 @@ > #include "string2.h" > #include "strbuf.h" > #include "debug.h" > -#include > -#include > #include > #include > #include > #include "pmu.h" > #include "pmus.h" > +#include "tp_pmu.h" > #include "asm/bug.h" > #include "ui/ui.h" > #include "util/parse-branch-options.h" > @@ -33,6 +32,7 @@ > #include "util/stat.h" > #include "util/util.h" > #include "tracepoint.h" > +#include > > #define MAX_NAME_LEN 100 > > @@ -558,105 +558,82 @@ static int add_tracepoint(struct parse_events_state *parse_state, > return 0; > } > > -static int add_tracepoint_multi_event(struct parse_events_state *parse_state, > - struct list_head *list, > - const char *sys_name, const char *evt_name, > - struct parse_events_error *err, > - struct parse_events_terms *head_config, YYLTYPE *loc) > -{ > - char *evt_path; > - struct io_dirent64 *evt_ent; > - struct io_dir evt_dir; > - int ret = 0, found = 0; > - > - evt_path = get_events_file(sys_name); > - if (!evt_path) { > - tracepoint_error(err, errno, sys_name, evt_name, loc->first_column); > - return -1; > - } > - io_dir__init(&evt_dir, open(evt_path, O_CLOEXEC | O_DIRECTORY | O_RDONLY)); > - if (evt_dir.dirfd < 0) { > - put_events_file(evt_path); > - tracepoint_error(err, errno, sys_name, evt_name, loc->first_column); > - return -1; > - } > +struct add_tracepoint_multi_args { > + struct parse_events_state *parse_state; > + struct list_head *list; > + const char *sys_glob; > + const char *evt_glob; > + struct parse_events_error *err; > + struct parse_events_terms *head_config; > + YYLTYPE *loc; > + int found; > +}; > > - while (!ret && (evt_ent = io_dir__readdir(&evt_dir))) { > - if (!strcmp(evt_ent->d_name, ".") > - || !strcmp(evt_ent->d_name, "..") > - || !strcmp(evt_ent->d_name, "enable") > - || !strcmp(evt_ent->d_name, "filter")) > - continue; > +static int add_tracepoint_multi_event_cb(void *state, const char *sys_name, const char *evt_name) > +{ > + struct add_tracepoint_multi_args *args = state; > + int ret; > > - if (!strglobmatch(evt_ent->d_name, evt_name)) > - continue; > + if (!strglobmatch(evt_name, args->evt_glob)) > + return 0; > > - found++; > + args->found++; > + ret = add_tracepoint(args->parse_state, args->list, sys_name, evt_name, > + args->err, args->head_config, args->loc); > > - ret = add_tracepoint(parse_state, list, sys_name, evt_ent->d_name, > - err, head_config, loc); > - } > + return ret; > +} > > - if (!found) { > - tracepoint_error(err, ENOENT, sys_name, evt_name, loc->first_column); > - ret = -1; > +static int add_tracepoint_multi_event(struct add_tracepoint_multi_args *args, const char *sys_name) > +{ > + if (strpbrk(args->evt_glob, "*?") == NULL) { > + /* Not a glob. */ > + args->found++; > + return add_tracepoint(args->parse_state, args->list, sys_name, args->evt_glob, > + args->err, args->head_config, args->loc); > } > > - put_events_file(evt_path); > - close(evt_dir.dirfd); > - return ret; > + return tp_pmu__for_each_tp_event(sys_name, args, add_tracepoint_multi_event_cb); > } > > -static int add_tracepoint_event(struct parse_events_state *parse_state, > - struct list_head *list, > - const char *sys_name, const char *evt_name, > - struct parse_events_error *err, > - struct parse_events_terms *head_config, YYLTYPE *loc) > +static int add_tracepoint_multi_sys_cb(void *state, const char *sys_name) > { > - return strpbrk(evt_name, "*?") ? > - add_tracepoint_multi_event(parse_state, list, sys_name, evt_name, > - err, head_config, loc) : > - add_tracepoint(parse_state, list, sys_name, evt_name, > - err, head_config, loc); > + struct add_tracepoint_multi_args *args = state; > + > + if (!strglobmatch(sys_name, args->sys_glob)) > + return 0; > + > + return add_tracepoint_multi_event(args, sys_name); > } > > static int add_tracepoint_multi_sys(struct parse_events_state *parse_state, > struct list_head *list, > - const char *sys_name, const char *evt_name, > + const char *sys_glob, const char *evt_glob, > struct parse_events_error *err, > struct parse_events_terms *head_config, YYLTYPE *loc) > { > - struct io_dirent64 *events_ent; > - struct io_dir events_dir; > - int ret = 0; > - char *events_dir_path = get_tracing_file("events"); > + struct add_tracepoint_multi_args args = { > + .parse_state = parse_state, > + .list = list, > + .sys_glob = sys_glob, > + .evt_glob = evt_glob, > + .err = err, > + .head_config = head_config, > + .loc = loc, > + .found = 0, > + }; > + int ret; > > - if (!events_dir_path) { > - tracepoint_error(err, errno, sys_name, evt_name, loc->first_column); > - return -1; > + if (strpbrk(sys_glob, "*?") == NULL) { > + /* Not a glob. */ > + ret = add_tracepoint_multi_event(&args, sys_glob); > + } else { > + ret = tp_pmu__for_each_tp_sys(&args, add_tracepoint_multi_sys_cb); > } > - io_dir__init(&events_dir, open(events_dir_path, O_CLOEXEC | O_DIRECTORY | O_RDONLY)); > - put_events_file(events_dir_path); > - if (events_dir.dirfd < 0) { > - tracepoint_error(err, errno, sys_name, evt_name, loc->first_column); > - return -1; > + if (args.found == 0) { > + tracepoint_error(err, ENOENT, sys_glob, evt_glob, loc->first_column); > + return -ENOENT; > } > - > - while (!ret && (events_ent = io_dir__readdir(&events_dir))) { > - if (!strcmp(events_ent->d_name, ".") > - || !strcmp(events_ent->d_name, "..") > - || !strcmp(events_ent->d_name, "enable") > - || !strcmp(events_ent->d_name, "header_event") > - || !strcmp(events_ent->d_name, "header_page")) > - continue; > - > - if (!strglobmatch(events_ent->d_name, sys_name)) > - continue; > - > - ret = add_tracepoint_event(parse_state, list, events_ent->d_name, > - evt_name, err, head_config, loc); > - } > - close(events_dir.dirfd); > return ret; > } > > @@ -1348,12 +1325,8 @@ int parse_events_add_tracepoint(struct parse_events_state *parse_state, > return -EINVAL; > } > > - if (strpbrk(sys, "*?")) > - return add_tracepoint_multi_sys(parse_state, list, sys, event, > - err, head_config, loc); > - else > - return add_tracepoint_event(parse_state, list, sys, event, > - err, head_config, loc); > + return add_tracepoint_multi_sys(parse_state, list, sys, event, > + err, head_config, loc); > } > > static int __parse_events_add_numeric(struct parse_events_state *parse_state, > diff --git a/tools/perf/util/tp_pmu.c b/tools/perf/util/tp_pmu.c > new file mode 100644 > index 000000000000..fd83164f8763 > --- /dev/null > +++ b/tools/perf/util/tp_pmu.c > @@ -0,0 +1,95 @@ > +// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) > +#include "tp_pmu.h" > +#include > +#include > +#include > +#include > +#include > +#include > + > +int tp_pmu__id(const char *sys, const char *name) > +{ > + char *tp_dir = get_events_file(sys); > + char path[PATH_MAX]; > + int id, err; > + > + if (!tp_dir) > + return -1; > + > + scnprintf(path, PATH_MAX, "%s/%s/id", tp_dir, name); > + put_events_file(tp_dir); > + err = filename__read_int(path, &id); > + if (err) > + return err; > + > + return id; > +} > + > + > +int tp_pmu__for_each_tp_event(const char *sys, void *state, tp_event_callback cb) > +{ > + char *evt_path; > + struct io_dirent64 *evt_ent; > + struct io_dir evt_dir; > + int ret = 0; > + > + evt_path = get_events_file(sys); > + if (!evt_path) > + return -errno; > + > + io_dir__init(&evt_dir, open(evt_path, O_CLOEXEC | O_DIRECTORY | O_RDONLY)); > + if (evt_dir.dirfd < 0) { > + ret = -errno; > + put_events_file(evt_path); > + return ret; > + } > + put_events_file(evt_path); > + > + while (!ret && (evt_ent = io_dir__readdir(&evt_dir))) { > + if (!strcmp(evt_ent->d_name, ".") > + || !strcmp(evt_ent->d_name, "..") > + || !strcmp(evt_ent->d_name, "enable") > + || !strcmp(evt_ent->d_name, "filter")) > + continue; I see that the previous code had this style, but since we're moving it to this separate file, can we have the || at the end of the lines please? > + > + ret = cb(state, sys, evt_ent->d_name); > + if (ret) > + break; > + } > + close(evt_dir.dirfd); > + return ret; > +} > + > +int tp_pmu__for_each_tp_sys(void *state, tp_sys_callback cb) > +{ > + struct io_dirent64 *events_ent; > + struct io_dir events_dir; > + int ret = 0; > + char *events_dir_path = get_tracing_file("events"); > + > + if (!events_dir_path) > + return -errno; > + > + io_dir__init(&events_dir, open(events_dir_path, O_CLOEXEC | O_DIRECTORY | O_RDONLY)); > + if (events_dir.dirfd < 0) { > + ret = -errno; > + put_events_file(events_dir_path); > + return ret; > + } > + put_events_file(events_dir_path); > + > + while (!ret && (events_ent = io_dir__readdir(&events_dir))) { > + if (!strcmp(events_ent->d_name, ".") > + || !strcmp(events_ent->d_name, "..") > + || !strcmp(events_ent->d_name, "enable") > + || !strcmp(events_ent->d_name, "header_event") > + || !strcmp(events_ent->d_name, "header_page")) > + continue; Ditto > + > + ret = cb(state, events_ent->d_name); > + if (ret) > + break; > + } > + close(events_dir.dirfd); > + return ret; > +} > diff --git a/tools/perf/util/tp_pmu.h b/tools/perf/util/tp_pmu.h > new file mode 100644 > index 000000000000..49537303bd73 > --- /dev/null > +++ b/tools/perf/util/tp_pmu.h > @@ -0,0 +1,12 @@ > +/* SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause) */ > +#ifndef __TP_PMU_H > +#define __TP_PMU_H > + > +typedef int (*tp_sys_callback)(void *state, const char *sys_name); > +typedef int (*tp_event_callback)(void *state, const char *sys_name, const char *evt_name); > + > +int tp_pmu__id(const char *sys, const char *name); > +int tp_pmu__for_each_tp_event(const char *sys, void *state, tp_event_callback cb); > +int tp_pmu__for_each_tp_sys(void *state, tp_sys_callback cb); > + > +#endif /* __TP_PMU_H */ > -- > 2.50.0.727.gbf7dc18ff4-goog >