Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 12 May 2021 11:07:54 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Aubrey Li <aubrey.intel@gmail.com>
Cc:     Joel Fernandes <joel@joelfernandes.org>,
        "Hyser,Chris" <chris.hyser@oracle.com>,
        Josh Don <joshdon@google.com>, Ingo Molnar <mingo@kernel.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Valentin Schneider <valentin.schneider@arm.com>,
        Mel Gorman <mgorman@suse.de>,
        Linux List Kernel Mailing <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>
Subject: Re: [PATCH v2 04/19] sched: Prepare for Core-wide rq->lock
Message-ID: <YJua6gu8IqSSkDmp@hirez.programming.kicks-ass.net>
References: <20210422120459.447350175@infradead.org>
 <20210422123308.196692074@infradead.org>
 <YJUNY0dmrJMD/BIm@hirez.programming.kicks-ass.net>
 <CAERHkrvBdFHWxXu=iHAPMe=1dB0qtG_HTMtUbvVviv7_7kc97Q@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAERHkrvBdFHWxXu=iHAPMe=1dB0qtG_HTMtUbvVviv7_7kc97Q@mail.gmail.com>
Precedence: bulk

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.