linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] sched/pelt: Fix task util_est update filtering
@ 2021-02-25 16:58 Vincent Donnefort
  2021-02-26  7:22 ` Vincent Guittot
                   ` (4 more replies)
  0 siblings, 5 replies; 9+ messages in thread
From: Vincent Donnefort @ 2021-02-25 16:58 UTC (permalink / raw)
  To: peterz, mingo, vincent.guittot
  Cc: dietmar.eggemann, linux-kernel, patrick.bellasi,
	valentin.schneider, Vincent Donnefort

Being called for each dequeue, util_est reduces the number of its updates
by filtering out when the EWMA signal is different from the task util_avg
by less than 1%. It is a problem for a sudden util_avg ramp-up. Due to the
decay from a previous high util_avg, EWMA might now be close enough to
the new util_avg. No update would then happen while it would leave
ue.enqueued with an out-of-date value.

Taking into consideration the two util_est members, EWMA and enqueued for
the filtering, ensures, for both, an up-to-date value.

This is for now an issue only for the trace probe that might return the
stale value. Functional-wise, it isn't a problem, as the value is always
accessed through max(enqueued, ewma).

This problem has been observed using LISA's UtilConvergence:test_means on
the sd845c board.

No regression observed with Hackbench on sd845c and Perf-bench sched pipe
on hikey/hikey960.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 9e4104ae39ae..214e02862994 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3966,24 +3966,27 @@ static inline void util_est_dequeue(struct cfs_rq *cfs_rq,
 	trace_sched_util_est_cfs_tp(cfs_rq);
 }
 
+#define UTIL_EST_MARGIN (SCHED_CAPACITY_SCALE / 100)
+
 /*
- * Check if a (signed) value is within a specified (unsigned) margin,
+ * Check if a (signed) value is within the (unsigned) util_est margin,
  * based on the observation that:
  *
  *     abs(x) < y := (unsigned)(x + y - 1) < (2 * y - 1)
  *
- * NOTE: this only works when value + maring < INT_MAX.
+ * NOTE: this only works when value + UTIL_EST_MARGIN < INT_MAX.
  */
