[PATCH v2 0/2] sched/eevdf: Minor fixes for reweight_entity()

[PATCH v2 0/2] sched/eevdf: Minor fixes for reweight_entity()

[Tianchen Ding]

According to the discussion in [1], fix a calculation bug in
reweight_entity().

[1] https://lore.kernel.org/all/59585184-d13d-46e0-8d68-42838e97a702@bytedance.com/

v2:
Add fixes tag to the 1st patch.

v1: https://lore.kernel.org/all/20240304030042.2690-1-dtcccc@linux.alibaba.com/

Tianchen Ding (2):
  sched/eevdf: Always update V if se->on_rq when reweighting
  sched/eevdf: Fix miscalculation in reweight_entity() when se is not
    curr

 kernel/sched/fair.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.39.3

[PATCH v2 1/2] sched/eevdf: Always update V if se->on_rq when reweighting

[Tianchen Ding]

reweight_eevdf() needs the latest V to do accurate calculation for new
ve and vd. So update V unconditionally when se is runnable.

Fixes: eab03c23c2a1 ("sched/eevdf: Fix vruntime adjustment on reweight")
Suggested-by: Abel Wu <wuyun.abel@bytedance.com>
Signed-off-by: Tianchen Ding <dtcccc@linux.alibaba.com>
---
 kernel/sched/fair.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 03be0d1330a6..5551ce2af73e 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3790,9 +3790,8 @@ static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
 
 	if (se->on_rq) {
 		/* commit outstanding execution time */
-		if (curr)
-			update_curr(cfs_rq);
-		else
+		update_curr(cfs_rq);
+		if (!curr)
 			__dequeue_entity(cfs_rq, se);
 		update_load_sub(&cfs_rq->load, se->load.weight);
 	}
-- 
2.39.3

[PATCH v2 2/2] sched/eevdf: Fix miscalculation in reweight_entity() when se is not curr

[Tianchen Ding]

reweight_eevdf() only keeps V unchanged inside itself. When se !=
cfs_rq->curr, it would be dequeued from rb tree first. So that V is
changed and the result is wrong. Pass the original V to reweight_eevdf()
to fix this issue.

Fixes: eab03c23c2a1 ("sched/eevdf: Fix vruntime adjustment on reweight")
Signed-off-by: Tianchen Ding <dtcccc@linux.alibaba.com>
Reviewed-by: Abel Wu <wuyun.abel@bytedance.com>
---
 kernel/sched/fair.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5551ce2af73e..091a1a750638 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3676,11 +3676,10 @@ static inline void
 dequeue_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se) { }
 #endif
 
