Re: [PATCH] cpufreq: Avoid unnecessary locking in show() and store()

From: Viresh Kumar
Date: Fri Feb 12 2016 - 01:31:36 EST


On 11-02-16, 02:25, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>
> The show() and store() routines in the cpufreq core don't need to
> acquire all of the locks to check if the struct freq_attr they want
> to use really provides the callbacks they need as expected, so change
> them to avoid doing that.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> ---
> drivers/cpufreq/cpufreq.c | 27 +++++++++++----------------
> 1 file changed, 11 insertions(+), 16 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq.c
> +++ linux-pm/drivers/cpufreq/cpufreq.c
> @@ -862,13 +862,11 @@ static ssize_t show(struct kobject *kobj
> struct freq_attr *fattr = to_attr(attr);
> ssize_t ret;
>
> - down_read(&policy->rwsem);
> -
> - if (fattr->show)
> - ret = fattr->show(policy, buf);
> - else
> - ret = -EIO;
> + if (!fattr->show)
> + return -EIO;
>
> + down_read(&policy->rwsem);
> + ret = fattr->show(policy, buf);
> up_read(&policy->rwsem);
>
> return ret;
> @@ -881,20 +879,17 @@ static ssize_t store(struct kobject *kob
> struct freq_attr *fattr = to_attr(attr);
> ssize_t ret = -EINVAL;
>
> - get_online_cpus();
> -
> - if (!cpu_online(policy->cpu))
> - goto unlock;
> + if (!fattr->store)
> + return -EIO;
>
> - down_write(&policy->rwsem);
> + get_online_cpus();
>
> - if (fattr->store)
> + if (cpu_online(policy->cpu)) {
> + down_write(&policy->rwsem);
> ret = fattr->store(policy, buf, count);
> - else
> - ret = -EIO;
> + up_write(&policy->rwsem);
> + }
>
> - up_write(&policy->rwsem);
> -unlock:

I have no problems with the patch as is, but how are we going to benefit from it
?

'if (fattr->show/store)' is never ever going to fail, unless we have a bug here.
So, even we may want to add a WARN_ON() for that case instead.

--
viresh