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?
> +
> + 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;
}
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
> +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
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.
>
>
> 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
On September 1, 2021 9:58:16 PM GMT-03:00, "Jin, Yao" <[email protected]> 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;
Yeah
>
>>> +
>>> + 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.
I've read the other message you sent, so do the constructor/destructor for the right struct, that long named one.
- Arnaldo
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