[PATCH V3 1/2] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[PATCH V3 1/2] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

[Daniel Lezcano]

The mutex protects a per_cpu variable access. The potential race can
happen only when the cpufreq governor module is loaded and at the same
time the cpu capacity is changed in the sysfs.

There is no real interest of using a mutex to protect a variable
assignation when there is no situation where a task can take the lock
and block.

Replace the mutex by READ_ONCE / WRITE_ONCE.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: Sudeep Holla <sudeep.holla@arm.com>
Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/base/arch_topology.c  | 7 +------
 include/linux/arch_topology.h | 2 +-
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index edfcf8d..fd5325b 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -31,12 +31,11 @@ void arch_set_freq_scale(struct cpumask *cpus, unsigned long cur_freq,
 		per_cpu(freq_scale, i) = scale;
 }
 
-static DEFINE_MUTEX(cpu_scale_mutex);
 DEFINE_PER_CPU(unsigned long, cpu_scale) = SCHED_CAPACITY_SCALE;
 
 void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity)
 {
-	per_cpu(cpu_scale, cpu) = capacity;
+	WRITE_ONCE(per_cpu(cpu_scale, cpu), capacity);
 }
 
 static ssize_t cpu_capacity_show(struct device *dev,
@@ -71,10 +70,8 @@ static ssize_t cpu_capacity_store(struct device *dev,
 	if (new_capacity > SCHED_CAPACITY_SCALE)
 		return -EINVAL;
 
-	mutex_lock(&cpu_scale_mutex);
 	for_each_cpu(i, &cpu_topology[this_cpu].core_sibling)
 		topology_set_cpu_scale(i, new_capacity);
-	mutex_unlock(&cpu_scale_mutex);
 
 	schedule_work(&update_topology_flags_work);
 
@@ -141,7 +138,6 @@ void topology_normalize_cpu_scale(void)
 		return;
 
 	pr_debug("cpu_capacity: capacity_scale=%u\n", capacity_scale);
-	mutex_lock(&cpu_scale_mutex);
 	for_each_possible_cpu(cpu) {
 		pr_debug("cpu_capacity: cpu=%d raw_capacity=%u\n",
 			 cpu, raw_capacity[cpu]);
@@ -151,7 +147,6 @@ void topology_normalize_cpu_scale(void)
 		pr_debug("cpu_capacity: CPU%d cpu_capacity=%lu\n",
 			cpu, topology_get_cpu_scale(NULL, cpu));
 	}
-	mutex_unlock(&cpu_scale_mutex);
 }
 
 bool __init topology_parse_cpu_capacity(struct device_node *cpu_node, int cpu)
diff --git a/include/linux/arch_topology.h b/include/linux/arch_topology.h
index d9bdc1a..12c439f 100644
--- a/include/linux/arch_topology.h
+++ b/include/linux/arch_topology.h
@@ -20,7 +20,7 @@ struct sched_domain;
 static inline
 unsigned long topology_get_cpu_scale(struct sched_domain *sd, int cpu)
 {
-	return per_cpu(cpu_scale, cpu);
+	return READ_ONCE(per_cpu(cpu_scale, cpu));
 }
 
 void topology_set_cpu_scale(unsigned int cpu, unsigned long capacity);
-- 
2.7.4

[PATCH V3 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Daniel Lezcano]

In the case of asymmetric SoC with the same micro-architecture, we
have a group of CPUs with smaller OPPs than the other group. One
example is the 96boards dragonboard 820c. There is no dmips/MHz
difference between both groups, so no need to specify the values in
the DT. Unfortunately, without these defined, there is no scaling
capacity computation triggered, so we need to write
'capacity-dmips-mhz' for each CPU with the same value in order to
force the scaled capacity computation.

In order to fix this situation, allocate 'raw_capacity' so the pointer
is set and the init_cpu_capacity_callback() function can be called.

This was tested on db820c:
 - specified values in the DT (correct results)
 - partial values defined in the DT (error + fallback to defaults)
 - no specified values in the DT (correct results)

correct results are:
  cat /sys/devices/system/cpu/cpu*/cpu_capacity
   758
   758
  1024
  1024

  ... respectively for CPU0, CPU1, CPU2 and CPU3.

That reflects the capacity for the max frequencies 1593600 and 2150400.

Cc: Chris Redpath <chris.redpath@linaro.org>
Cc: Quentin Perret <quentin.perret@linaro.org>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Amit Kucheria <amit.kucheria@linaro.org>
Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
Cc: Niklas Cassel <niklas.cassel@linaro.org>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/base/arch_topology.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
index fd5325b..e0c5b60 100644
--- a/drivers/base/arch_topology.c
+++ b/drivers/base/arch_topology.c
@@ -243,9 +243,20 @@ static int __init register_cpufreq_notifier(void)
 	 * until we have the necessary code to parse the cpu capacity, so
 	 * skip registering cpufreq notifier.
 	 */
-	if (!acpi_disabled || !raw_capacity)
+	if (!acpi_disabled)
 		return -EINVAL;
 
+	if (!raw_capacity) {
+
+		pr_info("cpu_capacity: No capacity defined in DT, set default "
+		       "values to %ld\n", SCHED_CAPACITY_SCALE);
+
+		raw_capacity = kmalloc_array(num_possible_cpus(),
+					     sizeof(*raw_capacity), GFP_KERNEL);
+		if (!raw_capacity)
+			return -ENOMEM;
+	}
+
 	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
 		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");
 		return -ENOMEM;
-- 
2.7.4

Re: [PATCH V3 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Viresh Kumar]

On 26-11-18, 09:44, Daniel Lezcano wrote:
> In the case of asymmetric SoC with the same micro-architecture, we
> have a group of CPUs with smaller OPPs than the other group. One
> example is the 96boards dragonboard 820c. There is no dmips/MHz
> difference between both groups, so no need to specify the values in
> the DT. Unfortunately, without these defined, there is no scaling
> capacity computation triggered, so we need to write
> 'capacity-dmips-mhz' for each CPU with the same value in order to
> force the scaled capacity computation.
> 
> In order to fix this situation, allocate 'raw_capacity' so the pointer
> is set and the init_cpu_capacity_callback() function can be called.
> 
> This was tested on db820c:
>  - specified values in the DT (correct results)
>  - partial values defined in the DT (error + fallback to defaults)
>  - no specified values in the DT (correct results)
> 
> correct results are:
>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>    758
>    758
>   1024
>   1024
> 
>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
> 
> That reflects the capacity for the max frequencies 1593600 and 2150400.
> 
> Cc: Chris Redpath <chris.redpath@linaro.org>
> Cc: Quentin Perret <quentin.perret@linaro.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Amit Kucheria <amit.kucheria@linaro.org>
> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
> Cc: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/arch_topology.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH V3 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Quentin Perret]

