diff mbox series

cpufreq: schedutil: Don't skip freq update if need_freq_update is set

Message ID 207ae817a778d79a99c30cb48f2ea1f527416519.1604042421.git.viresh.kumar@linaro.org
State Accepted
Commit 23a881852f3eff6a7ba8d240b57de076763fdef9
Headers show
Series cpufreq: schedutil: Don't skip freq update if need_freq_update is set | expand

Commit Message

Viresh Kumar Oct. 30, 2020, 7:21 a.m. UTC
The cpufreq policy's frequency limits (min/max) can get changed at any
point of time, while schedutil is trying to update the next frequency.
Though the schedutil governor has necessary locking and support in place
to make sure we don't miss any of those updates, there is a corner case
where the governor will find that the CPU is already running at the
desired frequency and so may skip an update.

For example, consider that the CPU can run at 1 GHz, 1.2 GHz and 1.4 GHz
and is running at 1 GHz currently. Schedutil tries to update the
frequency to 1.2 GHz, during this time the policy limits get changed as
policy->min = 1.4 GHz. As schedutil (and cpufreq core) does clamp the
frequency at various instances, we will eventually set the frequency to
1.4 GHz, while we will save 1.2 GHz in sg_policy->next_freq.

Now lets say the policy limits get changed back at this time with
policy->min as 1 GHz. The next time schedutil is invoked by the
scheduler, we will reevaluate the next frequency (because
need_freq_update will get set due to limits change event) and lets say
we want to set the frequency to 1.2 GHz again. At this point
sugov_update_next_freq() will find the next_freq == current_freq and
will abort the update, while the CPU actually runs at 1.4 GHz.

Until now need_freq_update was used as a flag to indicate that the
policy's frequency limits have changed, and that we should consider the
new limits while reevaluating the next frequency.

This patch fixes the above mentioned issue by extending the purpose of
the need_freq_update flag. If this flag is set now, the schedutil
governor will not try to abort a frequency change even if next_freq ==
current_freq.

As similar behavior is required in the case of
CPUFREQ_NEED_UPDATE_LIMITS flag as well, need_freq_update will never be
set to false if that flag is set for the driver.

We also don't need to consider the need_freq_update flag in
sugov_update_single() anymore to handle the special case of busy CPU, as
we won't abort a frequency update anymore.

Reported-by: zhuguangqing <zhuguangqing@xiaomi.com>
Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 kernel/sched/cpufreq_schedutil.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

Comments

Rafael J. Wysocki Oct. 30, 2020, 3:07 p.m. UTC | #1
On Fri, Oct 30, 2020 at 8:31 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>

> The cpufreq policy's frequency limits (min/max) can get changed at any

> point of time, while schedutil is trying to update the next frequency.

> Though the schedutil governor has necessary locking and support in place

> to make sure we don't miss any of those updates, there is a corner case

> where the governor will find that the CPU is already running at the

> desired frequency and so may skip an update.

>

> For example, consider that the CPU can run at 1 GHz, 1.2 GHz and 1.4 GHz

> and is running at 1 GHz currently. Schedutil tries to update the

> frequency to 1.2 GHz, during this time the policy limits get changed as

> policy->min = 1.4 GHz. As schedutil (and cpufreq core) does clamp the

> frequency at various instances, we will eventually set the frequency to

> 1.4 GHz, while we will save 1.2 GHz in sg_policy->next_freq.

>

> Now lets say the policy limits get changed back at this time with

> policy->min as 1 GHz. The next time schedutil is invoked by the

> scheduler, we will reevaluate the next frequency (because

> need_freq_update will get set due to limits change event) and lets say

> we want to set the frequency to 1.2 GHz again. At this point

> sugov_update_next_freq() will find the next_freq == current_freq and

> will abort the update, while the CPU actually runs at 1.4 GHz.

>

> Until now need_freq_update was used as a flag to indicate that the

> policy's frequency limits have changed, and that we should consider the

