Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20200326070236.235835-1-walken@google.com> <20200326070236.235835-2-walken@google.com>
 <20200326175644.GN20941@ziepe.ca> <20200326180621.GK22483@bombadil.infradead.org>
 <20200326180916.GL22483@bombadil.infradead.org>
In-Reply-To: <20200326180916.GL22483@bombadil.infradead.org>
From:   Michel Lespinasse <walken@google.com>
Date:   Thu, 26 Mar 2020 15:09:13 -0700
Message-ID: <CANN689GNvU68nQUmR-NJg8To3-8w7f+Hu6s9jAW8ZeKV21JAbA@mail.gmail.com>
Subject: Re: [PATCH 1/8] mmap locking API: initial implementation as rwsem wrappers
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Jason Gunthorpe <jgg@ziepe.ca>,
        Andrew Morton <akpm@linux-foundation.org>,
        linux-mm <linux-mm@kvack.org>,
        LKML <linux-kernel@vger.kernel.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Laurent Dufour <ldufour@linux.ibm.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Liam Howlett <Liam.Howlett@oracle.com>,
        Jerome Glisse <jglisse@redhat.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        David Rientjes <rientjes@google.com>,
        Hugh Dickins <hughd@google.com>, Ying Han <yinghan@google.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

I don't think we strongly need an API for such assertions at this
point. There are actually a number of them (mostly the lockdep form)
being handled in the last patch renaming the mmap_sem field.

If a new form of lock is introduced in the future, it is doable to
implement it in such a way that lockdep assertions will work on it (I
have that working in my range locking patchset). For a range lock you
would probably want to add a new API anyway so that the assert can
verify that the specific range is locked, but IMO there is no strong
justification for new assertion APIs until we get there.

If there is no opposition to replacing rwsem_is_locked with
lockdep_assert_held, then I think that is workable. mmap_is_locked()
only has 5 call sites, so that's not a very large change.

On Thu, Mar 26, 2020 at 11:09 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Thu, Mar 26, 2020 at 11:06:21AM -0700, Matthew Wilcox wrote:
> > On Thu, Mar 26, 2020 at 02:56:44PM -0300, Jason Gunthorpe wrote:
> > > On Thu, Mar 26, 2020 at 12:02:29AM -0700, Michel Lespinasse wrote:
> > >
> > > > +static inline bool mmap_is_locked(struct mm_struct *mm)
> > > > +{
> > > > + return rwsem_is_locked(&mm->mmap_sem) != 0;
> > > > +}
> > >
> > > I've been wondering if the various VM_BUG(rwsem_is_locked()) would be
> > > better as lockdep expressions? Certainly when lockdep is enabled it
> > > should be preferred, IMHO.
> > >
> > > So, I think if inlines are to be introduced this should be something
> > > similar to netdev's ASSERT_RTNL which seems to have worked well.
> > >
> > > Maybe ASSERT_MMAP_SEM_READ/WRITE/HELD() and do the VM_BUG or
> > > lockdep_is_held as appropriate?
> >
> > I'd rather see lockdep_assert_held() used directly rather than have
> > a wrapper.  This API includes options to check for it beind explicitly
> > held for read/write/any, which should be useful.
>
> ... oh, but that requires naming the lock, which we're trying to get
> away from.
>
> I guess we need a wrapper, but yes, by all means, let's level it up
> to put the VM_BUG_ON inside the wrapper, as that means we can get the
> mm dumped everywhere, rather than just the few places that have done
> VM_BUG_ON_MM instead of plain VM_BUG_ON.


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