Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <e17e3aa2c9ac1d6e410f66986da3c41efa9f7462.camel@redhat.com>
Subject: Re: [PATCH v2 1/4] sched/core: Provide sched_rtmutex() and expose
 sched work helpers
From:   Crystal Wood <swood@redhat.com>
To:     Peter Zijlstra <peterz@infradead.org>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc:     linux-kernel@vger.kernel.org, Ben Segall <bsegall@google.com>,
        Boqun Feng <boqun.feng@gmail.com>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Ingo Molnar <mingo@redhat.com>,
        John Stultz <jstultz@google.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Mel Gorman <mgorman@suse.de>,
        Steven Rostedt <rostedt@goodmis.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Valentin Schneider <vschneid@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Waiman Long <longman@redhat.com>, Will Deacon <will@kernel.org>
Date:   Tue, 09 May 2023 17:14:38 -0500
In-Reply-To: <20230503132051.GB1676736@hirez.programming.kicks-ass.net>
References: <20230427111937.2745231-1-bigeasy@linutronix.de>
         <20230427111937.2745231-2-bigeasy@linutronix.de>
         <20230503132051.GB1676736@hirez.programming.kicks-ass.net>
Organization: Red Hat
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-2.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-05-03 at 15:20 +0200, Peter Zijlstra wrote:
> On Thu, Apr 27, 2023 at 01:19:34PM +0200, Sebastian Andrzej Siewior wrote=
:
> > From: Thomas Gleixner <tglx@linutronix.de>
> >=20
> > schedule() invokes sched_submit_work() before scheduling and
> > sched_update_worker() afterwards to ensure that queued block requests
> > are
> > flushed and the (IO)worker machineries can instantiate new workers if
> > required. This avoids deadlocks and starvation.
> >=20
> > With rt_mutexes this can lead to subtle problem:
> >=20
> > =C2=A0 When rtmutex blocks current::pi_blocked_on points to the rtmutex=
 it
> > =C2=A0 blocks on. When one of the functions in sched_submit/resume_work=
()
> > =C2=A0 contends on a rtmutex based lock then that would corrupt
> > =C2=A0 current::pi_blocked_on.
> >=20
> > Make it possible to let rtmutex issue the calls outside of the slowpath=
,
> > i.e. when it is guaranteed that current::pi_blocked_on is NULL, by:
> >=20
> > =C2=A0 - Exposing sched_submit_work() and moving the task_running() con=
dition
> > =C2=A0=C2=A0=C2=A0 into schedule()
> >=20
> > =C2=A0 - Renamimg sched_update_worker() to sched_resume_work() and expo=
sing
> > it
> > =C2=A0=C2=A0=C2=A0 too.
> >=20
> > =C2=A0 - Providing sched_rtmutex() which just does the inner loop of
> > scheduling
> > =C2=A0=C2=A0=C2=A0 until need_resched() is not longer set. Split out th=
e loop so this
> > does
> > =C2=A0=C2=A0=C2=A0 not create yet another copy.
> >=20
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>=20
> Urgh, so I really don't like this.
>=20
> The end result is something like:
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0rt_mutex_lock()
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sched_submit_work(=
);
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // a n=
ested rt_mutex_lock() here will not clobber
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 // ->p=
i_blocked_on because it's not set yet.
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 task_blocks_on_rt_=
mutex();
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 tsk->p=
i_blocked_on =3D waiter;
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rt_mut=
ex_enqueue(lock, waiter); <-- the real problem
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 rt_mutex_slowlock_=
block();
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 schedu=
le_rtmutex();
>=20
> =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 sched_resume_work(=
);
>=20
> And all of this it not just because tsk->pi_blocked_on, but mostly
> because of task_blocks_on_rt_mutex() enqueueing the waiter. The whole
> enqueue thing is what makes the 'simple' solution of saving/restoring
> tsk->pi_blocked_on not work.
>=20
> Basically the pi_blocked_on curruption is a side effect, not the
> fundamental issue. One task having two waiters registered is the bigger
> issue.

Where do you see pi_blocked_on being saved/restored?  The whole point of
this patchset is to deal with sched_submit_work() before anything has
been done on the "outer" lock acquisition (not just pi_blocked_on, but
also enqueuing) other than failing the fast path.

> Now, sched_submit_work() could also use (regular) mutex -- after all
> it's a fully preemptible context. And then we're subject to the 'same'
> problem but with tsk->blocked_on (DEBUG_MUTEXES=3Dy).

It's fully preemptible but it still shouldn't be doing things that would
block on non-RT.  That'd already be broken for a number of reasons (task
state corruption, infinite recursion if current->plug isn't cleared
before doing whatever causes another standard schedule(), etc).

-Crystal