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

[Chengming Zhou <zhouchengming@bytedance.com>]

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!
>>
>