* [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX
@ 2021-07-06 9:07 alexander.antonov
2021-07-06 14:12 ` Liang, Kan
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: alexander.antonov @ 2021-07-06 9:07 UTC (permalink / raw)
To: peterz, linux-kernel, x86
Cc: kan.liang, ak, stable, alexander.antonov, alexey.v.bayduraev
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>
---
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
--
2.21.3
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX
2021-07-06 9:07 [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX alexander.antonov
@ 2021-07-06 14:12 ` Liang, Kan
2021-07-06 16:17 ` Alexander Antonov
2021-07-07 11:44 ` Peter Zijlstra
2021-07-06 14:50 ` Greg KH
2021-07-27 13:58 ` [tip: perf/core] " tip-bot2 for Alexander Antonov
2 siblings, 2 replies; 7+ messages in thread
From: Liang, Kan @ 2021-07-06 14:12 UTC (permalink / raw)
To: alexander.antonov, peterz, linux-kernel, x86
Cc: ak, stable, alexey.v.bayduraev
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
> ---
> 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
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX
2021-07-06 9:07 [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX alexander.antonov
2021-07-06 14:12 ` Liang, Kan
@ 2021-07-06 14:50 ` Greg KH
2021-07-27 13:58 ` [tip: perf/core] " tip-bot2 for Alexander Antonov
2 siblings, 0 replies; 7+ messages in thread
From: Greg KH @ 2021-07-06 14:50 UTC (permalink / raw)
To: alexander.antonov
Cc: peterz, linux-kernel, x86, kan.liang, ak, stable, alexey.v.bayduraev
On Tue, Jul 06, 2021 at 12:07:23PM +0300, 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>
> ---
> 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
> --
> 2.21.3
>
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX
2021-07-06 14:12 ` Liang, Kan
@ 2021-07-06 16:17 ` Alexander Antonov
2021-07-06 18:43 ` Liang, Kan
2021-07-07 11:44 ` Peter Zijlstra
1 sibling, 1 reply; 7+ messages in thread
From: Alexander Antonov @ 2021-07-06 16:17 UTC (permalink / raw)
To: Liang, Kan, peterz, linux-kernel, x86; +Cc: ak, alexey.v.bayduraev
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
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX
2021-07-06 16:17 ` Alexander Antonov
@ 2021-07-06 18:43 ` Liang, Kan
0 siblings, 0 replies; 7+ messages in thread
From: Liang, Kan @ 2021-07-06 18:43 UTC (permalink / raw)
To: Alexander Antonov, peterz, linux-kernel, x86; +Cc: ak, alexey.v.bayduraev
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
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX
2021-07-06 14:12 ` Liang, Kan
2021-07-06 16:17 ` Alexander Antonov
@ 2021-07-07 11:44 ` Peter Zijlstra
1 sibling, 0 replies; 7+ messages in thread
From: Peter Zijlstra @ 2021-07-07 11:44 UTC (permalink / raw)
To: Liang, Kan
Cc: alexander.antonov, linux-kernel, x86, ak, stable, alexey.v.bayduraev
On Tue, Jul 06, 2021 at 10:12:55AM -0400, 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>
Thanks!
---
Subject: perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX
From: Alexander Antonov <alexander.antonov@linux.intel.com>
Date: Tue, 06 Jul 2021 12:07:23 +0300
From: Alexander Antonov <alexander.antonov@linux.intel.com>
skx_iio_cleanup_mapping() is re-used for snr and icx, but in those
cases it fails to use the appropriate XXX_iio_mapping_group and as
such fails to free previously allocated resources, leading to memory
leaks.
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>
[peterz: Changelog]
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20210706090723.41850-1-alexander.antonov@linux.intel.com
---
^ permalink raw reply [flat|nested] 7+ messages in thread
* [tip: perf/core] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX
2021-07-06 9:07 [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX alexander.antonov
2021-07-06 14:12 ` Liang, Kan
2021-07-06 14:50 ` Greg KH
@ 2021-07-27 13:58 ` tip-bot2 for Alexander Antonov
2 siblings, 0 replies; 7+ messages in thread
From: tip-bot2 for Alexander Antonov @ 2021-07-27 13:58 UTC (permalink / raw)
To: linux-tip-commits
Cc: Alexander Antonov, Peter Zijlstra (Intel),
Kan Liang, stable, x86, linux-kernel
The following commit has been merged into the perf/core branch of tip:
Commit-ID: 3f2cbe3810a60111a33f5f6267bd5a237b826fc9
Gitweb: https://git.kernel.org/tip/3f2cbe3810a60111a33f5f6267bd5a237b826fc9
Author: Alexander Antonov <alexander.antonov@linux.intel.com>
AuthorDate: Tue, 06 Jul 2021 12:07:23 +03:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Fri, 16 Jul 2021 18:46:48 +02:00
perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX
skx_iio_cleanup_mapping() is re-used for snr and icx, but in those
cases it fails to use the appropriate XXX_iio_mapping_group and as
such fails to free previously allocated resources, leading to memory
leaks.
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>
[peterz: Changelog]
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Kan Liang <kan.liang@linux.intel.com>
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/20210706090723.41850-1-alexander.antonov@linux.intel.com
---
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 2558e26..f665b16 100644
--- a/arch/x86/events/intel/uncore_snbep.c
+++ b/arch/x86/events/intel/uncore_snbep.c
@@ -3847,26 +3847,32 @@ clear_attr_update:
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,
@@ -4510,6 +4516,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,
@@ -4526,7 +4537,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 = {
@@ -5113,6 +5124,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,
@@ -5130,7 +5146,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 = {
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-07-27 13:59 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-06 9:07 [PATCH] perf/x86/intel/uncore: Fix IIO cleanup mapping procedure for SNR/ICX alexander.antonov
2021-07-06 14:12 ` Liang, Kan
2021-07-06 16:17 ` Alexander Antonov
2021-07-06 18:43 ` Liang, Kan
2021-07-07 11:44 ` Peter Zijlstra
2021-07-06 14:50 ` Greg KH
2021-07-27 13:58 ` [tip: perf/core] " tip-bot2 for Alexander Antonov
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.