> new limits while reevaluating the next frequency.

>

> This patch fixes the above mentioned issue by extending the purpose of

> the need_freq_update flag. If this flag is set now, the schedutil

> governor will not try to abort a frequency change even if next_freq ==

> current_freq.

>

> As similar behavior is required in the case of

> CPUFREQ_NEED_UPDATE_LIMITS flag as well, need_freq_update will never be

> set to false if that flag is set for the driver.

>

> We also don't need to consider the need_freq_update flag in

> sugov_update_single() anymore to handle the special case of busy CPU, as

> we won't abort a frequency update anymore.

>

> Reported-by: zhuguangqing <zhuguangqing@xiaomi.com>

> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>


Thanks for following my suggestion!

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

> ---

>  kernel/sched/cpufreq_schedutil.c | 22 ++++++++++------------

>  1 file changed, 10 insertions(+), 12 deletions(-)

>

> diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c

> index c03a5775d019..c6861be02c86 100644

> --- a/kernel/sched/cpufreq_schedutil.c

> +++ b/kernel/sched/cpufreq_schedutil.c

> @@ -102,9 +102,12 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)

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

>                                    unsigned int next_freq)

>  {

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

> -           !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

> -               return false;

> +       if (!sg_policy->need_freq_update) {

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

> +                       return false;

> +       } else if (!cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS)) {

> +               sg_policy->need_freq_update = false;

> +       }

>

>         sg_policy->next_freq = next_freq;

>         sg_policy->last_freq_update_time = time;

> @@ -162,11 +165,9 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,

>

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

>

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

> -           !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

> +       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);

>  }

> @@ -442,7 +443,6 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,

>         struct sugov_policy *sg_policy = sg_cpu->sg_policy;

>         unsigned long util, max;

>         unsigned int next_f;

> -       bool busy;

>         unsigned int cached_freq = sg_policy->cached_raw_freq;

>

>         sugov_iowait_boost(sg_cpu, time, flags);

> @@ -453,9 +453,6 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,

>         if (!sugov_should_update_freq(sg_policy, time))

>                 return;

>

> -       /* Limits may have changed, don't skip frequency update */

> -       busy = !sg_policy->need_freq_update && sugov_cpu_is_busy(sg_cpu);

> -

>         util = sugov_get_util(sg_cpu);

>         max = sg_cpu->max;

>         util = sugov_iowait_apply(sg_cpu, time, util, max);

> @@ -464,7 +461,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,

>          * Do not reduce the frequency if the CPU has not been idle

>          * recently, as the reduction is likely to be premature then.

>          */

