Messages in this thread | | | Subject | Re: [PATCH V5 1/2] base/drivers/arch_topology: Replace mutex with READ_ONCE / WRITE_ONCE | From | Daniel Lezcano <> | Date | Wed, 28 Nov 2018 18:54:26 +0100 |
| |
On 28/11/2018 12:44, Juri Lelli wrote: > Hi Daniel, > > On 27/11/18 14:24, Daniel Lezcano wrote: >> 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); > > IIRC this was meant to ensure atomic updates of all siblings with the new > capacity value. I actually now wonder if readers should not grab the > mutex as well (cpu_capacity_show()). Can't we get into a situation where > a reader might see siblings with intermediate values (while the loop > above is performing an update)?
With or without this patch, it is the case:
task1 task2 | | read("/sys/.../cpu1/cpu_capacity) | | write("/sys/.../cpu1/cpu_capacity") read("/sys/.../cpu2/cpu_capacity) |
There is no guarantee userspace can have a consistent view of the capacity. As soon as it reads a capacity, it can be changed in its back.
> BTW, please update my email address. :-)
Sure.
-- <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
| |