diff mbox series

[v2.1,4/4] cpufreq: schedutil: Always call driver if need_freq_update is set

Message ID 12275472.W5IoEtXICo@kreacher
State New
Headers show
Series None | expand

Commit Message

Rafael J. Wysocki Oct. 27, 2020, 3:35 p.m. UTC 
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because sugov_update_next_freq() may skip a frequency update even if
the need_freq_update flag has been set for the policy at hand, policy
limits updates may not take effect as expected.

For example, if the intel_pstate driver operates in the passive mode
with HWP enabled, it needs to update the HWP min and max limits when
the policy min and max limits change, respectively, but that may not
happen if the target frequency does not change along with the limit
at hand.  In particular, if the policy min is changed first, causing
the target frequency to be adjusted to it, and the policy max limit
is changed later to the same value, the HWP max limit will not be
updated to follow it as expected, because the target frequency is
still equal to the policy min limit and it will not change until
that limit is updated.

To address this issue, modify get_next_freq() to let the driver
callback run if the CPUFREQ_NEED_UPDATE_LIMITS cpufreq driver flag
is set regardless of whether or not the new frequency to set is
equal to the previous one.

Fixes: f6ebbcf08f37 ("cpufreq: intel_pstate: Implement passive mode with HWP enabled")
Reported-by: Zhang Rui <rui.zhang@intel.com>
Tested-by: Zhang Rui <rui.zhang@intel.com>
Cc: 5.9+ <stable@vger.kernel.org> # 5.9+
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v2 -> v2.1
   * Fix typo in the subject.
   * Make get_next_freq() and sugov_update_next_freq() ignore the
     sg_policy->next_freq == next_freq case when CPUFREQ_NEED_UPDATE_LIMITS
     is set for the driver.
   * Add Tested-by from Rui (this version lets the driver callback run more
     often than the v2, so the behavior in the Rui's case doesn't change).

---
 kernel/sched/cpufreq_schedutil.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Viresh Kumar Oct. 28, 2020, 3:57 a.m. UTC | #1 
On 27-10-20, 16:35, Rafael J. Wysocki wrote:
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str
>  static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
>  				   unsigned int next_freq)
>  {
> -	if (sg_policy->next_freq == next_freq)
> +	if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)
>  		return false;
>  
>  	sg_policy->next_freq = next_freq;
>  	sg_policy->last_freq_update_time = time;
> +	sg_policy->need_freq_update = false;
>  
>  	return true;
>  }
> @@ -161,10 +162,12 @@ static unsigned int get_next_freq(struct
>  
>  	freq = map_util_freq(util, freq, max);
>  
> -	if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
> +	if (cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
> +		sg_policy->need_freq_update = true;
> +	else if (freq == sg_policy->cached_raw_freq &&
> +		 !sg_policy->need_freq_update)
>  		return sg_policy->next_freq;
>  
> -	sg_policy->need_freq_update = false;
>  	sg_policy->cached_raw_freq = freq;
>  	return cpufreq_driver_resolve_freq(policy, freq);
>  }

What about just this instead ?

  static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
  				   unsigned int next_freq)
  {
 -	if (sg_policy->next_freq == next_freq)
 +	if (sg_policy->next_freq == next_freq &&
 +          !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
  		return false;
  
  	sg_policy->next_freq = next_freq;
  	sg_policy->last_freq_update_time = time;
  
  	return true;
  }
Rafael J. Wysocki Oct. 29, 2020, 10:42 a.m. UTC | #2 
On Thu, Oct 29, 2020 at 12:10 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> On 27-10-20, 16:35, Rafael J. Wysocki wrote:

> > Index: linux-pm/kernel/sched/cpufreq_schedutil.c

> > ===================================================================

> > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c

> > +++ linux-pm/kernel/sched/cpufreq_schedutil.c

> > @@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str

> >  static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,

> >                                  unsigned int next_freq)

> >  {

> > -     if (sg_policy->next_freq == next_freq)

> > +     if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)

> >               return false;

> >

> >       sg_policy->next_freq = next_freq;

> >       sg_policy->last_freq_update_time = time;

> > +     sg_policy->need_freq_update = false;

> >

> >       return true;

> >  }

> > @@ -161,10 +162,12 @@ static unsigned int get_next_freq(struct

> >

> >       freq = map_util_freq(util, freq, max);

> >

> > -     if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)

