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

Groups > linux.kernel > #1316339 > unrolled thread

[PATCH] cpufreq: Use list_is_last() to check last entry of the policy list

Started by"Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
First post2016-01-25 10:50 +0100
Last post2016-01-27 07:40 +0100
Articles 10 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> - 2016-01-25 10:50 +0100
    Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the  policy list Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-25 11:00 +0100
      Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the  policy list Juri Lelli <juri.lelli@arm.com> - 2016-01-25 12:20 +0100
        Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the  policy list Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-25 12:30 +0100
          Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the  policy list Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2016-01-27 07:00 +0100
        Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the  policy list Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2016-01-27 07:10 +0100
          Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the  policy list Juri Lelli <juri.lelli@arm.com> - 2016-01-27 11:20 +0100
            Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the  policy list Gautham R Shenoy <ego@linux.vnet.ibm.com> - 2016-01-27 12:20 +0100
    [PATCH RESEND] cpufreq: Use list_is_last() to check last entry of the policy list "Gautham R. Shenoy" <ego@linux.vnet.ibm.com> - 2016-01-27 07:40 +0100
      Re: [PATCH RESEND] cpufreq: Use list_is_last() to check last entry  of the policy list Viresh Kumar <viresh.kumar@linaro.org> - 2016-01-27 07:40 +0100

#1316339 — [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list

