Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1331638 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH] cpufreq: Avoid unnecessary locking in show() and store() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-11 02:30 +0100
    Re: [PATCH] cpufreq: Avoid unnecessary locking in show() and store() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-12 07:40 +0100
      Re: [PATCH] cpufreq: Avoid unnecessary locking in show() and store() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-12 14:20 +0100
        Re: [PATCH] cpufreq: Avoid unnecessary locking in show() and store() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-12 17:00 +0100
          Re: [PATCH] cpufreq: Avoid unnecessary locking in show() and store() "Rafael J. Wysocki" <rjw@rjwysocki.net> - 2016-02-12 17:10 +0100
            Re: [PATCH] cpufreq: Avoid unnecessary locking in show() and store() Viresh Kumar <viresh.kumar@linaro.org> - 2016-02-12 17:30 +0100

#1331638 — [PATCH] cpufreq: Avoid unnecessary locking in show() and store()

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

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@intel.com>
---
 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:
 	put_online_cpus();
 
 	return ret;

[toc] | [next] | [standalone]

#1332543

On 11-02-16, 02:25, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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@intel.com>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

#1332729

On Friday, February 12, 2016 12:01:15 PM Viresh Kumar wrote:
> On 11-02-16, 02:25, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > 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@intel.com>
> > ---
> >  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.

Well, having a check that never fails is certainly unuseful.

> So, even we may want to add a WARN_ON() for that case instead.

I can add WARN_ON()s just fine.

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] cpufreq: Avoid unnecessary locking in show() and store()

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.

While at it, add WARN_ON()s around those checks as they are only supposed
to ever fail if there's a bug in the code.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 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 (WARN_ON(!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 (WARN_ON(!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:
 	put_online_cpus();
 
 	return ret;

[toc] | [prev] | [next] | [standalone]

#1332829

On 12-02-16, 14:18, Rafael J. Wysocki wrote:
> Well, having a check that never fails is certainly unuseful.
> 
> > So, even we may want to add a WARN_ON() for that case instead.
> 
> I can add WARN_ON()s just fine.

What about dropping the check completely ?

-- 
viresh

[toc] | [prev] | [next] | [standalone]

#1332838

On Friday, February 12, 2016 09:28:29 PM Viresh Kumar wrote:
> On 12-02-16, 14:18, Rafael J. Wysocki wrote:
> > Well, having a check that never fails is certainly unuseful.
> > 
> > > So, even we may want to add a WARN_ON() for that case instead.
> > 
> > I can add WARN_ON()s just fine.
> 
> What about dropping the check completely ?

Fine by me.

---
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Subject: [PATCH] cpufreq: Drop unnecessary checks from show() and store()

The show() and store() routines in the cpufreq core don't need to
check if the struct freq_attr they want to use really provides the
callbacks they need as expected (if that's not the case, it means
a bug in the code anyway), so change them to avoid doing that.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |   21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -863,12 +863,7 @@ static ssize_t show(struct kobject *kobj
 	ssize_t ret;
 
 	down_read(&policy->rwsem);
-
-	if (fattr->show)
-		ret = fattr->show(policy, buf);
-	else
-		ret = -EIO;
-
+	ret = fattr->show(policy, buf);
 	up_read(&policy->rwsem);
 
 	return ret;
@@ -883,18 +878,12 @@ static ssize_t store(struct kobject *kob
 
 	get_online_cpus();
 
-	if (!cpu_online(policy->cpu))
-		goto unlock;
-
-	down_write(&policy->rwsem);
-
-	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:
 	put_online_cpus();
 
 	return ret;

[toc] | [prev] | [next] | [standalone]

#1332868

On 12-02-16, 17:10, Rafael J. Wysocki wrote:
> On Friday, February 12, 2016 09:28:29 PM Viresh Kumar wrote:
> > On 12-02-16, 14:18, Rafael J. Wysocki wrote:
> > > Well, having a check that never fails is certainly unuseful.
> > > 
> > > > So, even we may want to add a WARN_ON() for that case instead.
> > > 
> > > I can add WARN_ON()s just fine.
> > 
> > What about dropping the check completely ?
> 
> Fine by me.
> 
> ---
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Subject: [PATCH] cpufreq: Drop unnecessary checks from show() and store()
> 
> The show() and store() routines in the cpufreq core don't need to
> check if the struct freq_attr they want to use really provides the
> callbacks they need as expected (if that's not the case, it means
> a bug in the code anyway), so change them to avoid doing that.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq.c |   21 +++++----------------
>  1 file changed, 5 insertions(+), 16 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web