-static inline bool within_margin(int value, int margin)
+static inline bool util_est_within_margin(int value)
 {
-	return ((unsigned int)(value + margin - 1) < (2 * margin - 1));
+	return ((unsigned int)(value + UTIL_EST_MARGIN - 1) <
+		(2 * UTIL_EST_MARGIN - 1));
 }
 
 static inline void util_est_update(struct cfs_rq *cfs_rq,
 				   struct task_struct *p,
 				   bool task_sleep)
 {
-	long last_ewma_diff;
+	long last_ewma_diff, last_enqueued_diff;
 	struct util_est ue;
 
 	if (!sched_feat(UTIL_EST))
@@ -4004,6 +4007,8 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 	if (ue.enqueued & UTIL_AVG_UNCHANGED)
 		return;
 
+	last_enqueued_diff = ue.enqueued;
+
 	/*
 	 * Reset EWMA on utilization increases, the moving average is used only
 	 * to smooth utilization decreases.
@@ -4017,12 +4022,17 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 	}
 
 	/*
-	 * Skip update of task's estimated utilization when its EWMA is
+	 * Skip update of task's estimated utilization when its members are
 	 * already ~1% close to its last activation value.
 	 */
 	last_ewma_diff = ue.enqueued - ue.ewma;
-	if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100)))
+	last_enqueued_diff -= ue.enqueued;
+	if (util_est_within_margin(last_ewma_diff)) {
+		if (!util_est_within_margin(last_enqueued_diff))
+			goto done;
+
 		return;
+	}
 
 	/*
 	 * To avoid overestimation of actual task utilization, skip updates if
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] sched/pelt: Fix task util_est update filtering
  2021-02-25 16:58 [PATCH v2] sched/pelt: Fix task util_est update filtering Vincent Donnefort
@ 2021-02-26  7:22 ` Vincent Guittot
  2021-02-26  8:41 ` Peter Zijlstra
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 9+ messages in thread
From: Vincent Guittot @ 2021-02-26  7:22 UTC (permalink / raw)
  To: Vincent Donnefort
  Cc: Peter Zijlstra, mingo, Dietmar Eggemann, linux-kernel,
	Patrick Bellasi, Valentin Schneider

On Thu, 25 Feb 2021 at 17:58, Vincent Donnefort
<vincent.donnefort@arm.com> wrote:
>
> Being called for each dequeue, util_est reduces the number of its updates
> by filtering out when the EWMA signal is different from the task util_avg
> by less than 1%. It is a problem for a sudden util_avg ramp-up. Due to the
> decay from a previous high util_avg, EWMA might now be close enough to
> the new util_avg. No update would then happen while it would leave
> ue.enqueued with an out-of-date value.
>
> Taking into consideration the two util_est members, EWMA and enqueued for
> the filtering, ensures, for both, an up-to-date value.
>
> This is for now an issue only for the trace probe that might return the
> stale value. Functional-wise, it isn't a problem, as the value is always
> accessed through max(enqueued, ewma).
>
> This problem has been observed using LISA's UtilConvergence:test_means on
> the sd845c board.
>
> No regression observed with Hackbench on sd845c and Perf-bench sched pipe
> on hikey/hikey960.
>
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>

Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>

>
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 9e4104ae39ae..214e02862994 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3966,24 +3966,27 @@ static inline void util_est_dequeue(struct cfs_rq *cfs_rq,
>         trace_sched_util_est_cfs_tp(cfs_rq);
>  }
>
> +#define UTIL_EST_MARGIN (SCHED_CAPACITY_SCALE / 100)
> +
>  /*
> - * Check if a (signed) value is within a specified (unsigned) margin,
> + * Check if a (signed) value is within the (unsigned) util_est margin,
>   * based on the observation that:
>   *
>   *     abs(x) < y := (unsigned)(x + y - 1) < (2 * y - 1)
>   *
> - * NOTE: this only works when value + maring < INT_MAX.
> + * NOTE: this only works when value + UTIL_EST_MARGIN < INT_MAX.
>   */
> -static inline bool within_margin(int value, int margin)
> +static inline bool util_est_within_margin(int value)
>  {
> -       return ((unsigned int)(value + margin - 1) < (2 * margin - 1));
> +       return ((unsigned int)(value + UTIL_EST_MARGIN - 1) <
> +               (2 * UTIL_EST_MARGIN - 1));
>  }
>
>  static inline void util_est_update(struct cfs_rq *cfs_rq,
>                                    struct task_struct *p,
>                                    bool task_sleep)
>  {
> -       long last_ewma_diff;
> +       long last_ewma_diff, last_enqueued_diff;
>         struct util_est ue;
>
>         if (!sched_feat(UTIL_EST))
> @@ -4004,6 +4007,8 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
>         if (ue.enqueued & UTIL_AVG_UNCHANGED)
>                 return;
>
> +       last_enqueued_diff = ue.enqueued;
> +
>         /*
>          * Reset EWMA on utilization increases, the moving average is used only
>          * to smooth utilization decreases.
> @@ -4017,12 +4022,17 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
>         }
>
>         /*
> -        * Skip update of task's estimated utilization when its EWMA is
> +        * Skip update of task's estimated utilization when its members are
>          * already ~1% close to its last activation value.
>          */
>         last_ewma_diff = ue.enqueued - ue.ewma;
> -       if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100)))
> +       last_enqueued_diff -= ue.enqueued;
> +       if (util_est_within_margin(last_ewma_diff)) {
> +               if (!util_est_within_margin(last_enqueued_diff))
> +                       goto done;
> +
>                 return;
> +       }
>
>         /*
>          * To avoid overestimation of actual task utilization, skip updates if
> --
> 2.25.1
>

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] sched/pelt: Fix task util_est update filtering
  2021-02-25 16:58 [PATCH v2] sched/pelt: Fix task util_est update filtering Vincent Donnefort
  2021-02-26  7:22 ` Vincent Guittot
@ 2021-02-26  8:41 ` Peter Zijlstra
  2021-03-01 16:34   ` Dietmar Eggemann
  2021-03-02  9:01 ` [tip: sched/core] " tip-bot2 for Vincent Donnefort
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2021-02-26  8:41 UTC (permalink / raw)
  To: Vincent Donnefort
  Cc: mingo, vincent.guittot, dietmar.eggemann, linux-kernel,
	patrick.bellasi, valentin.schneider

On Thu, Feb 25, 2021 at 04:58:20PM +0000, Vincent Donnefort wrote:
> +#define UTIL_EST_MARGIN (SCHED_CAPACITY_SCALE / 100)
> +
>  /*
> - * Check if a (signed) value is within a specified (unsigned) margin,
> + * Check if a (signed) value is within the (unsigned) util_est margin,
>   * based on the observation that:
>   *
>   *     abs(x) < y := (unsigned)(x + y - 1) < (2 * y - 1)
>   *
> - * NOTE: this only works when value + maring < INT_MAX.
> + * NOTE: this only works when value + UTIL_EST_MARGIN < INT_MAX.
>   */
> -static inline bool within_margin(int value, int margin)
> +static inline bool util_est_within_margin(int value)
>  {
> -	return ((unsigned int)(value + margin - 1) < (2 * margin - 1));
> +	return ((unsigned int)(value + UTIL_EST_MARGIN - 1) <
> +		(2 * UTIL_EST_MARGIN - 1));
>  }

