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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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