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

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

[Dan Williams]

Hi Andrew, please consider this series for 4.18.

For maintainability, as ZONE_DEVICE continues to attract new users,
it is useful to keep all users consolidated on devm_memremap_pages() as
the interface for create "device pages".

The devm_memremap_pages() implementation was recently reworked to make
it more generic for arbitrary users, like the proposed peer-to-peer
PCI-E enabling. HMM pre-dated this rework and opted to duplicate
devm_memremap_pages() as hmm_devmem_pages_create().

Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
the licensing on the exports given the deep dependencies on the mm.

Patches based on v4.17-rc6 where there are no upstream consumers of the
HMM functionality.

---

Dan Williams (5):
      mm, devm_memremap_pages: mark devm_memremap_pages() EXPORT_SYMBOL_GPL
      mm, devm_memremap_pages: handle errors allocating final devres action
      mm, hmm: use devm semantics for hmm_devmem_{add,remove}
      mm, hmm: replace hmm_devmem_pages_create() with devm_memremap_pages()
      mm, hmm: mark hmm_devmem_{add,add_resource} EXPORT_SYMBOL_GPL


 Documentation/vm/hmm.txt |    1 
 include/linux/hmm.h      |    4 -
 include/linux/memremap.h |    1 
 kernel/memremap.c        |   39 +++++-
 mm/hmm.c                 |  297 +++++++---------------------------------------
 5 files changed, 77 insertions(+), 265 deletions(-)

[PATCH 1/5] mm, devm_memremap_pages: mark devm_memremap_pages() EXPORT_SYMBOL_GPL

[Dan Williams]

The devm_memremap_pages() facility is tightly integrated with the
kernel's memory hotplug functionality. It injects an altmap argument
deep into the architecture specific vmemmap implementation to allow
allocating from specific reserved pages, and it has Linux specific
assumptions about page structure reference counting relative to
get_user_pages() and get_user_pages_fast(). It was an oversight that
this was not marked EXPORT_SYMBOL_GPL from the outset.

Cc: Michal Hocko <mhocko@suse.com>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 kernel/memremap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/memremap.c b/kernel/memremap.c
index 895e6b76b25e..c614645227a7 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -429,7 +429,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 	pgmap_radix_release(res, pgoff);
 	return ERR_PTR(error);
 }
-EXPORT_SYMBOL(devm_memremap_pages);
+EXPORT_SYMBOL_GPL(devm_memremap_pages);
 
 unsigned long vmem_altmap_offset(struct vmem_altmap *altmap)
 {

[PATCH 2/5] mm, devm_memremap_pages: handle errors allocating final devres action

[Dan Williams]

The last step before devm_memremap_pages() returns success is to
allocate a release action to tear the entire setup down. However, the
result from devm_add_action() is not checked.

Checking the error also means that we need to handle the fact that the
percpu_ref may not be killed by the time devm_memremap_pages_release()
runs. Add a new state flag for this case.

Cc: <stable@vger.kernel.org>
Fixes: e8d513483300 ("memremap: change devm_memremap_pages interface...")
Cc: Christoph Hellwig <hch@lst.de>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 include/linux/memremap.h |    1 +
 kernel/memremap.c        |    8 ++++++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index 7b4899c06f49..44a7ee517513 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -115,6 +115,7 @@ struct dev_pagemap {
 	dev_page_free_t page_free;
 	struct vmem_altmap altmap;
 	bool altmap_valid;
+	bool registered;
 	struct resource res;
 	struct percpu_ref *ref;
 	struct device *dev;
diff --git a/kernel/memremap.c b/kernel/memremap.c
index c614645227a7..30d96be5a965 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -296,7 +296,7 @@ static void devm_memremap_pages_release(void *data)
 	for_each_device_pfn(pfn, pgmap)
 		put_page(pfn_to_page(pfn));
 
-	if (percpu_ref_tryget_live(pgmap->ref)) {
+	if (pgmap->registered && percpu_ref_tryget_live(pgmap->ref)) {
 		dev_WARN(dev, "%s: page mapping is still live!\n", __func__);
 		percpu_ref_put(pgmap->ref);
 	}
@@ -418,7 +418,11 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 		percpu_ref_get(pgmap->ref);
 	}
 
-	devm_add_action(dev, devm_memremap_pages_release, pgmap);
+	error = devm_add_action_or_reset(dev, devm_memremap_pages_release,
+			pgmap);
+	if (error)
+		return ERR_PTR(error);
+	pgmap->registered = true;
 
 	return __va(res->start);
 

[PATCH 3/5] mm, hmm: use devm semantics for hmm_devmem_{add, remove}

[Dan Williams]

devm semantics arrange for resources to be torn down when
device-driver-probe fails or when device-driver-release completes.
Similar to devm_memremap_pages() there is no need to support an explicit
remove operation when the users properly adhere to devm semantics.

Cc: Christoph Hellwig <hch@lst.de>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 Documentation/vm/hmm.txt |    1 
 include/linux/hmm.h      |    4 -
 mm/hmm.c                 |  127 +++++++++++-----------------------------------
 3 files changed, 32 insertions(+), 100 deletions(-)

diff --git a/Documentation/vm/hmm.txt b/Documentation/vm/hmm.txt
index 2d1d6f69e91b..b2f944b87c5e 100644
--- a/Documentation/vm/hmm.txt
+++ b/Documentation/vm/hmm.txt
@@ -285,7 +285,6 @@ region needing a struct page. This is offered through a very simple API:
  struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
                                    struct device *device,
                                    unsigned long size);
- void hmm_devmem_remove(struct hmm_devmem *devmem);
 
 The hmm_devmem_ops is where most of the important things are:
 
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 39988924de3a..880f6bb9485c 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -499,8 +499,7 @@ struct hmm_devmem {
  * enough and allocate struct page for it.
  *
  * The device driver can wrap the hmm_devmem struct inside a private device
- * driver struct. The device driver must call hmm_devmem_remove() before the
- * device goes away and before freeing the hmm_devmem struct memory.
+ * driver struct.
  */
 struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 				  struct device *device,
@@ -508,7 +507,6 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
 					   struct device *device,
 					   struct resource *res);
-void hmm_devmem_remove(struct hmm_devmem *devmem);
 
 /*
  * hmm_devmem_page_set_drvdata - set per-page driver data field
diff --git a/mm/hmm.c b/mm/hmm.c
index 486dc394a5a3..8aa9d9fbb87b 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -993,15 +993,17 @@ static void hmm_devmem_radix_release(struct resource *resource)
 	mutex_unlock(&hmm_devmem_lock);
 }
 
-static void hmm_devmem_release(struct device *dev, void *data)
+static void hmm_devmem_release(void *data)
 {
 	struct hmm_devmem *devmem = data;
+	struct device *dev = devmem->device;
 	struct resource *resource = devmem->resource;
+	struct dev_pagemap *pgmap = &devmem->pagemap;
 	unsigned long start_pfn, npages;
 	struct zone *zone;
 	struct page *page;
 
-	if (percpu_ref_tryget_live(&devmem->ref)) {
+	if (pgmap->registered && percpu_ref_tryget_live(&devmem->ref)) {
 		dev_WARN(dev, "%s: page mapping is still live!\n", __func__);
 		percpu_ref_put(&devmem->ref);
 	}
@@ -1129,19 +1131,6 @@ static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
 	return ret;
 }
 
-static int hmm_devmem_match(struct device *dev, void *data, void *match_data)
-{
-	struct hmm_devmem *devmem = data;
-
-	return devmem->resource == match_data;
-}
-
-static void hmm_devmem_pages_remove(struct hmm_devmem *devmem)
-{
-	devres_release(devmem->device, &hmm_devmem_release,
-		       &hmm_devmem_match, devmem->resource);
-}
-
 /*
  * hmm_devmem_add() - hotplug ZONE_DEVICE memory for device memory
  *
@@ -1169,8 +1158,7 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 
 	static_branch_enable(&device_private_key);
 
-	devmem = devres_alloc_node(&hmm_devmem_release, sizeof(*devmem),
-				   GFP_KERNEL, dev_to_node(device));
+	devmem = devm_kzalloc(device, sizeof(*devmem), GFP_KERNEL);
 	if (!devmem)
 		return ERR_PTR(-ENOMEM);
 
@@ -1184,11 +1172,11 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	ret = percpu_ref_init(&devmem->ref, &hmm_devmem_ref_release,
 			      0, GFP_KERNEL);
 	if (ret)
-		goto error_percpu_ref;
+		return ERR_PTR(ret);
 
-	ret = devm_add_action(device, hmm_devmem_ref_exit, &devmem->ref);
+	ret = devm_add_action_or_reset(device, hmm_devmem_ref_exit, &devmem->ref);
 	if (ret)
-		goto error_devm_add_action;
+		return ERR_PTR(ret);
 
 	size = ALIGN(size, PA_SECTION_SIZE);
 	addr = min((unsigned long)iomem_resource.end,
@@ -1208,16 +1196,12 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 
 		devmem->resource = devm_request_mem_region(device, addr, size,
 							   dev_name(device));
-		if (!devmem->resource) {
-			ret = -ENOMEM;
-			goto error_no_resource;
-		}
+		if (!devmem->resource)
+			return ERR_PTR(-ENOMEM);
 		break;
 	}
-	if (!devmem->resource) {
-		ret = -ERANGE;
-		goto error_no_resource;
-	}
+	if (!devmem->resource)
+		return ERR_PTR(-ERANGE);
 
 	devmem->resource->desc = IORES_DESC_DEVICE_PRIVATE_MEMORY;
 	devmem->pfn_first = devmem->resource->start >> PAGE_SHIFT;
@@ -1226,28 +1210,18 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 
 	ret = hmm_devmem_pages_create(devmem);
 	if (ret)
-		goto error_pages;
+		return ERR_PTR(ret);
 
-	devres_add(device, devmem);
+	ret = devm_add_action_or_reset(device, hmm_devmem_release, devmem);
+	if (ret)
+		return ERR_PTR(ret);
+	devmem->pagemap.registered = true;
 
-	ret = devm_add_action(device, hmm_devmem_ref_kill, &devmem->ref);
-	if (ret) {
-		hmm_devmem_remove(devmem);
+	ret = devm_add_action_or_reset(device, hmm_devmem_ref_kill, &devmem->ref);
+	if (ret)
 		return ERR_PTR(ret);
-	}
 
 	return devmem;
-
-error_pages:
-	devm_release_mem_region(device, devmem->resource->start,
-				resource_size(devmem->resource));
-error_no_resource:
-error_devm_add_action:
-	hmm_devmem_ref_kill(&devmem->ref);
-	hmm_devmem_ref_exit(&devmem->ref);
-error_percpu_ref:
-	devres_free(devmem);
-	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL(hmm_devmem_add);
 
@@ -1263,8 +1237,7 @@ struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
 
 	static_branch_enable(&device_private_key);
 
-	devmem = devres_alloc_node(&hmm_devmem_release, sizeof(*devmem),
-				   GFP_KERNEL, dev_to_node(device));
+	devmem = devm_kzalloc(device, sizeof(*devmem), GFP_KERNEL);
 	if (!devmem)
 		return ERR_PTR(-ENOMEM);
 
@@ -1278,12 +1251,12 @@ struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
 	ret = percpu_ref_init(&devmem->ref, &hmm_devmem_ref_release,
 			      0, GFP_KERNEL);
 	if (ret)
-		goto error_percpu_ref;
+		return ERR_PTR(ret);
 
-	ret = devm_add_action(device, hmm_devmem_ref_exit, &devmem->ref);
+	ret = devm_add_action_or_reset(device, hmm_devmem_ref_exit,
+			&devmem->ref);
 	if (ret)
-		goto error_devm_add_action;
-
+		return ERR_PTR(ret);
 
 	devmem->pfn_first = devmem->resource->start >> PAGE_SHIFT;
 	devmem->pfn_last = devmem->pfn_first +
@@ -1291,60 +1264,22 @@ struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
 
 	ret = hmm_devmem_pages_create(devmem);
 	if (ret)
-		goto error_devm_add_action;
+		return ERR_PTR(ret);
 
-	devres_add(device, devmem);
+	ret = devm_add_action_or_reset(device, hmm_devmem_release, devmem);
+	if (ret)
+		return ERR_PTR(ret);
+	devmem->pagemap.registered = true;
 
-	ret = devm_add_action(device, hmm_devmem_ref_kill, &devmem->ref);
-	if (ret) {
-		hmm_devmem_remove(devmem);
+	ret = devm_add_action_or_reset(device, hmm_devmem_ref_kill, &devmem->ref);
+	if (ret)
 		return ERR_PTR(ret);
-	}
 
 	return devmem;
-
-error_devm_add_action:
-	hmm_devmem_ref_kill(&devmem->ref);
-	hmm_devmem_ref_exit(&devmem->ref);
-error_percpu_ref:
-	devres_free(devmem);
-	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL(hmm_devmem_add_resource);
 
 /*
- * hmm_devmem_remove() - remove device memory (kill and free ZONE_DEVICE)
- *
- * @devmem: hmm_devmem struct use to track and manage the ZONE_DEVICE memory
- *
- * This will hot-unplug memory that was hotplugged by hmm_devmem_add on behalf
- * of the device driver. It will free struct page and remove the resource that
- * reserved the physical address range for this device memory.
- */
-void hmm_devmem_remove(struct hmm_devmem *devmem)
-{
-	resource_size_t start, size;
-	struct device *device;
-	bool cdm = false;
-
-	if (!devmem)
-		return;
-
-	device = devmem->device;
-	start = devmem->resource->start;
-	size = resource_size(devmem->resource);
-
-	cdm = devmem->resource->desc == IORES_DESC_DEVICE_PUBLIC_MEMORY;
-	hmm_devmem_ref_kill(&devmem->ref);
-	hmm_devmem_ref_exit(&devmem->ref);
-	hmm_devmem_pages_remove(devmem);
-
-	if (!cdm)
-		devm_release_mem_region(device, start, size);
-}
-EXPORT_SYMBOL(hmm_devmem_remove);
-
-/*
  * A device driver that wants to handle multiple devices memory through a
  * single fake device can use hmm_device to do so. This is purely a helper
  * and it is not needed to make use of any HMM functionality.

[PATCH 4/5] mm, hmm: replace hmm_devmem_pages_create() with devm_memremap_pages()

[Dan Williams]

Commit e8d513483300 "memremap: change devm_memremap_pages interface to
use struct dev_pagemap" refactored devm_memremap_pages() to allow a
dev_pagemap instance to be supplied. Passing in a dev_pagemap interface
simplifies the design of pgmap type drivers in that they can rely on
container_of() to lookup any private data associated with the given
dev_pagemap instance.

In addition to the cleanups this also gives hmm users multi-order-radix
improvements that arrived with commit ab1b597ee0e4 "mm,
devm_memremap_pages: use multi-order radix for ZONE_DEVICE lookups"

Cc: Christoph Hellwig <hch@lst.de>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 kernel/memremap.c |   29 +++++++-
 mm/hmm.c          |  194 +++++++----------------------------------------------
 2 files changed, 49 insertions(+), 174 deletions(-)

diff --git a/kernel/memremap.c b/kernel/memremap.c
index 30d96be5a965..bece24419d6d 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -395,11 +395,32 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 		goto err_pfn_remap;
 
 	mem_hotplug_begin();
-	error = arch_add_memory(nid, align_start, align_size, altmap, false);
-	if (!error)
-		move_pfn_range_to_zone(&NODE_DATA(nid)->node_zones[ZONE_DEVICE],
-					align_start >> PAGE_SHIFT,
+
+	/*
+	 * For device private memory we call add_pages() as we only need to
+	 * allocate and initialize struct page for the device memory. More-
+	 * over the device memory is un-accessible thus we do not want to
+	 * create a linear mapping for the memory like arch_add_memory()
+	 * would do.
+	 *
+	 * For all other device memory types, which are accessible by
+	 * the CPU, we do want the linear mapping and thus use
+	 * arch_add_memory().
+	 */
+	if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
+		error = add_pages(nid, align_start >> PAGE_SHIFT,
+				align_size >> PAGE_SHIFT, NULL, false);
+	} else {
+		struct zone *zone;
+
+		error = arch_add_memory(nid, align_start, align_size, altmap,
+				false);
+		zone = &NODE_DATA(nid)->node_zones[ZONE_DEVICE];
+		if (!error)
+			move_pfn_range_to_zone(zone, align_start >> PAGE_SHIFT,
 					align_size >> PAGE_SHIFT, altmap);
+	}
+
 	mem_hotplug_done();
 	if (error)
 		goto err_add_memory;