> -	if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100)))
> +	if (util_est_within_margin(last_ewma_diff)) {

What was the purpose of this change? What was a generic helper is now
super specific.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] sched/pelt: Fix task util_est update filtering
  2021-02-26  8:41 ` Peter Zijlstra
@ 2021-03-01 16:34   ` Dietmar Eggemann
  2021-03-01 17:21     ` Peter Zijlstra
  0 siblings, 1 reply; 9+ messages in thread
From: Dietmar Eggemann @ 2021-03-01 16:34 UTC (permalink / raw)
  To: Peter Zijlstra, Vincent Donnefort
  Cc: mingo, vincent.guittot, linux-kernel, patrick.bellasi,
	valentin.schneider

On 26/02/2021 09:41, Peter Zijlstra wrote:
> On Thu, Feb 25, 2021 at 04:58:20PM +0000, Vincent Donnefort wrote:
>> +#define UTIL_EST_MARGIN (SCHED_CAPACITY_SCALE / 100)
>> +
>>  /*
>> - * Check if a (signed) value is within a specified (unsigned) margin,
>> + * Check if a (signed) value is within the (unsigned) util_est margin,
>>   * based on the observation that:
>>   *
>>   *     abs(x) < y := (unsigned)(x + y - 1) < (2 * y - 1)
>>   *
>> - * NOTE: this only works when value + maring < INT_MAX.
>> + * NOTE: this only works when value + UTIL_EST_MARGIN < INT_MAX.
>>   */
>> -static inline bool within_margin(int value, int margin)
>> +static inline bool util_est_within_margin(int value)
>>  {
>> -	return ((unsigned int)(value + margin - 1) < (2 * margin - 1));
>> +	return ((unsigned int)(value + UTIL_EST_MARGIN - 1) <
>> +		(2 * UTIL_EST_MARGIN - 1));
>>  }
> 
>> -	if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100)))
>> +	if (util_est_within_margin(last_ewma_diff)) {
> 
> What was the purpose of this change? What was a generic helper is now
> super specific.

I guess because it was only ever used in util_est for last_ewma_diff.

It's now used for last_ewma_diff and last_enqueued_diff, still only for
util_est though and both times with the same margin
(SCHED_CAPACITY_SCALE / 100)).

Vincent D. should be back on Wed from hols.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] sched/pelt: Fix task util_est update filtering
  2021-03-01 16:34   ` Dietmar Eggemann
@ 2021-03-01 17:21     ` Peter Zijlstra
  2021-03-03 10:06       ` Vincent Donnefort
  0 siblings, 1 reply; 9+ messages in thread
From: Peter Zijlstra @ 2021-03-01 17:21 UTC (permalink / raw)
  To: Dietmar Eggemann
  Cc: Vincent Donnefort, mingo, vincent.guittot, linux-kernel,
	patrick.bellasi, valentin.schneider