> -       if (busy && next_f < sg_policy->next_freq) {

> +       if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {

>                 next_f = sg_policy->next_freq;

>

>                 /* Restore cached freq as next_freq has changed */

> @@ -829,9 +826,10 @@ static int sugov_start(struct cpufreq_policy *policy)

>         sg_policy->next_freq                    = 0;

>         sg_policy->work_in_progress             = false;

>         sg_policy->limits_changed               = false;

> -       sg_policy->need_freq_update             = false;

>         sg_policy->cached_raw_freq              = 0;

>

> +       sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);

> +

>         for_each_cpu(cpu, policy->cpus) {

>                 struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);

>

> --


I'll queue it up for -rc3 next week, thanks!
Rafael J. Wysocki Oct. 30, 2020, 3:23 p.m. UTC | #2
On Fri, Oct 30, 2020 at 4:07 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>

> On Fri, Oct 30, 2020 at 8:31 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> >

> > The cpufreq policy's frequency limits (min/max) can get changed at any

> > point of time, while schedutil is trying to update the next frequency.

> > Though the schedutil governor has necessary locking and support in place

> > to make sure we don't miss any of those updates, there is a corner case

> > where the governor will find that the CPU is already running at the

> > desired frequency and so may skip an update.

> >

> > For example, consider that the CPU can run at 1 GHz, 1.2 GHz and 1.4 GHz

> > and is running at 1 GHz currently. Schedutil tries to update the

> > frequency to 1.2 GHz, during this time the policy limits get changed as

> > policy->min = 1.4 GHz. As schedutil (and cpufreq core) does clamp the

> > frequency at various instances, we will eventually set the frequency to

> > 1.4 GHz, while we will save 1.2 GHz in sg_policy->next_freq.

> >

> > Now lets say the policy limits get changed back at this time with

> > policy->min as 1 GHz. The next time schedutil is invoked by the

> > scheduler, we will reevaluate the next frequency (because

> > need_freq_update will get set due to limits change event) and lets say

> > we want to set the frequency to 1.2 GHz again. At this point

> > sugov_update_next_freq() will find the next_freq == current_freq and

> > will abort the update, while the CPU actually runs at 1.4 GHz.

> >

> > Until now need_freq_update was used as a flag to indicate that the

> > policy's frequency limits have changed, and that we should consider the

> > new limits while reevaluating the next frequency.

> >

> > This patch fixes the above mentioned issue by extending the purpose of

> > the need_freq_update flag. If this flag is set now, the schedutil

> > governor will not try to abort a frequency change even if next_freq ==

> > current_freq.

> >

> > As similar behavior is required in the case of

> > CPUFREQ_NEED_UPDATE_LIMITS flag as well, need_freq_update will never be

> > set to false if that flag is set for the driver.

> >

> > We also don't need to consider the need_freq_update flag in

> > sugov_update_single() anymore to handle the special case of busy CPU, as

> > we won't abort a frequency update anymore.

> >

> > Reported-by: zhuguangqing <zhuguangqing@xiaomi.com>

> > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

>

> Thanks for following my suggestion!

>

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

> > ---

> >  kernel/sched/cpufreq_schedutil.c | 22 ++++++++++------------

> >  1 file changed, 10 insertions(+), 12 deletions(-)

> >

> > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c

> > index c03a5775d019..c6861be02c86 100644

> > --- a/kernel/sched/cpufreq_schedutil.c

> > +++ b/kernel/sched/cpufreq_schedutil.c

> > @@ -102,9 +102,12 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)

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

> >                                    unsigned int next_freq)

> >  {

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

> > -           !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

> > -               return false;

> > +       if (!sg_policy->need_freq_update) {

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

> > +                       return false;

> > +       } else if (!cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS)) {

> > +               sg_policy->need_freq_update = false;


One nit, though.

This can be changed into

} else {
      sg_policy->need_freq_update =
cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
}

to save a branch and because need_freq_update is there in the cache
already, this should be a fast update.

So I'm going to make this change while applying the patch.

> > +       }

> >

> >         sg_policy->next_freq = next_freq;

> >         sg_policy->last_freq_update_time = time;

> > @@ -162,11 +165,9 @@ static unsigned int get_next_freq(struct sugov_policy *sg_policy,

> >

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

> >

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

> > -           !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

> > +       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);

> >  }

> > @@ -442,7 +443,6 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,

> >         struct sugov_policy *sg_policy = sg_cpu->sg_policy;

> >         unsigned long util, max;

> >         unsigned int next_f;

> > -       bool busy;

> >         unsigned int cached_freq = sg_policy->cached_raw_freq;

> >

> >         sugov_iowait_boost(sg_cpu, time, flags);

> > @@ -453,9 +453,6 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,

> >         if (!sugov_should_update_freq(sg_policy, time))

> >                 return;

> >

> > -       /* Limits may have changed, don't skip frequency update */

> > -       busy = !sg_policy->need_freq_update && sugov_cpu_is_busy(sg_cpu);

> > -

> >         util = sugov_get_util(sg_cpu);

> >         max = sg_cpu->max;

> >         util = sugov_iowait_apply(sg_cpu, time, util, max);