On Monday 26 Nov 2018 at 09:44:21 (+0100), Daniel Lezcano wrote:
> In the case of asymmetric SoC with the same micro-architecture, we
> have a group of CPUs with smaller OPPs than the other group. One
> example is the 96boards dragonboard 820c. There is no dmips/MHz
> difference between both groups, so no need to specify the values in
> the DT. Unfortunately, without these defined, there is no scaling
> capacity computation triggered, so we need to write
> 'capacity-dmips-mhz' for each CPU with the same value in order to
> force the scaled capacity computation.
> 
> In order to fix this situation, allocate 'raw_capacity' so the pointer
> is set and the init_cpu_capacity_callback() function can be called.
> 
> This was tested on db820c:
>  - specified values in the DT (correct results)
>  - partial values defined in the DT (error + fallback to defaults)
>  - no specified values in the DT (correct results)
> 
> correct results are:
>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>    758
>    758
>   1024
>   1024
> 
>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
> 
> That reflects the capacity for the max frequencies 1593600 and 2150400.
> 
> Cc: Chris Redpath <chris.redpath@linaro.org>
> Cc: Quentin Perret <quentin.perret@linaro.org>
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Cc: Amit Kucheria <amit.kucheria@linaro.org>
> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
> Cc: Niklas Cassel <niklas.cassel@linaro.org>
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  drivers/base/arch_topology.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
> index fd5325b..e0c5b60 100644
> --- a/drivers/base/arch_topology.c
> +++ b/drivers/base/arch_topology.c
> @@ -243,9 +243,20 @@ static int __init register_cpufreq_notifier(void)
>  	 * until we have the necessary code to parse the cpu capacity, so
>  	 * skip registering cpufreq notifier.
>  	 */
> -	if (!acpi_disabled || !raw_capacity)
> +	if (!acpi_disabled)
>  		return -EINVAL;
>  
> +	if (!raw_capacity) {
> +
> +		pr_info("cpu_capacity: No capacity defined in DT, set default "
> +		       "values to %ld\n", SCHED_CAPACITY_SCALE);
> +
> +		raw_capacity = kmalloc_array(num_possible_cpus(),
> +					     sizeof(*raw_capacity), GFP_KERNEL);
> +		if (!raw_capacity)
> +			return -ENOMEM;
> +	}
> +
>  	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
>  		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");
>  		return -ENOMEM;
> -- 
> 2.7.4