diff --git a/mm/hmm.c b/mm/hmm.c
index 8aa9d9fbb87b..a4162406067c 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -975,162 +975,6 @@ static void hmm_devmem_free(struct page *page, void *data)
 	devmem->ops->free(devmem, page);
 }
 
-static DEFINE_MUTEX(hmm_devmem_lock);
-static RADIX_TREE(hmm_devmem_radix, GFP_KERNEL);
-
-static void hmm_devmem_radix_release(struct resource *resource)
-{
-	resource_size_t key, align_start, align_size;
-
-	align_start = resource->start & ~(PA_SECTION_SIZE - 1);
-	align_size = ALIGN(resource_size(resource), PA_SECTION_SIZE);
-
-	mutex_lock(&hmm_devmem_lock);
-	for (key = resource->start;
-	     key <= resource->end;
-	     key += PA_SECTION_SIZE)
-		radix_tree_delete(&hmm_devmem_radix, key >> PA_SECTION_SHIFT);
-	mutex_unlock(&hmm_devmem_lock);
-}
-
-static void hmm_devmem_release(void *data)
-{
-	struct hmm_devmem *devmem = data;
-	struct device *dev = devmem->device;
-	struct resource *resource = devmem->resource;
-	struct dev_pagemap *pgmap = &devmem->pagemap;
-	unsigned long start_pfn, npages;
-	struct zone *zone;
-	struct page *page;
-
-	if (pgmap->registered && percpu_ref_tryget_live(&devmem->ref)) {
-		dev_WARN(dev, "%s: page mapping is still live!\n", __func__);
-		percpu_ref_put(&devmem->ref);
-	}
-
-	/* pages are dead and unused, undo the arch mapping */
-	start_pfn = (resource->start & ~(PA_SECTION_SIZE - 1)) >> PAGE_SHIFT;
-	npages = ALIGN(resource_size(resource), PA_SECTION_SIZE) >> PAGE_SHIFT;
-
-	page = pfn_to_page(start_pfn);
-	zone = page_zone(page);
-
-	mem_hotplug_begin();
-	if (resource->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY)
-		__remove_pages(zone, start_pfn, npages, NULL);
-	else
-		arch_remove_memory(start_pfn << PAGE_SHIFT,
-				   npages << PAGE_SHIFT, NULL);
-	mem_hotplug_done();
-
-	hmm_devmem_radix_release(resource);
-}
-
-static int hmm_devmem_pages_create(struct hmm_devmem *devmem)
-{
-	resource_size_t key, align_start, align_size, align_end;
-	struct device *device = devmem->device;
-	int ret, nid, is_ram;
-	unsigned long pfn;
-
-	align_start = devmem->resource->start & ~(PA_SECTION_SIZE - 1);
-	align_size = ALIGN(devmem->resource->start +
-			   resource_size(devmem->resource),
-			   PA_SECTION_SIZE) - align_start;
-
-	is_ram = region_intersects(align_start, align_size,
-				   IORESOURCE_SYSTEM_RAM,
-				   IORES_DESC_NONE);
-	if (is_ram == REGION_MIXED) {
-		WARN_ONCE(1, "%s attempted on mixed region %pr\n",
-				__func__, devmem->resource);
-		return -ENXIO;
-	}
-	if (is_ram == REGION_INTERSECTS)
-		return -ENXIO;
-
-	if (devmem->resource->desc == IORES_DESC_DEVICE_PUBLIC_MEMORY)
-		devmem->pagemap.type = MEMORY_DEVICE_PUBLIC;
-	else
-		devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
-
-	devmem->pagemap.res = *devmem->resource;
-	devmem->pagemap.page_fault = hmm_devmem_fault;
-	devmem->pagemap.page_free = hmm_devmem_free;
-	devmem->pagemap.dev = devmem->device;
-	devmem->pagemap.ref = &devmem->ref;
-	devmem->pagemap.data = devmem;
-
-	mutex_lock(&hmm_devmem_lock);
-	align_end = align_start + align_size - 1;
-	for (key = align_start; key <= align_end; key += PA_SECTION_SIZE) {
-		struct hmm_devmem *dup;
-
-		dup = radix_tree_lookup(&hmm_devmem_radix,
-					key >> PA_SECTION_SHIFT);
-		if (dup) {
-			dev_err(device, "%s: collides with mapping for %s\n",
-				__func__, dev_name(dup->device));
-			mutex_unlock(&hmm_devmem_lock);
-			ret = -EBUSY;
-			goto error;
-		}
-		ret = radix_tree_insert(&hmm_devmem_radix,
-					key >> PA_SECTION_SHIFT,
-					devmem);
-		if (ret) {
-			dev_err(device, "%s: failed: %d\n", __func__, ret);
-			mutex_unlock(&hmm_devmem_lock);
-			goto error_radix;
-		}
-	}
-	mutex_unlock(&hmm_devmem_lock);
-
-	nid = dev_to_node(device);
-	if (nid < 0)
-		nid = numa_mem_id();
-
-	mem_hotplug_begin();
-	/*
-	 * For device private memory we call add_pages() as we only need to
-	 * allocate and initialize struct page for the device memory. More-
-	 * over the device memory is un-accessible thus we do not want to
-	 * create a linear mapping for the memory like arch_add_memory()
-	 * would do.
-	 *
-	 * For device public memory, which is accesible by the CPU, we do
-	 * want the linear mapping and thus use arch_add_memory().
-	 */
-	if (devmem->pagemap.type == MEMORY_DEVICE_PUBLIC)
-		ret = arch_add_memory(nid, align_start, align_size, NULL,
-				false);
-	else
-		ret = add_pages(nid, align_start >> PAGE_SHIFT,
-				align_size >> PAGE_SHIFT, NULL, false);
-	if (ret) {
-		mem_hotplug_done();
-		goto error_add_memory;
-	}
-	move_pfn_range_to_zone(&NODE_DATA(nid)->node_zones[ZONE_DEVICE],
-				align_start >> PAGE_SHIFT,
-				align_size >> PAGE_SHIFT, NULL);
-	mem_hotplug_done();
-
-	for (pfn = devmem->pfn_first; pfn < devmem->pfn_last; pfn++) {
-		struct page *page = pfn_to_page(pfn);
-
-		page->pgmap = &devmem->pagemap;
-	}
-	return 0;
-
-error_add_memory:
-	untrack_pfn(NULL, PHYS_PFN(align_start), align_size);
-error_radix:
-	hmm_devmem_radix_release(devmem->resource);
-error:
-	return ret;
-}
-
 /*
  * hmm_devmem_add() - hotplug ZONE_DEVICE memory for device memory
  *
@@ -1154,6 +998,7 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 {
 	struct hmm_devmem *devmem;
 	resource_size_t addr;
+	void *result;
 	int ret;
 
 	static_branch_enable(&device_private_key);
@@ -1208,14 +1053,18 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	devmem->pfn_last = devmem->pfn_first +
 			   (resource_size(devmem->resource) >> PAGE_SHIFT);
 
-	ret = hmm_devmem_pages_create(devmem);
-	if (ret)
-		return ERR_PTR(ret);
+	devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
+	devmem->pagemap.res = *devmem->resource;
+	devmem->pagemap.page_fault = hmm_devmem_fault;
+	devmem->pagemap.page_free = hmm_devmem_free;
+	devmem->pagemap.altmap_valid = false;
+	devmem->pagemap.registered = false;
+	devmem->pagemap.ref = &devmem->ref;
+	devmem->pagemap.data = devmem;
 
-	ret = devm_add_action_or_reset(device, hmm_devmem_release, devmem);
-	if (ret)
-		return ERR_PTR(ret);
-	devmem->pagemap.registered = true;
+	result = devm_memremap_pages(devmem->device, &devmem->pagemap);
+	if (IS_ERR(result))
+		return result;
 
 	ret = devm_add_action_or_reset(device, hmm_devmem_ref_kill, &devmem->ref);
 	if (ret)
@@ -1230,6 +1079,7 @@ struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
 					   struct resource *res)
 {
 	struct hmm_devmem *devmem;
+	void *result;
 	int ret;
 
 	if (res->desc != IORES_DESC_DEVICE_PUBLIC_MEMORY)
@@ -1262,14 +1112,18 @@ struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
 	devmem->pfn_last = devmem->pfn_first +
 			   (resource_size(devmem->resource) >> PAGE_SHIFT);
 
-	ret = hmm_devmem_pages_create(devmem);
-	if (ret)
-		return ERR_PTR(ret);
+	devmem->pagemap.type = MEMORY_DEVICE_PUBLIC;
+	devmem->pagemap.res = *devmem->resource;
+	devmem->pagemap.page_fault = hmm_devmem_fault;
+	devmem->pagemap.page_free = hmm_devmem_free;
+	devmem->pagemap.altmap_valid = false;
+	devmem->pagemap.registered = false;
+	devmem->pagemap.ref = &devmem->ref;
+	devmem->pagemap.data = devmem;
 
-	ret = devm_add_action_or_reset(device, hmm_devmem_release, devmem);
-	if (ret)
-		return ERR_PTR(ret);
-	devmem->pagemap.registered = true;
+	result = devm_memremap_pages(devmem->device, &devmem->pagemap);
+	if (IS_ERR(result))
+		return result;
 
 	ret = devm_add_action_or_reset(device, hmm_devmem_ref_kill, &devmem->ref);
 	if (ret)

[PATCH 5/5] mm, hmm: mark hmm_devmem_{add, add_resource} EXPORT_SYMBOL_GPL

[Dan Williams]

The routines hmm_devmem_add(), and hmm_devmem_add_resource() are small
wrappers around devm_memremap_pages(). The devm_memremap_pages()
interface is a subset of the hmm functionality which has more and deeper
ties into the kernel memory management implementation. It was an
oversight that these symbols were not marked EXPORT_SYMBOL_GPL from the
outset due to how they originally copied (and now reuse)
devm_memremap_pages().

Cc: Christoph Hellwig <hch@lst.de>
Cc: "Jérôme Glisse" <jglisse@redhat.com>
Cc: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
 mm/hmm.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index a4162406067c..d9aef1266ed6 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1072,7 +1072,7 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 
 	return devmem;
 }
-EXPORT_SYMBOL(hmm_devmem_add);
+EXPORT_SYMBOL_GPL(hmm_devmem_add);
 
 struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
 					   struct device *device,
@@ -1131,7 +1131,7 @@ struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
 
 	return devmem;
 }
-EXPORT_SYMBOL(hmm_devmem_add_resource);
+EXPORT_SYMBOL_GPL(hmm_devmem_add_resource);
 
 /*
  * A device driver that wants to handle multiple devices memory through a

Re: [PATCH 2/5] mm, devm_memremap_pages: handle errors allocating final devres action

[Andrew Morton]

On Mon, 21 May 2018 15:35:24 -0700 Dan Williams <dan.j.williams@intel.com> wrote:

> The last step before devm_memremap_pages() returns success is to
> allocate a release action to tear the entire setup down. However, the
> result from devm_add_action() is not checked.
> 
> Checking the error also means that we need to handle the fact that the
> percpu_ref may not be killed by the time devm_memremap_pages_release()
> runs. Add a new state flag for this case.
> 
> Cc: <stable@vger.kernel.org>
> Fixes: e8d513483300 ("memremap: change devm_memremap_pages interface...")
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: "Jérôme Glisse" <jglisse@redhat.com>
> Cc: Logan Gunthorpe <logang@deltatee.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>

Why the cc:stable?  The changelog doesn't describe the end-user-visible
impact of the bug (it always should, for this reason).

AFAICT we only go wrong when a small GFP_KERNEL allocation fails
(basically never happens), with undescribed results :(

Re: [PATCH 2/5] mm, devm_memremap_pages: handle errors allocating final devres action

[Dan Williams]

[ resend as the last attempt dropped all the cc's ]

On Mon, May 21, 2018 at 4:10 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Mon, 21 May 2018 15:35:24 -0700 Dan Williams <dan.j.williams@intel.com> wrote:
>
>> The last step before devm_memremap_pages() returns success is to
>> allocate a release action to tear the entire setup down. However, the
>> result from devm_add_action() is not checked.
>>
>> Checking the error also means that we need to handle the fact that the
>> percpu_ref may not be killed by the time devm_memremap_pages_release()
>> runs. Add a new state flag for this case.
>>
>> Cc: <stable@vger.kernel.org>
>> Fixes: e8d513483300 ("memremap: change devm_memremap_pages interface...")
>> Cc: Christoph Hellwig <hch@lst.de>
>> Cc: "Jérôme Glisse" <jglisse@redhat.com>
>> Cc: Logan Gunthorpe <logang@deltatee.com>
>> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
>
> Why the cc:stable?  The changelog doesn't describe the end-user-visible
> impact of the bug (it always should, for this reason).

True, I should have included that, one of these years I'll stop making
this mistake.

>
> AFAICT we only go wrong when a small GFP_KERNEL allocation fails
> (basically never happens), with undescribed results :(
>

Here's a better changelog:

---
The last step before devm_memremap_pages() returns success is to
allocate a release action to tear the entire setup down. However, the
result from devm_add_action() is not checked.

Checking the error also means that we need to handle the fact that the
percpu_ref may not be killed by the time devm_memremap_pages_release()
runs. Add a new state flag for this case.

Without this change we could fail to register the teardown of
devm_memremap_pages(). The likelihood of hitting this failure is tiny
as small memory allocations almost always succeed. However, the impact
of the failure is large given any future reconfiguration, or
disable/enable, of an nvdimm namespace will fail forever as subsequent
calls to devm_memremap_pages() will fail to setup the pgmap_radix
since there will be stale entries for the physical address range.

Re: [PATCH 1/5] mm, devm_memremap_pages: mark devm_memremap_pages() EXPORT_SYMBOL_GPL

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/5] mm, devm_memremap_pages: handle errors allocating final devres action

[Christoph Hellwig]

On Mon, May 21, 2018 at 03:35:24PM -0700, Dan Williams wrote:
> The last step before devm_memremap_pages() returns success is to
> allocate a release action to tear the entire setup down. However, the
> result from devm_add_action() is not checked.
> 
> Checking the error also means that we need to handle the fact that the
> percpu_ref may not be killed by the time devm_memremap_pages_release()
> runs. Add a new state flag for this case.

Looks good (modulo any stable tag issues):

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/5] mm, hmm: use devm semantics for hmm_devmem_{add, remove}

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/5] mm, hmm: replace hmm_devmem_pages_create() with devm_memremap_pages()

[Christoph Hellwig]

On Mon, May 21, 2018 at 03:35:34PM -0700, Dan Williams wrote:
> Commit e8d513483300 "memremap: change devm_memremap_pages interface to
> use struct dev_pagemap" refactored devm_memremap_pages() to allow a
> dev_pagemap instance to be supplied. Passing in a dev_pagemap interface
> simplifies the design of pgmap type drivers in that they can rely on
> container_of() to lookup any private data associated with the given
> dev_pagemap instance.
> 
> In addition to the cleanups this also gives hmm users multi-order-radix
> improvements that arrived with commit ab1b597ee0e4 "mm,
> devm_memremap_pages: use multi-order radix for ZONE_DEVICE lookups"

None of them has any caller despite being in the tree for 9 month.
I think it's time to simply drop the whole hmm code instead instead of
carrying this dead weight around.

Re: [PATCH 5/5] mm, hmm: mark hmm_devmem_{add, add_resource} EXPORT_SYMBOL_GPL

[Christoph Hellwig]

On Mon, May 21, 2018 at 03:35:40PM -0700, Dan Williams wrote:
> The routines hmm_devmem_add(), and hmm_devmem_add_resource() are small
> wrappers around devm_memremap_pages(). The devm_memremap_pages()
> interface is a subset of the hmm functionality which has more and deeper
> ties into the kernel memory management implementation. It was an
> oversight that these symbols were not marked EXPORT_SYMBOL_GPL from the
> outset due to how they originally copied (and now reuse)
> devm_memremap_pages().

If we end up keeping this code: absolutely.  Then again I think without
an actual user this should have never been merged, and should be removed
until one shows up.

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/5] mm, devm_memremap_pages: handle errors allocating final devres action

[Logan Gunthorpe]

Hey Dan,

On 21/05/18 06:07 PM, Dan Williams wrote:
> Without this change we could fail to register the teardown of
> devm_memremap_pages(). The likelihood of hitting this failure is tiny
> as small memory allocations almost always succeed. However, the impact
> of the failure is large given any future reconfiguration, or
> disable/enable, of an nvdimm namespace will fail forever as subsequent
> calls to devm_memremap_pages() will fail to setup the pgmap_radix
> since there will be stale entries for the physical address range.

Sorry, I don't follow this. The change only seems to prevent a warning
from occurring in this situation. Won't pgmap_radix_release() still be
called regardless of whether this patch is applied?

But it looks to me like this patch doesn't quite solve the issue -- at
least when looking at dax/pmem.c: If devm_add_action_or_reset() fails,
then dax_pmem_percpu_kill() won't be registered as an action and the
percpu_ref will never get killed. Thus, dax_pmem_percpu_release() would
not get called and dax_pmem_percpu_exit() will hang waiting for a
completion that will never occur. So we probably need to add a kill call
somewhere on the failing path...


Logan

Re: [PATCH 2/5] mm, devm_memremap_pages: handle errors allocating final devres action

[Dan Williams]

On Tue, May 22, 2018 at 9:42 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
> Hey Dan,
>
> On 21/05/18 06:07 PM, Dan Williams wrote:
>> Without this change we could fail to register the teardown of
>> devm_memremap_pages(). The likelihood of hitting this failure is tiny
>> as small memory allocations almost always succeed. However, the impact
>> of the failure is large given any future reconfiguration, or
>> disable/enable, of an nvdimm namespace will fail forever as subsequent
>> calls to devm_memremap_pages() will fail to setup the pgmap_radix
>> since there will be stale entries for the physical address range.
>
> Sorry, I don't follow this. The change only seems to prevent a warning
> from occurring in this situation. Won't pgmap_radix_release() still be
> called regardless of whether this patch is applied?

devm_add_action() does not call the release function,
devm_add_action_or_reset() does.

> But it looks to me like this patch doesn't quite solve the issue -- at
> least when looking at dax/pmem.c: If devm_add_action_or_reset() fails,
> then dax_pmem_percpu_kill() won't be registered as an action and the
> percpu_ref will never get killed. Thus, dax_pmem_percpu_release() would
> not get called and dax_pmem_percpu_exit() will hang waiting for a
> completion that will never occur. So we probably need to add a kill call
> somewhere on the failing path...

Ah, true, good catch!

We should manually kill in the !registered case. I think this means we
need to pass in the custom kill routine, because for the pmem driver
it's blk_freeze_queue_start().

Re: [PATCH 2/5] mm, devm_memremap_pages: handle errors allocating final devres action

[Logan Gunthorpe]

On 22/05/18 10:56 AM, Dan Williams wrote:
> On Tue, May 22, 2018 at 9:42 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>> Hey Dan,
>>
>> On 21/05/18 06:07 PM, Dan Williams wrote:
>>> Without this change we could fail to register the teardown of
>>> devm_memremap_pages(). The likelihood of hitting this failure is tiny
>>> as small memory allocations almost always succeed. However, the impact
>>> of the failure is large given any future reconfiguration, or
>>> disable/enable, of an nvdimm namespace will fail forever as subsequent
>>> calls to devm_memremap_pages() will fail to setup the pgmap_radix
>>> since there will be stale entries for the physical address range.
>>
>> Sorry, I don't follow this. The change only seems to prevent a warning
>> from occurring in this situation. Won't pgmap_radix_release() still be
>> called regardless of whether this patch is applied?
> 
> devm_add_action() does not call the release function,
> devm_add_action_or_reset() does.

Oh, yes. Thanks I see that now.

> Ah, true, good catch!
> 
> We should manually kill in the !registered case. I think this means we
> need to pass in the custom kill routine, because for the pmem driver
> it's blk_freeze_queue_start().

It may be cleaner to just have the caller call the specific kill
function if devm_memremap_pages fails... Though, I don't fully
understand how the nvdimm pmem driver cleans up the percpu counter.

Logan

Re: [PATCH 4/5] mm, hmm: replace hmm_devmem_pages_create() with devm_memremap_pages()

[Logan Gunthorpe]

On 21/05/18 04:35 PM, Dan Williams wrote:
> +	/*
> +	 * For device private memory we call add_pages() as we only need to
> +	 * allocate and initialize struct page for the device memory. More-
> +	 * over the device memory is un-accessible thus we do not want to
> +	 * create a linear mapping for the memory like arch_add_memory()
> +	 * would do.
> +	 *
> +	 * For all other device memory types, which are accessible by
> +	 * the CPU, we do want the linear mapping and thus use
> +	 * arch_add_memory().
> +	 */
> +	if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
> +		error = add_pages(nid, align_start >> PAGE_SHIFT,
> +				align_size >> PAGE_SHIFT, NULL, false);
> +	} else {
> +		struct zone *zone;
> +
> +		error = arch_add_memory(nid, align_start, align_size, altmap,
> +				false);
> +		zone = &NODE_DATA(nid)->node_zones[ZONE_DEVICE];
> +		if (!error)
> +			move_pfn_range_to_zone(zone, align_start >> PAGE_SHIFT,
>  					align_size >> PAGE_SHIFT, altmap);
> +	}

