Re: [PATCH v6 1/2] perf pmu: Add PMU alias support

["Jin, Yao" <yao.jin@linux.intel.com>]

Hi Arnaldo,

On 9/2/2021 8:58 AM, Jin, Yao wrote:
> Hi Arnaldo,
> 
> On 9/1/2021 9:57 PM, Arnaldo Carvalho de Melo wrote:
>> Em Wed, Sep 01, 2021 at 01:46:01PM +0800, Jin Yao escreveu:
>> <SNIP>
>>
>>> +++ b/tools/perf/arch/x86/util/pmu.c
>>
>> <SNIP>
>>
>>> +static int setup_pmu_alias_list(void)
>>> +{
>>> +    char path[PATH_MAX];
>>> +    DIR *dir;
>>> +    struct dirent *dent;
>>> +    const char *sysfs = sysfs__mountpoint();
>>> +    struct perf_pmu_alias_name *pmu;
>>> +    char buf[MAX_PMU_NAME_LEN];
>>> +    FILE *file;
>>> +    int ret = 0;
>>> +
>>> +    if (!sysfs)
>>> +        return -1;
>>> +
>>> +    snprintf(path, PATH_MAX,
>>> +         "%s" EVENT_SOURCE_DEVICE_PATH, sysfs);
>>> +
>>> +    dir = opendir(path);
>>> +    if (!dir)
>>> +        return -1;
>>> +
>>> +    while ((dent = readdir(dir))) {
>>> +        if (!strcmp(dent->d_name, ".") ||
>>> +            !strcmp(dent->d_name, ".."))
>>> +            continue;
>>> +
>>> +        snprintf(path, PATH_MAX,
>>> +             TEMPLATE_ALIAS, sysfs, dent->d_name);
>>> +
>>> +        if (!file_available(path))
>>> +            continue;
>>> +
>>> +        file = fopen(path, "r");
>>> +        if (!file)
>>> +            continue;
>>> +
>>> +        if (!fgets(buf, sizeof(buf), file)) {
>>> +            fclose(file);
>>> +            continue;
>>> +        }
>>> +
>>> +        fclose(file);
>>> +
>>> +        pmu = zalloc(sizeof(*pmu));
>>> +        if (!pmu) {
>>> +            ret = -ENOMEM;
>>> +            break;
>>> +        }
>>> +
>>> +        /* Remove the last '\n' */
>>> +        buf[strlen(buf) - 1] = 0;
>>> +
>>> +        pmu->alias = strdup(buf);
>>> +        if (!pmu->alias)
>>> +            goto mem_err;
>>
>> This isn't returning -ENOMEM like when zalloc() fails above. Also you're
>> mixing 'return -1' with 'return -ENOMEM', please be consistent. Please
>> find some -E errno for the !sysfs case, perhaps -ENODEV?
>>
> 
> For opendir() error, can we just return -errno?
> 
> dir = opendir(path);
> if (!dir)
>      return -errno;
> 
>>> +
>>> +        pmu->name = strdup(dent->d_name);
>>> +        if (!pmu->name)
>>> +            goto mem_err;
>>> +
>>> +        list_add_tail(&pmu->list, &pmu_alias_name_list);
>>> +        continue;
>>
>>
>> Don't we have a 'struct pmu' constructor/destructor pair? I.e. instead
>> of doing all this in an open coded way as above, why not have:
>>
>> void pmu__delete(struct pmu *pmu)
>> {
>>     if (!pmu)
>>         return;
>>
>>     zfree(&pmu->name);
>>     zfree(&pmu->alias);
>>     free(pmu);
>> }
>>
>> struct pmu *pmu__new(const char *name, const char *alias)
>> {
>>     struct pmu *pmu = zalloc(sizeof(*pmu));
>>
>>     if (pmu) {
>>         pmu->name = strdup(name);
>>         if (!pmu->name)
>>             goto out_delete;
>>
>>         pmu->alias = strdup(alias);
>>         if (!pmu->alias)
>>             goto out_delete;
>>     }
>>
>>     return pmu;
>> out_err:
>>     pmu__delete(pmu);
>>     return NULL;
>> }
>>
>>     And then just:
>>
>>     pmu = pmu__new(dent->d_name, buf);
>>     if (!pmu)
>>         goto out_closedir;
>>
>>     list_add_tail(&pmu->list, &pmu_alias_name_list);
>>
>> And then you don't need the 'continue', as this is the end of the loop
>> block.
>>
>> That 'ret' probably should start with -ENOMEM and you end the function
>> with:
>>
>>     ret = 0;
>> out_closedir:
>>     closedir(dir);
>>     return ret;
>> }
> 
> Yes, using 'struct pmu' constructor/destructor is absolutely a good design.
> 
> I will follow this approach.
> 

Strictly, it's not 'struct pmu' constructor/destructor. It's actually the 'struct 
perf_pmu_alias_name' constructor/destructor.

We are both confused by the variable name 'pmu'. :(

Thanks
Jin Yao

>>
>>
>> The pmu__delete() should be used in perf's shutdown when running it
>> using memory leak detectors, when not using it, its ok to just leave it
>> as is to speed up the shutdown.
>>
>> - Arnaldo
>>
> 
> Yes, agree. I will create a separate patch to implement the 'struct pmu' constructor/destructor.
> 
> Thanks
> Jin Yao
> 
>>> +mem_err:
>>> +        free(pmu->alias);
>>> +        free(pmu->name);
>>> +        free(pmu);
>>> +        break;
>>> +    }
>>> +
>>> +    closedir(dir);
>>> +    return ret;
>>> +}
>>> +
>>> +static char *__pmu_find_real_name(const char *name)
>>> +{
>>> +    struct perf_pmu_alias_name *pmu;
>>> +
>>> +    list_for_each_entry(pmu, &pmu_alias_name_list, list) {
>>> +        if (!strcmp(name, pmu->alias))
>>> +            return pmu->name;
>>> +    }
>>> +
>>> +    return (char *)name;
>>> +}
>>> +
>>> +char *pmu_find_real_name(const char *name)
>>> +{
>>> +    if (cached_list)
>>> +        return __pmu_find_real_name(name);
>>> +
>>> +    setup_pmu_alias_list();
>>> +    cached_list = true;
>>> +
>>> +    return __pmu_find_real_name(name);
>>> +}
>>> +
>>> +static char *__pmu_find_alias_name(const char *name)
>>> +{
>>> +    struct perf_pmu_alias_name *pmu;
>>> +
>>> +    list_for_each_entry(pmu, &pmu_alias_name_list, list) {
>>> +        if (!strcmp(name, pmu->name))
>>> +            return pmu->alias;
>>> +    }
>>> +    return NULL;
>>> +}
>>> +
>>> +char *pmu_find_alias_name(const char *name)
>>> +{
>>> +    if (cached_list)
>>> +        return __pmu_find_alias_name(name);
>>> +
>>> +    setup_pmu_alias_list();
>>> +    cached_list = true;
>>> +
>>> +    return __pmu_find_alias_name(name);
>>> +}
>>> diff --git a/tools/perf/util/parse-events.y b/tools/perf/util/parse-events.y
>>> index 9321bd0e2f76..d94e48e1ff9b 100644
>>> --- a/tools/perf/util/parse-events.y
>>> +++ b/tools/perf/util/parse-events.y
>>> @@ -316,7 +316,8 @@ event_pmu_name opt_pmu_config
>>>               if (!strncmp(name, "uncore_", 7) &&
>>>                   strncmp($1, "uncore_", 7))
>>>                   name += 7;
>>> -            if (!perf_pmu__match(pattern, name, $1)) {
>>> +            if (!perf_pmu__match(pattern, name, $1) ||
>>> +                !perf_pmu__match(pattern, pmu->alias_name, $1)) {
>>>                   if (parse_events_copy_term_list(orig_terms, &terms))
>>>                       CLEANUP_YYABORT;
>>>                   if (!parse_events_add_pmu(_parse_state, list, pmu->name, terms, true, false))
>>> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
>>> index 6cdbee8a12e7..1a35915edf68 100644
>>> --- a/tools/perf/util/pmu.c
>>> +++ b/tools/perf/util/pmu.c
>>> @@ -945,6 +945,18 @@ perf_pmu__get_default_config(struct perf_pmu *pmu __maybe_unused)
>>>       return NULL;
>>>   }
>>> +char * __weak
>>> +pmu_find_real_name(const char *name)
>>> +{
>>> +    return (char *)name;
>>> +}
>>> +
>>> +char * __weak
>>> +pmu_find_alias_name(const char *name __maybe_unused)
>>> +{
>>> +    return NULL;
>>> +}
>>> +
>>>   static int pmu_max_precise(const char *name)
>>>   {
>>>       char path[PATH_MAX];
>>> @@ -958,13 +970,15 @@ static int pmu_max_precise(const char *name)
>>>       return max_precise;
>>>   }
>>> -static struct perf_pmu *pmu_lookup(const char *name)
>>> +static struct perf_pmu *pmu_lookup(const char *lookup_name)
>>>   {
>>>       struct perf_pmu *pmu;
>>>       LIST_HEAD(format);
>>>       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
>>> @@ -995,6 +1009,16 @@ static struct perf_pmu *pmu_lookup(const char *name)
>>>       pmu->cpus = pmu_cpumask(name);
>>>       pmu->name = strdup(name);
>>> +    if (!pmu->name)
>>> +        goto err;
>>> +
>>> +    alias_name = pmu_find_alias_name(name);
>>> +    if (alias_name) {
>>> +        pmu->alias_name = strdup(alias_name);
>>> +        if (!pmu->alias_name)
>>> +            goto err;
>>> +    }
>>> +
>>>       pmu->type = type;
>>>       pmu->is_uncore = pmu_is_uncore(name);
>>>       if (pmu->is_uncore)
>>> @@ -1017,15 +1041,22 @@ static struct perf_pmu *pmu_lookup(const char *name)
>>>       pmu->default_config = perf_pmu__get_default_config(pmu);
>>>       return pmu;
>>> +err:
>>> +    if (pmu->name)
>>> +        free(pmu->name);
>>> +    free(pmu);
>>> +    return NULL;
>>>   }
>>>   static struct perf_pmu *pmu_find(const char *name)
>>>   {
>>>       struct perf_pmu *pmu;
>>> -    list_for_each_entry(pmu, &pmus, list)
>>> -        if (!strcmp(pmu->name, name))
>>> +    list_for_each_entry(pmu, &pmus, list) {
>>> +        if (!strcmp(pmu->name, name) ||
>>> +            (pmu->alias_name && !strcmp(pmu->alias_name, name)))
>>>               return pmu;
>>> +    }
>>>       return NULL;
>>>   }
>>> @@ -1919,6 +1950,9 @@ bool perf_pmu__has_hybrid(void)
>>>   int perf_pmu__match(char *pattern, char *name, char *tok)
>>>   {
>>> +    if (!name)
>>> +        return -1;
>>> +
>>>       if (fnmatch(pattern, name, 0))
>>>           return -1;
>>> diff --git a/tools/perf/util/pmu.h b/tools/perf/util/pmu.h
>>> index 033e8211c025..6b122f97acf3 100644
>>> --- a/tools/perf/util/pmu.h
>>> +++ b/tools/perf/util/pmu.h
>>> @@ -21,6 +21,7 @@ enum {
>>>   #define PERF_PMU_FORMAT_BITS 64
>>>   #define EVENT_SOURCE_DEVICE_PATH "/bus/event_source/devices/"
>>>   #define CPUS_TEMPLATE_CPU    "%s/bus/event_source/devices/%s/cpus"
>>> +#define MAX_PMU_NAME_LEN 128
>>>   struct perf_event_attr;
>>> @@ -32,6 +33,7 @@ struct perf_pmu_caps {
>>>   struct perf_pmu {
>>>       char *name;
>>> +    char *alias_name;
>>>       char *id;
>>>       __u32 type;
>>>       bool selectable;
>>> @@ -136,4 +138,7 @@ void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
>>>   bool perf_pmu__has_hybrid(void);
>>>   int perf_pmu__match(char *pattern, char *name, char *tok);
>>> +char *pmu_find_real_name(const char *name);
>>> +char *pmu_find_alias_name(const char *name);
>>> +
>>>   #endif /* __PMU_H */
>>> -- 
>>> 2.17.1