lkml.org 
[lkml]   [2022]   [Nov]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] sched/core: Minor optimize ttwu_runnable()
From
On 2022/11/7 20:56, Valentin Schneider wrote:
> 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.

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).

Looks like this patch is still reasonable? Should I put this detailed
explanation in the changelog and send v2?

Thanks!

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

\
 
 \ /
  Last update: 2022-11-07 14:19    [W:0.051 / U:2.688 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site