Re: [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX

[Alexander Antonov]

On 7/6/2021 5:12 PM, Liang, Kan wrote:
> On 7/6/2021 5:07 AM, alexander.antonov@xxxxxxxxxxxxxxx wrote:
> > From: Alexander Antonov <alexander.antonov@xxxxxxxxxxxxxxx>
> >
> > Cleanup mapping procedure for IIO PMU is needed to free memory which was
> > allocated for topology data and for attributes in IIO mapping
> > attribute_group.
> > Current implementation of this procedure for Snowridge and Icelake 
> > Server
> > platforms doesn't free allocated memory that can be a reason for memory
> > leak issue.
> > Fix the issue with IIO cleanup mapping procedure for these platforms
> > to release allocated memory.
> >
> > Fixes: 10337e95e04c ("perf/x86/intel/uncore: Enable I/O stacks to IIO 
> > PMON mapping on ICX")
> >
> > Signed-off-by: Alexander Antonov <alexander.antonov@xxxxxxxxxxxxxxx>
>
> The patch looks good to me.
>
> Reviewed-by: Kan Liang <kan.liang@xxxxxxxxxxxxxxx>
>
>
> With this fix, there will be several similar codes repeat, e.g., 
> XXX_iio_set_mapping() and XXX_iio_cleanup_mapping(), for SKX, ICX, and 
> SNR for now.
> I guess there will be more for the future platforms. Have you 
> considered to add a macro or something to reduce the code repetition?
>
> Thanks,
> Kan

That's a good idea.
I suggest to do it together with enabling of mapping on future 
platforms. What do you think?

Thanks,
Alexander

> > ---
> >   arch/x86/events/intel/uncore_snbep.c | 40 +++++++++++++++++++---------
> >   1 file changed, 28 insertions(+), 12 deletions(-)
> >
> > diff --git a/arch/x86/events/intel/uncore_snbep.c 
> > b/arch/x86/events/intel/uncore_snbep.c
> > index bb6eb1e5569c..54cdbb96e628 100644
> > --- a/arch/x86/events/intel/uncore_snbep.c
> > +++ b/arch/x86/events/intel/uncore_snbep.c
> > @@ -3836,26 +3836,32 @@ pmu_iio_set_mapping(struct intel_uncore_type 
> > *type, struct attribute_group *ag)
> >       return ret;
> >   }
> >   -static int skx_iio_set_mapping(struct intel_uncore_type *type)
> > -{
> > -    return pmu_iio_set_mapping(type, &skx_iio_mapping_group);
> > -}
> > -
> > -static void skx_iio_cleanup_mapping(struct intel_uncore_type *type)
> > +static void
> > +pmu_iio_cleanup_mapping(struct intel_uncore_type *type, struct 
> > attribute_group *ag)
> >   {
> > -    struct attribute **attr = skx_iio_mapping_group.attrs;
> > +    struct attribute **attr = ag->attrs;
> >         if (!attr)
> >           return;
> >         for (; *attr; attr++)
> >           kfree((*attr)->name);
> > -    kfree(attr_to_ext_attr(*skx_iio_mapping_group.attrs));
> > -    kfree(skx_iio_mapping_group.attrs);
> > -    skx_iio_mapping_group.attrs = NULL;
> > +    kfree(attr_to_ext_attr(*ag->attrs));
> > +    kfree(ag->attrs);
> > +    ag->attrs = NULL;
> >       kfree(type->topology);
> >   }
> >   +static int skx_iio_set_mapping(struct intel_uncore_type *type)
> > +{
> > +    return pmu_iio_set_mapping(type, &skx_iio_mapping_group);
> > +}
> > +
> > +static void skx_iio_cleanup_mapping(struct intel_uncore_type *type)
> > +{
> > +    pmu_iio_cleanup_mapping(type, &skx_iio_mapping_group);
> > +}
> > +
> >   static struct intel_uncore_type skx_uncore_iio = {
> >       .name            = "iio",
> >       .num_counters        = 4,
> > @@ -4499,6 +4505,11 @@ static int snr_iio_set_mapping(struct 
> > intel_uncore_type *type)
> >       return pmu_iio_set_mapping(type, &snr_iio_mapping_group);
> >   }
> >   +static void snr_iio_cleanup_mapping(struct intel_uncore_type *type)
> > +{
> > +    pmu_iio_cleanup_mapping(type, &snr_iio_mapping_group);
> > +}
> > +
> >   static struct intel_uncore_type snr_uncore_iio = {
> >       .name            = "iio",
> >       .num_counters        = 4,
> > @@ -4515,7 +4526,7 @@ static struct intel_uncore_type snr_uncore_iio = {
> >       .attr_update        = snr_iio_attr_update,
> >       .get_topology        = snr_iio_get_topology,
> >       .set_mapping        = snr_iio_set_mapping,
> > -    .cleanup_mapping    = skx_iio_cleanup_mapping,
> > +    .cleanup_mapping    = snr_iio_cleanup_mapping,
> >   };
> >     static struct intel_uncore_type snr_uncore_irp = {
> > @@ -5090,6 +5101,11 @@ static int icx_iio_set_mapping(struct 
> > intel_uncore_type *type)
> >       return pmu_iio_set_mapping(type, &icx_iio_mapping_group);
> >   }
> >   +static void icx_iio_cleanup_mapping(struct intel_uncore_type *type)
> > +{
> > +    pmu_iio_cleanup_mapping(type, &icx_iio_mapping_group);
> > +}
> > +
> >   static struct intel_uncore_type icx_uncore_iio = {
> >       .name            = "iio",
> >       .num_counters        = 4,
> > @@ -5107,7 +5123,7 @@ static struct intel_uncore_type icx_uncore_iio = {
> >       .attr_update        = icx_iio_attr_update,
> >       .get_topology        = icx_iio_get_topology,
> >       .set_mapping        = icx_iio_set_mapping,
> > -    .cleanup_mapping    = skx_iio_cleanup_mapping,
> > +    .cleanup_mapping    = icx_iio_cleanup_mapping,
> >   };
> >     static struct intel_uncore_type icx_uncore_irp = {
> >
> > base-commit: 3dbdb38e286903ec220aaf1fb29a8d94297da246