| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <298a4efe-72f3-f99a-3923-939eab44885a@linux.intel.com>
Date: Thu, 2 Sep 2021 08:58:16 +0800
From: "Jin, Yao" <yao.jin@...ux.intel.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: jolsa@...nel.org, peterz@...radead.org, mingo@...hat.com,
alexander.shishkin@...ux.intel.com, Linux-kernel@...r.kernel.org,
linux-perf-users@...r.kernel.org, ak@...ux.intel.com,
kan.liang@...el.com, yao.jin@...el.com, rickyman7@...il.com,
john.garry@...wei.com, Kan Liang <kan.liang@...ux.intel.com>
Subject: Re: [PATCH v6 1/2] perf pmu: Add PMU alias support
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
Powered by blists - more mailing lists