Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1341768 > unrolled thread
| Started by | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| First post | 2016-02-24 11:30 +0100 |
| Last post | 2016-02-24 21:10 +0100 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH 2/5] x86/mbm: Intel Memory B/W Monitoring enumeration and init Thomas Gleixner <tglx@linutronix.de> - 2016-02-24 11:30 +0100
Re: [PATCH 2/5] x86/mbm: Intel Memory B/W Monitoring enumeration and init Vikas Shivappa <vikas.shivappa@intel.com> - 2016-02-24 19:20 +0100
Re: [PATCH 2/5] x86/mbm: Intel Memory B/W Monitoring enumeration and init Thomas Gleixner <tglx@linutronix.de> - 2016-02-24 20:30 +0100
Re: [PATCH 2/5] x86/mbm: Intel Memory B/W Monitoring enumeration and init Vikas Shivappa <vikas.shivappa@intel.com> - 2016-02-24 21:10 +0100
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2016-02-24 11:30 +0100 |
| Subject | Re: [PATCH 2/5] x86/mbm: Intel Memory B/W Monitoring enumeration and init |
| Message-ID | <r5ERJ-36A-43@gated-at.bofh.it> |
On Wed, 10 Feb 2016, Vikas Shivappa wrote:ar
> +static int intel_mbm_init(void)
> +{
> + int ret = 0, array_size, maxid = cqm_max_rmid + 1;
> +
> + mbm_socket_max = cpumask_weight(&cqm_cpumask);
This should use the new topology_max_packages() function, so you can alloc
your array correctly even if not all sockets are online/plugged yet.
> + array_size = sizeof(struct sample) * maxid * mbm_socket_max;
> + mbm_local = kmalloc(array_size, GFP_KERNEL);
> + if (!mbm_local) {
> + ret = -ENOMEM;
return -ENOMEM is sufficient here.
> + goto out;
> + }
> +
> + mbm_total = kmalloc(array_size, GFP_KERNEL);
> + if (!mbm_total) {
> + kfree(mbm_local);
> + ret = -ENOMEM;
> + }
> +out:
> +
> + return ret;
> +}
> +
> cqm_l3_scale = boot_cpu_data.x86_cache_occ_scale;
> @@ -1384,14 +1520,34 @@ static int __init intel_cqm_init(void)
>
> __perf_cpu_notifier(intel_cqm_cpu_notifier);
You really should register the notifier _AFTER_ registering the pmu. That
needs to be fixed anyway, because the existing code leaks the notifier AND
memory in case perf_pmu_register fails.
Thanks,
tglx
[toc] | [next] | [standalone]
| From | Vikas Shivappa <vikas.shivappa@intel.com> |
|---|---|
| Date | 2016-02-24 19:20 +0100 |
| Message-ID | <r5Mcy-8nW-3@gated-at.bofh.it> |
| In reply to | #1341768 |
On Wed, 24 Feb 2016, Thomas Gleixner wrote:
> On Wed, 10 Feb 2016, Vikas Shivappa wrote:ar
>> +static int intel_mbm_init(void)
>> +{
>> + int ret = 0, array_size, maxid = cqm_max_rmid + 1;
>> +
>> + mbm_socket_max = cpumask_weight(&cqm_cpumask);
>
> This should use the new topology_max_packages() function, so you can alloc
> your array correctly even if not all sockets are online/plugged yet.
Will fix. Thanks for pointing out.
>
>> + array_size = sizeof(struct sample) * maxid * mbm_socket_max;
>> + mbm_local = kmalloc(array_size, GFP_KERNEL);
>> + if (!mbm_local) {
>> + ret = -ENOMEM;
>
> return -ENOMEM is sufficient here.
Will fix
>
>> + goto out;
>> + }
>> +
>> + mbm_total = kmalloc(array_size, GFP_KERNEL);
>> + if (!mbm_total) {
>> + kfree(mbm_local);
>> + ret = -ENOMEM;
>> + }
>> +out:
>> +
>> + return ret;
>> +}
>> +
>> cqm_l3_scale = boot_cpu_data.x86_cache_occ_scale;
>> @@ -1384,14 +1520,34 @@ static int __init intel_cqm_init(void)
>>
>> __perf_cpu_notifier(intel_cqm_cpu_notifier);
>
> You really should register the notifier _AFTER_ registering the pmu. That
> needs to be fixed anyway, because the existing code leaks the notifier AND
> memory in case perf_pmu_register fails.
Correct, Will fix the notifier leak
Thanks,
Vikas
>
> Thanks,
>
> tglx
>
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2016-02-24 20:30 +0100 |
| Message-ID | <r5Nii-Ew-27@gated-at.bofh.it> |
| In reply to | #1342286 |
On Wed, 24 Feb 2016, Vikas Shivappa wrote: > On Wed, 24 Feb 2016, Thomas Gleixner wrote: > > You really should register the notifier _AFTER_ registering the pmu. That > > needs to be fixed anyway, because the existing code leaks the notifier AND > > memory in case perf_pmu_register fails. > > Correct, Will fix the notifier leak It's not only a notifier leak. The existing code also leaks memory. Please fix that before adding the new stuff in a seperate patch. Thanks, tglx
[toc] | [prev] | [next] | [standalone]
| From | Vikas Shivappa <vikas.shivappa@intel.com> |
|---|---|
| Date | 2016-02-24 21:10 +0100 |
| Message-ID | <r5NV1-1ay-17@gated-at.bofh.it> |
| In reply to | #1342367 |
On Wed, 24 Feb 2016, Thomas Gleixner wrote:
> On Wed, 24 Feb 2016, Vikas Shivappa wrote:
>> On Wed, 24 Feb 2016, Thomas Gleixner wrote:
>>> You really should register the notifier _AFTER_ registering the pmu. That
>>> needs to be fixed anyway, because the existing code leaks the notifier AND
>>> memory in case perf_pmu_register fails.
>>
>> Correct, Will fix the notifier leak
>
> It's not only a notifier leak. The existing code also leaks memory. Please fix
> that before adding the new stuff in a seperate patch.
ok..Makes sense. will send the notifier and memory leak fix in seperate patch as
its a existing cqm issue.
I had added a kfree(str) in the end for the memory leak fix in the current
patch.
out:
cpu_notifier_register_done();
if (ret) {
mbm_enabled = false;
cqm_enabled = false;
kfree(str);
}
>
> Thanks,
>
> tglx
>
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web