linux-kernel - Re: [PATCH] sched/fair: Always update

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <20231006195810.GI36277@noisy.programming.kicks-ass.net>
Date:   Fri, 6 Oct 2023 21:58:10 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Daniel Jordan <daniel.m.jordan@...cle.com>
Cc:     bristot@...hat.com, bsegall@...gle.com, chris.hyser@...cle.com,
        corbet@....net, dietmar.eggemann@....com, efault@....de,
        joel@...lfernandes.org, joshdon@...gle.com, juri.lelli@...hat.com,
        kprateek.nayak@....com, linux-kernel@...r.kernel.org,
        mgorman@...e.de, mingo@...nel.org, patrick.bellasi@...bug.net,
        pavel@....cz, pjt@...gle.com, qperret@...gle.com,
        qyousef@...alina.io, rostedt@...dmis.org, tglx@...utronix.de,
        tim.c.chen@...ux.intel.com, timj@....org,
        vincent.guittot@...aro.org, youssefesmat@...omium.org,
        yu.c.chen@...el.com
Subject: Re: [PATCH] sched/fair: Always update_curr() before placing at
 enqueue

On Fri, Oct 06, 2023 at 12:48:26PM -0400, Daniel Jordan wrote:
> Placing wants current's vruntime and the cfs_rq's min_vruntime up to
> date so that avg_runtime() is too, and similarly it wants the entity to
> be re-weighted and lag adjusted so vslice and vlag are fresh, so always
> do update_curr() and update_cfs_group() beforehand.
> 
> There doesn't seem to be a reason to treat the 'curr' case specially
> after e8f331bcc270 since vruntime doesn't get normalized anymore.
> 
> Fixes: e8f331bcc270 ("sched/smp: Use lag to simplify cross-runqueue placement")
> Signed-off-by: Daniel Jordan <daniel.m.jordan@...cle.com>
> ---
> 
> Not sure what the XXX above place_entity() is for, maybe it can go away?
> 
> Based on tip/sched/core.
> 
>  kernel/sched/fair.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 04fbcbda97d5f..db2ca9bf9cc49 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -5047,15 +5047,6 @@ static inline bool cfs_bandwidth_used(void);
>  static void
>  enqueue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
>  {
> -	bool curr = cfs_rq->curr == se;
> -
> -	/*
> -	 * If we're the current task, we must renormalise before calling
> -	 * update_curr().
> -	 */
> -	if (curr)
> -		place_entity(cfs_rq, se, flags);
> -
>  	update_curr(cfs_rq);

IIRC part of the reason for this order is the:

  dequeue
  update
  enqueue

pattern we have all over the place. You don't want the enqueue to move
time forward in this case.

Could be that all magically works, but please double check.