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

[John Stultz]

On Tue, Apr 9, 2024 at 9:12 AM Valentin Schneider <vschneid@xxxxxxxxxx> wrote:
> On 01/04/24 16:44, John Stultz wrote:
> > @@ -934,6 +942,7 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne
> >               }
> >       }
> >
> > +     preempt_disable();
> >       raw_spin_lock(&lock->wait_lock);
> >       debug_mutex_unlock(lock);
> >       if (!list_empty(&lock->wait_list)) {
> > @@ -952,8 +961,8 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne
> >               __mutex_handoff(lock, next);
> >
>
> (minor nit) Could the preempt_disable() be moved here instead? IMO if it's
> closer to the unlock it makes it clearer why it is there
> (e.g. sched/core.c::affine_move_task(), rt_mutex_setprio(), __sched_setscheduler().
>
> >       raw_spin_unlock(&lock->wait_lock);
> > -
> >       wake_up_q(&wake_q);
> > +     preempt_enable();
> >  }

Heh. Comically, that's how it started, but I was earlier advised to switch it:
  https://lore.kernel.org/lkml/034a302d-773d-5bdb-a32b-bd283d6c7710@xxxxxxxxxx/

I'm happy to go back if that's really preferred.  But the current
style also matches __mutex_lock_common's nesting.


> > @@ -1775,8 +1782,9 @@ static int __sched rt_mutex_slowlock(struct rt_mutex_base *lock,
> >        * irqsave/restore variants.
> >        */
> >       raw_spin_lock_irqsave(&lock->wait_lock, flags);
> > -     ret = __rt_mutex_slowlock_locked(lock, ww_ctx, state);
> > +     ret = __rt_mutex_slowlock_locked(lock, ww_ctx, state, &wake_q);
> >       raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
> > +     wake_up_q(&wake_q);
>
> Shouldn't this also be wrapped in a preempt-disabled region?
>
> >       rt_mutex_post_schedule();
> >
> >       return ret;
>
> > @@ -122,6 +123,7 @@ static int __sched __rwbase_read_lock(struct rwbase_rt *rwb,
> >       if (!ret)
> >               atomic_inc(&rwb->readers);
> >       raw_spin_unlock_irq(&rtm->wait_lock);
> > +     wake_up_q(&wake_q);
>
> Same question wrt preemption.

Yeah, thanks for pointing out that inconsistency. I'll rework and test
with that.

thanks again for the review and feedback!
-john