Messages in this thread | | | Subject | Re: [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX | From | "Liang, Kan" <> | Date | Tue, 6 Jul 2021 14:43:30 -0400 |
| |
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@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? >
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 >>>
| |