perf_pmu__hybrid_mounted is used to detect whether cpu_core or
cpu_atom 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 <[email protected]>
---
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);
--
2.40.1.606.ga4b1b128d6-goog
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
> 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 <[email protected]>
> ---
> 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);
On Sun, May 21, 2023 at 12:23 PM Liang, Kan <[email protected]> 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.
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 <[email protected]>
> > ---
> > 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);
On 2023-05-22 1:21 a.m., Ian Rogers wrote:
> On Sun, May 21, 2023 at 12:23 PM Liang, Kan <[email protected]> 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 <[email protected]>
>>> ---
>>> 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);
On Mon, May 22, 2023 at 4:55 AM Liang, Kan <[email protected]> wrote:
>
>
>
> On 2023-05-22 1:21 a.m., Ian Rogers wrote:
> > On Sun, May 21, 2023 at 12:23 PM Liang, Kan <[email protected]> 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
I don't disagree, but fixing all uses of is_pmu_hybrid and similarly
perf_pmus__has_hybrid is going to add yet more to a moderately long
patch series. I think in most cases is_pmu_hybrid can be replaced by a
core oriented alternative. For example, in pmu.c there is
perf_pmu__auto_merge_stats that normally returns true that we want to
merge counts for uncore or non-hybrid PMUs. For hybrid it returns
false so that cpu_atom and cpu_core counts aren't merged. A core
oriented alternative would be to return false if the PMU is core and
the number of core PMUs is >1 - this also avoids any hard coding of
PMU names and assuming >1 core PMU means they all begin with "cpu_".
The scope of fixing the remaining is_pmu_hybrid and perf_pmus__has_hybrid is:
```
$ grep -rn perf_pmus__has_hybrid tools/perf
tools/perf/util/header.c:1592: if (perf_pmus__has_hybrid()) {
tools/perf/util/mem-events.c:132: if (!perf_pmus__has_hybrid()) {
tools/perf/util/mem-events.c:199: if (!perf_pmus__has_hybrid()) {
tools/perf/util/evsel.c:3139: if (!perf_pmus__has_hybrid())
tools/perf/util/pmus.h:21:bool perf_pmus__has_hybrid(void);
tools/perf/util/stat-display.c:684: if (!perf_pmus__has_hybrid())
tools/perf/util/metricgroup.c:277: bool all_pmus = !strcmp(pmu,
"all") || !perf_pmus__has_hybrid() || !is_pmu_hybrid(pmu);
tools/perf/util/pmus.c:474:bool perf_pmus__has_hybrid(void)
tools/perf/util/cputopo.c:477: if (!perf_pmus__has_hybrid())
tools/perf/tests/attr.c:188: if (perf_pmus__has_hybrid())
tools/perf/tests/topology.c:44: if (!perf_pmus__has_hybrid()) {
tools/perf/tests/parse-metric.c:306: if (!perf_pmus__has_hybrid()) {
tools/perf/tests/switch-tracking.c:378: if (perf_pmus__has_hybrid()) {
tools/perf/builtin-record.c:1297: perf_pmus__has_hybrid()) {
tools/perf/builtin-record.c:2196: if (!perf_pmus__has_hybrid())
tools/perf/builtin-record.c:4196: rec->opts.target.hybrid =
perf_pmus__has_hybrid();
tools/perf/builtin-stat.c:2463: target.hybrid = perf_pmus__has_hybrid();
tools/perf/arch/x86/util/perf_regs.c:295: if (perf_pmus__has_hybrid()) {
tools/perf/arch/x86/util/evlist.c:21: if (!perf_pmus__has_hybrid())
tools/perf/arch/x86/tests/hybrid.c:284: if (!perf_pmus__has_hybrid())
$ grep -rn is_pmu_hybrid tools/perf
tools/perf/util/pmu.c:1433:bool is_pmu_hybrid(const char *name)
tools/perf/util/pmu.c:1445: return !is_pmu_hybrid(pmu->name);
tools/perf/util/pmu.h:224:bool is_pmu_hybrid(const char *name);
tools/perf/util/metricgroup.c:277: bool all_pmus = !strcmp(pmu,
"all") || !perf_pmus__
has_hybrid() || !is_pmu_hybrid(pmu);
tools/perf/util/pmus.c:482: if (is_pmu_hybrid(pmu->name)) {
```
So, I think it makes sense to do it as a follow up.
Thanks,
Ian
> >
> > 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 <[email protected]>
> >>> ---
> >>> 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);
On 2023-05-22 10:06 a.m., Ian Rogers wrote:
> On Mon, May 22, 2023 at 4:55 AM Liang, Kan <[email protected]> wrote:
>>
>>
>>
>> On 2023-05-22 1:21 a.m., Ian Rogers wrote:
>>> On Sun, May 21, 2023 at 12:23 PM Liang, Kan <[email protected]> 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
>
> I don't disagree, but fixing all uses of is_pmu_hybrid and similarly
> perf_pmus__has_hybrid is going to add yet more to a moderately long
> patch series. I think in most cases is_pmu_hybrid can be replaced by a
> core oriented alternative. For example, in pmu.c there is
> perf_pmu__auto_merge_stats that normally returns true that we want to
> merge counts for uncore or non-hybrid PMUs. For hybrid it returns
> false so that cpu_atom and cpu_core counts aren't merged. A core
> oriented alternative would be to return false if the PMU is core and
> the number of core PMUs is >1 - this also avoids any hard coding of
> PMU names and assuming >1 core PMU means they all begin with "cpu_".
I'm OK with the alternative method.
>
> The scope of fixing the remaining is_pmu_hybrid and perf_pmus__has_hybrid is:
I don't think we need to modify each places listed below.
We just need to update is_pmu_hybrid() and perf_pmus__has_hybrid(). And
probably use is_pmu_hybrid() to replace perf_pmu__skip_empty_cpus().
I don't think we need to change the interface.
It should not be a big patch.
Is there anything I missed?
Thanks,
Kan
> ```
> $ grep -rn perf_pmus__has_hybrid tools/perf
> tools/perf/util/header.c:1592: if (perf_pmus__has_hybrid()) {
> tools/perf/util/mem-events.c:132: if (!perf_pmus__has_hybrid()) {
> tools/perf/util/mem-events.c:199: if (!perf_pmus__has_hybrid()) {
> tools/perf/util/evsel.c:3139: if (!perf_pmus__has_hybrid())
> tools/perf/util/pmus.h:21:bool perf_pmus__has_hybrid(void);
> tools/perf/util/stat-display.c:684: if (!perf_pmus__has_hybrid())
> tools/perf/util/metricgroup.c:277: bool all_pmus = !strcmp(pmu,
> "all") || !perf_pmus__has_hybrid() || !is_pmu_hybrid(pmu);
> tools/perf/util/pmus.c:474:bool perf_pmus__has_hybrid(void)
> tools/perf/util/cputopo.c:477: if (!perf_pmus__has_hybrid())
> tools/perf/tests/attr.c:188: if (perf_pmus__has_hybrid())
> tools/perf/tests/topology.c:44: if (!perf_pmus__has_hybrid()) {
> tools/perf/tests/parse-metric.c:306: if (!perf_pmus__has_hybrid()) {
> tools/perf/tests/switch-tracking.c:378: if (perf_pmus__has_hybrid()) {
> tools/perf/builtin-record.c:1297: perf_pmus__has_hybrid()) {
> tools/perf/builtin-record.c:2196: if (!perf_pmus__has_hybrid())
> tools/perf/builtin-record.c:4196: rec->opts.target.hybrid =
> perf_pmus__has_hybrid();
> tools/perf/builtin-stat.c:2463: target.hybrid = perf_pmus__has_hybrid();
> tools/perf/arch/x86/util/perf_regs.c:295: if (perf_pmus__has_hybrid()) {
> tools/perf/arch/x86/util/evlist.c:21: if (!perf_pmus__has_hybrid())
> tools/perf/arch/x86/tests/hybrid.c:284: if (!perf_pmus__has_hybrid())
>
> $ grep -rn is_pmu_hybrid tools/perf
> tools/perf/util/pmu.c:1433:bool is_pmu_hybrid(const char *name)
> tools/perf/util/pmu.c:1445: return !is_pmu_hybrid(pmu->name);
> tools/perf/util/pmu.h:224:bool is_pmu_hybrid(const char *name);
> tools/perf/util/metricgroup.c:277: bool all_pmus = !strcmp(pmu,
> "all") || !perf_pmus__
> has_hybrid() || !is_pmu_hybrid(pmu);
> tools/perf/util/pmus.c:482: if (is_pmu_hybrid(pmu->name)) {
> ```
>
> So, I think it makes sense to do it as a follow up.
>
> Thanks,
> Ian
>
>>>
>>> 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 <[email protected]>
>>>>> ---
>>>>> 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);
On Tue, May 23, 2023 at 10:23 AM Liang, Kan <[email protected]> wrote:
>
>
>
> On 2023-05-22 10:06 a.m., Ian Rogers wrote:
> > On Mon, May 22, 2023 at 4:55 AM Liang, Kan <[email protected]> wrote:
> >>
> >>
> >>
> >> On 2023-05-22 1:21 a.m., Ian Rogers wrote:
> >>> On Sun, May 21, 2023 at 12:23 PM Liang, Kan <[email protected]> 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
> >
> > I don't disagree, but fixing all uses of is_pmu_hybrid and similarly
> > perf_pmus__has_hybrid is going to add yet more to a moderately long
> > patch series. I think in most cases is_pmu_hybrid can be replaced by a
> > core oriented alternative. For example, in pmu.c there is
> > perf_pmu__auto_merge_stats that normally returns true that we want to
> > merge counts for uncore or non-hybrid PMUs. For hybrid it returns
> > false so that cpu_atom and cpu_core counts aren't merged. A core
> > oriented alternative would be to return false if the PMU is core and
> > the number of core PMUs is >1 - this also avoids any hard coding of
> > PMU names and assuming >1 core PMU means they all begin with "cpu_".
>
> I'm OK with the alternative method.
>
> >
> > The scope of fixing the remaining is_pmu_hybrid and perf_pmus__has_hybrid is:
> I don't think we need to modify each places listed below.
> We just need to update is_pmu_hybrid() and perf_pmus__has_hybrid(). And
> probably use is_pmu_hybrid() to replace perf_pmu__skip_empty_cpus().
> I don't think we need to change the interface.
>
> It should not be a big patch.
>
> Is there anything I missed?
>
> Thanks,
> Kan
Okay, I'll do the longer patch. I'm not keen on an arch component as
it can be tricky to hook it up, weak functions, etc. I'll do it case
by case. I don't think we should get overly caught up on individual
patches in a series, here:
..
- bool is_hybrid = perf_pmu__hybrid_mounted(name);
..
- if (is_hybrid)
+ if (!strcmp(name, "cpu_core") || !strcmp(name, "cpu_atom"))
..
Yes, it hard codes the two hybrid PMU names but this code is deleted
in patch 16:
https://lore.kernel.org/lkml/[email protected]/
where the hybrid list is removed. There is something equivalent to the
hybrid list with perf_pmus__scan_core, and using this throughout the
code base will mean we have consistent code between hybrid and
non-hybrid architectures which I'm glad we're all agreed upon being
better - a nice win is that by scanning fewer PMUs for wildcards we
can improve start up time.
Thanks,
Ian
> > ```
> > $ grep -rn perf_pmus__has_hybrid tools/perf
> > tools/perf/util/header.c:1592: if (perf_pmus__has_hybrid()) {
> > tools/perf/util/mem-events.c:132: if (!perf_pmus__has_hybrid()) {
> > tools/perf/util/mem-events.c:199: if (!perf_pmus__has_hybrid()) {
> > tools/perf/util/evsel.c:3139: if (!perf_pmus__has_hybrid())
> > tools/perf/util/pmus.h:21:bool perf_pmus__has_hybrid(void);
> > tools/perf/util/stat-display.c:684: if (!perf_pmus__has_hybrid())
> > tools/perf/util/metricgroup.c:277: bool all_pmus = !strcmp(pmu,
> > "all") || !perf_pmus__has_hybrid() || !is_pmu_hybrid(pmu);
> > tools/perf/util/pmus.c:474:bool perf_pmus__has_hybrid(void)
> > tools/perf/util/cputopo.c:477: if (!perf_pmus__has_hybrid())
> > tools/perf/tests/attr.c:188: if (perf_pmus__has_hybrid())
> > tools/perf/tests/topology.c:44: if (!perf_pmus__has_hybrid()) {
> > tools/perf/tests/parse-metric.c:306: if (!perf_pmus__has_hybrid()) {
> > tools/perf/tests/switch-tracking.c:378: if (perf_pmus__has_hybrid()) {
> > tools/perf/builtin-record.c:1297: perf_pmus__has_hybrid()) {
> > tools/perf/builtin-record.c:2196: if (!perf_pmus__has_hybrid())
> > tools/perf/builtin-record.c:4196: rec->opts.target.hybrid =
> > perf_pmus__has_hybrid();
> > tools/perf/builtin-stat.c:2463: target.hybrid = perf_pmus__has_hybrid();
> > tools/perf/arch/x86/util/perf_regs.c:295: if (perf_pmus__has_hybrid()) {
> > tools/perf/arch/x86/util/evlist.c:21: if (!perf_pmus__has_hybrid())
> > tools/perf/arch/x86/tests/hybrid.c:284: if (!perf_pmus__has_hybrid())
> >
> > $ grep -rn is_pmu_hybrid tools/perf
> > tools/perf/util/pmu.c:1433:bool is_pmu_hybrid(const char *name)
> > tools/perf/util/pmu.c:1445: return !is_pmu_hybrid(pmu->name);
> > tools/perf/util/pmu.h:224:bool is_pmu_hybrid(const char *name);
> > tools/perf/util/metricgroup.c:277: bool all_pmus = !strcmp(pmu,
> > "all") || !perf_pmus__
> > has_hybrid() || !is_pmu_hybrid(pmu);
> > tools/perf/util/pmus.c:482: if (is_pmu_hybrid(pmu->name)) {
> > ```
> >
> > So, I think it makes sense to do it as a follow up.
> >
> > Thanks,
> > Ian
> >
> >>>
> >>> 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 <[email protected]>
> >>>>> ---
> >>>>> 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);