[PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit

[PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit

[Pan Xinhui]

policy->cpu in acpi_cpufreq_cpu_init/exit is the same cpu in most cases.
However during cpu hotplug,
cpufreq core might nominate a new cpu for policy->cpu.

Thar will cause a memory leak in acpi_cpufreq_cpu_exit.
To avoid this issue, use field *driver_data* to keep the the pointer
of acpi_cpufreq_data.

Add field *cpu* in acpi_cpufreq_data to do some proper cleanup work.

Signed-off-by: Pan Xinhui <xinhuix.pan@intel.com>
---
 drivers/cpufreq/acpi-cpufreq.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 0136dfc..1f3372a 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -70,6 +70,7 @@ struct acpi_cpufreq_data {
 	unsigned int resume;
 	unsigned int cpu_feature;
 	cpumask_var_t freqdomain_cpus;
+	unsigned int cpu;
 };
 
 static DEFINE_PER_CPU(struct acpi_cpufreq_data *, acfreq_data);
@@ -833,6 +834,9 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy *policy)
 	 */
 	data->resume = 1;
 
+	data->cpu = cpu;
+	policy->driver_data = data;
+
 	return result;
 
 err_freqfree:
@@ -850,14 +854,15 @@ err_free:
 
 static int acpi_cpufreq_cpu_exit(struct cpufreq_policy *policy)
 {
-	struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
+	struct acpi_cpufreq_data *data = policy->driver_data;
 
 	pr_debug("acpi_cpufreq_cpu_exit\n");
 
 	if (data) {
-		per_cpu(acfreq_data, policy->cpu) = NULL;
+		policy->driver_data = NULL;
+		per_cpu(acfreq_data, data->cpu) = NULL;
 		acpi_processor_unregister_performance(data->acpi_data,
-						      policy->cpu);
+						      data->cpu);
 		free_cpumask_var(data->freqdomain_cpus);
 		kfree(data->freq_table);
 		kfree(data);
-- 
1.9.1

Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit

[Viresh Kumar]

On 06-07-15, 14:30, Pan Xinhui wrote:
> 
> policy->cpu in acpi_cpufreq_cpu_init/exit is the same cpu in most cases.
> However during cpu hotplug,
> cpufreq core might nominate a new cpu for policy->cpu.

Why aren't above lines well aligned? A simple trick to share for vim
users:

- Select lines you want to auto-align with shift+v and up-down keys
- press gq
- That's it and vim will do it for you. You need to set vim's
  'textwidth' to 72 or 80, based on what you are editing, so that vim
  knows where you need to break the line. I have this in vimrc

        set textwidth=80
        au FileType gitcommit set textwidth=72



Back to the real stuff. Few core changes have gone into v4.2-rc1 and
policy->cpu doesn't change any longer on hotplug (unless its a
physical hotplug). So you shouldn't see any issues.

-- 
viresh

Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit

[Pan Xinhui]

hi, Viresh
	thanks for your reply.

On 2015年07月07日 14:54, Viresh Kumar wrote:
> On 06-07-15, 14:30, Pan Xinhui wrote:
>>
>> policy->cpu in acpi_cpufreq_cpu_init/exit is the same cpu in most cases.
>> However during cpu hotplug,
>> cpufreq core might nominate a new cpu for policy->cpu.
> 
> Why aren't above lines well aligned? A simple trick to share for vim
> users:
> 
> - Select lines you want to auto-align with shift+v and up-down keys
> - press gq
> - That's it and vim will do it for you. You need to set vim's
>   'textwidth' to 72 or 80, based on what you are editing, so that vim
>   knows where you need to break the line. I have this in vimrc
> 
>         set textwidth=80
>         au FileType gitcommit set textwidth=72
> 

thanks, that will save me time.

> 
> 
> Back to the real stuff. Few core changes have gone into v4.2-rc1 and
> policy->cpu doesn't change any longer on hotplug (unless its a
> physical hotplug). So you shouldn't see any issues.
> 
I have latest codes.
codes in cpufreq.c are below.
1436     down_write(&policy->rwsem);
1437     cpumask_clear_cpu(cpu, policy->cpus);
1438 
1439     if (policy_is_inactive(policy)) {
1440         if (has_target())
1441             strncpy(policy->last_governor, policy->governor->name,
1442                 CPUFREQ_NAME_LEN);
1443     } else if (cpu == policy->cpu) {
1444         /* Nominate new CPU */
1445         policy->cpu = cpumask_any(policy->cpus);
1446     }
1447     up_write(&policy->rwsem);

line 1445 will change the policy->cpu.
for example, cpu2,3 has same policy, and policy->cpu is 2 at beginning.
If we disable cpu2, policy->cpu is 3.
yes, at most time, cpu0,1,2,3,,etc share the same policy, and policy->cpu is 0 which can't be offline.
So no memory leak. it is just lucky. :)

back to my previous patch, you suggest me to use policy->driver_data to *store* data and don't need use per_cpu anymore.
codes in acpi-cpufreq.c are below.
 365 static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
 366 {
 367     struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
 368     unsigned int freq;
 369     unsigned int cached_freq;
 
we get *data* through per_cpu for now, as the parameter is cpu only.
If we store *data* in policy->driver_data, we need call
struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) to get policy.
We do a full codes review, and find there should be deadlock if we doing so.
But as cpufreq code offers
238 /* Only for cpufreq core internal use */
239 struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)