-static void reweight_eevdf(struct cfs_rq *cfs_rq, struct sched_entity *se,
+static void reweight_eevdf(struct sched_entity *se, u64 avruntime,
 			   unsigned long weight)
 {
 	unsigned long old_weight = se->load.weight;
-	u64 avruntime = avg_vruntime(cfs_rq);
 	s64 vlag, vslice;
 
 	/*
@@ -3787,10 +3786,12 @@ static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
 			    unsigned long weight)
 {
 	bool curr = cfs_rq->curr == se;
+	u64 avruntime;
 
 	if (se->on_rq) {
 		/* commit outstanding execution time */
 		update_curr(cfs_rq);
+		avruntime = avg_vruntime(cfs_rq);
 		if (!curr)
 			__dequeue_entity(cfs_rq, se);
 		update_load_sub(&cfs_rq->load, se->load.weight);
@@ -3804,7 +3805,7 @@ static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
 		 */
 		se->vlag = div_s64(se->vlag * se->load.weight, weight);
 	} else {
-		reweight_eevdf(cfs_rq, se, weight);
+		reweight_eevdf(se, avruntime, weight);
 	}
 
 	update_load_set(&se->load, weight);
-- 
2.39.3

Re: [PATCH v2 1/2] sched/eevdf: Always update V if se->on_rq when reweighting

[Abel Wu]

On 3/6/24 10:21 AM, Tianchen Ding Wrote:
> reweight_eevdf() needs the latest V to do accurate calculation for new
> ve and vd. So update V unconditionally when se is runnable.
> 
> Fixes: eab03c23c2a1 ("sched/eevdf: Fix vruntime adjustment on reweight")
> Suggested-by: Abel Wu <wuyun.abel@bytedance.com>
> Signed-off-by: Tianchen Ding <dtcccc@linux.alibaba.com>
> ---
>   kernel/sched/fair.c | 5 ++---
>   1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 03be0d1330a6..5551ce2af73e 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -3790,9 +3790,8 @@ static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
>   
>   	if (se->on_rq) {
>   		/* commit outstanding execution time */
> -		if (curr)
> -			update_curr(cfs_rq);
> -		else
> +		update_curr(cfs_rq);
> +		if (!curr)
>   			__dequeue_entity(cfs_rq, se);
>   		update_load_sub(&cfs_rq->load, se->load.weight);
>   	}

Reviewed-by: Abel Wu <wuyun.abel@bytedance.com>

Thanks!

Re: [PATCH v2 0/2] sched/eevdf: Minor fixes for reweight_entity()

[K Prateek Nayak]

Hello Tianchen,

On 3/6/2024 7:51 AM, Tianchen Ding wrote:
> According to the discussion in [1], fix a calculation bug in
> reweight_entity().
> 
> [1] https://lore.kernel.org/all/59585184-d13d-46e0-8d68-42838e97a702@bytedance.com/
> 
> v2:
> Add fixes tag to the 1st patch.
> 
> v1: https://lore.kernel.org/all/20240304030042.2690-1-dtcccc@linux.alibaba.com/
> 
> Tianchen Ding (2):
>   sched/eevdf: Always update V if se->on_rq when reweighting
>   sched/eevdf: Fix miscalculation in reweight_entity() when se is not
>     curr

Tested the changes on a dual socket 3rd Generation EPYC system and am
happy to inform that I see no regressions. I'll leave the full results
below:

o System Details

- 3rd Generation EPYC System
- 2 x 64C/128T
- NPS1 mode

o Kernels

tip:		tip:sched/core at commit 8cec3dd9e593
		("sched/core: Simplify code by removing
		 duplicate #ifdefs")

reweight-fix:	tip + this series.

o Results

==================================================================
Test          : hackbench
Units         : Normalized time in seconds
Interpretation: Lower is better
Statistic     : AMean
==================================================================
Case:           tip[pct imp](CV)    reweight-fix[pct imp](CV)
 1-groups     1.00 [ -0.00]( 2.08)     1.01 [ -1.06]( 4.18)
 2-groups     1.00 [ -0.00]( 0.89)     1.02 [ -1.58]( 1.18)
 4-groups     1.00 [ -0.00]( 0.81)     1.01 [ -1.05]( 0.81)
 8-groups     1.00 [ -0.00]( 0.78)     1.00 [  0.15]( 0.72)
16-groups     1.00 [ -0.00]( 1.60)     0.99 [  1.20]( 1.55)


==================================================================
Test          : tbench
Units         : Normalized throughput
Interpretation: Higher is better
Statistic     : AMean
==================================================================
Clients:    tip[pct imp](CV)    reweight-fix[pct imp](CV)
    1     1.00 [  0.00]( 0.71)     1.00 [  0.36]( 0.12)
    2     1.00 [  0.00]( 0.25)     0.99 [ -0.52]( 0.26)
    4     1.00 [  0.00]( 0.85)     1.00 [ -0.35]( 0.93)
    8     1.00 [  0.00]( 1.00)     1.00 [ -0.27]( 0.56)
   16     1.00 [  0.00]( 1.25)     1.02 [  1.73]( 0.63)
   32     1.00 [  0.00]( 0.35)     0.99 [ -0.75]( 1.01)
   64     1.00 [  0.00]( 0.71)     0.99 [ -1.00]( 0.51)
  128     1.00 [  0.00]( 0.46)     1.00 [  0.29]( 0.56)
  256     1.00 [  0.00]( 0.24)     1.00 [  0.08]( 1.07)
  512     1.00 [  0.00]( 0.30)     1.01 [  1.16]( 0.28)
 1024     1.00 [  0.00]( 0.40)     1.01 [  0.81]( 0.43)


==================================================================
Test          : stream-10
Units         : Normalized Bandwidth, MB/s
Interpretation: Higher is better
Statistic     : HMean
==================================================================
Test:       tip[pct imp](CV)    reweight-fix[pct imp](CV)
 Copy     1.00 [  0.00]( 9.73)     1.05 [  4.93]( 1.81)
Scale     1.00 [  0.00]( 5.57)     0.97 [ -2.57]( 6.28)
  Add     1.00 [  0.00]( 5.43)     0.98 [ -2.45]( 5.87)
Triad     1.00 [  0.00]( 5.50)     1.02 [  1.85]( 3.19)


==================================================================
Test          : stream-100
Units         : Normalized Bandwidth, MB/s
Interpretation: Higher is better
Statistic     : HMean
==================================================================
Test:           tip[pct imp](CV)    reweight-fix[pct imp](CV)
 Copy     1.00 [  0.00]( 3.26)     1.01 [  0.54]( 2.31)
Scale     1.00 [  0.00]( 1.26)     0.99 [ -0.68]( 3.86)
  Add     1.00 [  0.00]( 1.47)     0.99 [ -1.13]( 4.32)
Triad     1.00 [  0.00]( 1.77)     1.01 [  0.81]( 3.03)


==================================================================
Test          : netperf
Units         : Normalized Througput
Interpretation: Higher is better
Statistic     : AMean
==================================================================
Clients:         tip[pct imp](CV)    reweight-fix[pct imp](CV)
 1-clients     1.00 [  0.00]( 0.22)     1.00 [ -0.35]( 0.46)
 2-clients     1.00 [  0.00]( 0.57)     1.00 [ -0.12]( 0.16)
 4-clients     1.00 [  0.00]( 0.43)     1.00 [ -0.38]( 0.40)
 8-clients     1.00 [  0.00]( 0.27)     1.00 [ -0.33]( 0.59)
16-clients     1.00 [  0.00]( 0.46)     1.00 [ -0.15]( 0.39)
32-clients     1.00 [  0.00]( 0.95)     1.00 [ -0.26]( 0.60)
64-clients     1.00 [  0.00]( 1.79)     1.00 [  0.19]( 1.76)
128-clients    1.00 [  0.00]( 0.89)     1.00 [  0.45]( 0.81)
256-clients    1.00 [  0.00]( 3.88)     1.00 [  0.42]( 5.53)
512-clients    1.00 [  0.00](35.06)     1.03 [  3.05](53.21)


==================================================================
Test          : schbench
Units         : Normalized 99th percentile latency in us
Interpretation: Lower is better
Statistic     : Median
==================================================================
#workers: tip[pct imp](CV)    reweight-fix[pct imp](CV)
  1     1.00 [ -0.00](27.28)     0.81 [ 18.75](29.23)
  2     1.00 [ -0.00]( 3.85)     1.05 [ -5.00]( 4.76)
  4     1.00 [ -0.00](14.00)     0.95 [  5.26](17.91)
  8     1.00 [ -0.00]( 4.68)     0.85 [ 14.58]( 4.12)
 16     1.00 [ -0.00]( 4.08)     0.95 [  4.84]( 2.60)
 32     1.00 [ -0.00]( 6.68)     0.94 [  6.12]( 2.28)
 64     1.00 [ -0.00]( 1.79)     0.99 [  1.02]( 1.08)
128     1.00 [ -0.00]( 6.30)     0.99 [  1.13]( 3.68)
256     1.00 [ -0.00](43.39)     1.11 [-10.71](10.31)
512     1.00 [ -0.00]( 2.26)     1.00 [ -0.47]( 1.57)


==================================================================
Test          : Unixbench
Units         : Normalized scores
Interpretation: Lower is better
Statistic     : Various (Mentioned)
==================================================================
kernel                                tip             reweight-fix
Hmean     unixbench-dhry2reg-1       0.00%              -0.60%
Hmean     unixbench-dhry2reg-512     0.00%              -0.10%
Amean     unixbench-syscall-1        0.00%               1.61%
Amean     unixbench-syscall-512      0.00%               4.94%
Hmean     unixbench-pipe-1           0.00%               1.01%
Hmean     unixbench-pipe-512         0.00%               0.73%
Hmean     unixbench-spawn-1          0.00%              -5.24%
Hmean     unixbench-spawn-512        0.00%               2.83%
Hmean     unixbench-execl-1          0.00%               0.20%
Hmean     unixbench-execl-512        0.00%               0.10%
--

Feel free to include

Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>

> 
>  kernel/sched/fair.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
--
Thanks and Regards,
Prateek

Re: [PATCH v2 0/2] sched/eevdf: Minor fixes for reweight_entity()

[Tianchen Ding]

On 2024/3/6 10:21, Tianchen Ding wrote:

Gentle ping.

> According to the discussion in [1], fix a calculation bug in
> reweight_entity().
> 
> [1] https://lore.kernel.org/all/59585184-d13d-46e0-8d68-42838e97a702@bytedance.com/
> 
> v2:
> Add fixes tag to the 1st patch.
> 
> v1: https://lore.kernel.org/all/20240304030042.2690-1-dtcccc@linux.alibaba.com/
> 
> Tianchen Ding (2):
>    sched/eevdf: Always update V if se->on_rq when reweighting
>    sched/eevdf: Fix miscalculation in reweight_entity() when se is not
>      curr
> 
>   kernel/sched/fair.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 

Re: [PATCH v2 0/2] sched/eevdf: Minor fixes for reweight_entity()

[Chen Yu]

On 2024-03-06 at 10:21:31 +0800, Tianchen Ding wrote:
> According to the discussion in [1], fix a calculation bug in
> reweight_entity().
> 
> [1] https://lore.kernel.org/all/59585184-d13d-46e0-8d68-42838e97a702@bytedance.com/
> 
> v2:
> Add fixes tag to the 1st patch.
> 
> v1: https://lore.kernel.org/all/20240304030042.2690-1-dtcccc@linux.alibaba.com/
> 
> Tianchen Ding (2):
>   sched/eevdf: Always update V if se->on_rq when reweighting
>   sched/eevdf: Fix miscalculation in reweight_entity() when se is not
>     curr
> 
>  kernel/sched/fair.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> -- 
> 2.39.3
>

Applied this patch set on top of the reweight_entity fix at
https://lore.kernel.org/lkml/ZiEaKOQwiNEglYtS@chenyu5-mobl2/
it has passed 100 cycles of trinity test without any issue.

For the whole serie,

Tested-by: Chen Yu <yu.c.chen@intel.com>

thanks,
Chenyu

Re: [PATCH v2 0/2] sched/eevdf: Minor fixes for reweight_entity()

[Peter Zijlstra]

On Fri, Apr 19, 2024 at 03:27:30PM +0800, Chen Yu wrote:
> On 2024-03-06 at 10:21:31 +0800, Tianchen Ding wrote:
> > According to the discussion in [1], fix a calculation bug in
> > reweight_entity().
> > 
> > [1] https://lore.kernel.org/all/59585184-d13d-46e0-8d68-42838e97a702@bytedance.com/
> > 
> > v2:
> > Add fixes tag to the 1st patch.
> > 
> > v1: https://lore.kernel.org/all/20240304030042.2690-1-dtcccc@linux.alibaba.com/
> > 
> > Tianchen Ding (2):
> >   sched/eevdf: Always update V if se->on_rq when reweighting
> >   sched/eevdf: Fix miscalculation in reweight_entity() when se is not
> >     curr
> > 
> >  kernel/sched/fair.c | 12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > -- 
> > 2.39.3
> >
> 
> Applied this patch set on top of the reweight_entity fix at
> https://lore.kernel.org/lkml/ZiEaKOQwiNEglYtS@chenyu5-mobl2/
> it has passed 100 cycles of trinity test without any issue.
> 
> For the whole serie,
> 
> Tested-by: Chen Yu <yu.c.chen@intel.com>

OK, grabbed these patches (and made some minor edits), now let me go
stare at that other thread again.

[tip: sched/urgent] sched/eevdf: Fix miscalculation in reweight_entity() when se is not curr

[tip-bot2 for Tianchen Ding]

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

Commit-ID:     afae8002b4fd3560c8f5f1567f3c3202c30a70fa
Gitweb:        https://git.kernel.org/tip/afae8002b4fd3560c8f5f1567f3c3202c30a70fa
Author:        Tianchen Ding <dtcccc@linux.alibaba.com>
AuthorDate:    Wed, 06 Mar 2024 10:21:33 +08:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 22 Apr 2024 13:01:26 +02:00

sched/eevdf: Fix miscalculation in reweight_entity() when se is not curr

reweight_eevdf() only keeps V unchanged inside itself. When se !=
cfs_rq->curr, it would be dequeued from rb tree first. So that V is
changed and the result is wrong. Pass the original V to reweight_eevdf()
to fix this issue.

Fixes: eab03c23c2a1 ("sched/eevdf: Fix vruntime adjustment on reweight")
Signed-off-by: Tianchen Ding <dtcccc@linux.alibaba.com>
[peterz: flip if() condition for clarity]
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Abel Wu <wuyun.abel@bytedance.com>
Link: https://lkml.kernel.org/r/20240306022133.81008-3-dtcccc@linux.alibaba.com
---
 kernel/sched/fair.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 5551ce2..6d26691 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3676,11 +3676,10 @@ static inline void
 dequeue_load_avg(struct cfs_rq *cfs_rq, struct sched_entity *se) { }
 #endif
 
-static void reweight_eevdf(struct cfs_rq *cfs_rq, struct sched_entity *se,
+static void reweight_eevdf(struct sched_entity *se, u64 avruntime,
 			   unsigned long weight)
 {
 	unsigned long old_weight = se->load.weight;
-	u64 avruntime = avg_vruntime(cfs_rq);
 	s64 vlag, vslice;
 
 	/*
@@ -3787,24 +3786,26 @@ static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
 			    unsigned long weight)
 {
 	bool curr = cfs_rq->curr == se;
+	u64 avruntime;
 
 	if (se->on_rq) {
 		/* commit outstanding execution time */
 		update_curr(cfs_rq);
+		avruntime = avg_vruntime(cfs_rq);
 		if (!curr)
 			__dequeue_entity(cfs_rq, se);
 		update_load_sub(&cfs_rq->load, se->load.weight);
 	}
 	dequeue_load_avg(cfs_rq, se);
 
-	if (!se->on_rq) {
+	if (se->on_rq) {
+		reweight_eevdf(se, avruntime, weight);
+	} else {
 		/*
 		 * Because we keep se->vlag = V - v_i, while: lag_i = w_i*(V - v_i),
 		 * we need to scale se->vlag when w_i changes.
 		 */
 		se->vlag = div_s64(se->vlag * se->load.weight, weight);
-	} else {
-		reweight_eevdf(cfs_rq, se, weight);
 	}
 
 	update_load_set(&se->load, weight);

[tip: sched/urgent] sched/eevdf: Always update V if se->on_rq when reweighting

[tip-bot2 for Tianchen Ding]

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

Commit-ID:     11b1b8bc2b98e21ddf47e08b56c21502c685b2c3
Gitweb:        https://git.kernel.org/tip/11b1b8bc2b98e21ddf47e08b56c21502c685b2c3
Author:        Tianchen Ding <dtcccc@linux.alibaba.com>
AuthorDate:    Wed, 06 Mar 2024 10:21:32 +08:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 22 Apr 2024 13:01:26 +02:00

sched/eevdf: Always update V if se->on_rq when reweighting

reweight_eevdf() needs the latest V to do accurate calculation for new
ve and vd. So update V unconditionally when se is runnable.

Fixes: eab03c23c2a1 ("sched/eevdf: Fix vruntime adjustment on reweight")
Suggested-by: Abel Wu <wuyun.abel@bytedance.com>
Signed-off-by: Tianchen Ding <dtcccc@linux.alibaba.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Abel Wu <wuyun.abel@bytedance.com>
Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
Tested-by: Chen Yu <yu.c.chen@intel.com>
Link: https://lore.kernel.org/r/20240306022133.81008-2-dtcccc@linux.alibaba.com
---
 kernel/sched/fair.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 03be0d1..5551ce2 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -3790,9 +3790,8 @@ static void reweight_entity(struct cfs_rq *cfs_rq, struct sched_entity *se,
 
 	if (se->on_rq) {
 		/* commit outstanding execution time */
-		if (curr)
-			update_curr(cfs_rq);
-		else
+		update_curr(cfs_rq);
+		if (!curr)
 			__dequeue_entity(cfs_rq, se);
 		update_load_sub(&cfs_rq->load, se->load.weight);
 	}