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

[Juri Lelli <juri.lelli@redhat.com>]

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)?

BTW, please update my email address. :-)

Best,

- Juri