[PATCH][RFC] sched: cpufreq: Fix long idle judgement logic in load calculation

[PATCH][RFC] sched: cpufreq: Fix long idle judgement logic in load calculation

[Chen Yu]

According to current code implementation, detecting the long
idle period is done by checking if the interval between two
adjacent utilization update handers is long enough. Although
this mechanism can detect if the idle period is long enough
(no utilization hooks invoked during idle period), it might
not contain a corner case: if the task has occupied the cpu
for too long which causes no context switch during that
period, then no utilization handler will be launched until this
high prio task is switched out. As a result, the idle_periods
field might be calculated incorrectly because it regards the
100% load as 0% and makes the conservative governor who uses
this field confusing.

Change the judgement to compare the idle_time with sampling_rate
directly.

Reported-by: Artem S. Tashkinov <t.artem@mailcity.com>
Cc: Artem S Tashkinov <t.artem@mailcity.com>
Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Cc: Viresh Kumar <viresh.kumar@linaro.org>
Cc: linux-pm@vger.kernel.org
Signed-off-by: Chen Yu <yu.c.chen@intel.com>
---
 drivers/cpufreq/cpufreq_governor.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
index 871bf9c..9792c80 100644
--- a/drivers/cpufreq/cpufreq_governor.c
+++ b/drivers/cpufreq/cpufreq_governor.c
@@ -165,7 +165,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
 			 * calls, so the previous load value can be used then.
 			 */
 			load = j_cdbs->prev_load;
-		} else if (unlikely(time_elapsed > 2 * sampling_rate &&
+		} else if (((int)idle_time > 0) && unlikely(idle_time > 2 * sampling_rate &&
 				    j_cdbs->prev_load)) {
 			/*
 			 * If the CPU had gone completely idle and a task has
@@ -185,10 +185,8 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
 			 * clear prev_load to guarantee that the load will be
 			 * computed again next time.
 			 *
-			 * Detecting this situation is easy: the governor's
-			 * utilization update handler would not have run during
-			 * CPU-idle periods.  Hence, an unusually large
-			 * 'time_elapsed' (as compared to the sampling rate)
+			 * Detecting this situation is easy: an unusually large
+			 * 'idle_time' (as compared to the sampling rate)
 			 * indicates this scenario.
 			 */
 			load = j_cdbs->prev_load;
@@ -217,8 +215,8 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
 			j_cdbs->prev_load = load;
 		}
 
-		if (time_elapsed > 2 * sampling_rate) {
-			unsigned int periods = time_elapsed / sampling_rate;
+		if (((int)idle_time > 0) && (idle_time > 2 * sampling_rate)) {
+			unsigned int periods = idle_time / sampling_rate;
 
 			if (periods < idle_periods)
 				idle_periods = periods;
-- 
2.7.4

Re: [PATCH][RFC] sched: cpufreq: Fix long idle judgement logic in load calculation

[Viresh Kumar]

On 07-06-18, 11:17, Chen Yu wrote:
> diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> index 871bf9c..9792c80 100644
> --- a/drivers/cpufreq/cpufreq_governor.c
> +++ b/drivers/cpufreq/cpufreq_governor.c
> @@ -165,7 +165,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
>  			 * calls, so the previous load value can be used then.
>  			 */
>  			load = j_cdbs->prev_load;
> -		} else if (unlikely(time_elapsed > 2 * sampling_rate &&
> +		} else if (((int)idle_time > 0) && unlikely(idle_time > 2 * sampling_rate &&

Yes the figures are insane, but if the idle time is around 36 minutes, the
conversion to int will make a positive value look negative and we will exit the
conditional block. And if we don't think that we will ever get such insane idle
times or we don't want to care about them, then what about doing this instead:

		} else if ((unlikely((int)idle_time > 2 * sampling_rate &&

same below.

-- 
viresh

Re: [PATCH][RFC] sched: cpufreq: Fix long idle judgement logic in load calculation

[Yu Chen]

Hi Viresh,
thanks for the comment,
On Thu, Jun 07, 2018 at 10:15:43AM +0530, Viresh Kumar wrote:
> On 07-06-18, 11:17, Chen Yu wrote:
> > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c
> > index 871bf9c..9792c80 100644
> > --- a/drivers/cpufreq/cpufreq_governor.c
> > +++ b/drivers/cpufreq/cpufreq_governor.c
> > @@ -165,7 +165,7 @@ unsigned int dbs_update(struct cpufreq_policy *policy)
> >  			 * calls, so the previous load value can be used then.
> >  			 */
> >  			load = j_cdbs->prev_load;
> > -		} else if (unlikely(time_elapsed > 2 * sampling_rate &&
> > +		} else if (((int)idle_time > 0) && unlikely(idle_time > 2 * sampling_rate &&
> 
> Yes the figures are insane, but if the idle time is around 36 minutes, the
> conversion to int will make a positive value look negative and we will exit the
> conditional block. And if we don't think that we will ever get such insane idle
> times or we don't want to care about them, then what about doing this instead:
> 
> 		} else if ((unlikely((int)idle_time > 2 * sampling_rate &&
> 
> same below.
> 
Yes, this would be more straightforward.

Best,
Yu
> -- 
> viresh