> > @@ -464,7 +461,7 @@ static void sugov_update_single(struct update_util_data *hook, u64 time,

> >          * Do not reduce the frequency if the CPU has not been idle

> >          * recently, as the reduction is likely to be premature then.

> >          */

> > -       if (busy && next_f < sg_policy->next_freq) {

> > +       if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {

> >                 next_f = sg_policy->next_freq;

> >

> >                 /* Restore cached freq as next_freq has changed */

> > @@ -829,9 +826,10 @@ static int sugov_start(struct cpufreq_policy *policy)

> >         sg_policy->next_freq                    = 0;

> >         sg_policy->work_in_progress             = false;

> >         sg_policy->limits_changed               = false;

> > -       sg_policy->need_freq_update             = false;

> >         sg_policy->cached_raw_freq              = 0;

> >

> > +       sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);

> > +

> >         for_each_cpu(cpu, policy->cpus) {

> >                 struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);

> >

> > --

>

> I'll queue it up for -rc3 next week, thanks!
Viresh Kumar Nov. 2, 2020, 4:43 a.m. UTC | #3
On 30-10-20, 16:23, Rafael J. Wysocki wrote:
> On Fri, Oct 30, 2020 at 4:07 PM Rafael J. Wysocki <rafael@kernel.org> wrote:

> >

> > On Fri, Oct 30, 2020 at 8:31 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:

> > >

> > > The cpufreq policy's frequency limits (min/max) can get changed at any

> > > point of time, while schedutil is trying to update the next frequency.

> > > Though the schedutil governor has necessary locking and support in place

> > > to make sure we don't miss any of those updates, there is a corner case

> > > where the governor will find that the CPU is already running at the

> > > desired frequency and so may skip an update.

> > >

> > > For example, consider that the CPU can run at 1 GHz, 1.2 GHz and 1.4 GHz

> > > and is running at 1 GHz currently. Schedutil tries to update the

> > > frequency to 1.2 GHz, during this time the policy limits get changed as

> > > policy->min = 1.4 GHz. As schedutil (and cpufreq core) does clamp the

> > > frequency at various instances, we will eventually set the frequency to

> > > 1.4 GHz, while we will save 1.2 GHz in sg_policy->next_freq.

> > >

> > > Now lets say the policy limits get changed back at this time with

> > > policy->min as 1 GHz. The next time schedutil is invoked by the

> > > scheduler, we will reevaluate the next frequency (because

> > > need_freq_update will get set due to limits change event) and lets say

> > > we want to set the frequency to 1.2 GHz again. At this point

> > > sugov_update_next_freq() will find the next_freq == current_freq and

> > > will abort the update, while the CPU actually runs at 1.4 GHz.

> > >

> > > Until now need_freq_update was used as a flag to indicate that the

> > > policy's frequency limits have changed, and that we should consider the

> > > new limits while reevaluating the next frequency.

> > >

> > > This patch fixes the above mentioned issue by extending the purpose of

> > > the need_freq_update flag. If this flag is set now, the schedutil

> > > governor will not try to abort a frequency change even if next_freq ==

> > > current_freq.

> > >

> > > As similar behavior is required in the case of

> > > CPUFREQ_NEED_UPDATE_LIMITS flag as well, need_freq_update will never be

> > > set to false if that flag is set for the driver.

> > >

> > > We also don't need to consider the need_freq_update flag in

> > > sugov_update_single() anymore to handle the special case of busy CPU, as

> > > we won't abort a frequency update anymore.

> > >

> > > Reported-by: zhuguangqing <zhuguangqing@xiaomi.com>

> > > Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> >

> > Thanks for following my suggestion!

> >

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

> > > ---

> > >  kernel/sched/cpufreq_schedutil.c | 22 ++++++++++------------

> > >  1 file changed, 10 insertions(+), 12 deletions(-)

> > >

> > > diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c

> > > index c03a5775d019..c6861be02c86 100644

> > > --- a/kernel/sched/cpufreq_schedutil.c

> > > +++ b/kernel/sched/cpufreq_schedutil.c

> > > @@ -102,9 +102,12 @@ static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)

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

> > >                                    unsigned int next_freq)

