Re: [PATCH RFC 6/9] mm/gup: Grab head page refcount once for group of subpages

From: Jason Gunthorpe <jgg@ziepe.ca>
To: Joao Martins <joao.m.martins@oracle.com>
Cc: linux-mm@kvack.org, Dan Williams <dan.j.williams@intel.com>,
	Ira Weiny <ira.weiny@intel.com>,
	linux-nvdimm@lists.01.org, Matthew Wilcox <willy@infradead.org>,
	Jane Chu <jane.chu@oracle.com>,
	Muchun Song <songmuchun@bytedance.com>,
	Mike Kravetz <mike.kravetz@oracle.com>,
	Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [PATCH RFC 6/9] mm/gup: Grab head page refcount once for group of subpages
Date: Wed, 9 Dec 2020 11:15:05 -0400	[thread overview]
Message-ID: <20201209151505.GV5487@ziepe.ca> (raw)
In-Reply-To: <b7f5fe44-f2f5-aea6-57b3-7e14a9ef624d@oracle.com>

On Wed, Dec 09, 2020 at 11:05:39AM +0000, Joao Martins wrote:
> > Why is all of this special? Any time we see a PMD/PGD/etc pointing to
> > PFN we can apply this optimization. How come device has its own
> > special path to do this?? 
> 
> I think the reason is that zone_device struct pages have no
> relationship to one other. So you anyways need to change individual
> pages, as opposed to just the head page.

Huh? That can't be, unpin doesn't know the memory type when it unpins
it, and as your series shows unpin always operates on the compound
head. Thus pinning must also operate on compound heads

> I made it special to avoid breaking other ZONE_DEVICE users (and
> gating that with PGMAP_COMPOUND). But if there's no concerns with
> that, I can unilaterally enable it.

I didn't understand what PGMAP_COMPOUND was supposed to be for..

> > Why do we need to check PGMAP_COMPOUND? Why do we need to get pgmap?
> > (We already removed that from the hmm version of this, was that wrong?
> > Is this different?) Dan?

And this is the key question - why do we need to get a pgmap here?

I'm going to assert that a pgmap cannot be destroyed concurrently with
fast gup running. This is surely true on x86 as the TLB flush that
must have preceeded a pgmap destroy excludes fast gup. Other arches
must emulate this in their pgmap implementations.

So, why do we need pgmap here? Hoping Dan might know

If we delete the pgmap then the devmap stop being special.

CH and I looked at this and deleted it from the hmm side:

commit 068354ade5dd9e2b07d9b0c57055a681db6f4e37
Author: Jason Gunthorpe <jgg@ziepe.ca>
Date:   Fri Mar 27 17:00:13 2020 -0300

    mm/hmm: remove pgmap checking for devmap pages

    The checking boils down to some racy check if the pagemap is still
    available or not. Instead of checking this, rely entirely on the
    notifiers, if a pagemap is destroyed then all pages that belong to it must
    be removed from the tables and the notifiers triggered.

    Link: https://lore.kernel.org/r/20200327200021.29372-2-jgg@ziepe.ca

Though I am wondering if this whole hmm thing is racy with memory
unplug. Hmm.

Jason