> > +     if (cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

> > +             sg_policy->need_freq_update = true;

> > +     else if (freq == sg_policy->cached_raw_freq &&

> > +              !sg_policy->need_freq_update)

> >               return sg_policy->next_freq;

> >

> > -     sg_policy->need_freq_update = false;

> >       sg_policy->cached_raw_freq = freq;

> >       return cpufreq_driver_resolve_freq(policy, freq);

> >  }

>

> What about just this instead ?

>

>   static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,

>                                    unsigned int next_freq)

>   {

>  -      if (sg_policy->next_freq == next_freq)

>  +      if (sg_policy->next_freq == next_freq &&

>  +          !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

>                 return false;

>

>         sg_policy->next_freq = next_freq;

>         sg_policy->last_freq_update_time = time;

>

>         return true;

>   }

>


Without any changes in get_next_freq() this is not sufficient, because
get_next_freq() may skip the update too.

If the intention is to always let the driver callback run when
CPUFREQ_NEED_UPDATE_LIMITS is set, then both get_next_freq() and
sugov_update_next_freq() need to be modified.
Viresh Kumar Oct. 29, 2020, 10:54 a.m. UTC | #3 
On 29-10-20, 11:42, Rafael J. Wysocki wrote:
> On Thu, Oct 29, 2020 at 12:10 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> >

> > On 27-10-20, 16:35, Rafael J. Wysocki wrote:

> > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c

> > > ===================================================================

> > > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c

> > > +++ linux-pm/kernel/sched/cpufreq_schedutil.c

> > > @@ -102,11 +102,12 @@ static bool sugov_should_update_freq(str

> > >  static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,

> > >                                  unsigned int next_freq)

> > >  {

> > > -     if (sg_policy->next_freq == next_freq)

> > > +     if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)

> > >               return false;

> > >

> > >       sg_policy->next_freq = next_freq;

> > >       sg_policy->last_freq_update_time = time;

> > > +     sg_policy->need_freq_update = false;

> > >

> > >       return true;

> > >  }

> > > @@ -161,10 +162,12 @@ static unsigned int get_next_freq(struct

> > >

> > >       freq = map_util_freq(util, freq, max);

> > >

> > > -     if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)

> > > +     if (cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

> > > +             sg_policy->need_freq_update = true;

> > > +     else if (freq == sg_policy->cached_raw_freq &&

> > > +              !sg_policy->need_freq_update)

> > >               return sg_policy->next_freq;

> > >

> > > -     sg_policy->need_freq_update = false;

> > >       sg_policy->cached_raw_freq = freq;

> > >       return cpufreq_driver_resolve_freq(policy, freq);

> > >  }

> >

> > What about just this instead ?

> >

> >   static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,

> >                                    unsigned int next_freq)

> >   {

> >  -      if (sg_policy->next_freq == next_freq)

> >  +      if (sg_policy->next_freq == next_freq &&

> >  +          !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

> >                 return false;

> >

> >         sg_policy->next_freq = next_freq;

> >         sg_policy->last_freq_update_time = time;

> >

> >         return true;

> >   }

> >

> 

> Without any changes in get_next_freq() this is not sufficient, because

> get_next_freq() may skip the update too.

> 

> If the intention is to always let the driver callback run when

> CPUFREQ_NEED_UPDATE_LIMITS is set, then both get_next_freq() and

> sugov_update_next_freq() need to be modified.


Right, my mistake. I was just suggesting that we may not need to touch
need_freq_update at all but just check the flag.

-- 
viresh
diff mbox series

Patch

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -102,11 +102,12 @@  static bool sugov_should_update_freq(str
 static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
 				   unsigned int next_freq)
 {
-	if (sg_policy->next_freq == next_freq)
+	if (sg_policy->next_freq == next_freq && !sg_policy->need_freq_update)
 		return false;
 
 	sg_policy->next_freq = next_freq;
 	sg_policy->last_freq_update_time = time;
+	sg_policy->need_freq_update = false;
 
 	return true;
 }
@@ -161,10 +162,12 @@  static unsigned int get_next_freq(struct
 
 	freq = map_util_freq(util, freq, max);
 
-	if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update)
+	if (cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
+		sg_policy->need_freq_update = true;
+	else if (freq == sg_policy->cached_raw_freq &&
+		 !sg_policy->need_freq_update)
 		return sg_policy->next_freq;
 
-	sg_policy->need_freq_update = false;
 	sg_policy->cached_raw_freq = freq;
 	return cpufreq_driver_resolve_freq(policy, freq);
 }