I have a small question,if we can use *cpufreq_cpu_get_raw* in ->get callback, which is already lock hold,
But the comment(line 238) is... hmm.

thanks for your kind reply. any advices or comments are welcome.

thanks
xinhui

Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit

[Viresh Kumar]

On 07-07-15, 15:52, Pan Xinhui wrote:
> I have latest codes.
> codes in cpufreq.c are below.
> 1436     down_write(&policy->rwsem);
> 1437     cpumask_clear_cpu(cpu, policy->cpus);
> 1438 
> 1439     if (policy_is_inactive(policy)) {
> 1440         if (has_target())
> 1441             strncpy(policy->last_governor, policy->governor->name,
> 1442                 CPUFREQ_NAME_LEN);
> 1443     } else if (cpu == policy->cpu) {
> 1444         /* Nominate new CPU */
> 1445         policy->cpu = cpumask_any(policy->cpus);
> 1446     }
> 1447     up_write(&policy->rwsem);

Sigh. Too bad. So what has changed is that the sysfs directory is
allocated to policy->cpu during init and never changed. But
policy->cpu can surely change.

Sorry for that.

> back to my previous patch, you suggest me to use policy->driver_data to *store* data and don't need use per_cpu anymore.
> codes in acpi-cpufreq.c are below.
>  365 static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
>  366 {
>  367     struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
>  368     unsigned int freq;
>  369     unsigned int cached_freq;
>  
> we get *data* through per_cpu for now, as the parameter is cpu only.
> If we store *data* in policy->driver_data, we need call
> struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) to get policy.
> We do a full codes review, and find there should be deadlock if we doing so.

Why?

> But as cpufreq code offers
> 238 /* Only for cpufreq core internal use */
> 239 struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
> 
> I have a small question,if we can use *cpufreq_cpu_get_raw* in ->get callback, which is already lock hold,
> But the comment(line 238) is... hmm.

That is more internal to the core. Better don't use it.

> thanks for your kind reply. any advices or comments are welcome.

Anyway, your patch is far from complete. You have just fixed a single
place where per-cpu data is accessed with policy->cpu. What about rest
of the code? Like target() :)

--
viresh

Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit

[Pan Xinhui]

hi, Viresh
	thanks for your quick reply :)

On 2015年07月07日 16:53, Viresh Kumar wrote:
> On 07-07-15, 15:52, Pan Xinhui wrote:
>> I have latest codes.
>> codes in cpufreq.c are below.
>> 1436     down_write(&policy->rwsem);
>> 1437     cpumask_clear_cpu(cpu, policy->cpus);
>> 1438 
>> 1439     if (policy_is_inactive(policy)) {
>> 1440         if (has_target())
>> 1441             strncpy(policy->last_governor, policy->governor->name,
>> 1442                 CPUFREQ_NAME_LEN);
>> 1443     } else if (cpu == policy->cpu) {
>> 1444         /* Nominate new CPU */
>> 1445         policy->cpu = cpumask_any(policy->cpus);
>> 1446     }
>> 1447     up_write(&policy->rwsem);
> 
> Sigh. Too bad. So what has changed is that the sysfs directory is
> allocated to policy->cpu during init and never changed. But
> policy->cpu can surely change.
> 
> Sorry for that.
> 

That's OKay. You are very busy reviewing codes written by still fresh guys like me.

>> back to my previous patch, you suggest me to use policy->driver_data to *store* data and don't need use per_cpu anymore.
>> codes in acpi-cpufreq.c are below.
>>  365 static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
>>  366 {
>>  367     struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
>>  368     unsigned int freq;
>>  369     unsigned int cached_freq;
>>  
>> we get *data* through per_cpu for now, as the parameter is cpu only.
>> If we store *data* in policy->driver_data, we need call
>> struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu) to get policy.
>> We do a full codes review, and find there should be deadlock if we doing so.
> 
> Why?
> 
sorry, after double check. it's not caused by cpufreq_cpu_get.
I am working on several branches, these codes are little different, it's OKay here.
Sorry for mistakes.

>> But as cpufreq code offers
>> 238 /* Only for cpufreq core internal use */
>> 239 struct cpufreq_policy *cpufreq_cpu_get_raw(unsigned int cpu)
>>
>> I have a small question,if we can use *cpufreq_cpu_get_raw* in ->get callback, which is already lock hold,
>> But the comment(line 238) is... hmm.
> 
> That is more internal to the core. Better don't use it.
>

yes, *cpufreq_cpu_get* is OKay. thanks.
 
>> thanks for your kind reply. any advices or comments are welcome.
> 
> Anyway, your patch is far from complete. You have just fixed a single
> place where per-cpu data is accessed with policy->cpu. What about rest
> of the code? Like target() :)
> 
I have generated one patch which replacing all per_cpu with *driver_data*, it works well in our Intel's branch for at least 2 days.
Let me do more codes review and tests before sending to LKML. :)
thanks for your advices :) it's really good.

thanks
xinhui

> --
> viresh
>