Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
On Thursday, July 09, 2015 09:28:30 AM Pan Xinhui wrote: > hi, Rafael > thanks for your kind reply. :) > > On 2015年07月09日 08:20, Rafael J. Wysocki wrote: > > On Tuesday, July 07, 2015 08:04:43 PM Viresh Kumar wrote: > >> On 07-07-15, 20:43, Pan Xinhui wrote: > >>> > >>> Drivers can store their internal per-policy information in > >>> policy->driver_data, lets use it. > >>> > >>> we have benefits after this replacing. > >>> 1) memory saving. > >>> 2) policy is shared by several cpus, per_cpu seems not correct. using > >>> *driver_data* is more reasonable. > >>> 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy->cpu might > >>> change during cpu hotplug. So sometimes we cant't free *data*, use > >>> *driver_data* to fix it. > >>> 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of > >>> policy->cpu is set to *data*, if we try to get cpufreq on other cpus, we > >>> get zero instead of correct values. Use *driver_data* to fix it. > >>> > >>> Signed-off-by: Pan Xinhui > >>> --- > >>> Changes from V1: > >>> codes style fix, comments update > >>> move cpufreq_cpu_put(policy) after we get *driver_data* > >>> --- > >>> drivers/cpufreq/acpi-cpufreq.c | 40 > >>> ++-- > >>> 1 file changed, 22 insertions(+), 18 deletions(-) > >> > >> Acked-by: Viresh Kumar > > > > OK > > > > Does it fix any recent regressions or is it just an old bug? > > > > This patch achieve old bug fix and codes improvements. In past days, policy > has no field *driver_data*, So > acpi-cpufreq driver has to use per_cpu to store some extra information. But > it did not take good care of every scenarios. > Now cpufreq core makes awesome effort to store more per-policy information in > policy. We can make use of this feature. So I > cook this patch. :) OK, patch applied, thanks! > I am preparing two patches for other two issues in acpi-cpufreq driver based > on this patch. > I will fix them step by step. :) Thanks for doing that! Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
On Thursday, July 09, 2015 09:28:30 AM Pan Xinhui wrote: hi, Rafael thanks for your kind reply. :) On 2015年07月09日 08:20, Rafael J. Wysocki wrote: On Tuesday, July 07, 2015 08:04:43 PM Viresh Kumar wrote: On 07-07-15, 20:43, Pan Xinhui wrote: Drivers can store their internal per-policy information in policy-driver_data, lets use it. we have benefits after this replacing. 1) memory saving. 2) policy is shared by several cpus, per_cpu seems not correct. using *driver_data* is more reasonable. 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy-cpu might change during cpu hotplug. So sometimes we cant't free *data*, use *driver_data* to fix it. 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of policy-cpu is set to *data*, if we try to get cpufreq on other cpus, we get zero instead of correct values. Use *driver_data* to fix it. Signed-off-by: Pan Xinhui xinhuix@intel.com --- Changes from V1: codes style fix, comments update move cpufreq_cpu_put(policy) after we get *driver_data* --- drivers/cpufreq/acpi-cpufreq.c | 40 ++-- 1 file changed, 22 insertions(+), 18 deletions(-) Acked-by: Viresh Kumar viresh.ku...@linaro.org OK Does it fix any recent regressions or is it just an old bug? This patch achieve old bug fix and codes improvements. In past days, policy has no field *driver_data*, So acpi-cpufreq driver has to use per_cpu to store some extra information. But it did not take good care of every scenarios. Now cpufreq core makes awesome effort to store more per-policy information in policy. We can make use of this feature. So I cook this patch. :) OK, patch applied, thanks! I am preparing two patches for other two issues in acpi-cpufreq driver based on this patch. I will fix them step by step. :) Thanks for doing that! Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
hi, Rafael thanks for your kind reply. :) On 2015年07月09日 08:20, Rafael J. Wysocki wrote: > On Tuesday, July 07, 2015 08:04:43 PM Viresh Kumar wrote: >> On 07-07-15, 20:43, Pan Xinhui wrote: >>> >>> Drivers can store their internal per-policy information in >>> policy->driver_data, lets use it. >>> >>> we have benefits after this replacing. >>> 1) memory saving. >>> 2) policy is shared by several cpus, per_cpu seems not correct. using >>> *driver_data* is more reasonable. >>> 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy->cpu might >>> change during cpu hotplug. So sometimes we cant't free *data*, use >>> *driver_data* to fix it. >>> 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of >>> policy->cpu is set to *data*, if we try to get cpufreq on other cpus, we >>> get zero instead of correct values. Use *driver_data* to fix it. >>> >>> Signed-off-by: Pan Xinhui >>> --- >>> Changes from V1: >>> codes style fix, comments update >>> move cpufreq_cpu_put(policy) after we get *driver_data* >>> --- >>> drivers/cpufreq/acpi-cpufreq.c | 40 >>> ++-- >>> 1 file changed, 22 insertions(+), 18 deletions(-) >> >> Acked-by: Viresh Kumar > > OK > > Does it fix any recent regressions or is it just an old bug? > This patch achieve old bug fix and codes improvements. In past days, policy has no field *driver_data*, So acpi-cpufreq driver has to use per_cpu to store some extra information. But it did not take good care of every scenarios. Now cpufreq core makes awesome effort to store more per-policy information in policy. We can make use of this feature. So I cook this patch. :) I am preparing two patches for other two issues in acpi-cpufreq driver based on this patch. I will fix them step by step. :) thanks xinhui > Rafael > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
On Tuesday, July 07, 2015 08:04:43 PM Viresh Kumar wrote: > On 07-07-15, 20:43, Pan Xinhui wrote: > > > > Drivers can store their internal per-policy information in > > policy->driver_data, lets use it. > > > > we have benefits after this replacing. > > 1) memory saving. > > 2) policy is shared by several cpus, per_cpu seems not correct. using > > *driver_data* is more reasonable. > > 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy->cpu might > > change during cpu hotplug. So sometimes we cant't free *data*, use > > *driver_data* to fix it. > > 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of > > policy->cpu is set to *data*, if we try to get cpufreq on other cpus, we > > get zero instead of correct values. Use *driver_data* to fix it. > > > > Signed-off-by: Pan Xinhui > > --- > > Changes from V1: > > codes style fix, comments update > > move cpufreq_cpu_put(policy) after we get *driver_data* > > --- > > drivers/cpufreq/acpi-cpufreq.c | 40 > > ++-- > > 1 file changed, 22 insertions(+), 18 deletions(-) > > Acked-by: Viresh Kumar OK Does it fix any recent regressions or is it just an old bug? Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
hi, Dmitry
thanks for your reply
On 2015年07月08日 01:11, Dmitry Torokhov wrote:
> Hi Pan,
>
> On Tue, Jul 07, 2015 at 08:43:26PM +0800, Pan Xinhui wrote:
>> @@ -364,19 +363,24 @@ static u32 get_cur_val(const struct cpumask *mask)
>>
>> static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
>> {
>> -struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
>> +struct acpi_cpufreq_data *data;
>> +struct cpufreq_policy *policy;
>> unsigned int freq;
>> unsigned int cached_freq;
>>
>> pr_debug("get_cur_freq_on_cpu (%d)\n", cpu);
>>
>> -if (unlikely(data == NULL ||
>> - data->acpi_data == NULL || data->freq_table == NULL)) {
>> +policy = cpufreq_cpu_get(cpu);
>> +if (unlikely(!policy))
>> +return 0;
>> +
>> +data = policy->driver_data;
>> +cpufreq_cpu_put(policy);
>
> If we put policy here can we guarantee that memory pointed to by data
> stays valid? Shoudln't we issue cpufreq_cpu_put(policy) after we done
> assessing the pointer?
>
*driver_data* is used internal by acpi-cpufreq driver. So probably issuing
cpufreq_cpu_put(policy) after we get *driver_data* is OKay.
The worry you have is about the race. we set *driver_data* to NULL then
free it in ->exit callback while ->get callback is using it.
CPU A CPU B
->get ->exit
data = policy->driver_data;
if (!data )
policy->driver_data = NULL;
kfree(data);
access data
yes, it might happen in real world. As Viresh says, it is more like to be a
core level work.
But this race exists in current codes, too. Maybe down_write policy->rwsem can
avoid this race(need double check).
thanks for pointing out it. :)
thanks
xinhui
>> +if (unlikely(!data || !data->acpi_data || !data->freq_table))
>> return 0;
>> -}
>>
>> cached_freq = data->freq_table[data->acpi_data->state].frequency;
>> -freq = extract_freq(get_cur_val(cpumask_of(cpu)), data);
>> +freq = extract_freq(get_cur_val(cpumask_of(cpu), data), data);
>> if (freq != cached_freq) {
>> /*
>> * The dreaded BIOS frequency change behind our back.
>
> Thanks.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
hi, Dmitry
thanks for your reply
On 2015年07月08日 01:11, Dmitry Torokhov wrote:
Hi Pan,
On Tue, Jul 07, 2015 at 08:43:26PM +0800, Pan Xinhui wrote:
@@ -364,19 +363,24 @@ static u32 get_cur_val(const struct cpumask *mask)
static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
{
-struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
+struct acpi_cpufreq_data *data;
+struct cpufreq_policy *policy;
unsigned int freq;
unsigned int cached_freq;
pr_debug(get_cur_freq_on_cpu (%d)\n, cpu);
-if (unlikely(data == NULL ||
- data-acpi_data == NULL || data-freq_table == NULL)) {
+policy = cpufreq_cpu_get(cpu);
+if (unlikely(!policy))
+return 0;
+
+data = policy-driver_data;
+cpufreq_cpu_put(policy);
If we put policy here can we guarantee that memory pointed to by data
stays valid? Shoudln't we issue cpufreq_cpu_put(policy) after we done
assessing the pointer?
*driver_data* is used internal by acpi-cpufreq driver. So probably issuing
cpufreq_cpu_put(policy) after we get *driver_data* is OKay.
The worry you have is about the race. we set *driver_data* to NULL then
free it in -exit callback while -get callback is using it.
CPU A CPU B
-get -exit
data = policy-driver_data;
if (!data )
policy-driver_data = NULL;
kfree(data);
access data
yes, it might happen in real world. As Viresh says, it is more like to be a
core level work.
But this race exists in current codes, too. Maybe down_write policy-rwsem can
avoid this race(need double check).
thanks for pointing out it. :)
thanks
xinhui
+if (unlikely(!data || !data-acpi_data || !data-freq_table))
return 0;
-}
cached_freq = data-freq_table[data-acpi_data-state].frequency;
-freq = extract_freq(get_cur_val(cpumask_of(cpu)), data);
+freq = extract_freq(get_cur_val(cpumask_of(cpu), data), data);
if (freq != cached_freq) {
/*
* The dreaded BIOS frequency change behind our back.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
On Tuesday, July 07, 2015 08:04:43 PM Viresh Kumar wrote: On 07-07-15, 20:43, Pan Xinhui wrote: Drivers can store their internal per-policy information in policy-driver_data, lets use it. we have benefits after this replacing. 1) memory saving. 2) policy is shared by several cpus, per_cpu seems not correct. using *driver_data* is more reasonable. 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy-cpu might change during cpu hotplug. So sometimes we cant't free *data*, use *driver_data* to fix it. 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of policy-cpu is set to *data*, if we try to get cpufreq on other cpus, we get zero instead of correct values. Use *driver_data* to fix it. Signed-off-by: Pan Xinhui xinhuix@intel.com --- Changes from V1: codes style fix, comments update move cpufreq_cpu_put(policy) after we get *driver_data* --- drivers/cpufreq/acpi-cpufreq.c | 40 ++-- 1 file changed, 22 insertions(+), 18 deletions(-) Acked-by: Viresh Kumar viresh.ku...@linaro.org OK Does it fix any recent regressions or is it just an old bug? Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
hi, Rafael thanks for your kind reply. :) On 2015年07月09日 08:20, Rafael J. Wysocki wrote: On Tuesday, July 07, 2015 08:04:43 PM Viresh Kumar wrote: On 07-07-15, 20:43, Pan Xinhui wrote: Drivers can store their internal per-policy information in policy-driver_data, lets use it. we have benefits after this replacing. 1) memory saving. 2) policy is shared by several cpus, per_cpu seems not correct. using *driver_data* is more reasonable. 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy-cpu might change during cpu hotplug. So sometimes we cant't free *data*, use *driver_data* to fix it. 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of policy-cpu is set to *data*, if we try to get cpufreq on other cpus, we get zero instead of correct values. Use *driver_data* to fix it. Signed-off-by: Pan Xinhui xinhuix@intel.com --- Changes from V1: codes style fix, comments update move cpufreq_cpu_put(policy) after we get *driver_data* --- drivers/cpufreq/acpi-cpufreq.c | 40 ++-- 1 file changed, 22 insertions(+), 18 deletions(-) Acked-by: Viresh Kumar viresh.ku...@linaro.org OK Does it fix any recent regressions or is it just an old bug? This patch achieve old bug fix and codes improvements. In past days, policy has no field *driver_data*, So acpi-cpufreq driver has to use per_cpu to store some extra information. But it did not take good care of every scenarios. Now cpufreq core makes awesome effort to store more per-policy information in policy. We can make use of this feature. So I cook this patch. :) I am preparing two patches for other two issues in acpi-cpufreq driver based on this patch. I will fix them step by step. :) thanks xinhui Rafael -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
Hi Dmitry, On 07-07-15, 10:11, Dmitry Torokhov wrote: > > + data = policy->driver_data; > > + cpufreq_cpu_put(policy); > > If we put policy here can we guarantee that memory pointed to by data > stays valid? Shoudln't we issue cpufreq_cpu_put(policy) after we done > assessing the pointer? Ideally yes, you are right. But this was a special case as a callback of the cpufreq-driver is being called and any such issues must be handled at the core level, it at all they are relevant. So probably we can do cpufreq_cpu_put() as soon as we have used it. -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
Hi Pan,
On Tue, Jul 07, 2015 at 08:43:26PM +0800, Pan Xinhui wrote:
> @@ -364,19 +363,24 @@ static u32 get_cur_val(const struct cpumask *mask)
>
> static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
> {
> - struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
> + struct acpi_cpufreq_data *data;
> + struct cpufreq_policy *policy;
> unsigned int freq;
> unsigned int cached_freq;
>
> pr_debug("get_cur_freq_on_cpu (%d)\n", cpu);
>
> - if (unlikely(data == NULL ||
> - data->acpi_data == NULL || data->freq_table == NULL)) {
> + policy = cpufreq_cpu_get(cpu);
> + if (unlikely(!policy))
> + return 0;
> +
> + data = policy->driver_data;
> + cpufreq_cpu_put(policy);
If we put policy here can we guarantee that memory pointed to by data
stays valid? Shoudln't we issue cpufreq_cpu_put(policy) after we done
assessing the pointer?
> + if (unlikely(!data || !data->acpi_data || !data->freq_table))
> return 0;
> - }
>
> cached_freq = data->freq_table[data->acpi_data->state].frequency;
> - freq = extract_freq(get_cur_val(cpumask_of(cpu)), data);
> + freq = extract_freq(get_cur_val(cpumask_of(cpu), data), data);
> if (freq != cached_freq) {
> /*
>* The dreaded BIOS frequency change behind our back.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
On 07-07-15, 20:43, Pan Xinhui wrote: > > Drivers can store their internal per-policy information in > policy->driver_data, lets use it. > > we have benefits after this replacing. > 1) memory saving. > 2) policy is shared by several cpus, per_cpu seems not correct. using > *driver_data* is more reasonable. > 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy->cpu might > change during cpu hotplug. So sometimes we cant't free *data*, use > *driver_data* to fix it. > 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of > policy->cpu is set to *data*, if we try to get cpufreq on other cpus, we > get zero instead of correct values. Use *driver_data* to fix it. > > Signed-off-by: Pan Xinhui > --- > Changes from V1: > codes style fix, comments update > move cpufreq_cpu_put(policy) after we get *driver_data* > --- > drivers/cpufreq/acpi-cpufreq.c | 40 ++-- > 1 file changed, 22 insertions(+), 18 deletions(-) Acked-by: Viresh Kumar -- viresh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
hi, all
thanks for reviewing.
any comments or advices are welcome :)
I am OOO tomorrow, so response may arrive later.
thanks
xinhui
On 2015年07月07日 20:43, Pan Xinhui wrote:
>
> Drivers can store their internal per-policy information in
> policy->driver_data, lets use it.
>
> we have benefits after this replacing.
> 1) memory saving.
> 2) policy is shared by several cpus, per_cpu seems not correct. using
> *driver_data* is more reasonable.
> 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy->cpu might
> change during cpu hotplug. So sometimes we cant't free *data*, use
> *driver_data* to fix it.
> 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of
> policy->cpu is set to *data*, if we try to get cpufreq on other cpus, we
> get zero instead of correct values. Use *driver_data* to fix it.
>
> Signed-off-by: Pan Xinhui
> ---
> Changes from V1:
> codes style fix, comments update
> move cpufreq_cpu_put(policy) after we get *driver_data*
> ---
> drivers/cpufreq/acpi-cpufreq.c | 40 ++--
> 1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
> index 0136dfc..e7fcaa6 100644
> --- a/drivers/cpufreq/acpi-cpufreq.c
> +++ b/drivers/cpufreq/acpi-cpufreq.c
> @@ -72,8 +72,6 @@ struct acpi_cpufreq_data {
> cpumask_var_t freqdomain_cpus;
> };
>
> -static DEFINE_PER_CPU(struct acpi_cpufreq_data *, acfreq_data);
> -
> /* acpi_perf_data is a pointer to percpu data. */
> static struct acpi_processor_performance __percpu *acpi_perf_data;
>
> @@ -144,7 +142,7 @@ static int _store_boost(int val)
>
> static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
> {
> - struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
> + struct acpi_cpufreq_data *data = policy->driver_data;
>
> return cpufreq_show_cpus(data->freqdomain_cpus, buf);
> }
> @@ -327,7 +325,8 @@ static void drv_write(struct drv_cmd *cmd)
> put_cpu();
> }
>
> -static u32 get_cur_val(const struct cpumask *mask)
> +static u32
> +get_cur_val(const struct cpumask *mask, struct acpi_cpufreq_data *data)
> {
> struct acpi_processor_performance *perf;
> struct drv_cmd cmd;
> @@ -335,7 +334,7 @@ static u32 get_cur_val(const struct cpumask *mask)
> if (unlikely(cpumask_empty(mask)))
> return 0;
>
> - switch (per_cpu(acfreq_data, cpumask_first(mask))->cpu_feature) {
> + switch (data->cpu_feature) {
> case SYSTEM_INTEL_MSR_CAPABLE:
> cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
> cmd.addr.msr.reg = MSR_IA32_PERF_CTL;
> @@ -346,7 +345,7 @@ static u32 get_cur_val(const struct cpumask *mask)
> break;
> case SYSTEM_IO_CAPABLE:
> cmd.type = SYSTEM_IO_CAPABLE;
> - perf = per_cpu(acfreq_data, cpumask_first(mask))->acpi_data;
> + perf = data->acpi_data;
> cmd.addr.io.port = perf->control_register.address;
> cmd.addr.io.bit_width = perf->control_register.bit_width;
> break;
> @@ -364,19 +363,24 @@ static u32 get_cur_val(const struct cpumask *mask)
>
> static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
> {
> - struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
> + struct acpi_cpufreq_data *data;
> + struct cpufreq_policy *policy;
> unsigned int freq;
> unsigned int cached_freq;
>
> pr_debug("get_cur_freq_on_cpu (%d)\n", cpu);
>
> - if (unlikely(data == NULL ||
> - data->acpi_data == NULL || data->freq_table == NULL)) {
> + policy = cpufreq_cpu_get(cpu);
> + if (unlikely(!policy))
> + return 0;
> +
> + data = policy->driver_data;
> + cpufreq_cpu_put(policy);
> + if (unlikely(!data || !data->acpi_data || !data->freq_table))
> return 0;
> - }
>
> cached_freq = data->freq_table[data->acpi_data->state].frequency;
> - freq = extract_freq(get_cur_val(cpumask_of(cpu)), data);
> + freq = extract_freq(get_cur_val(cpumask_of(cpu), data), data);
> if (freq != cached_freq) {
> /*
>* The dreaded BIOS frequency change behind our back.
> @@ -397,7 +401,7 @@ static unsigned int check_freqs(const struct cpumask
> *mask, unsigned int freq,
> unsigned int i;
>
> for (i = 0; i < 100; i++) {
> - cur_freq = extract_freq(get_cur_val(mask), data);
> + cur_freq = extract_freq(get_cur_val(mask, data), data);
> if (cur_freq == freq)
> return 1;
> udelay(10);
> @@ -408,7 +412,7 @@ static unsigned int check_freqs(const struct cpumask
> *mask, unsigned int freq,
> static int acpi_cpufreq_target(struct cpufreq_policy *policy,
> unsigned int index)
> {
> - struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
>
[PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
Drivers can store their internal per-policy information in
policy->driver_data, lets use it.
we have benefits after this replacing.
1) memory saving.
2) policy is shared by several cpus, per_cpu seems not correct. using
*driver_data* is more reasonable.
3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy->cpu might
change during cpu hotplug. So sometimes we cant't free *data*, use
*driver_data* to fix it.
4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of
policy->cpu is set to *data*, if we try to get cpufreq on other cpus, we
get zero instead of correct values. Use *driver_data* to fix it.
Signed-off-by: Pan Xinhui
---
Changes from V1:
codes style fix, comments update
move cpufreq_cpu_put(policy) after we get *driver_data*
---
drivers/cpufreq/acpi-cpufreq.c | 40 ++--
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 0136dfc..e7fcaa6 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -72,8 +72,6 @@ struct acpi_cpufreq_data {
cpumask_var_t freqdomain_cpus;
};
-static DEFINE_PER_CPU(struct acpi_cpufreq_data *, acfreq_data);
-
/* acpi_perf_data is a pointer to percpu data. */
static struct acpi_processor_performance __percpu *acpi_perf_data;
@@ -144,7 +142,7 @@ static int _store_boost(int val)
static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
+ struct acpi_cpufreq_data *data = policy->driver_data;
return cpufreq_show_cpus(data->freqdomain_cpus, buf);
}
@@ -327,7 +325,8 @@ static void drv_write(struct drv_cmd *cmd)
put_cpu();
}
-static u32 get_cur_val(const struct cpumask *mask)
+static u32
+get_cur_val(const struct cpumask *mask, struct acpi_cpufreq_data *data)
{
struct acpi_processor_performance *perf;
struct drv_cmd cmd;
@@ -335,7 +334,7 @@ static u32 get_cur_val(const struct cpumask *mask)
if (unlikely(cpumask_empty(mask)))
return 0;
- switch (per_cpu(acfreq_data, cpumask_first(mask))->cpu_feature) {
+ switch (data->cpu_feature) {
case SYSTEM_INTEL_MSR_CAPABLE:
cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
cmd.addr.msr.reg = MSR_IA32_PERF_CTL;
@@ -346,7 +345,7 @@ static u32 get_cur_val(const struct cpumask *mask)
break;
case SYSTEM_IO_CAPABLE:
cmd.type = SYSTEM_IO_CAPABLE;
- perf = per_cpu(acfreq_data, cpumask_first(mask))->acpi_data;
+ perf = data->acpi_data;
cmd.addr.io.port = perf->control_register.address;
cmd.addr.io.bit_width = perf->control_register.bit_width;
break;
@@ -364,19 +363,24 @@ static u32 get_cur_val(const struct cpumask *mask)
static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
+ struct acpi_cpufreq_data *data;
+ struct cpufreq_policy *policy;
unsigned int freq;
unsigned int cached_freq;
pr_debug("get_cur_freq_on_cpu (%d)\n", cpu);
- if (unlikely(data == NULL ||
-data->acpi_data == NULL || data->freq_table == NULL)) {
+ policy = cpufreq_cpu_get(cpu);
+ if (unlikely(!policy))
+ return 0;
+
+ data = policy->driver_data;
+ cpufreq_cpu_put(policy);
+ if (unlikely(!data || !data->acpi_data || !data->freq_table))
return 0;
- }
cached_freq = data->freq_table[data->acpi_data->state].frequency;
- freq = extract_freq(get_cur_val(cpumask_of(cpu)), data);
+ freq = extract_freq(get_cur_val(cpumask_of(cpu), data), data);
if (freq != cached_freq) {
/*
* The dreaded BIOS frequency change behind our back.
@@ -397,7 +401,7 @@ static unsigned int check_freqs(const struct cpumask *mask,
unsigned int freq,
unsigned int i;
for (i = 0; i < 100; i++) {
- cur_freq = extract_freq(get_cur_val(mask), data);
+ cur_freq = extract_freq(get_cur_val(mask, data), data);
if (cur_freq == freq)
return 1;
udelay(10);
@@ -408,7 +412,7 @@ static unsigned int check_freqs(const struct cpumask *mask,
unsigned int freq,
static int acpi_cpufreq_target(struct cpufreq_policy *policy,
unsigned int index)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy->cpu);
+ struct acpi_cpufreq_data *data = policy->driver_data;
struct acpi_processor_performance *perf;
struct drv_cmd cmd;
unsigned int next_perf_state = 0; /* Index into perf table */
@@ -673,7 +677,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy
*policy)
}
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
Hi Pan,
On Tue, Jul 07, 2015 at 08:43:26PM +0800, Pan Xinhui wrote:
@@ -364,19 +363,24 @@ static u32 get_cur_val(const struct cpumask *mask)
static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
+ struct acpi_cpufreq_data *data;
+ struct cpufreq_policy *policy;
unsigned int freq;
unsigned int cached_freq;
pr_debug(get_cur_freq_on_cpu (%d)\n, cpu);
- if (unlikely(data == NULL ||
- data-acpi_data == NULL || data-freq_table == NULL)) {
+ policy = cpufreq_cpu_get(cpu);
+ if (unlikely(!policy))
+ return 0;
+
+ data = policy-driver_data;
+ cpufreq_cpu_put(policy);
If we put policy here can we guarantee that memory pointed to by data
stays valid? Shoudln't we issue cpufreq_cpu_put(policy) after we done
assessing the pointer?
+ if (unlikely(!data || !data-acpi_data || !data-freq_table))
return 0;
- }
cached_freq = data-freq_table[data-acpi_data-state].frequency;
- freq = extract_freq(get_cur_val(cpumask_of(cpu)), data);
+ freq = extract_freq(get_cur_val(cpumask_of(cpu), data), data);
if (freq != cached_freq) {
/*
* The dreaded BIOS frequency change behind our back.
Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
hi, all
thanks for reviewing.
any comments or advices are welcome :)
I am OOO tomorrow, so response may arrive later.
thanks
xinhui
On 2015年07月07日 20:43, Pan Xinhui wrote:
Drivers can store their internal per-policy information in
policy-driver_data, lets use it.
we have benefits after this replacing.
1) memory saving.
2) policy is shared by several cpus, per_cpu seems not correct. using
*driver_data* is more reasonable.
3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy-cpu might
change during cpu hotplug. So sometimes we cant't free *data*, use
*driver_data* to fix it.
4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of
policy-cpu is set to *data*, if we try to get cpufreq on other cpus, we
get zero instead of correct values. Use *driver_data* to fix it.
Signed-off-by: Pan Xinhui xinhuix@intel.com
---
Changes from V1:
codes style fix, comments update
move cpufreq_cpu_put(policy) after we get *driver_data*
---
drivers/cpufreq/acpi-cpufreq.c | 40 ++--
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 0136dfc..e7fcaa6 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -72,8 +72,6 @@ struct acpi_cpufreq_data {
cpumask_var_t freqdomain_cpus;
};
-static DEFINE_PER_CPU(struct acpi_cpufreq_data *, acfreq_data);
-
/* acpi_perf_data is a pointer to percpu data. */
static struct acpi_processor_performance __percpu *acpi_perf_data;
@@ -144,7 +142,7 @@ static int _store_boost(int val)
static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy-cpu);
+ struct acpi_cpufreq_data *data = policy-driver_data;
return cpufreq_show_cpus(data-freqdomain_cpus, buf);
}
@@ -327,7 +325,8 @@ static void drv_write(struct drv_cmd *cmd)
put_cpu();
}
-static u32 get_cur_val(const struct cpumask *mask)
+static u32
+get_cur_val(const struct cpumask *mask, struct acpi_cpufreq_data *data)
{
struct acpi_processor_performance *perf;
struct drv_cmd cmd;
@@ -335,7 +334,7 @@ static u32 get_cur_val(const struct cpumask *mask)
if (unlikely(cpumask_empty(mask)))
return 0;
- switch (per_cpu(acfreq_data, cpumask_first(mask))-cpu_feature) {
+ switch (data-cpu_feature) {
case SYSTEM_INTEL_MSR_CAPABLE:
cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
cmd.addr.msr.reg = MSR_IA32_PERF_CTL;
@@ -346,7 +345,7 @@ static u32 get_cur_val(const struct cpumask *mask)
break;
case SYSTEM_IO_CAPABLE:
cmd.type = SYSTEM_IO_CAPABLE;
- perf = per_cpu(acfreq_data, cpumask_first(mask))-acpi_data;
+ perf = data-acpi_data;
cmd.addr.io.port = perf-control_register.address;
cmd.addr.io.bit_width = perf-control_register.bit_width;
break;
@@ -364,19 +363,24 @@ static u32 get_cur_val(const struct cpumask *mask)
static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
+ struct acpi_cpufreq_data *data;
+ struct cpufreq_policy *policy;
unsigned int freq;
unsigned int cached_freq;
pr_debug(get_cur_freq_on_cpu (%d)\n, cpu);
- if (unlikely(data == NULL ||
- data-acpi_data == NULL || data-freq_table == NULL)) {
+ policy = cpufreq_cpu_get(cpu);
+ if (unlikely(!policy))
+ return 0;
+
+ data = policy-driver_data;
+ cpufreq_cpu_put(policy);
+ if (unlikely(!data || !data-acpi_data || !data-freq_table))
return 0;
- }
cached_freq = data-freq_table[data-acpi_data-state].frequency;
- freq = extract_freq(get_cur_val(cpumask_of(cpu)), data);
+ freq = extract_freq(get_cur_val(cpumask_of(cpu), data), data);
if (freq != cached_freq) {
/*
* The dreaded BIOS frequency change behind our back.
@@ -397,7 +401,7 @@ static unsigned int check_freqs(const struct cpumask
*mask, unsigned int freq,
unsigned int i;
for (i = 0; i 100; i++) {
- cur_freq = extract_freq(get_cur_val(mask), data);
+ cur_freq = extract_freq(get_cur_val(mask, data), data);
if (cur_freq == freq)
return 1;
udelay(10);
@@ -408,7 +412,7 @@ static unsigned int check_freqs(const struct cpumask
*mask, unsigned int freq,
static int acpi_cpufreq_target(struct cpufreq_policy *policy,
unsigned int index)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy-cpu);
+ struct acpi_cpufreq_data *data = policy-driver_data;
struct acpi_processor_performance *perf;
struct
[PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
Drivers can store their internal per-policy information in
policy-driver_data, lets use it.
we have benefits after this replacing.
1) memory saving.
2) policy is shared by several cpus, per_cpu seems not correct. using
*driver_data* is more reasonable.
3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy-cpu might
change during cpu hotplug. So sometimes we cant't free *data*, use
*driver_data* to fix it.
4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of
policy-cpu is set to *data*, if we try to get cpufreq on other cpus, we
get zero instead of correct values. Use *driver_data* to fix it.
Signed-off-by: Pan Xinhui xinhuix@intel.com
---
Changes from V1:
codes style fix, comments update
move cpufreq_cpu_put(policy) after we get *driver_data*
---
drivers/cpufreq/acpi-cpufreq.c | 40 ++--
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/cpufreq/acpi-cpufreq.c b/drivers/cpufreq/acpi-cpufreq.c
index 0136dfc..e7fcaa6 100644
--- a/drivers/cpufreq/acpi-cpufreq.c
+++ b/drivers/cpufreq/acpi-cpufreq.c
@@ -72,8 +72,6 @@ struct acpi_cpufreq_data {
cpumask_var_t freqdomain_cpus;
};
-static DEFINE_PER_CPU(struct acpi_cpufreq_data *, acfreq_data);
-
/* acpi_perf_data is a pointer to percpu data. */
static struct acpi_processor_performance __percpu *acpi_perf_data;
@@ -144,7 +142,7 @@ static int _store_boost(int val)
static ssize_t show_freqdomain_cpus(struct cpufreq_policy *policy, char *buf)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy-cpu);
+ struct acpi_cpufreq_data *data = policy-driver_data;
return cpufreq_show_cpus(data-freqdomain_cpus, buf);
}
@@ -327,7 +325,8 @@ static void drv_write(struct drv_cmd *cmd)
put_cpu();
}
-static u32 get_cur_val(const struct cpumask *mask)
+static u32
+get_cur_val(const struct cpumask *mask, struct acpi_cpufreq_data *data)
{
struct acpi_processor_performance *perf;
struct drv_cmd cmd;
@@ -335,7 +334,7 @@ static u32 get_cur_val(const struct cpumask *mask)
if (unlikely(cpumask_empty(mask)))
return 0;
- switch (per_cpu(acfreq_data, cpumask_first(mask))-cpu_feature) {
+ switch (data-cpu_feature) {
case SYSTEM_INTEL_MSR_CAPABLE:
cmd.type = SYSTEM_INTEL_MSR_CAPABLE;
cmd.addr.msr.reg = MSR_IA32_PERF_CTL;
@@ -346,7 +345,7 @@ static u32 get_cur_val(const struct cpumask *mask)
break;
case SYSTEM_IO_CAPABLE:
cmd.type = SYSTEM_IO_CAPABLE;
- perf = per_cpu(acfreq_data, cpumask_first(mask))-acpi_data;
+ perf = data-acpi_data;
cmd.addr.io.port = perf-control_register.address;
cmd.addr.io.bit_width = perf-control_register.bit_width;
break;
@@ -364,19 +363,24 @@ static u32 get_cur_val(const struct cpumask *mask)
static unsigned int get_cur_freq_on_cpu(unsigned int cpu)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, cpu);
+ struct acpi_cpufreq_data *data;
+ struct cpufreq_policy *policy;
unsigned int freq;
unsigned int cached_freq;
pr_debug(get_cur_freq_on_cpu (%d)\n, cpu);
- if (unlikely(data == NULL ||
-data-acpi_data == NULL || data-freq_table == NULL)) {
+ policy = cpufreq_cpu_get(cpu);
+ if (unlikely(!policy))
+ return 0;
+
+ data = policy-driver_data;
+ cpufreq_cpu_put(policy);
+ if (unlikely(!data || !data-acpi_data || !data-freq_table))
return 0;
- }
cached_freq = data-freq_table[data-acpi_data-state].frequency;
- freq = extract_freq(get_cur_val(cpumask_of(cpu)), data);
+ freq = extract_freq(get_cur_val(cpumask_of(cpu), data), data);
if (freq != cached_freq) {
/*
* The dreaded BIOS frequency change behind our back.
@@ -397,7 +401,7 @@ static unsigned int check_freqs(const struct cpumask *mask,
unsigned int freq,
unsigned int i;
for (i = 0; i 100; i++) {
- cur_freq = extract_freq(get_cur_val(mask), data);
+ cur_freq = extract_freq(get_cur_val(mask, data), data);
if (cur_freq == freq)
return 1;
udelay(10);
@@ -408,7 +412,7 @@ static unsigned int check_freqs(const struct cpumask *mask,
unsigned int freq,
static int acpi_cpufreq_target(struct cpufreq_policy *policy,
unsigned int index)
{
- struct acpi_cpufreq_data *data = per_cpu(acfreq_data, policy-cpu);
+ struct acpi_cpufreq_data *data = policy-driver_data;
struct acpi_processor_performance *perf;
struct drv_cmd cmd;
unsigned int next_perf_state = 0; /* Index into perf table */
@@ -673,7 +677,7 @@ static int acpi_cpufreq_cpu_init(struct cpufreq_policy
*policy)
}
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
Hi Dmitry, On 07-07-15, 10:11, Dmitry Torokhov wrote: + data = policy-driver_data; + cpufreq_cpu_put(policy); If we put policy here can we guarantee that memory pointed to by data stays valid? Shoudln't we issue cpufreq_cpu_put(policy) after we done assessing the pointer? Ideally yes, you are right. But this was a special case as a callback of the cpufreq-driver is being called and any such issues must be handled at the core level, it at all they are relevant. So probably we can do cpufreq_cpu_put() as soon as we have used it. -- viresh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH V2] acpi-cpufreq: replace per_cpu with driver_data of policy
On 07-07-15, 20:43, Pan Xinhui wrote: Drivers can store their internal per-policy information in policy-driver_data, lets use it. we have benefits after this replacing. 1) memory saving. 2) policy is shared by several cpus, per_cpu seems not correct. using *driver_data* is more reasonable. 3) fix a memory leak in acpi_cpufreq_cpu_exit. as policy-cpu might change during cpu hotplug. So sometimes we cant't free *data*, use *driver_data* to fix it. 4) fix a zero return value of get_cur_freq_on_cpu. Only per_cpu of policy-cpu is set to *data*, if we try to get cpufreq on other cpus, we get zero instead of correct values. Use *driver_data* to fix it. Signed-off-by: Pan Xinhui xinhuix@intel.com --- Changes from V1: codes style fix, comments update move cpufreq_cpu_put(policy) after we get *driver_data* --- drivers/cpufreq/acpi-cpufreq.c | 40 ++-- 1 file changed, 22 insertions(+), 18 deletions(-) Acked-by: Viresh Kumar viresh.ku...@linaro.org -- viresh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
