linux-kernel - Re: [PATCH v2] sched/fair: Preserve PLACE_DEADLINE

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: <ZR2IzepAXhxnsqLp@chenyu5-mobl2.ccr.corp.intel.com>
Date:   Wed, 4 Oct 2023 23:46:21 +0800
From:   Chen Yu <yu.c.chen@...el.com>
To:     Daniel Jordan <daniel.m.jordan@...cle.com>
CC:     <peterz@...radead.org>, <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.chen.surf@...il.com>
Subject: Re: [PATCH v2] sched/fair: Preserve PLACE_DEADLINE_INITIAL deadline

Hi Daniel,

On 2023-10-04 at 09:09:08 -0400, Daniel Jordan wrote:
> An entity is supposed to get an earlier deadline with
> PLACE_DEADLINE_INITIAL when it's forked, but the deadline gets
> overwritten soon after in enqueue_entity() the first time a forked
> entity is woken so that PLACE_DEADLINE_INITIAL is effectively a no-op.
> 
> Placing in task_fork_fair() seems unnecessary since none of the values
> that get set (slice, vruntime, deadline) are used before they're set
> again at enqueue time, so get rid of that (and with it all of
> task_fork_fair()) and just pass ENQUEUE_INITIAL to enqueue_entity() via
> wake_up_new_task().
> 
> Fixes: e8f331bcc270 ("sched/smp: Use lag to simplify cross-runqueue placement")
> Signed-off-by: Daniel Jordan <daniel.m.jordan@...cle.com>
> ---
> 
> v2
>  - place_entity() seems like the only reason for task_fork_fair() to exist
>    after the recent removal of sysctl_sched_child_runs_first, so take out
>    the whole function.

At first glance I thought if we remove task_fork_fair(), do we lose one chance to
update the parent task's statistic in update_curr()?  We might get out-of-date
parent task's deadline and make preemption decision based on the stale data in
wake_up_new_task() -> wakeup_preempt() -> pick_eevdf(). But after a second thought,
I found that wake_up_new_task() -> enqueue_entity() itself would invoke update_curr(),
so this should not be a problem.

Then I was wondering why can't we just skip place_entity() in enqueue_entity()
if ENQUEUE_WAKEUP is not set, just like the code before e8f331bcc270? In this
way the new fork task's deadline will not be overwritten by wake_up_new_task()->
enqueue_entity(). Then I realized that, after e8f331bcc270, the task's vruntime
and deadline are all calculated by place_entity() rather than being renormalised
to cfs_rq->min_vruntime in enqueue_entity(), so we can not simply skip place_entity()
in enqueue_entity().

Per my understanding, this patch looks good,

Reviewed-by: Chen Yu <yu.c.chen@...el.com>

thanks,
Chenyu