[PATCH] sched/eevdf: Fixed se->deadline possibly being refilled twice in yield_task_fair()

[PATCH] sched/eevdf: Fixed se->deadline possibly being refilled twice in yield_task_fair()

[Hao Jia]

We call update_deadline() in yield_task_fair(), if se->deadline has been
refilled in update_deadline(). We should avoid filling se->deadline again
in yield_task_fair().

Fixes: 5e963f2bd465 ("sched/fair: Commit to EEVDF")
Signed-off-by: Hao Jia <jiahao.os@bytedance.com>
---
 kernel/sched/fair.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 795ceef5e7e1..b0949e670bc4 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -8695,6 +8695,7 @@ static void yield_task_fair(struct rq *rq)
 	struct task_struct *curr = rq->curr;
 	struct cfs_rq *cfs_rq = task_cfs_rq(curr);
 	struct sched_entity *se = &curr->se;
+	u64 deadline_snap = se->deadline;
 
 	/*
 	 * Are we the only task in the tree?
@@ -8716,6 +8717,14 @@ static void yield_task_fair(struct rq *rq)
 	 */
 	rq_clock_skip_update(rq);
 
+	/*
+	 * If se->deadline has been refilled in
+	 * update_curr()->update_deadline(),
+	 * skip updating again.
+	 */
+	if (READ_ONCE(se->deadline) != deadline_snap)
+		return;
+
 	se->deadline += calc_delta_fair(se->slice, se);
 }
 
-- 
2.20.1

Re: [PATCH] sched/eevdf: Fixed se->deadline possibly being refilled twice in yield_task_fair()

[Peter Zijlstra]

On Thu, Aug 01, 2024 at 11:59:06AM +0800, Hao Jia wrote:
> We call update_deadline() in yield_task_fair(), if se->deadline has been
> refilled in update_deadline(). We should avoid filling se->deadline again
> in yield_task_fair().

Why do you think so? This is purely a timing artifact.

update_curr() simplu catches up with the present. That is, consider two
otherwise identical scenarios, that is, both A and B have the exact same
vruntime/deadline and will call yield() at the exact same time. Except,
A will get a timer tick right before it calls yield().

Then A will get update_curr() from the tick and your new check below
will not avoid it getting its deadline pushed out further.

While B will update_curr() in yield() and then does avoid its deadline
being pushed out.

After which A dna B are no longer the same, even though they did the
exact same thing.

So no, I don't think what you propose is right.

update_rq_clock()/update_curr() simply catch up with 'now'. If that
includes pushing out the deadline so be it, that could've been done
right before by any number of scheduling operations and is immaterial
for the action yield() should take itself.

Does that make sense?

> Fixes: 5e963f2bd465 ("sched/fair: Commit to EEVDF")
> Signed-off-by: Hao Jia <jiahao.os@bytedance.com>
> ---
>  kernel/sched/fair.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 795ceef5e7e1..b0949e670bc4 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -8695,6 +8695,7 @@ static void yield_task_fair(struct rq *rq)
>  	struct task_struct *curr = rq->curr;
>  	struct cfs_rq *cfs_rq = task_cfs_rq(curr);
>  	struct sched_entity *se = &curr->se;
> +	u64 deadline_snap = se->deadline;
>  
>  	/*
>  	 * Are we the only task in the tree?
> @@ -8716,6 +8717,14 @@ static void yield_task_fair(struct rq *rq)
>  	 */
>  	rq_clock_skip_update(rq);
>  
> +	/*
> +	 * If se->deadline has been refilled in
> +	 * update_curr()->update_deadline(),
> +	 * skip updating again.
> +	 */
> +	if (READ_ONCE(se->deadline) != deadline_snap)
> +		return;
> +
>  	se->deadline += calc_delta_fair(se->slice, se);
>  }
>  
> -- 
> 2.20.1
>