Re: [HMM 07/15] mm/ZONE_DEVICE: new type of ZONE_DEVICE for unaddressable memory v3

[Jerome Glisse]

On Thu, Jun 08, 2017 at 08:55:05PM -0700, John Hubbard wrote:
> On 05/24/2017 10:20 AM, Jérôme Glisse wrote:
> [...8<...]
> > +#if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
> > +int device_private_entry_fault(struct vm_area_struct *vma,
> > +		       unsigned long addr,
> > +		       swp_entry_t entry,
> > +		       unsigned int flags,
> > +		       pmd_t *pmdp)
> > +{
> > +	struct page *page = device_private_entry_to_page(entry);
> > +
> > +	/*
> > +	 * The page_fault() callback must migrate page back to system memory
> > +	 * so that CPU can access it. This might fail for various reasons
> > +	 * (device issue, device was unsafely unplugged, ...). When such
> > +	 * error conditions happen, the callback must return VM_FAULT_SIGBUS.
> > +	 *
> > +	 * Note that because memory cgroup charges are accounted to the device
> > +	 * memory, this should never fail because of memory restrictions (but
> > +	 * allocation of regular system page might still fail because we are
> > +	 * out of memory).
> > +	 *
> > +	 * There is a more in-depth description of what that callback can and
> > +	 * cannot do, in include/linux/memremap.h
> > +	 */
> > +	return page->pgmap->page_fault(vma, addr, page, flags, pmdp);
> > +}
> > +EXPORT_SYMBOL(device_private_entry_fault);
> > +#endif /* CONFIG_DEVICE_PRIVATE */
> > +
> >   static void pgmap_radix_release(struct resource *res)
> >   {
> >   	resource_size_t key, align_start, align_size, align_end;
> > @@ -321,6 +351,10 @@ void *devm_memremap_pages(struct device *dev, struct resource *res,
> >   	}
> >   	pgmap->ref = ref;
> >   	pgmap->res = &page_map->res;
> > +	pgmap->type = MEMORY_DEVICE_PUBLIC;
> > +	pgmap->page_fault = NULL;
> > +	pgmap->page_free = NULL;
> > +	pgmap->data = NULL;
> >   	mutex_lock(&pgmap_lock);
> >   	error = 0;
> > diff --git a/mm/Kconfig b/mm/Kconfig
> > index d744cff..f5357ff 100644
> > --- a/mm/Kconfig
> > +++ b/mm/Kconfig
> > @@ -736,6 +736,19 @@ config ZONE_DEVICE
> >   	  If FS_DAX is enabled, then say Y.
> > +config DEVICE_PRIVATE
> > +	bool "Unaddressable device memory (GPU memory, ...)"
> > +	depends on X86_64
> > +	depends on ZONE_DEVICE
> > +	depends on MEMORY_HOTPLUG
> > +	depends on MEMORY_HOTREMOVE
> > +	depends on SPARSEMEM_VMEMMAP
> > +
> > +	help
> > +	  Allows creation of struct pages to represent unaddressable device
> > +	  memory; i.e., memory that is only accessible from the device (or
> > +	  group of devices).
> > +
> 
> Hi Jerome,
> 
> CONFIG_DEVICE_PRIVATE has caused me some problems, because it's not coupled to HMM_DEVMEM.
> 
> To fix this, my first choice would be to just s/DEVICE_PRIVATE/HMM_DEVMEM/g
> , because I don't see any value to DEVICE_PRIVATE as an independent Kconfig
> choice. It's complicating the Kconfig choices, and adding problems. However,
> if DEVICE_PRIVATE must be kept, then something like this also fixes my HMM
> tests:


Better is depend on so that you can not select HMM_DEVMEM if you do not have
DEVICE_PRIVATE. But maybe this can be merge under one config option, i do not
have any strong preference personnaly. The HMM_DEVMEM just enable helper code
that make using CONFIG_DEVICE_PRIVATE easier for device driver but is not
strictly needed ie device driver can reimplement what HMM_DEVMEM provides.

I might just merge this kernel option as part of CDM patchset that i am about
to send.

Cheers,
Jérôme

> 
> From: John Hubbard <jhubbard@xxxxxxxxxx>
> Date: Thu, 8 Jun 2017 20:13:13 -0700
> Subject: [PATCH] hmm: select CONFIG_DEVICE_PRIVATE with HMM_DEVMEM
> 
> The HMM_DEVMEM feature is useless without the various
> features that are guarded with CONFIG_DEVICE_PRIVATE.
> Therefore, auto-select DEVICE_PRIVATE when selecting
> HMM_DEVMEM.
> 
> Otherwise, you can easily end up with a partially
> working HMM installation: if you select HMM_DEVMEM,
> but do not select DEVICE_PRIVATE, then faulting and
> migrating to a device (such as a GPU) works, but CPU
> page faults are ignored, so the page never migrates
> back to the CPU.
> 
> Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
> ---
>  mm/Kconfig | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 46296d5d7570..23d2f5ec865e 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -318,6 +318,8 @@ config HMM_DEVMEM
>  	bool "HMM device memory helpers (to leverage ZONE_DEVICE)"
>  	depends on ARCH_HAS_HMM
>  	select HMM
> +	select DEVICE_PRIVATE
> +
>  	help
>  	  HMM devmem is a set of helper routines to leverage the ZONE_DEVICE
>  	  feature. This is just to avoid having device drivers to replicating a lot
> -- 
> 2.13.1
> 
> This is a minor thing, and I don't think this needs to hold up merging HMM
> v23 into -mm, IMHO. But I would like it fixed at some point.
> 
> thanks,
> --
> John Hubbard
> NVIDIA
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@xxxxxxxxxx  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>