Maybe I missed it in the patch but, don't we need the same thing in
devm_memremap_pages_release() such that it calls the correct remove
function? Similar to the replaced hmm code:

> -	mem_hotplug_begin();
> -	if (resource->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY)
> -		__remove_pages(zone, start_pfn, npages, NULL);
> -	else
> -		arch_remove_memory(start_pfn << PAGE_SHIFT,
> -				   npages << PAGE_SHIFT, NULL);
> -	mem_hotplug_done();
> -
> -	hmm_devmem_radix_release(resource);

Perhaps it should be a separate patch too as it would be easier to see
outside the big removal of HMM code.

Logan

Re: [PATCH 2/5] mm, devm_memremap_pages: handle errors allocating final devres action

[Dan Williams]

On Tue, May 22, 2018 at 10:03 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
> On 22/05/18 10:56 AM, Dan Williams wrote:
>> On Tue, May 22, 2018 at 9:42 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>>> Hey Dan,
>>>
>>> On 21/05/18 06:07 PM, Dan Williams wrote:
>>>> Without this change we could fail to register the teardown of
>>>> devm_memremap_pages(). The likelihood of hitting this failure is tiny
>>>> as small memory allocations almost always succeed. However, the impact
>>>> of the failure is large given any future reconfiguration, or
>>>> disable/enable, of an nvdimm namespace will fail forever as subsequent
>>>> calls to devm_memremap_pages() will fail to setup the pgmap_radix
>>>> since there will be stale entries for the physical address range.
>>>
>>> Sorry, I don't follow this. The change only seems to prevent a warning
>>> from occurring in this situation. Won't pgmap_radix_release() still be
>>> called regardless of whether this patch is applied?
>>
>> devm_add_action() does not call the release function,
>> devm_add_action_or_reset() does.
>
> Oh, yes. Thanks I see that now.
>
>> Ah, true, good catch!
>>
>> We should manually kill in the !registered case. I think this means we
>> need to pass in the custom kill routine, because for the pmem driver
>> it's blk_freeze_queue_start().
>
> It may be cleaner to just have the caller call the specific kill
> function if devm_memremap_pages fails...

