[v2.2,4/4] cpufreq: schedutil: Always call driver if CPUFREQ_NEED_UPDATE_LIMITS is set
diff mbox series

Message ID 4720046.CcxZZ2xs9j@kreacher
State New, archived
Headers show
Series
  • Untitled series #469396
Related show

Commit Message

Rafael J. Wysocki Oct. 29, 2020, 11:12 a.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.1 -> v2.2:
   * Instead of updating need_freq_update if CPUFREQ_NEED_UPDATE_LIMITS is set
     in get_next_freq() and checking it again in sugov_update_next_freq(),
     check CPUFREQ_NEED_UPDATE_LIMITS directly in sugov_update_next_freq().
   * Update the subject.

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 |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Viresh Kumar Oct. 29, 2020, 11:23 a.m. UTC | #1
On 29-10-20, 12:12, Rafael J. Wysocki wrote:
> 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.1 -> v2.2:
>    * Instead of updating need_freq_update if CPUFREQ_NEED_UPDATE_LIMITS is set
>      in get_next_freq() and checking it again in sugov_update_next_freq(),
>      check CPUFREQ_NEED_UPDATE_LIMITS directly in sugov_update_next_freq().
>    * Update the subject.
> 
> 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 |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> @@ -102,7 +102,8 @@ 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 &&
> +	    !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
>  		return false;

Since sg_policy->next_freq is used elsewhere as well, this is the best
we can do here.

>  	sg_policy->next_freq = next_freq;
> @@ -161,7 +162,8 @@ 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 (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update &&
> +	    !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
>  		return sg_policy->next_freq;
>  
>  	sg_policy->need_freq_update = false;

But I was wondering if instead of this we just do this here:

        if (!cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
                sg_policy->cached_raw_freq = freq;

And so the above check will always fail.

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Rafael J. Wysocki Oct. 29, 2020, 11:29 a.m. UTC | #2
On Thu, Oct 29, 2020 at 12:23 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 29-10-20, 12:12, Rafael J. Wysocki wrote:
> > 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.1 -> v2.2:
> >    * Instead of updating need_freq_update if CPUFREQ_NEED_UPDATE_LIMITS is set
> >      in get_next_freq() and checking it again in sugov_update_next_freq(),
> >      check CPUFREQ_NEED_UPDATE_LIMITS directly in sugov_update_next_freq().
> >    * Update the subject.
> >
> > 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 |    6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > Index: linux-pm/kernel/sched/cpufreq_schedutil.c
> > ===================================================================
> > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
> > +++ linux-pm/kernel/sched/cpufreq_schedutil.c
> > @@ -102,7 +102,8 @@ 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 &&
> > +         !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
> >               return false;
>
> Since sg_policy->next_freq is used elsewhere as well, this is the best
> we can do here.
>
> >       sg_policy->next_freq = next_freq;
> > @@ -161,7 +162,8 @@ 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 (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update &&
> > +         !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
> >               return sg_policy->next_freq;
> >
> >       sg_policy->need_freq_update = false;
>
> But I was wondering if instead of this we just do this here:
>
>         if (!cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
>                 sg_policy->cached_raw_freq = freq;
>
> And so the above check will always fail.

I wrote it this way, because I want to avoid looking at the driver
flags at all unless the update is going to be skipped.  Otherwise we
may end up fetching a new cache line here every time even if that is
not needed.

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

Thanks!

Patch
diff mbox series

Index: linux-pm/kernel/sched/cpufreq_schedutil.c
===================================================================
--- linux-pm.orig/kernel/sched/cpufreq_schedutil.c
+++ linux-pm/kernel/sched/cpufreq_schedutil.c
@@ -102,7 +102,8 @@  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 &&
+	    !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
 		return false;
 
 	sg_policy->next_freq = next_freq;
@@ -161,7 +162,8 @@  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 (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update &&
+	    !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
 		return sg_policy->next_freq;
 
 	sg_policy->need_freq_update = false;