On Mon, Mar 01, 2021 at 05:34:09PM +0100, Dietmar Eggemann wrote:
> On 26/02/2021 09:41, Peter Zijlstra wrote:
> > On Thu, Feb 25, 2021 at 04:58:20PM +0000, Vincent Donnefort wrote:
> >> +#define UTIL_EST_MARGIN (SCHED_CAPACITY_SCALE / 100)
> >> +
> >>  /*
> >> - * Check if a (signed) value is within a specified (unsigned) margin,
> >> + * Check if a (signed) value is within the (unsigned) util_est margin,
> >>   * based on the observation that:
> >>   *
> >>   *     abs(x) < y := (unsigned)(x + y - 1) < (2 * y - 1)
> >>   *
> >> - * NOTE: this only works when value + maring < INT_MAX.
> >> + * NOTE: this only works when value + UTIL_EST_MARGIN < INT_MAX.
> >>   */
> >> -static inline bool within_margin(int value, int margin)
> >> +static inline bool util_est_within_margin(int value)
> >>  {
> >> -	return ((unsigned int)(value + margin - 1) < (2 * margin - 1));
> >> +	return ((unsigned int)(value + UTIL_EST_MARGIN - 1) <
> >> +		(2 * UTIL_EST_MARGIN - 1));
> >>  }
> > 
> >> -	if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100)))
> >> +	if (util_est_within_margin(last_ewma_diff)) {
> > 
> > What was the purpose of this change? What was a generic helper is now
> > super specific.
> 
> I guess because it was only ever used in util_est for last_ewma_diff.
> 
> It's now used for last_ewma_diff and last_enqueued_diff, still only for
> util_est though and both times with the same margin
> (SCHED_CAPACITY_SCALE / 100)).
> 
> Vincent D. should be back on Wed from hols.

Fair enough; I've un-done it but kept the rest of the patch.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [tip: sched/core] sched/pelt: Fix task util_est update filtering
  2021-02-25 16:58 [PATCH v2] sched/pelt: Fix task util_est update filtering Vincent Donnefort
  2021-02-26  7:22 ` Vincent Guittot
  2021-02-26  8:41 ` Peter Zijlstra