With this, if the DT is partially filled, we will still do the frequency
scaling thing now right ?

I'm not sure if this is the expected behaviour. If the DT is partially
filled, we probably want to have 1024 of capacity for all CPUs to match
the doc.

Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the
top of topology_parse_cpu_capacity() ?

Thanks,
Quentin

Re: [PATCH V3 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Daniel Lezcano]

On 26/11/2018 10:52, Quentin Perret wrote:
> On Monday 26 Nov 2018 at 09:44:21 (+0100), Daniel Lezcano wrote:
>> In the case of asymmetric SoC with the same micro-architecture, we
>> have a group of CPUs with smaller OPPs than the other group. One
>> example is the 96boards dragonboard 820c. There is no dmips/MHz
>> difference between both groups, so no need to specify the values in
>> the DT. Unfortunately, without these defined, there is no scaling
>> capacity computation triggered, so we need to write
>> 'capacity-dmips-mhz' for each CPU with the same value in order to
>> force the scaled capacity computation.
>>
>> In order to fix this situation, allocate 'raw_capacity' so the pointer
>> is set and the init_cpu_capacity_callback() function can be called.
>>
>> This was tested on db820c:
>>  - specified values in the DT (correct results)
>>  - partial values defined in the DT (error + fallback to defaults)
>>  - no specified values in the DT (correct results)
>>
>> correct results are:
>>   cat /sys/devices/system/cpu/cpu*/cpu_capacity
>>    758
>>    758
>>   1024
>>   1024
>>
>>   ... respectively for CPU0, CPU1, CPU2 and CPU3.
>>
>> That reflects the capacity for the max frequencies 1593600 and 2150400.
>>
>> Cc: Chris Redpath <chris.redpath@linaro.org>
>> Cc: Quentin Perret <quentin.perret@linaro.org>
>> Cc: Viresh Kumar <viresh.kumar@linaro.org>
>> Cc: Amit Kucheria <amit.kucheria@linaro.org>
>> Cc: Nicolas Dechesne <nicolas.dechesne@linaro.org>
>> Cc: Niklas Cassel <niklas.cassel@linaro.org>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>  drivers/base/arch_topology.c | 13 ++++++++++++-
>>  1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/base/arch_topology.c b/drivers/base/arch_topology.c
>> index fd5325b..e0c5b60 100644
>> --- a/drivers/base/arch_topology.c
>> +++ b/drivers/base/arch_topology.c
>> @@ -243,9 +243,20 @@ static int __init register_cpufreq_notifier(void)
>>  	 * until we have the necessary code to parse the cpu capacity, so
>>  	 * skip registering cpufreq notifier.
>>  	 */
>> -	if (!acpi_disabled || !raw_capacity)
>> +	if (!acpi_disabled)
>>  		return -EINVAL;
>>  
>> +	if (!raw_capacity) {
>> +
>> +		pr_info("cpu_capacity: No capacity defined in DT, set default "
>> +		       "values to %ld\n", SCHED_CAPACITY_SCALE);
>> +
>> +		raw_capacity = kmalloc_array(num_possible_cpus(),
>> +					     sizeof(*raw_capacity), GFP_KERNEL);
>> +		if (!raw_capacity)
>> +			return -ENOMEM;
>> +	}
>> +
>>  	if (!alloc_cpumask_var(&cpus_to_visit, GFP_KERNEL)) {
>>  		pr_err("cpu_capacity: failed to allocate memory for cpus_to_visit\n");
>>  		return -ENOMEM;
>> -- 
>> 2.7.4
> 
> With this, if the DT is partially filled, we will still do the frequency
> scaling thing now right ?

Right, if the DT is partially filled. We end up with the error, the
raw_capacity is free and set to NULL.

register_cpufreq_notifier() will allocate it and the capacity is computed.

> I'm not sure if this is the expected behaviour. If the DT is partially
> filled, we probably want to have 1024 of capacity for all CPUs to match
> the doc.

Yes if they have the same number of OPP which is the case of 99% of the
boards (excluding the big Little). Otherwise setting all CPU with a
capacity of 1024 but having different OPP (like qcom gold-silver arch)
does not make sense and the patch fix this.

> Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the
> top of topology_parse_cpu_capacity() ?

I prefer to update the documentation, it makes more sense than adding
more cumbersome tests in the current code.



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH V3 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Viresh Kumar]

