Re: [PATCH] sched/core: Minor optimize ttwu_runnable()

[Peter Zijlstra <peterz@infradead.org>]

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.