Re: [PATCH 0/5] mm: rework hmm to use devm_memremap_pages

[Jerome Glisse]

On Tue, Jun 05, 2018 at 06:33:04PM -0700, Dan Williams wrote:
> On Tue, Jun 5, 2018 at 5:08 PM, Jerome Glisse <jglisse@xxxxxxxxxx> wrote:
> > On Tue, Jun 05, 2018 at 04:06:12PM -0700, Dan Williams wrote:
> [..]
> >> I want the EXPORT_SYMBOL_GPL on devm_memremap_pages() primarily for
> >> development purposes. Any new users of devm_memremap_pages() should be
> >> aware that they are subscribing to the whims of the core-VM, i.e. the
> >> ongoing evolution of 'struct page', and encourage those drivers to be
> >> upstream to improve the implementation, and consolidate use cases. I'm
> >> not qualified to comment on your "nor will it change anyone's legal
> >> position.", but I'm saying it's in the Linux kernel's best interest
> >> that new users of this interface assume they need to be GPL.
> >
> > Note that HMM isolate the device driver from struct page as long as
> > the driver only use HMM helpers to get to the information it needs.
> > I intend to be pedantic about that with any driver using HMM. I want
> > HMM to be an impedance layer that provide stable and simple API to
> > device driver while preserving freedom of change to mm.
> >
> 
> I would not classify redefining put_page() and recompiling the
> entirety of the kernel to turn on HMM as "isolating the driver from
> 'struct page'". HMM is instead isolating these out of drivers from
> ever needing to go upstream.

Well i guess it is better to leave it there as i don't think we can
agree on that. I spelled out the API contract HMM is providing and it
can be implemented in other ways. The fact that it uses ZONE_DEVICE is
an implementation details to driver using HMM. In essence driver is
not subscribing in anyway to the whims of the core-VM.

> 
> Unless the nouveau patches are using the entirety of what is already
> upstream for HMM, we should look to pare HMM back.

Nouveau patches use everything in HMM, including ZONE_DEVICE private,
excluding ZONE_DEVICE public for now. The ZONE_DEVICE public is only
meaningful on PowerPC platform for now and it requires Volta GPU.

Volta GPU is in the process of being enabled in the open source driver
and it will take few months before we reach the point where we will look
into adding ZONE_DEVICE public support for PowerPC.


> There is plenty of precedent of building a large capability
> out-of-tree and piecemeal merging it later, so I do not buy the
> "chicken-egg" argument. The change in the export is to make sure we
> don't repeat this backward "merge first, ask questions later" mistake
> in the future as devm_memremap_pages() is continuing to find new users
> like peer-to-peer DMA support and Linux is better off if that
> development is upstream. From a purely technical standpoint
> devm_memremap_pages() is EXPORT_SYMBOL_GPL because it hacks around
> several implementation details in the core kernel to achieve its goal,
> and it leaks new assumptions all over the kernel. It is strictly not a
> self contained interface.

HMM is self contain interface but i doubt i can convince you of that.

Cheers,
Jérôme