Messages in this thread | | | Subject | Re: [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX | From | Alexander Antonov <> | Date | Tue, 6 Jul 2021 19:17:36 +0300 |
| |
On 7/6/2021 5:12 PM, Liang, Kan wrote: > > > On 7/6/2021 5:07 AM, alexander.antonov@linux.intel.com wrote: >> From: Alexander Antonov <alexander.antonov@linux.intel.com> >> >> 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@linux.intel.com> > > The patch looks good to me. > > Reviewed-by: Kan Liang <kan.liang@linux.intel.com> > > > 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 >>
| |