linux-kernel - Re: [PATCH] sched/core: Minor optimize ttwu

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: <Y2odVSwrCSI2LW2m@hirez.programming.kicks-ass.net>
Date:   Tue, 8 Nov 2022 10:11:49 +0100
From:   Peter Zijlstra <peterz@...radead.org>
To:     Valentin Schneider <vschneid@...hat.com>
Cc:     Chengming Zhou <zhouchengming@...edance.com>, mingo@...hat.com,
        juri.lelli@...hat.com, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, bsegall@...gle.com,
        mgorman@...e.de, bristot@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/core: Minor optimize ttwu_runnable()

On Mon, Nov 07, 2022 at 03:54:38PM +0000, Valentin Schneider wrote:

> So that's the part for the p->sched_class->task_woken() callback, which
> only affects RT and DL (and only does something when !p->on_cpu). I *think*
> it's fine to remove it from ttwu_runnable() as any push/pull should have
> happened when other tasks were enqueued on the same CPU - with that said,
> it wouldn't hurt to double check this :-)
> 
> 
> As for the check_preempt_curr(), since per the above p can be preempted,
> you could have scenarios right now with CFS tasks where
> ttwu_runnable()->check_preempt_curr() causes NEED_RESCHED to be set.
> 
> e.g. p0 does
> 
>   set_current_state(TASK_UNINTERRUPTIBLE)
> 
> but then gets interrupted by the tick, a p1 gets selected to run instead
> because of check_preempt_tick(), and then runs long enough to have
> check_preempt_curr() decide to let p0 preempt p1.
> 
> That does require specific timing (lower tick frequency should make this
> more likely) and probably task niceness distribution too, but isn't
> impossible.
> 
> Maybe try reading p->on_cpu, and only do the quick task state update if
> it's still the current task, otherwise do the preemption checks?

I'm confused...

So all relevant parties take rq->lock:

 - __schedule()
 - scheduler_tick()
 - ttwu_runnable()

So if ttwu_runnable() sees on_rq and switches state back to RUNNING then
neither check_preempt_curr() nor task_woken() make any sense.

Specifically:

 - you can't very well preempt yourself (which is what
   check_preempt_curr() is trying to determine -- if the woken task
   should preempt the running task, they're both the same in this case);

 - the task did not actually wake up, because it was still on the
   runqueue to begin with. This path prevents a sleep, rather than
   issues a wakeup.

So yes, I think the patch as proposed is ok.