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;
MIME-Version: 1.0
References: <CANN689HMLC4uKCjztqfZsDO7pPbsvViOQQ_HDWWYU4=8cZbKvQ@mail.gmail.com>
 <20200822163909.GR1362448@hirez.programming.kicks-ass.net>
In-Reply-To: <20200822163909.GR1362448@hirez.programming.kicks-ass.net>
From:   Michel Lespinasse <walken@google.com>
Date:   Sat, 22 Aug 2020 10:24:25 -0700
Message-ID: <CANN689HSUgvJX0bH=V56YfOJ4CYKLo2KA-9GP-wghhc9td=dUw@mail.gmail.com>
Subject: Re: Lockdep question regarding two-level locks
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
        Davidlohr Bueso <dave@stgolabs.net>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, Aug 22, 2020 at 9:39 AM <peterz@infradead.org> wrote:
> On Sat, Aug 22, 2020 at 09:04:09AM -0700, Michel Lespinasse wrote:
> > Hi,
> >
> > I am wondering about how to describe the following situation to lockdep:
> >
> > - lock A would be something that's already implemented (a mutex or
> > possibly a spinlock).
> > - lock B is a range lock, which I would be writing the code for
> > (including lockdep hooks). I do not expect lockdep to know about range
> > locking, but I want it to treat lock B like any other and detect lock
> > ordering issues related to it.
> > - lock A protects a number of structures, including lock B's list of
> > locked ranges, but other structures as well.
> > - lock A is intended to be held for only short periods of time, lock
> > B's ranges might be held for longer.
>
> That's the 'normal' state for blocking locks, no?
>
> See how both struct mutex and struct rw_semaphore have internal locks.

Right (note that I already have an implementation of my range lock
('B') along these lines, with a low-level lock ('A') tucked into it
and all the expected lockdep support).

> > Usage would be along the following lines:
> >
> > acquire:
> > A_lock();
> > // might access data protected by A here
> > bool blocked = B_lock(range); // must be called under lock A; will
> > release lock A if blocking on B.
> > // might access data protected by A here (especially to re-validate in
> > case A was released while blocking on B...)
> > A_unlock()
> >
> > release:
> > A_lock()
> > // might access data protected by A here
> > A_B_unlock(range); // must be called under lock A; releases locks A and B.
>
> up_{read,write}() / mutex_unlock() release 'B', the actual lock, early,
> and then take 'A', the internal lock, to actually implement the release.
>
> That way lockdep doesn't see the B-A order :-)
>
> > There might also be other places that need to lock A for a short time,
> > either inside and outside of lock B.
>
> Any cases that aren't covered by the current implementation of rwsems ?

My issue is that I have other data, unrelated to B, which frequently
needs to be accessed or updated at just the same points where we are
acquiring or releasing B.

(to go into use cases: that data would be the vma rbtree and per-MM
statistics; one may want to find a free gap between vmas before range
locking it, or take vmas in and out of the rbtree as we acquire or
release a lock on the corresponding address range, etc...)

As the accesses to that data tend to naturally align with the places
where we take or release the B lock, it is tempting to expose A to the
caller so that A can protect additional data not directly related to
B. It seems like changing B's internal lock into a public one which
the caller would take and release explicitly around B calls would be
straighforward, but it causes issues as lockdep doesn't understand
that construct...

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.