Re: [PATCH v3 16/35] perf pmu: Remove perf_pmu__hybrid_mounted

[Liang, Kan]

On 2023-05-26 2:33 p.m., Ian Rogers wrote:
> On Fri, May 26, 2023 at 11:14 AM Liang, Kan <kan.liang@xxxxxxxxxxxxxxx> wrote:
>>
>>
>>
>> On 2023-05-24 6:18 p.m., Ian Rogers wrote:
>>> 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 <irogers@xxxxxxxxxx>
>>> ---
>>>  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 cd94abe7a87a..e9f3e6a777c0 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;
>>> @@ -900,6 +897,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");
>>
>> Can we use the below to replace?
>> return !strncmp(name, "cpu_", 4);
>>
>> Otherwise, anytime a new core PMU name is introduced, I have to patch
>> the function.
> 
> I dislike this function but was carrying it forward, I think we can
> get rid of it. The point of erroring is to not have core PMUs when
> there are no online CPUs associated with it. For existing core PMUs
> this just isn't something that can happen as otherwise what CPU are
> you running on. For hybrid it can happen and we know we care because
> the PMU's type is core. 

For hybrid, I think it can only happen when there is a kernel bug, e.g.,
a new core PMU is added but forgets to set the CPU mask.

> So why not change the error to be when the cpu
> map is empty and the CPU is core?

I don't think PT has cpu map. Other PMUs, e.g., msr, don't have cpu map
either. They are not core PMU.

Actually, I'm OK with just removing the function. Maybe we can add a
test to check the CPU mask on hybrid. If it doesn't exist, it should be
a bug of perf.

Thanks,
Kan

> I'm going to assume that my logic is
> sound and change the code in v4, but please complain.
> 
> Thanks,
> Ian
> 
>> Thanks,
>> Kan
>>
>>> +}
>>> +
>>>  static struct perf_pmu *pmu_lookup(int dirfd, const char *lookup_name)
>>>  {
>>>       struct perf_pmu *pmu;
>>> @@ -907,15 +914,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
>>> @@ -935,8 +935,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;
>>>
>>> @@ -967,7 +969,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);