2022-06-01 18:48:28

by Ravi Bangoria

Subject: [PATCH v5 2/8] perf tool: Parse pmu caps sysfs only once

In addition to returning nr_caps, cache it locally in struct perf_pmu.
Similarly, cache status of whether caps sysfs has already been parsed
or not. These will help to avoid parsing sysfs every time the function
gets called.

Signed-off-by: Ravi Bangoria <[email protected]>
---
tools/perf/util/pmu.c | 15 +++++++++++----
tools/perf/util/pmu.h | 2 ++
2 files changed, 13 insertions(+), 4 deletions(-)

diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
index 9a1c7e63e663..0112e1c36418 100644
--- a/tools/perf/util/pmu.c
+++ b/tools/perf/util/pmu.c
@@ -1890,7 +1890,11 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
const char *sysfs = sysfs__mountpoint();
DIR *caps_dir;
struct dirent *evt_ent;
- int nr_caps = 0;
+
+ if (pmu->caps_initialized)
+ return pmu->nr_caps;
+
+ pmu->nr_caps = 0;

if (!sysfs)
return -1;
@@ -1898,8 +1902,10 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
snprintf(caps_path, PATH_MAX,
"%s" EVENT_SOURCE_DEVICE_PATH "%s/caps", sysfs, pmu->name);

- if (stat(caps_path, &st) < 0)
+ if (stat(caps_path, &st) < 0) {
+ pmu->caps_initialized = true;
return 0; /* no error if caps does not exist */
+ }

caps_dir = opendir(caps_path);
if (!caps_dir)
@@ -1926,13 +1932,14 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
continue;
}

- nr_caps++;
+ pmu->nr_caps++;
fclose(file);
}

closedir(caps_dir);

- return nr_caps;
+ pmu->caps_initialized = true;
+ return pmu->nr_caps;
}

void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
index 541889fa9f9c..4b45fd8da5a3 100644
--- a/tools/perf/util/pmu.h
+++ b/tools/perf/util/pmu.h
@@ -46,6 +46,8 @@ struct perf_pmu {
struct perf_cpu_map *cpus;
struct list_head format; /* HEAD struct perf_pmu_format -> list */
struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
+ bool caps_initialized;
+ u32 nr_caps;
struct list_head caps; /* HEAD struct perf_pmu_caps -> list */
struct list_head list; /* ELEM */
struct list_head hybrid_list;
--
2.31.1

2022-06-01 19:14:28

by Ravi Bangoria

Subject: Re: [PATCH v5 2/8] perf tool: Parse pmu caps sysfs only once

Hi Kan,

[...]

>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>> index 541889fa9f9c..4b45fd8da5a3 100644
>> --- a/tools/perf/util/pmu.h
>> +++ b/tools/perf/util/pmu.h
>> @@ -46,6 +46,8 @@ struct perf_pmu {
>>       struct perf_cpu_map *cpus;
>>       struct list_head format;  /* HEAD struct perf_pmu_format -> list */
>>       struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
>> +    bool caps_initialized;
>> +    u32 nr_caps;
>
> If they are just used for the cache purpose, I don't think we need to add the variables in the struct perf_pmu.
>
> A static variable should be good enough. See sysctl__nmi_watchdog_enabled().

These fields are per pmu. Static variable won't help :) And they are
used in subsequent patches as well.

Thanks,
Ravi

2022-06-01 19:32:43

by Liang, Kan

Subject: Re: [PATCH v5 2/8] perf tool: Parse pmu caps sysfs only once

On 5/31/2022 11:26 PM, Ravi Bangoria wrote:
> In addition to returning nr_caps, cache it locally in struct perf_pmu.
> Similarly, cache status of whether caps sysfs has already been parsed
> or not. These will help to avoid parsing sysfs every time the function
> gets called.
>
> Signed-off-by: Ravi Bangoria <[email protected]>
> ---
> tools/perf/util/pmu.c | 15 +++++++++++----
> tools/perf/util/pmu.h | 2 ++
> 2 files changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 9a1c7e63e663..0112e1c36418 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1890,7 +1890,11 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
> const char *sysfs = sysfs__mountpoint();
> DIR *caps_dir;
> struct dirent *evt_ent;
> - int nr_caps = 0;
> +
> + if (pmu->caps_initialized)
> + return pmu->nr_caps;
> +
> + pmu->nr_caps = 0;
>
> if (!sysfs)
> return -1;
> @@ -1898,8 +1902,10 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
> snprintf(caps_path, PATH_MAX,
> "%s" EVENT_SOURCE_DEVICE_PATH "%s/caps", sysfs, pmu->name);
>
> - if (stat(caps_path, &st) < 0)
> + if (stat(caps_path, &st) < 0) {
> + pmu->caps_initialized = true;
> return 0; /* no error if caps does not exist */
> + }
>
> caps_dir = opendir(caps_path);
> if (!caps_dir)
> @@ -1926,13 +1932,14 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
> continue;
> }
>
> - nr_caps++;
> + pmu->nr_caps++;
> fclose(file);
> }
>
> closedir(caps_dir);
>
> - return nr_caps;
> + pmu->caps_initialized = true;
> + return pmu->nr_caps;
> }
>
> void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
> index 541889fa9f9c..4b45fd8da5a3 100644
> --- a/tools/perf/util/pmu.h
> +++ b/tools/perf/util/pmu.h
> @@ -46,6 +46,8 @@ struct perf_pmu {
> struct perf_cpu_map *cpus;
> struct list_head format; /* HEAD struct perf_pmu_format -> list */
> struct list_head aliases; /* HEAD struct perf_pmu_alias -> list */
> + bool caps_initialized;
> + u32 nr_caps;

If they are just used for the cache purpose, I don't think we need to
add the variables in the struct perf_pmu.

A static variable should be good enough. See sysctl__nmi_watchdog_enabled().

Thanks,
Kan

> struct list_head caps; /* HEAD struct perf_pmu_caps -> list */
> struct list_head list; /* ELEM */
> struct list_head hybrid_list;