@ 2021-03-02  9:01 ` tip-bot2 for Vincent Donnefort
  2021-03-03  9:49 ` tip-bot2 for Vincent Donnefort
  2021-03-06 11:42 ` tip-bot2 for Vincent Donnefort
  4 siblings, 0 replies; 9+ messages in thread
From: tip-bot2 for Vincent Donnefort @ 2021-03-02  9:01 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Vincent Donnefort, Peter Zijlstra (Intel),
	Dietmar Eggemann, Vincent Guittot, x86, linux-kernel

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     78ca1ab2718a5518171f2e7d0afad0b9752c4453
Gitweb:        https://git.kernel.org/tip/78ca1ab2718a5518171f2e7d0afad0b9752c4453
Author:        Vincent Donnefort <vincent.donnefort@arm.com>
AuthorDate:    Thu, 25 Feb 2021 16:58:20 
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 01 Mar 2021 18:17:26 +01:00

sched/pelt: Fix task util_est update filtering

Being called for each dequeue, util_est reduces the number of its updates
by filtering out when the EWMA signal is different from the task util_avg
by less than 1%. It is a problem for a sudden util_avg ramp-up. Due to the
decay from a previous high util_avg, EWMA might now be close enough to
the new util_avg. No update would then happen while it would leave
ue.enqueued with an out-of-date value.

Taking into consideration the two util_est members, EWMA and enqueued for
the filtering, ensures, for both, an up-to-date value.

This is for now an issue only for the trace probe that might return the
stale value. Functional-wise, it isn't a problem, as the value is always
accessed through max(enqueued, ewma).

This problem has been observed using LISA's UtilConvergence:test_means on
the sd845c board.

No regression observed with Hackbench on sd845c and Perf-bench sched pipe
on hikey/hikey960.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
Link: https://lkml.kernel.org/r/20210225165820.1377125-1-vincent.donnefort@arm.com
---
 kernel/sched/fair.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 1af51a6..f5d6541 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3941,6 +3941,8 @@ static inline void util_est_dequeue(struct cfs_rq *cfs_rq,
 	trace_sched_util_est_cfs_tp(cfs_rq);
 }
 
+#define UTIL_EST_MARGIN (SCHED_CAPACITY_SCALE / 100)
+
 /*
  * Check if a (signed) value is within a specified (unsigned) margin,
  * based on the observation that:
@@ -3958,7 +3960,7 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 				   struct task_struct *p,
 				   bool task_sleep)
 {
-	long last_ewma_diff;
+	long last_ewma_diff, last_enqueued_diff;
 	struct util_est ue;
 
 	if (!sched_feat(UTIL_EST))
@@ -3979,6 +3981,8 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 	if (ue.enqueued & UTIL_AVG_UNCHANGED)
 		return;
 
+	last_enqueued_diff = ue.enqueued;
+
 	/*
 	 * Reset EWMA on utilization increases, the moving average is used only
 	 * to smooth utilization decreases.
@@ -3992,12 +3996,17 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 	}
 
 	/*
-	 * Skip update of task's estimated utilization when its EWMA is
+	 * Skip update of task's estimated utilization when its members are
 	 * already ~1% close to its last activation value.
 	 */
 	last_ewma_diff = ue.enqueued - ue.ewma;
-	if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100)))
+	last_enqueued_diff -= ue.enqueued;
+	if (within_margin(last_ewma_diff, UTIL_EST_MARGIN)) {
+		if (!within_margin(last_enqueued_diff, UTIL_EST_MARGIN))
+			goto done;
+
 		return;
+	}
 
 	/*
 	 * To avoid overestimation of actual task utilization, skip updates if

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* [tip: sched/core] sched/pelt: Fix task util_est update filtering
  2021-02-25 16:58 [PATCH v2] sched/pelt: Fix task util_est update filtering Vincent Donnefort
                   ` (2 preceding siblings ...)
  2021-03-02  9:01 ` [tip: sched/core] " tip-bot2 for Vincent Donnefort
@ 2021-03-03  9:49 ` tip-bot2 for Vincent Donnefort
  2021-03-06 11:42 ` tip-bot2 for Vincent Donnefort
  4 siblings, 0 replies; 9+ messages in thread
From: tip-bot2 for Vincent Donnefort @ 2021-03-03  9:49 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Vincent Donnefort, Peter Zijlstra (Intel),
	Dietmar Eggemann, Vincent Guittot, x86, linux-kernel

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     9357e217ba642b39ce89f9cd5b5f3e5a21712283
Gitweb:        https://git.kernel.org/tip/9357e217ba642b39ce89f9cd5b5f3e5a21712283
Author:        Vincent Donnefort <vincent.donnefort@arm.com>
AuthorDate:    Thu, 25 Feb 2021 16:58:20 
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Wed, 03 Mar 2021 10:33:00 +01:00

sched/pelt: Fix task util_est update filtering

Being called for each dequeue, util_est reduces the number of its updates
by filtering out when the EWMA signal is different from the task util_avg
by less than 1%. It is a problem for a sudden util_avg ramp-up. Due to the
decay from a previous high util_avg, EWMA might now be close enough to
the new util_avg. No update would then happen while it would leave
ue.enqueued with an out-of-date value.

Taking into consideration the two util_est members, EWMA and enqueued for
the filtering, ensures, for both, an up-to-date value.

This is for now an issue only for the trace probe that might return the
stale value. Functional-wise, it isn't a problem, as the value is always
accessed through max(enqueued, ewma).

This problem has been observed using LISA's UtilConvergence:test_means on
the sd845c board.

No regression observed with Hackbench on sd845c and Perf-bench sched pipe
on hikey/hikey960.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
Link: https://lkml.kernel.org/r/20210225165820.1377125-1-vincent.donnefort@arm.com
---
 kernel/sched/fair.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 1af51a6..f5d6541 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3941,6 +3941,8 @@ static inline void util_est_dequeue(struct cfs_rq *cfs_rq,
 	trace_sched_util_est_cfs_tp(cfs_rq);
 }
 
+#define UTIL_EST_MARGIN (SCHED_CAPACITY_SCALE / 100)
+
 /*
  * Check if a (signed) value is within a specified (unsigned) margin,
  * based on the observation that:
@@ -3958,7 +3960,7 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 				   struct task_struct *p,
 				   bool task_sleep)
 {
-	long last_ewma_diff;
+	long last_ewma_diff, last_enqueued_diff;
 	struct util_est ue;
 
 	if (!sched_feat(UTIL_EST))
@@ -3979,6 +3981,8 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 	if (ue.enqueued & UTIL_AVG_UNCHANGED)
 		return;
 
+	last_enqueued_diff = ue.enqueued;
+
 	/*
 	 * Reset EWMA on utilization increases, the moving average is used only
 	 * to smooth utilization decreases.
@@ -3992,12 +3996,17 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 	}
 
 	/*
-	 * Skip update of task's estimated utilization when its EWMA is
+	 * Skip update of task's estimated utilization when its members are
 	 * already ~1% close to its last activation value.
 	 */
 	last_ewma_diff = ue.enqueued - ue.ewma;
-	if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100)))
+	last_enqueued_diff -= ue.enqueued;
+	if (within_margin(last_ewma_diff, UTIL_EST_MARGIN)) {
+		if (!within_margin(last_enqueued_diff, UTIL_EST_MARGIN))
+			goto done;
+
 		return;
+	}
 
 	/*
 	 * To avoid overestimation of actual task utilization, skip updates if

^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] sched/pelt: Fix task util_est update filtering
  2021-03-01 17:21     ` Peter Zijlstra