As far as I can see by then it's too late, or we need to expose
release details to the caller which defeats the purpose of devm
semantics.

> Though, I don't fully
> understand how the nvdimm pmem driver cleans up the percpu counter.

The dev_pagemap setup for pmem is entirely too subtle and arguably a
layering violation as it reuses the block layer q_usage_counter
percpu_ref. We arrange for that counter to be shutdown before the
blk_cleanup_queue() does the same.

Re: [PATCH 2/5] mm, devm_memremap_pages: handle errors allocating final devres action

[Logan Gunthorpe]

On 22/05/18 11:25 AM, Dan Williams wrote:
> As far as I can see by then it's too late, or we need to expose
> release details to the caller which defeats the purpose of devm
> semantics.

In the dax/pmem case, I *think* it should be fine...

devm_add_action_or_reset() only calls the action it is passed on failure
not the entire devm chain. In which case, it should drop all the
references to the percpu counter. Then, if on an error return from
devm_memremap_pages() we call dax_pmem_percpu_kill(), the rest of the
devm chain should be called when we return from a failed probe and it
should proceed as usual.

I think dax_pmem_percpu_kill() also must be called on any error return
from devm_memremap_pages and it is currently not...

Logan

Re: [PATCH 5/5] mm, hmm: mark hmm_devmem_{add, add_resource} EXPORT_SYMBOL_GPL

[Andrew Morton]

On Tue, 22 May 2018 08:32:36 +0200 Christoph Hellwig <hch@lst.de> wrote:

> On Mon, May 21, 2018 at 03:35:40PM -0700, Dan Williams wrote:
> > The routines hmm_devmem_add(), and hmm_devmem_add_resource() are small
> > wrappers around devm_memremap_pages(). The devm_memremap_pages()
> > interface is a subset of the hmm functionality which has more and deeper
> > ties into the kernel memory management implementation. It was an
> > oversight that these symbols were not marked EXPORT_SYMBOL_GPL from the
> > outset due to how they originally copied (and now reuse)
> > devm_memremap_pages().
> 
> If we end up keeping this code: absolutely.  Then again I think without
> an actual user this should have never been merged, and should be removed
> until one shows up.
> 

