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

[Liang, Kan]

On 7/6/2021 12:17 PM, Alexander Antonov wrote:
> 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?

It's OK for me.

Thanks.
Kan
> > > ---
> > >   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