Re: [PATCH v2 2/5] perf pmus: Restructure pmu_read_sysfs to scan fewer PMUs

Ian Rogers
2025-02-01T07:26:14Z

On Thu, Jan 30, 2025 at 11:24 AM Liang, Kan <kan.liang@linux.intel.com> wrote:
>
>
>
> On 2025-01-23 2:46 a.m., Ian Rogers wrote:
> > Rather than scanning core or all PMUs, allow pmu_read_sysfs to read
> > some combination of core, other, hwmon and tool PMUs. The PMUs that
> > should be read and are already read are held as bitmaps. It is known
> > that a "hwmon_" prefix is necessary for a hwmon PMU's name, similarly
> > with "tool", so only scan those PMUs in situations the PMU name or the
> > PMU's type number make sense to.
> >
> > The number of openat system calls reduces from 276 to 98 for a hwmon
> > event. The number of openats for regular perf events isn't changed.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> > tools/perf/util/pmu.h | 2 +
> > tools/perf/util/pmus.c | 144 ++++++++++++++++++++++++++---------------
> > 2 files changed, 95 insertions(+), 51 deletions(-)
> >
> > diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> > index dbed6c243a5e..edd36c20aedc 100644
> > --- a/tools/perf/util/pmu.h
> > +++ b/tools/perf/util/pmu.h
> > @@ -37,6 +37,8 @@ struct perf_pmu_caps {
> > };
> >
> > enum {
> > + PERF_PMU_TYPE_PE_START = 0,
> > + PERF_PMU_TYPE_PE_END = 0xFFFEFFFF,
> > PERF_PMU_TYPE_HWMON_START = 0xFFFF0000,
> > PERF_PMU_TYPE_HWMON_END = 0xFFFFFFFD,
> > PERF_PMU_TYPE_TOOL = 0xFFFFFFFE,
> > diff --git a/tools/perf/util/pmus.c b/tools/perf/util/pmus.c
> > index b493da0d22ef..3e3ffafcad71 100644
> > --- a/tools/perf/util/pmus.c
> > +++ b/tools/perf/util/pmus.c
> > @@ -37,10 +37,23 @@
> > */
> > static LIST_HEAD(core_pmus);
> > static LIST_HEAD(other_pmus);
> > -static bool read_sysfs_core_pmus;
> > -static bool read_sysfs_all_pmus;
> > +enum perf_tool_pmu_type {
> > + PERF_TOOL_PMU_TYPE_PE_CORE,
> > +#define PERF_TOOL_PMU_TYPE_PE_CORE_MASK (1 << PERF_TOOL_PMU_TYPE_PE_CORE)
> > + PERF_TOOL_PMU_TYPE_PE_OTHER,
>
> What is the PE short for?

Perf event, rather than type numbers being used for things like the fake PMU.

> The terminology "other" confused me at first glance.

Agreed. It corresponds to the other list immediately above this code.

> I think the PE_OTHER means the PMUs not core, hwmon or tool.
> There is a "other_pmus" nearby, which means the PMUs not core, but
> include hwmon and tool.
>
> Please add some comments to explain the scope of the TYPE_PE_OTHER.

This is already this immediately above:
https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/pmus.c?h=perf-tools-next#n31
```
* other_pmus: All other PMUs which are not part of core_pmus list. It doesn't
* matter whether PMU is present per SMT-thread or outside of the
* core in the hw. For e.g., an instance of AMD ibs_fetch// and
* ibs_op// PMUs is present in each hw SMT thread, however they
* are captured under other_pmus. PMUs belonging to other_pmus
* must have pmu->is_core=0 but pmu->is_uncore could be 0 or 1.
```

> > +#define PERF_TOOL_PMU_TYPE_PE_OTHER_MASK (1 << PERF_TOOL_PMU_TYPE_PE_OTHER)
> > + PERF_TOOL_PMU_TYPE_TOOL,
> > +#define PERF_TOOL_PMU_TYPE_TOOL_MASK (1 << PERF_TOOL_PMU_TYPE_TOOL)
> > + PERF_TOOL_PMU_TYPE_HWMON,
> > +#define PERF_TOOL_PMU_TYPE_HWMON_MASK (1 << PERF_TOOL_PMU_TYPE_HWMON)
> > +#define PERF_TOOL_PMU_TYPE_ALL_MASK (PERF_TOOL_PMU_TYPE_PE_CORE_MASK | \
> > + PERF_TOOL_PMU_TYPE_PE_OTHER_MASK | \
> > + PERF_TOOL_PMU_TYPE_TOOL_MASK | \
> > + PERF_TOOL_PMU_TYPE_HWMON_MASK)
> > +};
>
> Nit: Maybe it's better to split the enum perf_tool_pmu_type and #define.
> It's a little bit hard for me to locate how many types there are at
> first glance.

