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

[Jin, Yao]

Hi Riccardo,

On 7/30/2021 4:28 PM, Riccardo Mancini wrote:
> 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@xxxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxx>
> > Signed-off-by: Jin Yao <yao.jin@xxxxxxxxxxxxxxx>
> > Signed-off-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
> > ---
> > 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

Thanks so much for pointing this out. I also find I need to rebase the patch because it has some 
conflicts with John's patch. I will prepare v4 (which will be based on tmp.perf/urgent).

Thanks
Jin Yao

> > @@ -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 */