@ 2021-03-03 10:06       ` Vincent Donnefort
  0 siblings, 0 replies; 9+ messages in thread
From: Vincent Donnefort @ 2021-03-03 10:06 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Dietmar Eggemann, vincent.guittot, linux-kernel, patrick.bellasi,
	valentin.schneider

On Mon, Mar 01, 2021 at 06:21:23PM +0100, Peter Zijlstra wrote:
> On Mon, Mar 01, 2021 at 05:34:09PM +0100, Dietmar Eggemann wrote:
> > On 26/02/2021 09:41, Peter Zijlstra wrote:
> > > On Thu, Feb 25, 2021 at 04:58:20PM +0000, Vincent Donnefort wrote:
> > >> +#define UTIL_EST_MARGIN (SCHED_CAPACITY_SCALE / 100)
> > >> +
> > >>  /*
> > >> - * Check if a (signed) value is within a specified (unsigned) margin,
> > >> + * Check if a (signed) value is within the (unsigned) util_est margin,
> > >>   * based on the observation that:
> > >>   *
> > >>   *     abs(x) < y := (unsigned)(x + y - 1) < (2 * y - 1)
> > >>   *
> > >> - * NOTE: this only works when value + maring < INT_MAX.
> > >> + * NOTE: this only works when value + UTIL_EST_MARGIN < INT_MAX.
> > >>   */
> > >> -static inline bool within_margin(int value, int margin)
> > >> +static inline bool util_est_within_margin(int value)
> > >>  {
> > >> -	return ((unsigned int)(value + margin - 1) < (2 * margin - 1));
> > >> +	return ((unsigned int)(value + UTIL_EST_MARGIN - 1) <
> > >> +		(2 * UTIL_EST_MARGIN - 1));
> > >>  }
> > > 
> > >> -	if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100)))
> > >> +	if (util_est_within_margin(last_ewma_diff)) {
> > > 
> > > What was the purpose of this change? What was a generic helper is now
> > > super specific.
> > 
> > I guess because it was only ever used in util_est for last_ewma_diff.
> > 
> > It's now used for last_ewma_diff and last_enqueued_diff, still only for
> > util_est though and both times with the same margin
> > (SCHED_CAPACITY_SCALE / 100)).
> > 
> > Vincent D. should be back on Wed from hols.
> 
> Fair enough; I've un-done it but kept the rest of the patch.

Indeed I was off for couple of days. Apologies for the delay and thanks for
applying the patch.

-- 
Vincent

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [tip: sched/core] sched/pelt: Fix task util_est update filtering
  2021-02-25 16:58 [PATCH v2] sched/pelt: Fix task util_est update filtering Vincent Donnefort
                   ` (3 preceding siblings ...)
  2021-03-03  9:49 ` tip-bot2 for Vincent Donnefort
@ 2021-03-06 11:42 ` tip-bot2 for Vincent Donnefort
  4 siblings, 0 replies; 9+ messages in thread
From: tip-bot2 for Vincent Donnefort @ 2021-03-06 11:42 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Vincent Donnefort, Peter Zijlstra (Intel),
	Ingo Molnar, Dietmar Eggemann, Vincent Guittot, x86,
	linux-kernel