It wasn't simple.  Quite a lot of manufacturers were (are?) developing
quite complex driver code which utilizes hmm.  Merging hmm to give a
stable target for that development and in the expectation that those
things would be coming along was a risk and I don't think we yet know
the outcome.

Jerome, are you able to provide any updates on all of this?

Re: [PATCH 4/5] mm, hmm: replace hmm_devmem_pages_create() with devm_memremap_pages()

[Dan Williams]

On Tue, May 22, 2018 at 10:13 AM, Logan Gunthorpe <logang@deltatee.com> wrote:
>
>
> On 21/05/18 04:35 PM, Dan Williams wrote:
>> +     /*
>> +      * For device private memory we call add_pages() as we only need to
>> +      * allocate and initialize struct page for the device memory. More-
>> +      * over the device memory is un-accessible thus we do not want to
>> +      * create a linear mapping for the memory like arch_add_memory()
>> +      * would do.
>> +      *
>> +      * For all other device memory types, which are accessible by
>> +      * the CPU, we do want the linear mapping and thus use
>> +      * arch_add_memory().
>> +      */
>> +     if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
>> +             error = add_pages(nid, align_start >> PAGE_SHIFT,
>> +                             align_size >> PAGE_SHIFT, NULL, false);
>> +     } else {
>> +             struct zone *zone;
>> +
>> +             error = arch_add_memory(nid, align_start, align_size, altmap,
>> +                             false);
>> +             zone = &NODE_DATA(nid)->node_zones[ZONE_DEVICE];
>> +             if (!error)
>> +                     move_pfn_range_to_zone(zone, align_start >> PAGE_SHIFT,
>>                                       align_size >> PAGE_SHIFT, altmap);
>> +     }
>
> Maybe I missed it in the patch but, don't we need the same thing in
> devm_memremap_pages_release() such that it calls the correct remove
> function? Similar to the replaced hmm code:
>
>> -     mem_hotplug_begin();
>> -     if (resource->desc == IORES_DESC_DEVICE_PRIVATE_MEMORY)
>> -             __remove_pages(zone, start_pfn, npages, NULL);
>> -     else
>> -             arch_remove_memory(start_pfn << PAGE_SHIFT,
>> -                                npages << PAGE_SHIFT, NULL);
>> -     mem_hotplug_done();
>> -
>> -     hmm_devmem_radix_release(resource);
>
> Perhaps it should be a separate patch too as it would be easier to see
> outside the big removal of HMM code.
>

Yes, I'll split it out.

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

[Jerome Glisse]

On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
> Hi Andrew, please consider this series for 4.18.
> 
> For maintainability, as ZONE_DEVICE continues to attract new users,
> it is useful to keep all users consolidated on devm_memremap_pages() as
> the interface for create "device pages".
> 
> The devm_memremap_pages() implementation was recently reworked to make
> it more generic for arbitrary users, like the proposed peer-to-peer
> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
> devm_memremap_pages() as hmm_devmem_pages_create().
> 
> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
> the licensing on the exports given the deep dependencies on the mm.

I am on PTO right now so i won't be able to quickly review it all
but forcing GPL export is problematic for me now. I rather have
device driver using "sane" common helpers than creating their own
crazy thing.

Back in couple weeks i will review this some more.

> 
> Patches based on v4.17-rc6 where there are no upstream consumers of the
> HMM functionality.
> 
> ---
> 
> Dan Williams (5):
>       mm, devm_memremap_pages: mark devm_memremap_pages() EXPORT_SYMBOL_GPL
>       mm, devm_memremap_pages: handle errors allocating final devres action
>       mm, hmm: use devm semantics for hmm_devmem_{add,remove}
>       mm, hmm: replace hmm_devmem_pages_create() with devm_memremap_pages()
>       mm, hmm: mark hmm_devmem_{add,add_resource} EXPORT_SYMBOL_GPL
> 
> 
>  Documentation/vm/hmm.txt |    1 
>  include/linux/hmm.h      |    4 -
>  include/linux/memremap.h |    1 
>  kernel/memremap.c        |   39 +++++-
>  mm/hmm.c                 |  297 +++++++---------------------------------------
>  5 files changed, 77 insertions(+), 265 deletions(-)

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

[Dan Williams]

On Wed, May 23, 2018 at 5:10 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
>> Hi Andrew, please consider this series for 4.18.
>>
>> For maintainability, as ZONE_DEVICE continues to attract new users,
>> it is useful to keep all users consolidated on devm_memremap_pages() as
>> the interface for create "device pages".
>>
>> The devm_memremap_pages() implementation was recently reworked to make
>> it more generic for arbitrary users, like the proposed peer-to-peer
>> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
>> devm_memremap_pages() as hmm_devmem_pages_create().
>>
>> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
>> the licensing on the exports given the deep dependencies on the mm.
>
> I am on PTO right now so i won't be able to quickly review it all
> but forcing GPL export is problematic for me now. I rather have
> device driver using "sane" common helpers than creating their own
> crazy thing.

Sane drivers that need this level of deep integration with Linux
memory management need to be upstream. Otherwise, HMM is an
unprecedented departure from the norms of Linux kernel development.

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

[Christoph Hellwig]

On Wed, May 23, 2018 at 08:18:11PM -0700, Dan Williams wrote:
> On Wed, May 23, 2018 at 5:10 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> > On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
> >> Hi Andrew, please consider this series for 4.18.
> >>
> >> For maintainability, as ZONE_DEVICE continues to attract new users,
> >> it is useful to keep all users consolidated on devm_memremap_pages() as
> >> the interface for create "device pages".
> >>
> >> The devm_memremap_pages() implementation was recently reworked to make
> >> it more generic for arbitrary users, like the proposed peer-to-peer
> >> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
> >> devm_memremap_pages() as hmm_devmem_pages_create().
> >>
> >> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
> >> the licensing on the exports given the deep dependencies on the mm.
> >
> > I am on PTO right now so i won't be able to quickly review it all
> > but forcing GPL export is problematic for me now. I rather have
> > device driver using "sane" common helpers than creating their own
> > crazy thing.
> 
> Sane drivers that need this level of deep integration with Linux
> memory management need to be upstream. Otherwise, HMM is an
> unprecedented departure from the norms of Linux kernel development.

Agreed.  I consider every driver using this a derived work, independ
on the marking or not.  And I'm willing to enforce this.

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

[Dave Airlie]

On 24 May 2018 at 13:18, Dan Williams <dan.j.williams@intel.com> wrote:
> On Wed, May 23, 2018 at 5:10 PM, Jerome Glisse <jglisse@redhat.com> wrote:
>> On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
>>> Hi Andrew, please consider this series for 4.18.
>>>
>>> For maintainability, as ZONE_DEVICE continues to attract new users,
>>> it is useful to keep all users consolidated on devm_memremap_pages() as
>>> the interface for create "device pages".
>>>
>>> The devm_memremap_pages() implementation was recently reworked to make
>>> it more generic for arbitrary users, like the proposed peer-to-peer
>>> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
>>> devm_memremap_pages() as hmm_devmem_pages_create().
>>>
>>> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
>>> the licensing on the exports given the deep dependencies on the mm.
>>
>> I am on PTO right now so i won't be able to quickly review it all
>> but forcing GPL export is problematic for me now. I rather have
>> device driver using "sane" common helpers than creating their own
>> crazy thing.
>
> Sane drivers that need this level of deep integration with Linux
> memory management need to be upstream. Otherwise, HMM is an
> unprecedented departure from the norms of Linux kernel development.

Isn't it the author of code choice what EXPORT_SYMBOL to use? and
isn't the agreement that if something is EXPORT_SYMBOL now, changing
underlying exports isn't considered a good idea. We've seen this before
with the refcount fun,

See d557d1b58b3546bab2c5bc2d624c5709840e6b10

Not commenting on the legality or what derived works are considered,
since really the markings are just an indication of the authors opinion,
and at this stage I think are actually meaningless, since we've diverged
considerably from the advice given to Linus back when this started.

If Christoph is willing to enforce it the markings won't matter either way.

Dave.

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

[Dan Williams]

On Tue, May 29, 2018 at 3:22 PM, Dave Airlie <airlied@gmail.com> wrote:
>
> On 24 May 2018 at 13:18, Dan Williams <dan.j.williams@intel.com> wrote:
> > On Wed, May 23, 2018 at 5:10 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> >> On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
> >>> Hi Andrew, please consider this series for 4.18.
> >>>
> >>> For maintainability, as ZONE_DEVICE continues to attract new users,
> >>> it is useful to keep all users consolidated on devm_memremap_pages() as
> >>> the interface for create "device pages".
> >>>
> >>> The devm_memremap_pages() implementation was recently reworked to make
> >>> it more generic for arbitrary users, like the proposed peer-to-peer
> >>> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
> >>> devm_memremap_pages() as hmm_devmem_pages_create().
> >>>
> >>> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
> >>> the licensing on the exports given the deep dependencies on the mm.
> >>
> >> I am on PTO right now so i won't be able to quickly review it all
> >> but forcing GPL export is problematic for me now. I rather have
> >> device driver using "sane" common helpers than creating their own
> >> crazy thing.
> >
> > Sane drivers that need this level of deep integration with Linux
> > memory management need to be upstream. Otherwise, HMM is an
> > unprecedented departure from the norms of Linux kernel development.
>
> Isn't it the author of code choice what EXPORT_SYMBOL to use? and
> isn't the agreement that if something is EXPORT_SYMBOL now, changing
> underlying exports isn't considered a good idea. We've seen this before
> with the refcount fun,
>
> See d557d1b58b3546bab2c5bc2d624c5709840e6b10
>
> Not commenting on the legality or what derived works are considered,
> since really the markings are just an indication of the authors opinion,
> and at this stage I think are actually meaningless, since we've diverged
> considerably from the advice given to Linus back when this started.

Yes, and in this case devm_memremap_pages() was originally written by
Christoph and I:

    41e94a851304 add devm_memremap_pages

These patches correct the export type on that routine in patch1.

HMM started off by duplicating devm_memremap_pages() which is fixed up
by this series:

    5 files changed, 77 insertions(+), 265 deletions(-)

...and these patches take the extra step of marking any exports that
call devm_memremap_pages() also match the same base export.

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

[Dave Airlie]

