Message-ID: <559B8537.5010309@intel.com>
Date: Tue, 07 Jul 2015 15:52:23 +0800
From: Pan Xinhui <xinhuix.pan@intel.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.5.0
MIME-Version: 1.0
To: Viresh Kumar <viresh.kumar@linaro.org>
CC: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        linux-pm@vger.kernel.org, rjw@rjwysocki.net,
        "yanmin_zhang@linux.intel.com" <yanmin_zhang@linux.intel.com>,
        "mnipxh@163.com" <mnipxh@163.com>
Subject: Re: [PATCH] acpi-cpufreq.c: fix a memory leak in acpi_cpufreq_cpu_exit
References: <559A2073.1000301@intel.com> <20150707065449.GD14598@linux>
In-Reply-To: <20150707065449.GD14598@linux>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3018
Lines: 80

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/