Messages in this thread | | | Date | Tue, 8 Nov 2022 15:38:54 +0800 | Subject | Re: [PATCH] sched/core: Minor optimize ttwu_runnable() | From | Chengming Zhou <> |
| |
On 2022/11/7 23:54, Valentin Schneider wrote: > 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 :-)
Yes, ttwu_runnable() should be fine to remove p->sched_class->task_woken().
> > > 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 think it's a good suggestion, so we still have the preemption check when p0 is preempted by p1, letting p0 to be scheduled in more timely.
I will update and send v2 later.
> >> Thanks! >> >
| |