Re: [PATCH v11 1/7] locking/mutex: Remove wakeups from under mutex::wait_lock

[John Stultz]

On Wed, Jul 10, 2024 at 10:42 AM 'K Prateek Nayak' via kernel-team
<kernel-team@xxxxxxxxxxx> wrote:
> On 7/10/2024 2:01 AM, John Stultz wrote:
> > @@ -681,6 +682,11 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas
> >               }
> >
> >               raw_spin_unlock(&lock->wait_lock);
> > +             /* Make sure we do wakeups before calling schedule */
> > +             if (!wake_q_empty(&wake_q)) {
>
> nit.
>
> This checks seems unnecessary (to my untrained eye). Any harm in
> skipping it and simply doing a wake_up_q() followed by wake_q_init()
> unconditionally?
>
> > +                     wake_up_q(&wake_q);
> > +                     wake_q_init(&wake_q);
> > +             }
> >               schedule_preempt_disabled();

Ah, thank you for the suggestion. I've reworked this in my tree!

> > @@ -1207,6 +1209,7 @@ static int __sched task_blocks_on_rt_mutex(struct rt_mutex_base *lock,
> >       struct rt_mutex_waiter *top_waiter = waiter;
> >       struct rt_mutex_base *next_lock;
> >       int chain_walk = 0, res;
> > +     DEFINE_WAKE_Q(wake_q);
> >
> >       lockdep_assert_held(&lock->wait_lock);
> >
> > @@ -1245,7 +1248,10 @@ static int __sched task_blocks_on_rt_mutex(struct rt_mutex_base *lock,
> >
> >               /* Check whether the waiter should back out immediately */
> >               rtm = container_of(lock, struct rt_mutex, rtmutex);
> > -             res = __ww_mutex_add_waiter(waiter, rtm, ww_ctx);
> > +             preempt_disable();
> > +             res = __ww_mutex_add_waiter(waiter, rtm, ww_ctx, &wake_q);
> > +             wake_up_q(&wake_q);
> > +             preempt_enable();
>
> I'm trying to understand this - we enter task_blocks_on_rt_mutex() with
> "wait_lock" held (I believe the lockdep_assert_held() in the previous
> hunk checks for the same). I walked down the call chain (although
> briefly) and could only spot "task->pi_lock" being locked and unlocked
> before this call to "wake_up_q()" but the "wait_lock" seems to be held
> throughout, only being unlocked and locked again for
> "rt_mutex_adjust_prio_chain()" later down.
>
> Did I miss something or is disabling preemption for this specific hunk
> enough to enable safe nesting?

Thank you for pointing this out! It looks like I need to pipe the
wake_q down through task_blocks_on_rt_mutex(), and
rtlock_slowlock_locked() and define one in rtlock_slowlock().

Really appreciate the review and feedback here!
-john