linux-kernel - Re: [RESEND][PATCH v9 1/7] locking/mutex: Remove wakeups from under mutex::wait

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] [day] [month] [year] [list]

Message-ID: <CANDhNCp0hOp9zmfnthFmFM13vGDMBf4TFYHPeRZ=69FjdQVc5w@mail.gmail.com>
Date: Thu, 11 Apr 2024 11:52:57 -0700
From: John Stultz <jstultz@...gle.com>
To: Valentin Schneider <vschneid@...hat.com>
Cc: LKML <linux-kernel@...r.kernel.org>, Peter Zijlstra <peterz@...radead.org>, 
	Joel Fernandes <joelaf@...gle.com>, Qais Yousef <qyousef@...gle.com>, Ingo Molnar <mingo@...hat.com>, 
	Juri Lelli <juri.lelli@...hat.com>, Vincent Guittot <vincent.guittot@...aro.org>, 
	Dietmar Eggemann <dietmar.eggemann@....com>, Steven Rostedt <rostedt@...dmis.org>, 
	Ben Segall <bsegall@...gle.com>, Zimuzo Ezeozue <zezeozue@...gle.com>, 
	Youssef Esmat <youssefesmat@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Daniel Bristot de Oliveira <bristot@...hat.com>, Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>, 
	Boqun Feng <boqun.feng@...il.com>, "Paul E. McKenney" <paulmck@...nel.org>, 
	Metin Kaya <Metin.Kaya@....com>, Xuewen Yan <xuewen.yan94@...il.com>, 
	K Prateek Nayak <kprateek.nayak@....com>, Thomas Gleixner <tglx@...utronix.de>, kernel-team@...roid.com, 
	Davidlohr Bueso <dave@...olabs.net>
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@...hat.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