Skip to contentSkip to conversation

base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

PATCH Superseded Daniel Lezcano 2 participants 5 messages Nov 22, 2018 – Nov 26, 2018

Subsystems (2)
Driver core Generic architecture topology

v2 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Daniel Lezcano

author
#0 lore raw

PATCH v2 1/2 base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE

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 <[email protected]>
Acked-by Viresh Kumar <[email protected]>
---
 drivers/base/arch_topology.c  | 7 +------
 include/linux/arch_topology.h | 2 +-
 2 files changed, 2 insertions(+), 7 deletions(-)
drivers/base/arch_topology.c +1 -6
index edfcf8d..fd5325b 100644
@@ -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)
include/linux/arch_topology.h +1 -1
index d9bdc1a..12c439f 100644
@@ -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);
Signature · 1 line
2.7.4
author
#1 lore raw

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

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.

Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no 'capacity-dmips-mhz' is defined in the DT.

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 <[email protected]>
Cc Quentin Perret <[email protected]>
Cc Viresh Kumar <[email protected]>
Cc Amit Kucheria <[email protected]>
Cc Nicolas Dechesne <[email protected]>
Cc Niklas Cassel <[email protected]>
Signed-off-by Daniel Lezcano <[email protected]>
---
 drivers/base/arch_topology.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/base/arch_topology.c +12 -1
index fd5325b..e0c5b60 100644
@@ -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;
Signature · 1 line
2.7.4
#2 lore raw

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

Quoted 11 lines · 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.

Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no 'capacity-dmips-mhz' is defined in the DT.

We aren't doing this anymore. You should rather explain that we just allocate raw_capacity now and rest is left for init_cpu_capacity_callback() to fix.

Signature · 1 line
viresh
author
#3 lore raw

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

Quoted 10 lines · Viresh Kumar
Quoted 9 lines · 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.

Quoted 6 lines
Quoted 2 lines

Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no 'capacity-dmips-mhz' is defined in the DT.

We aren't doing this anymore. You should rather explain that we just allocate raw_capacity now and rest is left for init_cpu_capacity_callback() to fix.

What about?

"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."

Signature · 5 lines
 <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
#4 lore raw

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

Quoted 29 lines · Daniel Lezcano
Quoted 10 lines · Viresh Kumar
Quoted 9 lines · 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.

Quoted 6 lines
Quoted 2 lines

Fix this by setting a default capacity to SCHED_CAPACITY_SCALE, if no 'capacity-dmips-mhz' is defined in the DT.

We aren't doing this anymore. You should rather explain that we just allocate raw_capacity now and rest is left for init_cpu_capacity_callback() to fix.

What about?

"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.

Quoted 2 lines

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

LGTM

Signature · 1 line
viresh