Ok, will do in v3.

> > +static unsigned int read_pmu_types;
> >
> > -static void pmu_read_sysfs(bool core_only);
> > +static void pmu_read_sysfs(unsigned int to_read_pmus);
> >
> > size_t pmu_name_len_no_suffix(const char *str)
> > {
> > @@ -102,8 +115,7 @@ void perf_pmus__destroy(void)
> >
> > perf_pmu__delete(pmu);
> > }
> > - read_sysfs_core_pmus = false;
> > - read_sysfs_all_pmus = false;
> > + read_pmu_types = 0;
> > }
> >
> > static struct perf_pmu *pmu_find(const char *name)
> > @@ -129,6 +141,7 @@ struct perf_pmu *perf_pmus__find(const char *name)
> > struct perf_pmu *pmu;
> > int dirfd;
> > bool core_pmu;
> > + unsigned int to_read_pmus = 0;
> >
> > /*
> > * Once PMU is loaded it stays in the list,
> > @@ -139,11 +152,11 @@ struct perf_pmu *perf_pmus__find(const char *name)
> > if (pmu)
> > return pmu;
> >
> > - if (read_sysfs_all_pmus)
> > + if (read_pmu_types == PERF_TOOL_PMU_TYPE_ALL_MASK)
> > return NULL;
> >
> > core_pmu = is_pmu_core(name);
> > - if (core_pmu && read_sysfs_core_pmus)
> > + if (core_pmu && (read_pmu_types & PERF_TOOL_PMU_TYPE_PE_CORE_MASK))
> > return NULL;
> >
> > dirfd = perf_pmu__event_source_devices_fd();
> > @@ -151,15 +164,27 @@ struct perf_pmu *perf_pmus__find(const char *name)
> > /*eager_load=*/false);
> > close(dirfd);
> >
> > - if (!pmu) {
> > - /*
> > - * Looking up an inidividual PMU failed. This may mean name is
> > - * an alias, so read the PMUs from sysfs and try to find again.
> > - */
> > - pmu_read_sysfs(core_pmu);
> > + if (pmu)
> > + return pmu;
> > +
> > + /* Looking up an individual perf event PMU failed, check if a tool PMU should be read. */
> > + if (!strncmp(name, "hwmon_", 6))
> > + to_read_pmus |= PERF_TOOL_PMU_TYPE_HWMON_MASK;
> > + else if (!strcmp(name, "tool"))
> > + to_read_pmus |= PERF_TOOL_PMU_TYPE_TOOL_MASK;
> > +
> > + if (to_read_pmus) {
> > + pmu_read_sysfs(to_read_pmus);
> > pmu = pmu_find(name);
> > + if (pmu)
> > + return pmu;
> > }
> > - return pmu;
> > + /* Read all necessary PMUs from sysfs and see if the PMU is found. */
> > + to_read_pmus = PERF_TOOL_PMU_TYPE_PE_CORE_MASK;
> > + if (!core_pmu)
> > + to_read_pmus |= PERF_TOOL_PMU_TYPE_PE_OTHER_MASK;
> > + pmu_read_sysfs(to_read_pmus);
> > + return pmu_find(name);
> > }
> >
> > static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
> > @@ -176,11 +201,11 @@ static struct perf_pmu *perf_pmu__find2(int dirfd, const char *name)
> > if (pmu)
> > return pmu;
> >
> > - if (read_sysfs_all_pmus)
> > + if (read_pmu_types == PERF_TOOL_PMU_TYPE_ALL_MASK)
> > return NULL;
> >
> > core_pmu = is_pmu_core(name);
> > - if (core_pmu && read_sysfs_core_pmus)
> > + if (core_pmu && (read_pmu_types & PERF_TOOL_PMU_TYPE_PE_CORE_MASK))
> > return NULL;
> >
> > return perf_pmu__lookup(core_pmu ? &core_pmus : &other_pmus, dirfd, name,
> > @@ -197,52 +222,60 @@ static int pmus_cmp(void *priv __maybe_unused,
> > }
> >
> > /* Add all pmus in sysfs to pmu list: */
> > -static void pmu_read_sysfs(bool core_only)
> > +static void pmu_read_sysfs(unsigned int to_read_types)
> > {
> > - int fd;
> > - DIR *dir;
> > - struct dirent *dent;
> > struct perf_pmu *tool_pmu;
> >
> > - if (read_sysfs_all_pmus || (core_only && read_sysfs_core_pmus))
> > + if ((read_pmu_types & to_read_types) == to_read_types) {
> > + /* All requested PMU types have been read. */
> > return;
> > + }
> >
> > - fd = perf_pmu__event_source_devices_fd();
> > - if (fd < 0)
> > - return;
> > + if (to_read_types & (PERF_TOOL_PMU_TYPE_PE_CORE_MASK | PERF_TOOL_PMU_TYPE_PE_OTHER_MASK)) {
> > + int fd = perf_pmu__event_source_devices_fd();
> > + DIR *dir;
> > + struct dirent *dent;
> > + bool core_only = (to_read_types & PERF_TOOL_PMU_TYPE_PE_OTHER_MASK) == 0;
> >
> > - dir = fdopendir(fd);
> > - if (!dir) {
> > - close(fd);
> > - return;
> > - }
> > + if (fd < 0)
> > + goto skip_pe_pmus;
> >
> > - while ((dent = readdir(dir))) {
> > - if (!strcmp(dent->d_name, ".") || !strcmp(dent->d_name, ".."))
> > - continue;
> > - if (core_only && !is_pmu_core(dent->d_name))
> > - continue;
> > - /* add to static LIST_HEAD(core_pmus) or LIST_HEAD(other_pmus): */
> > - perf_pmu__find2(fd, dent->d_name);
> > - }
> > + dir = fdopendir(fd);
> > + if (!dir) {
> > + close(fd);
> > + goto skip_pe_pmus;
> > + }
> >
> > - closedir(dir);
> > - if (list_empty(&core_pmus)) {
> > + while ((dent = readdir(dir))) {
> > + if (!strcmp(dent->d_name, ".") || !strcmp(dent->d_name, ".."))
> > + continue;
> > + if (core_only && !is_pmu_core(dent->d_name))
> > + continue;
> > + /* add to static LIST_HEAD(core_pmus) or LIST_HEAD(other_pmus): */
> > + perf_pmu__find2(fd, dent->d_name);
> > + }
> > +
> > + closedir(dir);
> > + }
> > +skip_pe_pmus:
> > + if ((to_read_types & PERF_TOOL_PMU_TYPE_PE_CORE_MASK) && list_empty(&core_pmus)) {
> > if (!perf_pmu__create_placeholder_core_pmu(&core_pmus))
> > pr_err("Failure to set up any core PMUs\n");
> > }
> > list_sort(NULL, &core_pmus, pmus_cmp);
> > - if (!core_only) {
> > +
> > + if ((to_read_types & PERF_TOOL_PMU_TYPE_TOOL_MASK) != 0 &&
> > + (read_pmu_types & PERF_TOOL_PMU_TYPE_TOOL_MASK) == 0) {
>
> Nit: Maybe
>
> if ((to_read_types & PERF_TOOL_PMU_TYPE_TOOL_MASK) &&
> !(read_pmu_types & PERF_TOOL_PMU_TYPE_TOOL_MASK))

Thanks. I'm not convinced on the readability, which was why I was
being a bit more verbose in what I wrote. I worrited the & reads like
an && and the ! is hard to miss.

Thanks,
Ian

> > tool_pmu = perf_pmus__tool_pmu();
> > list_add_tail(&tool_pmu->list, &other_pmus);
> > - perf_pmus__read_hwmon_pmus(&other_pmus);
> > }
> > + if ((to_read_types & PERF_TOOL_PMU_TYPE_HWMON_MASK) != 0 &&
> > + (read_pmu_types & PERF_TOOL_PMU_TYPE_HWMON_MASK) == 0)
>
> ditto
>
> Thanks,
> Kan
> > + perf_pmus__read_hwmon_pmus(&other_pmus);
> > +
> > list_sort(NULL, &other_pmus, pmus_cmp);
> > - if (!list_empty(&core_pmus)) {
> > - read_sysfs_core_pmus = true;
> > - if (!core_only)
> > - read_sysfs_all_pmus = true;
> > - }
> > +
> > + read_pmu_types |= to_read_types;
> > }
> >
> > static struct perf_pmu *__perf_pmus__find_by_type(unsigned int type)
> > @@ -263,12 +296,21 @@ static struct perf_pmu *__perf_pmus__find_by_type(unsigned int type)
> >
> > struct perf_pmu *perf_pmus__find_by_type(unsigned int type)
> > {
> > + unsigned int to_read_pmus;
> > struct perf_pmu *pmu = __perf_pmus__find_by_type(type);
> >
> > - if (pmu || read_sysfs_all_pmus)
> > + if (pmu || (read_pmu_types == PERF_TOOL_PMU_TYPE_ALL_MASK))
> > return pmu;
> >
> > - pmu_read_sysfs(/*core_only=*/false);
> > + if (type >= PERF_PMU_TYPE_PE_START && type <= PERF_PMU_TYPE_PE_END) {
> > + to_read_pmus = PERF_TOOL_PMU_TYPE_PE_CORE_MASK |
> > + PERF_TOOL_PMU_TYPE_PE_OTHER_MASK;
> > + } else if (type >= PERF_PMU_TYPE_HWMON_START && type <= PERF_PMU_TYPE_HWMON_END) {
> > + to_read_pmus = PERF_TOOL_PMU_TYPE_HWMON_MASK;
> > + } else {
> > + to_read_pmus = PERF_TOOL_PMU_TYPE_TOOL_MASK;
> > + }
> > + pmu_read_sysfs(to_read_pmus);
> > pmu = __perf_pmus__find_by_type(type);
> > return pmu;
> > }
> > @@ -282,7 +324,7 @@ struct perf_pmu *perf_pmus__scan(struct perf_pmu *pmu)
> > bool use_core_pmus = !pmu || pmu->is_core;
> >
> > if (!pmu) {
> > - pmu_read_sysfs(/*core_only=*/false);
> > + pmu_read_sysfs(PERF_TOOL_PMU_TYPE_ALL_MASK);
> > pmu = list_prepare_entry(pmu, &core_pmus, list);
> > }
> > if (use_core_pmus) {
> > @@ -300,7 +342,7 @@ struct perf_pmu *perf_pmus__scan(struct perf_pmu *pmu)
> > struct perf_pmu *perf_pmus__scan_core(struct perf_pmu *pmu)
> > {
> > if (!pmu) {
> > - pmu_read_sysfs(/*core_only=*/true);
> > + pmu_read_sysfs(PERF_TOOL_PMU_TYPE_PE_CORE_MASK);
> > return list_first_entry_or_null(&core_pmus, typeof(*pmu), list);
> > }
> > list_for_each_entry_continue(pmu, &core_pmus, list)
> > @@ -316,7 +358,7 @@ static struct perf_pmu *perf_pmus__scan_skip_duplicates(struct perf_pmu *pmu)
> > const char *last_pmu_name = (pmu && pmu->name) ? pmu->name : "";
> >
> > if (!pmu) {
> > - pmu_read_sysfs(/*core_only=*/false);
> > + pmu_read_sysfs(PERF_TOOL_PMU_TYPE_ALL_MASK);
> > pmu = list_prepare_entry(pmu, &core_pmus, list);
> > } else
> > last_pmu_name_len = pmu_name_len_no_suffix(pmu->name ?: "");
>