linux-kernel - Re: [PATCH v2 04/19] sched: Prepare for Core-wide rq->lock

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

Message-ID: <YJua6gu8IqSSkDmp@hirez.programming.kicks-ass.net>
Date:   Wed, 12 May 2021 11:07:54 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Aubrey Li <aubrey.intel@...il.com>
Cc:     Joel Fernandes <joel@...lfernandes.org>,
        "Hyser,Chris" <chris.hyser@...cle.com>,
        Josh Don <joshdon@...gle.com>, Ingo Molnar <mingo@...nel.org>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Valentin Schneider <valentin.schneider@....com>,
        Mel Gorman <mgorman@...e.de>,
        Linux List Kernel Mailing <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2 04/19] sched: Prepare for Core-wide rq->lock

On Sat, May 08, 2021 at 04:07:35PM +0800, Aubrey Li wrote:
> > +#ifdef CONFIG_SMP
> > +/*
> > + * double_rq_lock - safely lock two runqueues
> > + */
> > +void double_rq_lock(struct rq *rq1, struct rq *rq2)
> 
> Do we need the static lock checking here?
>         __acquires(rq1->lock)
>         __acquires(rq2->lock)
> 
> > +{
> > +       lockdep_assert_irqs_disabled();
> > +
> > +       if (rq_order_less(rq2, rq1))
> > +               swap(rq1, rq2);
> > +
> > +       raw_spin_rq_lock(rq1);
> > +       if (rq_lockp(rq1) == rq_lockp(rq2)) {
> 
> And here?
>                 __acquire(rq2->lock);
> 
> > +               return;
> }
> > +
> > +       raw_spin_rq_lock_nested(rq2, SINGLE_DEPTH_NESTING);
> > +}
> > +#endif

I'd as soon rip out all that sparse annotation crud; I don't think I've
ever had any benefit from it.


> > @@ -2368,11 +2354,11 @@ static inline void double_rq_unlock(stru
> >         __releases(rq1->lock)
> >         __releases(rq2->lock)
> >  {
> > -       raw_spin_rq_unlock(rq1);
> >         if (rq_lockp(rq1) != rq_lockp(rq2))
> >                 raw_spin_rq_unlock(rq2);
> >         else
> >                 __release(rq2->lock);
> > +       raw_spin_rq_unlock(rq1);
> 
> This change seems not necessary, as the softlockup root cause is not
> the misorder lock release.

No, it really is needed; rq_lockp() is not stable if we don't hold a
lock.