Messages in this thread | | | From | John Stultz <> | Date | Thu, 11 Apr 2024 11:52:57 -0700 | Subject | Re: [RESEND][PATCH v9 1/7] locking/mutex: Remove wakeups from under mutex::wait_lock |
| |
On Tue, Apr 9, 2024 at 9:12 AM Valentin Schneider <vschneid@redhat.com> 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@redhat.com/
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
| |