On 26-11-18, 11:08, Daniel Lezcano wrote:
> On 26/11/2018 10:52, Quentin Perret wrote:
> > Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the
> > top of topology_parse_cpu_capacity() ?
> 
> I prefer to update the documentation, it makes more sense than adding
> more cumbersome tests in the current code.

+1

Throwing an error and ignoring DT number completely for the capacity
are good enough in my opinion as well.

And who cares for the platforms that can't even fill the DT properly :)

-- 
viresh

Re: [PATCH V3 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Quentin Perret]

On Monday 26 Nov 2018 at 15:49:55 (+0530), Viresh Kumar wrote:
> On 26-11-18, 11:08, Daniel Lezcano wrote:
> > On 26/11/2018 10:52, Quentin Perret wrote:
> > > Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the
> > > top of topology_parse_cpu_capacity() ?
> > 
> > I prefer to update the documentation, it makes more sense than adding
> > more cumbersome tests in the current code.
> 
> +1
> 
> Throwing an error and ignoring DT number completely for the capacity
> are good enough in my opinion as well.
> 
> And who cares for the platforms that can't even fill the DT properly :)

Right, I think we all agree the case with a partially filled DT is
broken. I don't actually care too much about the behaviour in this case,
but it needs to be consistent with the doc.

So, as long as you fix the doc, that change is fine by me :-)

Thanks,
Quentin

Re: [PATCH V3 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Daniel Lezcano]

On 26/11/2018 12:09, Quentin Perret wrote:
> On Monday 26 Nov 2018 at 15:49:55 (+0530), Viresh Kumar wrote:
>> On 26-11-18, 11:08, Daniel Lezcano wrote:
>>> On 26/11/2018 10:52, Quentin Perret wrote:
>>>> Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the
>>>> top of topology_parse_cpu_capacity() ?
>>>
>>> I prefer to update the documentation, it makes more sense than adding
>>> more cumbersome tests in the current code.
>>
>> +1
>>
>> Throwing an error and ignoring DT number completely for the capacity
>> are good enough in my opinion as well.
>>
>> And who cares for the platforms that can't even fill the DT properly :)
> 
> Right, I think we all agree the case with a partially filled DT is
> broken. I don't actually care too much about the behaviour in this case,
> but it needs to be consistent with the doc.
> 
> So, as long as you fix the doc, that change is fine by me :-)

Ok what about the following change ?

"

If capacity-dmips-mhz is not specified or if the parsing fails, the
default capacity value will be computed against the highest frequency,
it will result most of the time on the same capacity value. However on
some platform with different OPP set but the same micro-architecture,
the capacity will be scaled down for CPUs having lower frequencies.

"



-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

Re: [PATCH V3 2/2] base/drivers/arch_topology: Default dmips-mhz if they are not set in DT

[Quentin Perret]

On Monday 26 Nov 2018 at 12:36:31 (+0100), Daniel Lezcano wrote:
> On 26/11/2018 12:09, Quentin Perret wrote:
> > On Monday 26 Nov 2018 at 15:49:55 (+0530), Viresh Kumar wrote:
> >> On 26-11-18, 11:08, Daniel Lezcano wrote:
> >>> On 26/11/2018 10:52, Quentin Perret wrote:
> >>>> Maybe you want to test 'if (!raw_capacity || cap_parsing_failed)' at the
> >>>> top of topology_parse_cpu_capacity() ?
> >>>
> >>> I prefer to update the documentation, it makes more sense than adding
> >>> more cumbersome tests in the current code.
> >>
> >> +1
> >>
> >> Throwing an error and ignoring DT number completely for the capacity
> >> are good enough in my opinion as well.
> >>
> >> And who cares for the platforms that can't even fill the DT properly :)
> > 
> > Right, I think we all agree the case with a partially filled DT is
> > broken. I don't actually care too much about the behaviour in this case,
> > but it needs to be consistent with the doc.
> > 
> > So, as long as you fix the doc, that change is fine by me :-)
> 
> Ok what about the following change ?
> 
> "
> 
> If capacity-dmips-mhz is not specified or if the parsing fails, the
> default capacity value will be computed against the highest frequency,
> it will result most of the time on the same capacity value.

That "most of the time" sounds a bit odd no ? Maybe mention explicitly
the case you're referring to (that is when all CPUs have the same max
freq) ?

> However on
> some platform with different OPP set but the same micro-architecture,
> the capacity will be scaled down for CPUs having lower frequencies.
> 
> "

Other than that LGTM.

Thanks,
Quentin