linux-kernel - Re: [PATCH] sched/core: Minor optimize ttwu

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <346228d3-8b80-4e9b-0157-662429b11a05@bytedance.com>
Date:   Mon, 7 Nov 2022 21:19:16 +0800
From:   Chengming Zhou <zhouchengming@...edance.com>
To:     Valentin Schneider <vschneid@...hat.com>, mingo@...hat.com,
        peterz@...radead.org, juri.lelli@...hat.com,
        vincent.guittot@...aro.org, dietmar.eggemann@....com,
        rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
        bristot@...hat.com
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] sched/core: Minor optimize ttwu_runnable()

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