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: <xhsmhtu3an4jl.mognet@vschneid.remote.csb>
Date:   Mon, 07 Nov 2022 15:54:38 +0000
From:   Valentin Schneider <vschneid@...hat.com>
To:     Chengming Zhou <zhouchengming@...edance.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 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 :-)

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?

> Thanks!
>