linux-kernel - Re: [PATCH] sched/eevdf: Fixed se->deadline possibly being refilled twice in yield_task

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240801091159.GN33588@noisy.programming.kicks-ass.net>
Date: Thu, 1 Aug 2024 11:11:59 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Hao Jia <jiahao.os@...edance.com>
Cc: mingo@...hat.com, mingo@...nel.org, juri.lelli@...hat.com,
	vincent.guittot@...aro.org, dietmar.eggemann@....com,
	rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
	bristot@...hat.com, vschneid@...hat.com,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/eevdf: Fixed se->deadline possibly being refilled
 twice in yield_task_fair()

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@...edance.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
>