Re: [PATCH 07/10] mm/hmm: add an helper function that fault pages and map them to a device

From: Dan Williams <dan.j.williams@intel.com>
To: Jerome Glisse <jglisse@redhat.com>
Cc: linux-mm <linux-mm@kvack.org>,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Andrew Morton <akpm@linux-foundation.org>,
	Ralph Campbell <rcampbell@nvidia.com>,
	John Hubbard <jhubbard@nvidia.com>
Subject: Re: [PATCH 07/10] mm/hmm: add an helper function that fault pages and map them to a device
Date: Mon, 18 Mar 2019 14:30:15 -0700	[thread overview]
Message-ID: <CAPcyv4he6v5JQMucezZV4J3i+Ea-i7AsaGpCOnc4f-stCrhGag@mail.gmail.com> (raw)
In-Reply-To: <20190318204134.GD6786@redhat.com>

On Mon, Mar 18, 2019 at 1:41 PM Jerome Glisse <jglisse@redhat.com> wrote:
>
> On Mon, Mar 18, 2019 at 01:21:00PM -0700, Dan Williams wrote:
> > On Tue, Jan 29, 2019 at 8:55 AM <jglisse@redhat.com> wrote:
> > >
> > > From: Jérôme Glisse <jglisse@redhat.com>
> > >
> > > This is a all in one helper that fault pages in a range and map them to
> > > a device so that every single device driver do not have to re-implement
> > > this common pattern.
> >
> > Ok, correct me if I am wrong but these seem effectively be the typical
> > "get_user_pages() + dma_map_page()" pattern that non-HMM drivers would
> > follow. Could we just teach get_user_pages() to take an HMM shortcut
> > based on the range?
> >
> > I'm interested in being able to share code across drivers and not have
> > to worry about the HMM special case at the api level.
> >
> > And to be clear this isn't an anti-HMM critique this is a "yes, let's
> > do this, but how about a more fundamental change".
>
> It is a yes and no, HMM have the synchronization with mmu notifier
> which is not common to all device driver ie you have device driver
> that do not synchronize with mmu notifier and use GUP. For instance
> see the range->valid test in below code this is HMM specific and it
> would not apply to GUP user.
>
> Nonetheless i want to remove more HMM code and grow GUP to do some
> of this too so that HMM and non HMM driver can share the common part
> (under GUP). But right now updating GUP is a too big endeavor.

I'm open to that argument, but that statement then seems to indicate
that these apis are indeed temporary. If the end game is common api
between HMM and non-HMM drivers then I think these should at least
come with /* TODO: */ comments about what might change in the future,
and then should be EXPORT_SYMBOL_GPL since they're already planning to
be deprecated. They are a point in time export for a work-in-progress
interface.

> I need
> to make progress on more driver with HMM before thinking of messing
> with GUP code. Making that code HMM only for now will make the GUP
> factorization easier and smaller down the road (should only need to
> update HMM helper and not each individual driver which use HMM).
>
> FYI here is my todo list:
>     - this patchset
>     - HMM ODP
>     - mmu notifier changes for optimization and device range binding
>     - device range binding (amdgpu/nouveau/...)
>     - factor out some nouveau deep inner-layer code to outer-layer for
>       more code sharing
>     - page->mapping endeavor for generic page protection for instance
>       KSM with file back page
>     - grow GUP to remove HMM code and consolidate with GUP code

Sounds workable as a plan.