From"Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Date2016-01-25 10:50 +0100
Subject[PATCH] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qULWz-1WT-35@gated-at.bofh.it>
Currently next_policy() explicitly checks if a policy is the last
policy in the cpufreq_policy_list. Use the standard list_is_last
primitive instead.

Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
 drivers/cpufreq/cpufreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 78b1e2f..b3059a3 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -67,11 +67,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
 {
 	lockdep_assert_held(&cpufreq_driver_lock);
 	do {
-		policy = list_next_entry(policy, policy_list);
-
 		/* No more policies in the list */
-		if (&policy->policy_list == &cpufreq_policy_list)
+		if (list_is_last(&policy->policy_list, &cpufreq_policy_list))
 			return NULL;
+
+		policy = list_next_entry(policy, policy_list);
 	} while (!suitable_policy(policy, active));
 
 	return policy;
-- 
1.9.3

[toc] | [next] | [standalone]

#1316358 — Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-01-25 11:00 +0100
SubjectRe: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qUM6f-20J-23@gated-at.bofh.it>
In reply to#1316339
On 25-01-16, 15:16, Gautham R. Shenoy wrote:
> Currently next_policy() explicitly checks if a policy is the last
> policy in the cpufreq_policy_list. Use the standard list_is_last
> primitive instead.
> 
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
>  drivers/cpufreq/cpufreq.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 78b1e2f..b3059a3 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -67,11 +67,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
>  {
>  	lockdep_assert_held(&cpufreq_driver_lock);
>  	do {
> -		policy = list_next_entry(policy, policy_list);
> -
>  		/* No more policies in the list */
> -		if (&policy->policy_list == &cpufreq_policy_list)
> +		if (list_is_last(&policy->policy_list, &cpufreq_policy_list))
>  			return NULL;
> +
> +		policy = list_next_entry(policy, policy_list);
>  	} while (!suitable_policy(policy, active));
>  
>  	return policy;

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

-- 
viresh

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

#1316498 — Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list

FromJuri Lelli <juri.lelli@arm.com>
Date2016-01-25 12:20 +0100
SubjectRe: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qUNlE-33Y-27@gated-at.bofh.it>
In reply to#1316358
Hi,

On 25/01/16 15:20, Viresh Kumar wrote:
> On 25-01-16, 15:16, Gautham R. Shenoy wrote:
> > Currently next_policy() explicitly checks if a policy is the last
> > policy in the cpufreq_policy_list. Use the standard list_is_last
> > primitive instead.
> > 
> > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > ---
> >  drivers/cpufreq/cpufreq.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 78b1e2f..b3059a3 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -67,11 +67,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
> >  {
> >  	lockdep_assert_held(&cpufreq_driver_lock);

Which branch is this patch based on?

Thanks,

- Juri

> >  	do {
> > -		policy = list_next_entry(policy, policy_list);
> > -
> >  		/* No more policies in the list */
> > -		if (&policy->policy_list == &cpufreq_policy_list)
> > +		if (list_is_last(&policy->policy_list, &cpufreq_policy_list))
> >  			return NULL;
> > +
> > +		policy = list_next_entry(policy, policy_list);
> >  	} while (!suitable_policy(policy, active));
> >  
> >  	return policy;
> 
> Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> -- 
> viresh
>

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

#1316512 — Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-01-25 12:30 +0100
SubjectRe: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qUNvl-37P-43@gated-at.bofh.it>
In reply to#1316498
On 25-01-16, 11:18, Juri Lelli wrote:
> Hi,
> 
> On 25/01/16 15:20, Viresh Kumar wrote:
> > On 25-01-16, 15:16, Gautham R. Shenoy wrote:
> > > Currently next_policy() explicitly checks if a policy is the last
> > > policy in the cpufreq_policy_list. Use the standard list_is_last
> > > primitive instead.
> > > 
> > > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > > ---
> > >  drivers/cpufreq/cpufreq.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 78b1e2f..b3059a3 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -67,11 +67,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
> > >  {
> > >  	lockdep_assert_held(&cpufreq_driver_lock);
> 
> Which branch is this patch based on?

Dude, what's going on here? How come you rebased on Juri's patches ?
:)

-- 
viresh

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

#1318677 — Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list

FromGautham R Shenoy <ego@linux.vnet.ibm.com>
Date2016-01-27 07:00 +0100
SubjectRe: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qVrj4-7je-3@gated-at.bofh.it>
In reply to#1316512
On Mon, Jan 25, 2016 at 04:52:15PM +0530, Viresh Kumar wrote:
> On 25-01-16, 11:18, Juri Lelli wrote:
> > Hi,
> > 
> > On 25/01/16 15:20, Viresh Kumar wrote:
> > > On 25-01-16, 15:16, Gautham R. Shenoy wrote:
> > > > Currently next_policy() explicitly checks if a policy is the last
> > > > policy in the cpufreq_policy_list. Use the standard list_is_last
> > > > primitive instead.
> > > > 
> > > > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > > > ---
> > > >  drivers/cpufreq/cpufreq.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > index 78b1e2f..b3059a3 100644
> > > > --- a/drivers/cpufreq/cpufreq.c
> > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > @@ -67,11 +67,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
> > > >  {
> > > >  	lockdep_assert_held(&cpufreq_driver_lock);
> > 
> > Which branch is this patch based on?
> 
> Dude, what's going on here? How come you rebased on Juri's patches ?
> :)

Ah right! I found this issue while reviewing Juri's patches from the
cpufreq-cleanups branch and didn't switch back to pm-next before
making this change. Shall resend the patch.

> 
> -- 
> viresh
> 

--
Thanks and Regards
gautham.

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

#1318682 — Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list

FromGautham R Shenoy <ego@linux.vnet.ibm.com>
Date2016-01-27 07:10 +0100
SubjectRe: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qVrsK-7FL-15@gated-at.bofh.it>
In reply to#1316498
On Mon, Jan 25, 2016 at 11:18:24AM +0000, Juri Lelli wrote:
> Hi,
> 
> On 25/01/16 15:20, Viresh Kumar wrote:
> > On 25-01-16, 15:16, Gautham R. Shenoy wrote:
> > > Currently next_policy() explicitly checks if a policy is the last
> > > policy in the cpufreq_policy_list. Use the standard list_is_last
> > > primitive instead.
> > > 
> > > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > > ---
> > >  drivers/cpufreq/cpufreq.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > index 78b1e2f..b3059a3 100644
> > > --- a/drivers/cpufreq/cpufreq.c
> > > +++ b/drivers/cpufreq/cpufreq.c
> > > @@ -67,11 +67,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
> > >  {
> > >  	lockdep_assert_held(&cpufreq_driver_lock);
> 
> Which branch is this patch based on?

My bad! This is based on your branch  git://linux-arm.org/linux-jl.git
upstream/cpufreq_cleanups. I found this issue while reviewing your
cleanup patches.


> 
> Thanks,
> 
> - Juri

--
Thanks and Regards
gautham.

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

#1318821 — Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list

FromJuri Lelli <juri.lelli@arm.com>
Date2016-01-27 11:20 +0100
SubjectRe: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qVvmF-1Z6-11@gated-at.bofh.it>
In reply to#1318682
On 27/01/16 11:39, Gautham R Shenoy wrote:
> On Mon, Jan 25, 2016 at 11:18:24AM +0000, Juri Lelli wrote:
> > Hi,
> > 
> > On 25/01/16 15:20, Viresh Kumar wrote:
> > > On 25-01-16, 15:16, Gautham R. Shenoy wrote:
> > > > Currently next_policy() explicitly checks if a policy is the last
> > > > policy in the cpufreq_policy_list. Use the standard list_is_last
> > > > primitive instead.
> > > > 
> > > > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > > > ---
> > > >  drivers/cpufreq/cpufreq.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > index 78b1e2f..b3059a3 100644
> > > > --- a/drivers/cpufreq/cpufreq.c
> > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > @@ -67,11 +67,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
> > > >  {
> > > >  	lockdep_assert_held(&cpufreq_driver_lock);
> > 
> > Which branch is this patch based on?
> 
> My bad! This is based on your branch  git://linux-arm.org/linux-jl.git
> upstream/cpufreq_cleanups. I found this issue while reviewing your
> cleanup patches.
> 

No problem, and thanks for reviewing those! Any feedback? :)

Best,

- Juri

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

#1318862 — Re: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list

FromGautham R Shenoy <ego@linux.vnet.ibm.com>
Date2016-01-27 12:20 +0100
SubjectRe: [PATCH] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qVwiK-2Fz-17@gated-at.bofh.it>
In reply to#1318821
On Wed, Jan 27, 2016 at 10:10:20AM +0000, Juri Lelli wrote:
> On 27/01/16 11:39, Gautham R Shenoy wrote:
> > On Mon, Jan 25, 2016 at 11:18:24AM +0000, Juri Lelli wrote:
> > > Hi,
> > > 
> > > On 25/01/16 15:20, Viresh Kumar wrote:
> > > > On 25-01-16, 15:16, Gautham R. Shenoy wrote:
> > > > > Currently next_policy() explicitly checks if a policy is the last
> > > > > policy in the cpufreq_policy_list. Use the standard list_is_last
> > > > > primitive instead.
> > > > > 
> > > > > Cc: Viresh Kumar <viresh.kumar@linaro.org>
> > > > > Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> > > > > ---
> > > > >  drivers/cpufreq/cpufreq.c | 6 +++---
> > > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > > > > index 78b1e2f..b3059a3 100644
> > > > > --- a/drivers/cpufreq/cpufreq.c
> > > > > +++ b/drivers/cpufreq/cpufreq.c
> > > > > @@ -67,11 +67,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
> > > > >  {
> > > > >  	lockdep_assert_held(&cpufreq_driver_lock);
> > > 
> > > Which branch is this patch based on?
> > 
> > My bad! This is based on your branch  git://linux-arm.org/linux-jl.git
> > upstream/cpufreq_cleanups. I found this issue while reviewing your
> > cleanup patches.
> > 
> 
> No problem, and thanks for reviewing those! Any feedback? :)

It's a timely patchset! 

Off late on POWER systems we've been observing a lot of jitters due to
the on-demand worker thread periodically interrupting a running task
to monitor (not necessarily change if the task!) frequency. We would
very much like to see the frequency monitoring/change happen from an
the timer-context instead of waking up a separate worker thread,
something similar to CPUFREQ_DRIVER_FAST in sched_governor.  However,
that approach required a careful audit of all the locks that are
currently taken in cpufreq core and this patch set is a good attempt
in this direction.

Barring the issues raised by Viresh with respect to the locking
conventions around CPUFREQ_GOV_POLICY_EXIT, I didn't have any
particular issues with it.


> 
> Best,
> 
> - Juri
> 

--
Thanks and Regards
gautham.

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

#1318701 — [PATCH RESEND] cpufreq: Use list_is_last() to check last entry of the policy list

From"Gautham R. Shenoy" <ego@linux.vnet.ibm.com>
Date2016-01-27 07:40 +0100
Subject[PATCH RESEND] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qVrVL-7Tq-1@gated-at.bofh.it>
In reply to#1316339
Currently next_policy() explicitly checks if a policy is the last
policy in the cpufreq_policy_list. Use the standard list_is_last
primitive instead.

Cc: Viresh Kumar <viresh.kumar@linaro.org>
Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
---
The earlier version one was based on an Juri's experimental branch. 
I have based this one on linux-pm.git linux-next branch.

 drivers/cpufreq/cpufreq.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index c35e7da..e979ec7 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -48,11 +48,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
 					  bool active)
 {
 	do {
-		policy = list_next_entry(policy, policy_list);
-
 		/* No more policies in the list */
-		if (&policy->policy_list == &cpufreq_policy_list)
+		if (list_is_last(&policy->policy_list, &cpufreq_policy_list))
 			return NULL;
+
+		policy = list_next_entry(policy, policy_list);
 	} while (!suitable_policy(policy, active));
 
 	return policy;
-- 
1.9.3

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

#1318702 — Re: [PATCH RESEND] cpufreq: Use list_is_last() to check last entry of the policy list

FromViresh Kumar <viresh.kumar@linaro.org>
Date2016-01-27 07:40 +0100
SubjectRe: [PATCH RESEND] cpufreq: Use list_is_last() to check last entry of the policy list
Message-ID<qVrVL-7Tq-3@gated-at.bofh.it>
In reply to#1318701
On 27-01-16, 12:02, Gautham R. Shenoy wrote:
> Currently next_policy() explicitly checks if a policy is the last
> policy in the cpufreq_policy_list. Use the standard list_is_last
> primitive instead.
> 
> Cc: Viresh Kumar <viresh.kumar@linaro.org>
> Signed-off-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
> The earlier version one was based on an Juri's experimental branch. 
> I have based this one on linux-pm.git linux-next branch.
> 
>  drivers/cpufreq/cpufreq.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index c35e7da..e979ec7 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -48,11 +48,11 @@ static struct cpufreq_policy *next_policy(struct cpufreq_policy *policy,
>  					  bool active)
>  {
>  	do {
> -		policy = list_next_entry(policy, policy_list);
> -
>  		/* No more policies in the list */
> -		if (&policy->policy_list == &cpufreq_policy_list)
> +		if (list_is_last(&policy->policy_list, &cpufreq_policy_list))
>  			return NULL;
> +
> +		policy = list_next_entry(policy, policy_list);
>  	} while (!suitable_policy(policy, active));
>  
>  	return policy;

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

-- 
viresh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web