On 30 May 2018 at 08:31, Dan Williams <dan.j.williams@intel.com> wrote:
> On Tue, May 29, 2018 at 3:22 PM, Dave Airlie <airlied@gmail.com> wrote:
>>
>> On 24 May 2018 at 13:18, Dan Williams <dan.j.williams@intel.com> wrote:
>> > On Wed, May 23, 2018 at 5:10 PM, Jerome Glisse <jglisse@redhat.com> wrote:
>> >> On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
>> >>> Hi Andrew, please consider this series for 4.18.
>> >>>
>> >>> For maintainability, as ZONE_DEVICE continues to attract new users,
>> >>> it is useful to keep all users consolidated on devm_memremap_pages() as
>> >>> the interface for create "device pages".
>> >>>
>> >>> The devm_memremap_pages() implementation was recently reworked to make
>> >>> it more generic for arbitrary users, like the proposed peer-to-peer
>> >>> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
>> >>> devm_memremap_pages() as hmm_devmem_pages_create().
>> >>>
>> >>> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
>> >>> the licensing on the exports given the deep dependencies on the mm.
>> >>
>> >> I am on PTO right now so i won't be able to quickly review it all
>> >> but forcing GPL export is problematic for me now. I rather have
>> >> device driver using "sane" common helpers than creating their own
>> >> crazy thing.
>> >
>> > Sane drivers that need this level of deep integration with Linux
>> > memory management need to be upstream. Otherwise, HMM is an
>> > unprecedented departure from the norms of Linux kernel development.
>>
>> Isn't it the author of code choice what EXPORT_SYMBOL to use? and
>> isn't the agreement that if something is EXPORT_SYMBOL now, changing
>> underlying exports isn't considered a good idea. We've seen this before
>> with the refcount fun,
>>
>> See d557d1b58b3546bab2c5bc2d624c5709840e6b10
>>
>> Not commenting on the legality or what derived works are considered,
>> since really the markings are just an indication of the authors opinion,
>> and at this stage I think are actually meaningless, since we've diverged
>> considerably from the advice given to Linus back when this started.
>
> Yes, and in this case devm_memremap_pages() was originally written by
> Christoph and I:
>
>     41e94a851304 add devm_memremap_pages

So you wrote some code in 2015 (3 years ago) and you've now decided
to change the EXPORT marker on it? what changed in 3 years, and why
would changing that marker 3 years later have any effect on your original
statement that it was an EXPORT_SYMBOL.

Think what EXPORT_SYMBOL vs GPL means, it isn't a bit stick that magically
makes things into derived works. If something wasn't a derived work for 3 years
using that API, then it isn't a derived work now 3 years later because you
changed the marker. Retrospectively changing the markers doesn't really
make any sense legally or otherwise.

>
> HMM started off by duplicating devm_memremap_pages() which is fixed up
> by this series:

Just looking in my current tree hmm_devmem_pages_create and
devm_memremap_pages don't look like duplicates, they might have
code but they definitely aren't one for one copies. I'm not sure you can
just say Jerome copied that code in, you've now refactored the code
so HMM can use it and are changing the symbol exports underneath it,

Again if Christoph believes all uses of this are a derived work he didn't
indicate it 3 years ago, but neither does the mark make any legal difference
in this case, since everything in the kernel is GPL, and if you
consider something
a derived work or not is well into legal land.

I'd rather anyways the original author of HMM wishes were respected
on his code, or at least you wait until he gets back from holidays before
pushing to merge this.

Dave.

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

[Dan Williams]

On Tue, May 29, 2018 at 4:00 PM, Dave Airlie <airlied@gmail.com> wrote:
> On 30 May 2018 at 08:31, Dan Williams <dan.j.williams@intel.com> wrote:
>> On Tue, May 29, 2018 at 3:22 PM, Dave Airlie <airlied@gmail.com> wrote:
>>>
>>> On 24 May 2018 at 13:18, Dan Williams <dan.j.williams@intel.com> wrote:
>>> > On Wed, May 23, 2018 at 5:10 PM, Jerome Glisse <jglisse@redhat.com> wrote:
>>> >> On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
>>> >>> Hi Andrew, please consider this series for 4.18.
>>> >>>
>>> >>> For maintainability, as ZONE_DEVICE continues to attract new users,
>>> >>> it is useful to keep all users consolidated on devm_memremap_pages() as
>>> >>> the interface for create "device pages".
>>> >>>
>>> >>> The devm_memremap_pages() implementation was recently reworked to make
>>> >>> it more generic for arbitrary users, like the proposed peer-to-peer
>>> >>> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
>>> >>> devm_memremap_pages() as hmm_devmem_pages_create().
>>> >>>
>>> >>> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
>>> >>> the licensing on the exports given the deep dependencies on the mm.
>>> >>
>>> >> I am on PTO right now so i won't be able to quickly review it all
>>> >> but forcing GPL export is problematic for me now. I rather have
>>> >> device driver using "sane" common helpers than creating their own
>>> >> crazy thing.
>>> >
>>> > Sane drivers that need this level of deep integration with Linux
>>> > memory management need to be upstream. Otherwise, HMM is an
>>> > unprecedented departure from the norms of Linux kernel development.
>>>
>>> Isn't it the author of code choice what EXPORT_SYMBOL to use? and
>>> isn't the agreement that if something is EXPORT_SYMBOL now, changing
>>> underlying exports isn't considered a good idea. We've seen this before
>>> with the refcount fun,
>>>
>>> See d557d1b58b3546bab2c5bc2d624c5709840e6b10
>>>
>>> Not commenting on the legality or what derived works are considered,
>>> since really the markings are just an indication of the authors opinion,
>>> and at this stage I think are actually meaningless, since we've diverged
>>> considerably from the advice given to Linus back when this started.
>>
>> Yes, and in this case devm_memremap_pages() was originally written by
>> Christoph and I:
>>
>>     41e94a851304 add devm_memremap_pages
>
> So you wrote some code in 2015 (3 years ago) and you've now decided
> to change the EXPORT marker on it? what changed in 3 years, and why
> would changing that marker 3 years later have any effect on your original
> statement that it was an EXPORT_SYMBOL.
>
> Think what EXPORT_SYMBOL vs GPL means, it isn't a bit stick that magically
> makes things into derived works. If something wasn't a derived work for 3 years
> using that API, then it isn't a derived work now 3 years later because you
> changed the marker. Retrospectively changing the markers doesn't really
> make any sense legally or otherwise.

It honestly was an oversight, and as we've gone on to add deeper and
deeper ties into the mm and filesystems [1] I realized this symbol was
mis-labeled.  It would be one thing if this was just some random
kernel leaf / library function, but this capability when turned on
causes the entire kernel to be recompiled as things like the
definition of put_page() changes. It's deeply integrated with how
Linux manages memory.

>> HMM started off by duplicating devm_memremap_pages() which is fixed up
>> by this series:
>
> Just looking in my current tree hmm_devmem_pages_create and
> devm_memremap_pages don't look like duplicates, they might have
> code but they definitely aren't one for one copies. I'm not sure you can
> just say Jerome copied that code in, you've now refactored the code
> so HMM can use it and are changing the symbol exports underneath it,

The initial patches for HMM used devm_memremap_pages() directly, and
during review I asked for the exact same arrangement as implemented
here, i.e. for the dev_pagemap structure to be a sub-structure of the
HMM data [2]. At some point along the way we lost that review
feedback. It was not until Christoph and Logan recently reworked
devm_memermap_pages() that I realized that HMM had unnecessarily
diverged.

> Again if Christoph believes all uses of this are a derived work he didn't
> indicate it 3 years ago, but neither does the mark make any legal difference
> in this case, since everything in the kernel is GPL, and if you
> consider something
> a derived work or not is well into legal land.
>
> I'd rather anyways the original author of HMM wishes were respected
> on his code, or at least you wait until he gets back from holidays before
> pushing to merge this.

To be clear this only affects the usage of the ZONE_DEVICE facility,
I'm not touching the other pieces of HMM that are original to HMM. I
didn't realize Jerome was on vacation when I sent the patches, and I
think it is otherwise healthy to have this discussion in the meantime.

[1]: https://lists.01.org/pipermail/linux-nvdimm/2018-May/015905.html
[2]: http://lkml.iu.edu/hypermail/linux/kernel/1701.2/00812.html

Re: [PATCH 5/5] mm, hmm: mark hmm_devmem_{add, add_resource} EXPORT_SYMBOL_GPL

[Jerome Glisse]

On Tue, May 22, 2018 at 02:31:21PM -0700, Andrew Morton wrote:
> On Tue, 22 May 2018 08:32:36 +0200 Christoph Hellwig <hch@lst.de> wrote:
> 
> > On Mon, May 21, 2018 at 03:35:40PM -0700, Dan Williams wrote:
> > > The routines hmm_devmem_add(), and hmm_devmem_add_resource() are small
> > > wrappers around devm_memremap_pages(). The devm_memremap_pages()
> > > interface is a subset of the hmm functionality which has more and deeper
> > > ties into the kernel memory management implementation. It was an
> > > oversight that these symbols were not marked EXPORT_SYMBOL_GPL from the
> > > outset due to how they originally copied (and now reuse)
> > > devm_memremap_pages().
> > 
> > If we end up keeping this code: absolutely.  Then again I think without
> > an actual user this should have never been merged, and should be removed
> > until one shows up.
> > 
> 
> It wasn't simple.  Quite a lot of manufacturers were (are?) developing
> quite complex driver code which utilizes hmm.  Merging hmm to give a
> stable target for that development and in the expectation that those
> things would be coming along was a risk and I don't think we yet know
> the outcome.
> 
> Jerome, are you able to provide any updates on all of this?

Sorry for taking so long to reply to this, I am just back from vacation.

I posted a v1 for nouveau to use HMM back in April or early May. I want
to post a v2 soon in June. For it to get upstream it needs to fullfill
linux drm sub-system requirement which are an open source userspace for
any functionality added to GPU driver. Work for this have been going
on for a while too and userspace bits are slowly getting upstream inside
Mesa. I need to sync up to see what is still missing in Mesa.

So i won't be able to get nouveau HMM bits merge before the userspace
bits are merge too. I was hopping for 4.18 but more likely 4.19.

I know HMM have been a big chicken and egg thing and that timing for
the egg did not match the timing for the chicken :) But it is getting
there.


Also I expect more hardware and associated upstream driver to make use
of HMM but i can not comment further on that at this time because of
NDA.

Cheers,
Jérôme

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

[Jerome Glisse]

