Messages in this thread | | | Date | Mon, 22 May 2023 07:55:27 -0400 | Subject | Re: [PATCH v1 05/23] perf pmu: Remove perf_pmu__hybrid_mounted | From | "Liang, Kan" <> |
| |
On 2023-05-22 1:21 a.m., Ian Rogers wrote: > On Sun, May 21, 2023 at 12:23 PM Liang, Kan <kan.liang@linux.intel.com> wrote: >> >> >> >> On 2023-05-17 10:57 a.m., Ian Rogers wrote: >>> perf_pmu__hybrid_mounted is used to detect whether cpu_core or >>> cpu_atom >> >> Currently, there are only two CPU types for a hybrid machine, core and >> atom. But there may be more CPU types added later. Please see the CPUID >> 1AH EAX enumeration in SDM VOL2. It has several reserved encodings for >> CPU types. It's better not using the hardcode cpu_core/cpu_atom to >> replace the perf_pmu__hybrid_mounted(). >> >> Thanks, >> Kan > > This is covered by later patches. Specifically patches like: > patch 7: perf pmu: Add is_core to pmu > patch 20: Split pmus list into core and uncore > > Ultimately in pmus.h we have two scan routines, one for all PMUs and > one for core PMUs. For everything except hybrid (and the name varies a > bit on ARM) the core scan only scans "cpu", on hybrid it scans > "cpu_atom" and "cpu_core". The determination of core vs uncore is > done without using the name, so can support >2 hybrid PMUs. At this > point in the patch series I'm trying to simplify everything so that I > can then build the pmus interface.
But if we add a new core type "cpu_whatever" later, we have to hardcode the new name to the perf tool, right? Users have to update the perf tool for the new platforms, otherwise I think the new type will be treated as an uncore PMU.
Since the hybrid is Intel only, I think it may be better move the is_pmu_hybrid() to X86 specifc code. For the Intel only code, we already have a naming rule for the hybrid name, "cpu_$". So we don't need to update the tool for every new CPU type.
Thanks, Kan
> > Thanks, > Ian > >>> is mounted with a non-empty cpus file by >>> pmu_lookup. pmu_lookup will attempt to read the cpus file too and so >>> the check can be folded into this. >>> >>> Checking hybrid_mounted in pmu_is_uncore is redundant as the next >>> cpumask read will fail returning false. >>> >>> Reduce the scope of perf_pmu__find_hybrid_pmu by making it static. >>> >>> Signed-off-by: Ian Rogers <irogers@google.com> >>> --- >>> tools/perf/util/pmu-hybrid.c | 15 +-------------- >>> tools/perf/util/pmu-hybrid.h | 3 --- >>> tools/perf/util/pmu.c | 26 ++++++++++++++------------ >>> 3 files changed, 15 insertions(+), 29 deletions(-) >>> >>> diff --git a/tools/perf/util/pmu-hybrid.c b/tools/perf/util/pmu-hybrid.c >>> index bc4cb0738c35..7fe943dd3217 100644 >>> --- a/tools/perf/util/pmu-hybrid.c >>> +++ b/tools/perf/util/pmu-hybrid.c >>> @@ -18,20 +18,7 @@ >>> >>> LIST_HEAD(perf_pmu__hybrid_pmus); >>> >>> -bool perf_pmu__hybrid_mounted(const char *name) >>> -{ >>> - int cpu; >>> - char pmu_name[PATH_MAX]; >>> - struct perf_pmu pmu = {.name = pmu_name}; >>> - >>> - if (strncmp(name, "cpu_", 4)) >>> - return false; >>> - >>> - strlcpy(pmu_name, name, sizeof(pmu_name)); >>> - return perf_pmu__scan_file(&pmu, "cpus", "%u", &cpu) > 0; >>> -} >>> - >>> -struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name) >>> +static struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name) >>> { >>> struct perf_pmu *pmu; >>> >>> diff --git a/tools/perf/util/pmu-hybrid.h b/tools/perf/util/pmu-hybrid.h >>> index 206b94931531..8dbcae935020 100644 >>> --- a/tools/perf/util/pmu-hybrid.h >>> +++ b/tools/perf/util/pmu-hybrid.h >>> @@ -13,9 +13,6 @@ extern struct list_head perf_pmu__hybrid_pmus; >>> #define perf_pmu__for_each_hybrid_pmu(pmu) \ >>> list_for_each_entry(pmu, &perf_pmu__hybrid_pmus, hybrid_list) >>> >>> -bool perf_pmu__hybrid_mounted(const char *name); >>> - >>> -struct perf_pmu *perf_pmu__find_hybrid_pmu(const char *name); >>> bool perf_pmu__is_hybrid(const char *name); >>> >>> static inline int perf_pmu__hybrid_pmu_num(void) >>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c >>> index 1e0be23d4dd7..729b1f166f80 100644 >>> --- a/tools/perf/util/pmu.c >>> +++ b/tools/perf/util/pmu.c >>> @@ -617,9 +617,6 @@ static bool pmu_is_uncore(int dirfd, const char *name) >>> { >>> int fd; >>> >>> - if (perf_pmu__hybrid_mounted(name)) >>> - return false; >>> - >>> fd = perf_pmu__pathname_fd(dirfd, name, "cpumask", O_PATH); >>> if (fd < 0) >>> return false; >>> @@ -898,6 +895,16 @@ static int pmu_max_precise(int dirfd, struct perf_pmu *pmu) >>> return max_precise; >>> } >>> >>> +/** >>> + * perf_pmu__skip_empty_cpus() - should pmu_lookup skip the named PMU if the >>> + * cpus or cpumask file isn't present? >>> + * @name: Name of PMU. >>> + */ >>> +static bool perf_pmu__skip_empty_cpus(const char *name) >>> +{ >>> + return !strcmp(name, "cpu_core") || !strcmp(name, "cpu_atom"); >>> +} >>> + >>> static struct perf_pmu *pmu_lookup(int dirfd, const char *lookup_name) >>> { >>> struct perf_pmu *pmu; >>> @@ -905,15 +912,8 @@ static struct perf_pmu *pmu_lookup(int dirfd, const char *lookup_name) >>> LIST_HEAD(aliases); >>> __u32 type; >>> char *name = pmu_find_real_name(lookup_name); >>> - bool is_hybrid = perf_pmu__hybrid_mounted(name); >>> char *alias_name; >>> >>> - /* >>> - * Check pmu name for hybrid and the pmu may be invalid in sysfs >>> - */ >>> - if (!strncmp(name, "cpu_", 4) && !is_hybrid) >>> - return NULL; >>> - >>> /* >>> * The pmu data we store & need consists of the pmu >>> * type value and format definitions. Load both right >>> @@ -933,8 +933,10 @@ static struct perf_pmu *pmu_lookup(int dirfd, const char *lookup_name) >>> return NULL; >>> >>> pmu->cpus = pmu_cpumask(dirfd, name); >>> - pmu->name = strdup(name); >>> + if (!pmu->cpus && perf_pmu__skip_empty_cpus(name)) >>> + goto err; >>> >>> + pmu->name = strdup(name); >>> if (!pmu->name) >>> goto err; >>> >>> @@ -964,7 +966,7 @@ static struct perf_pmu *pmu_lookup(int dirfd, const char *lookup_name) >>> list_splice(&aliases, &pmu->aliases); >>> list_add_tail(&pmu->list, &pmus); >>> >>> - if (is_hybrid) >>> + if (!strcmp(name, "cpu_core") || !strcmp(name, "cpu_atom")) >>> list_add_tail(&pmu->hybrid_list, &perf_pmu__hybrid_pmus); >>> else >>> INIT_LIST_HEAD(&pmu->hybrid_list);
| |