Messages in this thread Patch in this message | | | Subject | Re: [PATCH v3 1/2] perf pmu: Add PMU alias support | From | Riccardo Mancini <> | Date | Fri, 30 Jul 2021 10:28:30 +0200 |
| |
Hi,
thanks for your quick revision.
There is still one small memory issue with a variable not being freed, which I noticed when running this patchset together with John's patchset.
On Fri, 2021-07-30 at 15:07 +0800, Jin Yao wrote: > From: Kan Liang <kan.liang@linux.intel.com> > > A perf uncore PMU may have two PMU names, a real name and an alias. The > alias is exported at /sys/bus/event_source/devices/uncore_*/alias. > The perf tool should support the alias as well. > > Add alias_name in the struct perf_pmu to store the alias. For the PMU > which doesn't have an alias. It's NULL. > > Introduce two X86 specific functions to retrieve the real name and the > alias separately. > > Only go through the sysfs to retrieve the mapping between the real name > and the alias once. The result is cached in a list, uncore_pmu_list. > > Nothing changed for the other ARCHs. > > With the patch, the perf tool can monitor the PMU with either the real > name or the alias. > > Use the real name, > $ perf stat -e uncore_cha_2/event=1/ -x, > 4044879584,,uncore_cha_2/event=1/,2528059205,100.00,, > > Use the alias, > $ perf stat -e uncore_type_0_2/event=1/ -x, > 3659675336,,uncore_type_0_2/event=1/,2287306455,100.00,, > > Co-developed-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Jin Yao <yao.jin@linux.intel.com> > Signed-off-by: Kan Liang <kan.liang@linux.intel.com> > --- > v3: > - Use fgets to read alias string from sysfs. > - Resource cleanup. > > v2: > - No change. > > tools/perf/arch/x86/util/pmu.c | 129 ++++++++++++++++++++++++++++++++- > tools/perf/util/parse-events.y | 3 +- > tools/perf/util/pmu.c | 26 ++++++- > tools/perf/util/pmu.h | 5 ++ > 4 files changed, 158 insertions(+), 5 deletions(-) > <SNIP> > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c > index 44b90d638ad5..cc9af7942e7b 100644 > --- a/tools/perf/util/pmu.c > +++ b/tools/perf/util/pmu.c > @@ -944,13 +944,28 @@ static int pmu_max_precise(const char *name) > return max_precise; > } > > -static struct perf_pmu *pmu_lookup(const char *name) > +char * __weak > +pmu_find_real_name(const char *name) > +{ > + return strdup(name); > +} > + > +char * __weak > +pmu_find_alias_name(const char *name __maybe_unused) > +{ > + return NULL; > +} > + > +static struct perf_pmu *pmu_lookup(const char *lookup_name) > { > struct perf_pmu *pmu; > + char *name; > LIST_HEAD(format); > LIST_HEAD(aliases); > __u32 type; > > + name = pmu_find_real_name(lookup_name);
name is not freed if one of the following checks fails.
/* * The pmu data we store & need consists of the pmu * type value and format definitions. Load both right * now. */ if (pmu_format(name, &format)) return NULL;
/* * Check the type first to avoid unnecessary work. */ if (pmu_type(name, &type)) return NULL;
if (pmu_aliases(name, &aliases)) return NULL;
Thanks, Riccardo
> @@ -973,7 +988,8 @@ static struct perf_pmu *pmu_lookup(const char *name) > return NULL; > > pmu->cpus = pmu_cpumask(name); > - pmu->name = strdup(name); > + pmu->name = name; > + pmu->alias_name = pmu_find_alias_name(name); > pmu->type = type; > pmu->is_uncore = pmu_is_uncore(name); > if (pmu->is_uncore) > @@ -1003,7 +1019,8 @@ 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)) > + if (!strcmp(pmu->name, name) || > + (pmu->alias_name && !strcmp(pmu->alias_name, name))) > return pmu; > > return NULL; > @@ -1898,6 +1915,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 926da483a141..f6ca9f6a06ef 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; /* PMU alias name */ > char *id; > __u32 type; > bool selectable; > @@ -135,4 +137,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 */
| |