On Tue, May 29, 2018 at 04:33:49PM -0700, Dan Williams wrote:
> On Tue, May 29, 2018 at 4:00 PM, Dave Airlie <airlied@gmail.com> wrote:
> > On 30 May 2018 at 08:31, Dan Williams <dan.j.williams@intel.com> wrote:
> >> On Tue, May 29, 2018 at 3:22 PM, Dave Airlie <airlied@gmail.com> wrote:
> >>>
> >>> On 24 May 2018 at 13:18, Dan Williams <dan.j.williams@intel.com> wrote:
> >>> > On Wed, May 23, 2018 at 5:10 PM, Jerome Glisse <jglisse@redhat.com> wrote:
> >>> >> On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
> >>> >>> Hi Andrew, please consider this series for 4.18.
> >>> >>>
> >>> >>> For maintainability, as ZONE_DEVICE continues to attract new users,
> >>> >>> it is useful to keep all users consolidated on devm_memremap_pages() as
> >>> >>> the interface for create "device pages".
> >>> >>>
> >>> >>> The devm_memremap_pages() implementation was recently reworked to make
> >>> >>> it more generic for arbitrary users, like the proposed peer-to-peer
> >>> >>> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
> >>> >>> devm_memremap_pages() as hmm_devmem_pages_create().
> >>> >>>
> >>> >>> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
> >>> >>> the licensing on the exports given the deep dependencies on the mm.
> >>> >>
> >>> >> I am on PTO right now so i won't be able to quickly review it all
> >>> >> but forcing GPL export is problematic for me now. I rather have
> >>> >> device driver using "sane" common helpers than creating their own
> >>> >> crazy thing.
> >>> >
> >>> > Sane drivers that need this level of deep integration with Linux
> >>> > memory management need to be upstream. Otherwise, HMM is an
> >>> > unprecedented departure from the norms of Linux kernel development.
> >>>
> >>> Isn't it the author of code choice what EXPORT_SYMBOL to use? and
> >>> isn't the agreement that if something is EXPORT_SYMBOL now, changing
> >>> underlying exports isn't considered a good idea. We've seen this before
> >>> with the refcount fun,
> >>>
> >>> See d557d1b58b3546bab2c5bc2d624c5709840e6b10
> >>>
> >>> Not commenting on the legality or what derived works are considered,
> >>> since really the markings are just an indication of the authors opinion,
> >>> and at this stage I think are actually meaningless, since we've diverged
> >>> considerably from the advice given to Linus back when this started.
> >>
> >> Yes, and in this case devm_memremap_pages() was originally written by
> >> Christoph and I:
> >>
> >>     41e94a851304 add devm_memremap_pages
> >
> > So you wrote some code in 2015 (3 years ago) and you've now decided
> > to change the EXPORT marker on it? what changed in 3 years, and why
> > would changing that marker 3 years later have any effect on your original
> > statement that it was an EXPORT_SYMBOL.
> >
> > Think what EXPORT_SYMBOL vs GPL means, it isn't a bit stick that magically
> > makes things into derived works. If something wasn't a derived work for 3 years
> > using that API, then it isn't a derived work now 3 years later because you
> > changed the marker. Retrospectively changing the markers doesn't really
> > make any sense legally or otherwise.
> 
> It honestly was an oversight, and as we've gone on to add deeper and
> deeper ties into the mm and filesystems [1] I realized this symbol was
> mis-labeled.  It would be one thing if this was just some random
> kernel leaf / library function, but this capability when turned on
> causes the entire kernel to be recompiled as things like the
> definition of put_page() changes. It's deeply integrated with how
> Linux manages memory.

I am personaly on the fence on deciding GPL versus non GPL export
base on subjective view of what is deeply integrated and what is
not. I think one can argue that every single linux kernel function
is deeply integrated within the kernel, starting with all device
drivers functions. One could similarly argue that nothing is ...

I see more value in consistency of symbol export over time. Once
we pick one it should stay that way.


> >> HMM started off by duplicating devm_memremap_pages() which is fixed up
> >> by this series:
> >
> > Just looking in my current tree hmm_devmem_pages_create and
> > devm_memremap_pages don't look like duplicates, they might have
> > code but they definitely aren't one for one copies. I'm not sure you can
> > just say Jerome copied that code in, you've now refactored the code
> > so HMM can use it and are changing the symbol exports underneath it,
> 
> The initial patches for HMM used devm_memremap_pages() directly, and
> during review I asked for the exact same arrangement as implemented
> here, i.e. for the dev_pagemap structure to be a sub-structure of the
> HMM data [2]. At some point along the way we lost that review
> feedback. It was not until Christoph and Logan recently reworked
> devm_memermap_pages() that I realized that HMM had unnecessarily
> diverged.
> 
> > Again if Christoph believes all uses of this are a derived work he didn't
> > indicate it 3 years ago, but neither does the mark make any legal difference
> > in this case, since everything in the kernel is GPL, and if you
> > consider something
> > a derived work or not is well into legal land.
> >
> > I'd rather anyways the original author of HMM wishes were respected
> > on his code, or at least you wait until he gets back from holidays before
> > pushing to merge this.
> 
> To be clear this only affects the usage of the ZONE_DEVICE facility,
> I'm not touching the other pieces of HMM that are original to HMM. I
> didn't realize Jerome was on vacation when I sent the patches, and I
> think it is otherwise healthy to have this discussion in the meantime.

While HMM is a toolbox and i intend to use some part of HMM to replace
some existing GUP user. Biggest user of HMM will use all the tools in
the HMM box. Thus having a mix of GPL and non GPL defeat its usefulness
for out of tree drivers.

I still want to review this patchset, i am going through a backlog of
urgent emails so i probably won't be able to review before a day or two.

I am just strongly against changing to GPL only export.

Cheers,
Jérôme

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

[Dave Airlie]

On 6 June 2018 at 04:48, Jerome Glisse <jglisse@redhat.com> wrote:
> On Tue, May 29, 2018 at 04:33:49PM -0700, Dan Williams wrote:
>> On Tue, May 29, 2018 at 4:00 PM, Dave Airlie <airlied@gmail.com> wrote:
>> > On 30 May 2018 at 08:31, Dan Williams <dan.j.williams@intel.com> wrote:
>> >> On Tue, May 29, 2018 at 3:22 PM, Dave Airlie <airlied@gmail.com> wrote:
>> >>>
>> >>> On 24 May 2018 at 13:18, Dan Williams <dan.j.williams@intel.com> wrote:
>> >>> > On Wed, May 23, 2018 at 5:10 PM, Jerome Glisse <jglisse@redhat.com> wrote:
>> >>> >> On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
>> >>> >>> Hi Andrew, please consider this series for 4.18.
>> >>> >>>
>> >>> >>> For maintainability, as ZONE_DEVICE continues to attract new users,
>> >>> >>> it is useful to keep all users consolidated on devm_memremap_pages() as
>> >>> >>> the interface for create "device pages".
>> >>> >>>
>> >>> >>> The devm_memremap_pages() implementation was recently reworked to make
>> >>> >>> it more generic for arbitrary users, like the proposed peer-to-peer
>> >>> >>> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
>> >>> >>> devm_memremap_pages() as hmm_devmem_pages_create().
>> >>> >>>
>> >>> >>> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
>> >>> >>> the licensing on the exports given the deep dependencies on the mm.
>> >>> >>
>> >>> >> I am on PTO right now so i won't be able to quickly review it all
>> >>> >> but forcing GPL export is problematic for me now. I rather have
>> >>> >> device driver using "sane" common helpers than creating their own
>> >>> >> crazy thing.
>> >>> >
>> >>> > Sane drivers that need this level of deep integration with Linux
>> >>> > memory management need to be upstream. Otherwise, HMM is an
>> >>> > unprecedented departure from the norms of Linux kernel development.
>> >>>
>> >>> Isn't it the author of code choice what EXPORT_SYMBOL to use? and
>> >>> isn't the agreement that if something is EXPORT_SYMBOL now, changing
>> >>> underlying exports isn't considered a good idea. We've seen this before
>> >>> with the refcount fun,
>> >>>
>> >>> See d557d1b58b3546bab2c5bc2d624c5709840e6b10
>> >>>
>> >>> Not commenting on the legality or what derived works are considered,
>> >>> since really the markings are just an indication of the authors opinion,
>> >>> and at this stage I think are actually meaningless, since we've diverged
>> >>> considerably from the advice given to Linus back when this started.
>> >>
>> >> Yes, and in this case devm_memremap_pages() was originally written by
>> >> Christoph and I:
>> >>
>> >>     41e94a851304 add devm_memremap_pages
>> >
>> > So you wrote some code in 2015 (3 years ago) and you've now decided
>> > to change the EXPORT marker on it? what changed in 3 years, and why
>> > would changing that marker 3 years later have any effect on your original
>> > statement that it was an EXPORT_SYMBOL.
>> >
>> > Think what EXPORT_SYMBOL vs GPL means, it isn't a bit stick that magically
>> > makes things into derived works. If something wasn't a derived work for 3 years
>> > using that API, then it isn't a derived work now 3 years later because you
>> > changed the marker. Retrospectively changing the markers doesn't really
>> > make any sense legally or otherwise.
>>
>> It honestly was an oversight, and as we've gone on to add deeper and
>> deeper ties into the mm and filesystems [1] I realized this symbol was
>> mis-labeled.  It would be one thing if this was just some random
>> kernel leaf / library function, but this capability when turned on
>> causes the entire kernel to be recompiled as things like the
>> definition of put_page() changes. It's deeply integrated with how
>> Linux manages memory.
>
> I am personaly on the fence on deciding GPL versus non GPL export
> base on subjective view of what is deeply integrated and what is
> not. I think one can argue that every single linux kernel function
> is deeply integrated within the kernel, starting with all device
> drivers functions. One could similarly argue that nothing is ...

This is the point I wasn't making so well, the whole deciding on a derived
work from the pov of one of the works isn't really going to be how a court
looks at it.

At day 0, you have a Linux kernel, and a separate Windows kernel driver,
clearly they are not derived works.

You add interfaces to the Windows kernel driver and it becomes a Linux
kernel driver, you never ship them together, derived work only if those
interfaces are GPL only? or derived work only if shipped together?
only shipped together and GPL only? Clearly not a clearcut case here.

The code base is 99% the same, the kernel changes an export to a GPL
export, the external driver hasn't changed one line of code, and it suddenly
becomes a derived work?

Oversights happen, but 3 years of advertising an interface under the non-GPL
and changing it doesn't change whether the external driver is derived or not,
nor will it change anyone's legal position.

Dave.

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

[Dan Williams]