The following commit has been merged into the sched/core branch of tip:

Commit-ID:     b89997aa88f0b07d8a6414c908af75062103b8c9
Gitweb:        https://git.kernel.org/tip/b89997aa88f0b07d8a6414c908af75062103b8c9
Author:        Vincent Donnefort <vincent.donnefort@arm.com>
AuthorDate:    Thu, 25 Feb 2021 16:58:20 
Committer:     Ingo Molnar <mingo@kernel.org>
CommitterDate: Sat, 06 Mar 2021 12:40:22 +01:00

sched/pelt: Fix task util_est update filtering

Being called for each dequeue, util_est reduces the number of its updates
by filtering out when the EWMA signal is different from the task util_avg
by less than 1%. It is a problem for a sudden util_avg ramp-up. Due to the
decay from a previous high util_avg, EWMA might now be close enough to
the new util_avg. No update would then happen while it would leave
ue.enqueued with an out-of-date value.

Taking into consideration the two util_est members, EWMA and enqueued for
the filtering, ensures, for both, an up-to-date value.

This is for now an issue only for the trace probe that might return the
stale value. Functional-wise, it isn't a problem, as the value is always
accessed through max(enqueued, ewma).

This problem has been observed using LISA's UtilConvergence:test_means on
the sd845c board.

No regression observed with Hackbench on sd845c and Perf-bench sched pipe
on hikey/hikey960.

Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Signed-off-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
Link: https://lkml.kernel.org/r/20210225165820.1377125-1-vincent.donnefort@arm.com
---
 kernel/sched/fair.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 1af51a6..f5d6541 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3941,6 +3941,8 @@ static inline void util_est_dequeue(struct cfs_rq *cfs_rq,
 	trace_sched_util_est_cfs_tp(cfs_rq);
 }
 
+#define UTIL_EST_MARGIN (SCHED_CAPACITY_SCALE / 100)
+
 /*
  * Check if a (signed) value is within a specified (unsigned) margin,
  * based on the observation that:
@@ -3958,7 +3960,7 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 				   struct task_struct *p,
 				   bool task_sleep)
 {
-	long last_ewma_diff;
+	long last_ewma_diff, last_enqueued_diff;
 	struct util_est ue;
 
 	if (!sched_feat(UTIL_EST))
@@ -3979,6 +3981,8 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 	if (ue.enqueued & UTIL_AVG_UNCHANGED)
 		return;
 
+	last_enqueued_diff = ue.enqueued;
+
 	/*
 	 * Reset EWMA on utilization increases, the moving average is used only
 	 * to smooth utilization decreases.
@@ -3992,12 +3996,17 @@ static inline void util_est_update(struct cfs_rq *cfs_rq,
 	}
 
 	/*
-	 * Skip update of task's estimated utilization when its EWMA is
+	 * Skip update of task's estimated utilization when its members are
 	 * already ~1% close to its last activation value.
 	 */
 	last_ewma_diff = ue.enqueued - ue.ewma;
-	if (within_margin(last_ewma_diff, (SCHED_CAPACITY_SCALE / 100)))
+	last_enqueued_diff -= ue.enqueued;
+	if (within_margin(last_ewma_diff, UTIL_EST_MARGIN)) {
+		if (!within_margin(last_enqueued_diff, UTIL_EST_MARGIN))
+			goto done;
+
 		return;
+	}
 
 	/*
 	 * To avoid overestimation of actual task utilization, skip updates if

^ permalink raw reply related	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-03-06 11:43 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-25 16:58 [PATCH v2] sched/pelt: Fix task util_est update filtering Vincent Donnefort
2021-02-26  7:22 ` Vincent Guittot
2021-02-26  8:41 ` Peter Zijlstra
2021-03-01 16:34   ` Dietmar Eggemann
2021-03-01 17:21     ` Peter Zijlstra
2021-03-03 10:06       ` Vincent Donnefort
2021-03-02  9:01 ` [tip: sched/core] " tip-bot2 for Vincent Donnefort
2021-03-03  9:49 ` tip-bot2 for Vincent Donnefort
2021-03-06 11:42 ` tip-bot2 for Vincent Donnefort

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).