> > >  {

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

> > > -           !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))

> > > -               return false;

> > > +       if (!sg_policy->need_freq_update) {

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

> > > +                       return false;

> > > +       } else if (!cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS)) {

> > > +               sg_policy->need_freq_update = false;

> 

> One nit, though.

> 

> This can be changed into

> 

> } else {

>       sg_policy->need_freq_update =

> cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);

> }

> 

> to save a branch and because need_freq_update is there in the cache

> already, this should be a fast update.


Nice.

-- 
viresh
diff mbox series

Patch

diff --git a/kernel/sched/cpufreq_schedutil.c b/kernel/sched/cpufreq_schedutil.c
index c03a5775d019..c6861be02c86 100644
--- a/kernel/sched/cpufreq_schedutil.c
+++ b/kernel/sched/cpufreq_schedutil.c
@@ -102,9 +102,12 @@  static bool sugov_should_update_freq(struct sugov_policy *sg_policy, u64 time)
 static bool sugov_update_next_freq(struct sugov_policy *sg_policy, u64 time,
 				   unsigned int next_freq)
 {
-	if (sg_policy->next_freq == next_freq &&
-	    !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
-		return false;
+	if (!sg_policy->need_freq_update) {
+		if (sg_policy->next_freq == next_freq)
+			return false;
+	} else if (!cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS)) {
+		sg_policy->need_freq_update = false;
+	}
 
 	sg_policy->next_freq = next_freq;
 	sg_policy->last_freq_update_time = time;
@@ -162,11 +165,9 @@  static unsigned int get_next_freq(struct sugov_policy *sg_policy,
 
 	freq = map_util_freq(util, freq, max);
 
-	if (freq == sg_policy->cached_raw_freq && !sg_policy->need_freq_update &&
-	    !cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS))
+	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);
 }
@@ -442,7 +443,6 @@  static void sugov_update_single(struct update_util_data *hook, u64 time,
 	struct sugov_policy *sg_policy = sg_cpu->sg_policy;
 	unsigned long util, max;
 	unsigned int next_f;
-	bool busy;
 	unsigned int cached_freq = sg_policy->cached_raw_freq;
 
 	sugov_iowait_boost(sg_cpu, time, flags);
@@ -453,9 +453,6 @@  static void sugov_update_single(struct update_util_data *hook, u64 time,
 	if (!sugov_should_update_freq(sg_policy, time))
 		return;
 
-	/* Limits may have changed, don't skip frequency update */
-	busy = !sg_policy->need_freq_update && sugov_cpu_is_busy(sg_cpu);
-
 	util = sugov_get_util(sg_cpu);
 	max = sg_cpu->max;
 	util = sugov_iowait_apply(sg_cpu, time, util, max);
@@ -464,7 +461,7 @@  static void sugov_update_single(struct update_util_data *hook, u64 time,
 	 * Do not reduce the frequency if the CPU has not been idle
 	 * recently, as the reduction is likely to be premature then.
 	 */
-	if (busy && next_f < sg_policy->next_freq) {
+	if (sugov_cpu_is_busy(sg_cpu) && next_f < sg_policy->next_freq) {
 		next_f = sg_policy->next_freq;
 
 		/* Restore cached freq as next_freq has changed */
@@ -829,9 +826,10 @@  static int sugov_start(struct cpufreq_policy *policy)
 	sg_policy->next_freq			= 0;
 	sg_policy->work_in_progress		= false;
 	sg_policy->limits_changed		= false;
-	sg_policy->need_freq_update		= false;
 	sg_policy->cached_raw_freq		= 0;
 
+	sg_policy->need_freq_update = cpufreq_driver_test_flags(CPUFREQ_NEED_UPDATE_LIMITS);
+
 	for_each_cpu(cpu, policy->cpus) {
 		struct sugov_cpu *sg_cpu = &per_cpu(sugov_cpu, cpu);