On Tue, Jun 5, 2018 at 3:19 PM, Dave Airlie <airlied@gmail.com> wrote:
> On 6 June 2018 at 04:48, Jerome Glisse <jglisse@redhat.com> wrote:
>> On Tue, May 29, 2018 at 04:33:49PM -0700, Dan Williams wrote:
>>> On Tue, May 29, 2018 at 4:00 PM, Dave Airlie <airlied@gmail.com> wrote:
>>> > On 30 May 2018 at 08:31, Dan Williams <dan.j.williams@intel.com> wrote:
>>> >> On Tue, May 29, 2018 at 3:22 PM, Dave Airlie <airlied@gmail.com> wrote:
>>> >>>
>>> >>> On 24 May 2018 at 13:18, Dan Williams <dan.j.williams@intel.com> wrote:
>>> >>> > On Wed, May 23, 2018 at 5:10 PM, Jerome Glisse <jglisse@redhat.com> wrote:
>>> >>> >> On Mon, May 21, 2018 at 03:35:14PM -0700, Dan Williams wrote:
>>> >>> >>> Hi Andrew, please consider this series for 4.18.
>>> >>> >>>
>>> >>> >>> For maintainability, as ZONE_DEVICE continues to attract new users,
>>> >>> >>> it is useful to keep all users consolidated on devm_memremap_pages() as
>>> >>> >>> the interface for create "device pages".
>>> >>> >>>
>>> >>> >>> The devm_memremap_pages() implementation was recently reworked to make
>>> >>> >>> it more generic for arbitrary users, like the proposed peer-to-peer
>>> >>> >>> PCI-E enabling. HMM pre-dated this rework and opted to duplicate
>>> >>> >>> devm_memremap_pages() as hmm_devmem_pages_create().
>>> >>> >>>
>>> >>> >>> Rework HMM to be a consumer of devm_memremap_pages() directly and fix up
>>> >>> >>> the licensing on the exports given the deep dependencies on the mm.
>>> >>> >>
>>> >>> >> I am on PTO right now so i won't be able to quickly review it all
>>> >>> >> but forcing GPL export is problematic for me now. I rather have
>>> >>> >> device driver using "sane" common helpers than creating their own
>>> >>> >> crazy thing.
>>> >>> >
>>> >>> > Sane drivers that need this level of deep integration with Linux
>>> >>> > memory management need to be upstream. Otherwise, HMM is an
>>> >>> > unprecedented departure from the norms of Linux kernel development.
>>> >>>
>>> >>> Isn't it the author of code choice what EXPORT_SYMBOL to use? and
>>> >>> isn't the agreement that if something is EXPORT_SYMBOL now, changing
>>> >>> underlying exports isn't considered a good idea. We've seen this before
>>> >>> with the refcount fun,
>>> >>>
>>> >>> See d557d1b58b3546bab2c5bc2d624c5709840e6b10
>>> >>>
>>> >>> Not commenting on the legality or what derived works are considered,
>>> >>> since really the markings are just an indication of the authors opinion,
>>> >>> and at this stage I think are actually meaningless, since we've diverged
>>> >>> considerably from the advice given to Linus back when this started.
>>> >>
>>> >> Yes, and in this case devm_memremap_pages() was originally written by
>>> >> Christoph and I:
>>> >>
>>> >>     41e94a851304 add devm_memremap_pages
>>> >
>>> > So you wrote some code in 2015 (3 years ago) and you've now decided
>>> > to change the EXPORT marker on it? what changed in 3 years, and why
>>> > would changing that marker 3 years later have any effect on your original
>>> > statement that it was an EXPORT_SYMBOL.
>>> >
>>> > Think what EXPORT_SYMBOL vs GPL means, it isn't a bit stick that magically
>>> > makes things into derived works. If something wasn't a derived work for 3 years
>>> > using that API, then it isn't a derived work now 3 years later because you
>>> > changed the marker. Retrospectively changing the markers doesn't really
>>> > make any sense legally or otherwise.
>>>
>>> It honestly was an oversight, and as we've gone on to add deeper and
>>> deeper ties into the mm and filesystems [1] I realized this symbol was
>>> mis-labeled.  It would be one thing if this was just some random
>>> kernel leaf / library function, but this capability when turned on
>>> causes the entire kernel to be recompiled as things like the
>>> definition of put_page() changes. It's deeply integrated with how
>>> Linux manages memory.
>>
>> I am personaly on the fence on deciding GPL versus non GPL export
>> base on subjective view of what is deeply integrated and what is
>> not. I think one can argue that every single linux kernel function
>> is deeply integrated within the kernel, starting with all device
>> drivers functions. One could similarly argue that nothing is ...
>
> This is the point I wasn't making so well, the whole deciding on a derived
> work from the pov of one of the works isn't really going to be how a court
> looks at it.
>
> At day 0, you have a Linux kernel, and a separate Windows kernel driver,
> clearly they are not derived works.
>
> You add interfaces to the Windows kernel driver and it becomes a Linux
> kernel driver, you never ship them together, derived work only if those
> interfaces are GPL only? or derived work only if shipped together?
> only shipped together and GPL only? Clearly not a clearcut case here.
>
> The code base is 99% the same, the kernel changes an export to a GPL
> export, the external driver hasn't changed one line of code, and it suddenly
> becomes a derived work?
>
> Oversights happen, but 3 years of advertising an interface under the non-GPL
> and changing it doesn't change whether the external driver is derived or not,
> nor will it change anyone's legal position.

My concern is the long term health and maintainability of the Linux
kernel. HMM exports deep Linux internals out to proprietary drivers
with no way for folks in the wider kernel community to validate that
the interfaces are necessary or sufficient besides "take Jerome's word
for it". Every time I've pushed back on any HMM feature the response
is something to the effect of, "no, out of tree drivers need this".
HMM needs to grow upstream users and the functionality needs to be
limited to whatever those upstream users exploit. Since there are no
upstream users of HMM, we should delete it unless / until those users
arrive.

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.

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

[Jerome Glisse]

On Tue, Jun 05, 2018 at 04:06:12PM -0700, Dan Williams wrote:
> On Tue, Jun 5, 2018 at 3:19 PM, Dave Airlie <airlied@gmail.com> wrote:
> > On 6 June 2018 at 04:48, Jerome Glisse <jglisse@redhat.com> wrote:
> >> On Tue, May 29, 2018 at 04:33:49PM -0700, Dan Williams wrote:
> >>> On Tue, May 29, 2018 at 4:00 PM, Dave Airlie <airlied@gmail.com> wrote:
> >>> > On 30 May 2018 at 08:31, Dan Williams <dan.j.williams@intel.com> wrote:

[...]

> >>> It honestly was an oversight, and as we've gone on to add deeper and
> >>> deeper ties into the mm and filesystems [1] I realized this symbol was
> >>> mis-labeled.  It would be one thing if this was just some random
> >>> kernel leaf / library function, but this capability when turned on
> >>> causes the entire kernel to be recompiled as things like the
> >>> definition of put_page() changes. It's deeply integrated with how
> >>> Linux manages memory.
> >>
> >> I am personaly on the fence on deciding GPL versus non GPL export
> >> base on subjective view of what is deeply integrated and what is
> >> not. I think one can argue that every single linux kernel function
> >> is deeply integrated within the kernel, starting with all device
> >> drivers functions. One could similarly argue that nothing is ...
> >
> > This is the point I wasn't making so well, the whole deciding on a derived
> > work from the pov of one of the works isn't really going to be how a court
> > looks at it.
> >
> > At day 0, you have a Linux kernel, and a separate Windows kernel driver,
> > clearly they are not derived works.
> >
> > You add interfaces to the Windows kernel driver and it becomes a Linux
> > kernel driver, you never ship them together, derived work only if those
> > interfaces are GPL only? or derived work only if shipped together?
> > only shipped together and GPL only? Clearly not a clearcut case here.
> >
> > The code base is 99% the same, the kernel changes an export to a GPL
> > export, the external driver hasn't changed one line of code, and it suddenly
> > becomes a derived work?
> >
> > Oversights happen, but 3 years of advertising an interface under the non-GPL
> > and changing it doesn't change whether the external driver is derived or not,
> > nor will it change anyone's legal position.
> 
> My concern is the long term health and maintainability of the Linux
> kernel. HMM exports deep Linux internals out to proprietary drivers
> with no way for folks in the wider kernel community to validate that
> the interfaces are necessary or sufficient besides "take Jerome's word
> for it". Every time I've pushed back on any HMM feature the response
> is something to the effect of, "no, out of tree drivers need this".
> HMM needs to grow upstream users and the functionality needs to be
> limited to whatever those upstream users exploit. Since there are no
> upstream users of HMM, we should delete it unless / until those users
> arrive.

The raison d'être of HMM is to isolate driver from mm internal gut and
thus provide a clear contract and API to device driver. I tried to spell
that contract in include/linux/hmm.h which i can re-formulate shortly in:
  - provide call back when CPU try to access a device page so that
    memory can be migrated back to CPU accessible page under the
    control of the device driver for device synchronization reasons
    (the whole gory mm details is still in mm/migrate.c it just does
    provide way point in the migration process so that the device
    driver can synchronize and update the hardware along the way too)
  - provide a 64bits storage inside struct page so that the device
    driver can store either pointer to its internal data structure
    or store necessary informations there while page is in use in a
    process
  - inform device driver once a page is freed (ie no longer use in a
    process address space)

This virtualy isolate device driver from the inner gut of mm and allow
mm to change as long as we can keep this contract in place. As long as
device driver only use HMM API to perform any of the above and this is
my intention to push for that and try to enforce it as strongly as i
can.

Nouveau patchset have been posted and i will post newer updated version
this month and i hope this can get upstream in 4.19 abidding by the drm
sub-system requirement of having open source userspace upstream in mesa
project too (which have been under work for last few months).

This whole thing have been a big chicken and egg nightmare with moving
pieces everywhere. I wish i was better at getting all the pieces ready
at the same time but alas i was not.

> 
> 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.

Cheers,
Jérôme

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

[Dan Williams]

On Tue, Jun 5, 2018 at 5:08 PM, Jerome Glisse <jglisse@redhat.com> 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.

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

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.

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

[Christoph Hellwig]

On Tue, Jun 05, 2018 at 06:33:04PM -0700, Dan Williams wrote:
> Unless the nouveau patches are using the entirety of what is already
> upstream for HMM, we should look to pare HMM back.
> 
> 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.

Agreed with all of that.  And remember EXPORT_SYMBOL_GPL really just is
a clear expression of the authors they think these are internals.
The lack of it doesn't make it any less a derived work, we just remove
a very clear hint to users that they are poking very deeply into internals.

And with HMM they very clearly do.

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@redhat.com> 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

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

[Dan Williams]

On Thu, Jun 7, 2018 at 7:16 AM, Jerome Glisse <jglisse@redhat.com> wrote:
[..]
> HMM is self contain interface but i doubt i can convince you of that.

I agree that HMM is self contained... after it completely subverts the
core-mm. It's that deep understanding and working around of core
memory management infrastructure that makes devm_memremap_pages() and
derivatives EXPORT_SYMBOL_GPL ("considered an
internal implementation issue, and not really an interface").