linux-kernel - Re: [PATCH 2/2] sched: Consider task_struct::saved_state in wait_task

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]

Date:   Tue, 26 Jul 2022 11:18:20 +0100
From:   Valentin Schneider <vschneid@...hat.com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc:     linux-kernel@...r.kernel.org,
        "Eric W. Biederman" <ebiederm@...ssion.com>,
        Ben Segall <bsegall@...gle.com>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Mel Gorman <mgorman@...e.de>, Oleg Nesterov <oleg@...hat.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Vincent Guittot <vincent.guittot@...aro.org>
Subject: Re: [PATCH 2/2] sched: Consider task_struct::saved_state in
 wait_task_inactive().

On 26/07/22 08:17, Sebastian Andrzej Siewior wrote:
> On 2022-07-25 18:47:58 [+0100], Valentin Schneider wrote:
>> > --- a/kernel/sched/core.c
>> > +++ b/kernel/sched/core.c
>> > @@ -3257,6 +3257,40 @@ int migrate_swap(struct task_struct *cur
>> >  }
>> >  #endif /* CONFIG_NUMA_BALANCING */
>> >
>> > +#ifdef CONFIG_PREEMPT_RT
>>
>> Would something like the below be useful?
>>
>> /*
>>  * If p->saved_state is anything else than TASK_RUNNING, then p blocked on an
>>  * rtlock *before* voluntarily calling into schedule() after setting its state
>>  * to X. For things like ptrace (X=TASK_TRACED), the task could have more work
>>  * to do upon acquiring the lock before whoever called wait_task_inactive()
>>  * should return. IOW, we have to wait for:
>>  *
>>  *   p.saved_state = TASK_RUNNING
>>  *   p.__state     = X
>>  *
>>  * which implies the task isn't blocked on an RT lock and got to schedule() by
>>  * itself.
>>  *
>>  * Also see comments in ttwu_state_match().
>>  */
>
> This sums up the code. I would s/schedule/schedule_rtlock/ since there
> are two entrypoints.

Right, this any better?

/*
 * Consider:
 *
 *  set_special_state(X);
 *
 *  do_things()
 *    // Somewhere in there is an rtlock that can be contended:
 *    current_save_and_set_rtlock_wait_state();
 *    [...]
 *    schedule_rtlock(); (A)
 *    [...]
 *    current_restore_rtlock_saved_state();
 *
 *  schedule(); (B)
 *
 * If p->saved_state is anything else than TASK_RUNNING, then p blocked on an
 * rtlock (A) *before* voluntarily calling into schedule() (B) after setting its
 * state to X. For things like ptrace (X=TASK_TRACED), the task could have more
 * work to do upon acquiring the lock in do_things() before whoever called
 * wait_task_inactive() should return. IOW, we have to wait for:
 *
 *   p.saved_state = TASK_RUNNING
 *   p.__state     = X
 *
 * which implies the task isn't blocked on an RT lock and got to schedule() (B).
 *
 * Also see comments in ttwu_state_match().
 */