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

[Valentin Schneider <vschneid@redhat.com>]

On 05/11/22 09:34, Chengming Zhou wrote:
> On 2022/11/5 02:19, Valentin Schneider wrote:
>> On 02/11/22 18:23, Chengming Zhou wrote:
>>> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>>> index 87c9cdf37a26..3785418de127 100644
>>> --- a/kernel/sched/core.c
>>> +++ b/kernel/sched/core.c
>>> @@ -3718,9 +3718,8 @@ static int ttwu_runnable(struct task_struct *p, int wake_flags)
>>>
>>>       rq = __task_rq_lock(p, &rf);
>>>       if (task_on_rq_queued(p)) {
>>> -		/* check_preempt_curr() may use rq clock */
>>> -		update_rq_clock(rq);
>>> -		ttwu_do_wakeup(rq, p, wake_flags, &rf);
>>> +		WRITE_ONCE(p->__state, TASK_RUNNING);
>>> +		trace_sched_wakeup(p);
>>
>> This also loses the class->task_woken() call, AFAICT we could have
>> !p->on_cpu here (e.g. an IRQ hit before the task got to schedule()), but
>> then again if there is a need to push we should have done that at the IRQ
>> preempt via set_next_task_{rt, dl}()... So I'm starting to think this is
>> OK, but that needs elaborating in the changelog.
>
> Sorry, I don't get why we could have !p->on_cpu here?
>
> I thought if we have task_on_rq_queued(p) here, it means p hasn't got to
> __schedule() to deactivate_task(), so p should still be on_cpu?
>
> Thanks.
>

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.

ISTR that's why Peter renamed that function ttwu_*runnable*() and not
ttwu_*running*().

>>
>>
>>>               ret = 1;
>>>       }
>>>       __task_rq_unlock(rq, &rf);
>>> --
>>> 2.37.2
>>