Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH] sched/core: Minor optimize ttwu_runnable() | Date | Mon, 07 Nov 2022 15:54:38 +0000 |
| |
On 07/11/22 21:19, Chengming Zhou wrote: > On 2022/11/7 20:56, Valentin Schneider wrote: >> >> So p is still on the rq for sure, but it may not be the currently running >> task. Consider, on a CONFIG_PREEMPT kernel, task p0 in a wait loop: >> >> 0 for (;;) { >> 1 set_current_state(TASK_UNINTERRUPTIBLE); >> 2 >> 3 if (CONDITION) >> 4 break; >> 5 >> 6 schedule(); >> 7 } >> 8 __set_current_state(TASK_RUNNING); >> >> Now further consider p0 executes line 1, but then gets interrupted by an >> IRQ, at the end of which preempt_schedule_irq() happens. We enter >> __schedule(SM_PREEMPT), so p0 isn't dequeued due to its non-zero task >> state, but we *can* end up switching the CPU's current task. > > Thank you very much for this detailed explanation, I get it. Yes, this > process can be seen on CONFIG_PREEMPT kernel. > >> >> ISTR that's why Peter renamed that function ttwu_*runnable*() and not >> ttwu_*running*(). > > So this task p didn't really sleep, it's preempted, later scheduled in, > get to execute line 6 schedule(), but its state has been set to TASK_RUNNING, > it won't deactivate_task(p). >
Right!
> Looks like this patch is still reasonable? Should I put this detailed > explanation in the changelog and send v2? >
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?
> Thanks! >
| |