Linux-PCI Archive on lore.kernel.org
 help / color / Atom feed
* dev_pagemap related cleanups v3
@ 2019-06-26 12:26 Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 01/25] mm: remove the unused ARCH_HAS_HMM_DEVICE Kconfig option Christoph Hellwig
                   ` (24 more replies)
  0 siblings, 25 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:26 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

Hi Dan, Jérôme and Jason,

below is a series that cleans up the dev_pagemap interface so that
it is more easily usable, which removes the need to wrap it in hmm
and thus allowing to kill a lot of code

Note: this series is on top of Linux 5.2-rc5 and has some minor
conflicts with the hmm tree that are easy to resolve.

Diffstat summary:

 32 files changed, 361 insertions(+), 1012 deletions(-)

Git tree:

    git://git.infradead.org/users/hch/misc.git hmm-devmem-cleanup.3

Gitweb:

    http://git.infradead.org/users/hch/misc.git/shortlog/refs/heads/hmm-devmem-cleanup.3


Changes since v2:
 - fix nvdimm kunit build
 - add a new memory type for device dax
 - fix a few issues in intermediate patches that didn't show up in the end
   result
 - incorporate feedback from Michal Hocko, including killing of
   the DEVICE_PUBLIC memory type entirely

Changes since v1:
 - rebase
 - also switch p2pdma to the internal refcount
 - add type checking for pgmap->type
 - rename the migrate method to migrate_to_ram
 - cleanup the altmap_valid flag
 - various tidbits from the reviews

^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 01/25] mm: remove the unused ARCH_HAS_HMM_DEVICE Kconfig option
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 02/25] mm: remove the struct hmm_device infrastructure Christoph Hellwig
                   ` (23 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
---
 mm/Kconfig | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/mm/Kconfig b/mm/Kconfig
index f0c76ba47695..0d2ba7e1f43e 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -675,16 +675,6 @@ config ARCH_HAS_HMM_MIRROR
 	depends on (X86_64 || PPC64)
 	depends on MMU && 64BIT
 
-config ARCH_HAS_HMM_DEVICE
-	bool
-	default y
-	depends on (X86_64 || PPC64)
-	depends on MEMORY_HOTPLUG
-	depends on MEMORY_HOTREMOVE
-	depends on SPARSEMEM_VMEMMAP
-	depends on ARCH_HAS_ZONE_DEVICE
-	select XARRAY_MULTI
-
 config ARCH_HAS_HMM
 	bool
 	default y
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 02/25] mm: remove the struct hmm_device infrastructure
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 01/25] mm: remove the unused ARCH_HAS_HMM_DEVICE Kconfig option Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 03/25] mm: remove hmm_devmem_add_resource Christoph Hellwig
                   ` (22 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci,
	linux-kernel, John Hubbard

This code is a trivial wrapper around device model helpers, which
should have been integrated into the driver device model usage from
the start.  Assuming it actually had users, which it never had since
the code was added more than 1 1/2 years ago.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
---
 include/linux/hmm.h | 20 ------------
 mm/hmm.c            | 80 ---------------------------------------------
 2 files changed, 100 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 044a36d7c3f8..99765be3284d 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -751,26 +751,6 @@ static inline unsigned long hmm_devmem_page_get_drvdata(const struct page *page)
 {
 	return page->hmm_data;
 }
-
-
-/*
- * struct hmm_device - fake device to hang device memory onto
- *
- * @device: device struct
- * @minor: device minor number
- */
-struct hmm_device {
-	struct device		device;
-	unsigned int		minor;
-};
-
-/*
- * 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 strictly needed, in order to make use of any HMM functionality.
- */
-struct hmm_device *hmm_device_new(void *drvdata);
-void hmm_device_put(struct hmm_device *hmm_device);
 #endif /* CONFIG_DEVICE_PRIVATE || CONFIG_DEVICE_PUBLIC */
 #else /* IS_ENABLED(CONFIG_HMM) */
 static inline void hmm_mm_destroy(struct mm_struct *mm) {}
diff --git a/mm/hmm.c b/mm/hmm.c
index f702a3895d05..00cc642b3d7e 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1528,84 +1528,4 @@ struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
 	return devmem;
 }
 EXPORT_SYMBOL_GPL(hmm_devmem_add_resource);
-
-/*
- * 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.
- */
-#define HMM_DEVICE_MAX 256
-
-static DECLARE_BITMAP(hmm_device_mask, HMM_DEVICE_MAX);
-static DEFINE_SPINLOCK(hmm_device_lock);
-static struct class *hmm_device_class;
-static dev_t hmm_device_devt;
-
-static void hmm_device_release(struct device *device)
-{
-	struct hmm_device *hmm_device;
-
-	hmm_device = container_of(device, struct hmm_device, device);
-	spin_lock(&hmm_device_lock);
-	clear_bit(hmm_device->minor, hmm_device_mask);
-	spin_unlock(&hmm_device_lock);
-
-	kfree(hmm_device);
-}
-
-struct hmm_device *hmm_device_new(void *drvdata)
-{
-	struct hmm_device *hmm_device;
-
-	hmm_device = kzalloc(sizeof(*hmm_device), GFP_KERNEL);
-	if (!hmm_device)
-		return ERR_PTR(-ENOMEM);
-
-	spin_lock(&hmm_device_lock);
-	hmm_device->minor = find_first_zero_bit(hmm_device_mask, HMM_DEVICE_MAX);
-	if (hmm_device->minor >= HMM_DEVICE_MAX) {
-		spin_unlock(&hmm_device_lock);
-		kfree(hmm_device);
-		return ERR_PTR(-EBUSY);
-	}
-	set_bit(hmm_device->minor, hmm_device_mask);
-	spin_unlock(&hmm_device_lock);
-
-	dev_set_name(&hmm_device->device, "hmm_device%d", hmm_device->minor);
-	hmm_device->device.devt = MKDEV(MAJOR(hmm_device_devt),
-					hmm_device->minor);
-	hmm_device->device.release = hmm_device_release;
-	dev_set_drvdata(&hmm_device->device, drvdata);
-	hmm_device->device.class = hmm_device_class;
-	device_initialize(&hmm_device->device);
-
-	return hmm_device;
-}
-EXPORT_SYMBOL(hmm_device_new);
-
-void hmm_device_put(struct hmm_device *hmm_device)
-{
-	put_device(&hmm_device->device);
-}
-EXPORT_SYMBOL(hmm_device_put);
-
-static int __init hmm_init(void)
-{
-	int ret;
-
-	ret = alloc_chrdev_region(&hmm_device_devt, 0,
-				  HMM_DEVICE_MAX,
-				  "hmm_device");
-	if (ret)
-		return ret;
-
-	hmm_device_class = class_create(THIS_MODULE, "hmm_device");
-	if (IS_ERR(hmm_device_class)) {
-		unregister_chrdev_region(hmm_device_devt, HMM_DEVICE_MAX);
-		return PTR_ERR(hmm_device_class);
-	}
-	return 0;
-}
-
-device_initcall(hmm_init);
 #endif /* CONFIG_DEVICE_PRIVATE || CONFIG_DEVICE_PUBLIC */
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 03/25] mm: remove hmm_devmem_add_resource
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 01/25] mm: remove the unused ARCH_HAS_HMM_DEVICE Kconfig option Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 02/25] mm: remove the struct hmm_device infrastructure Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-27 16:18   ` Jason Gunthorpe
  2019-06-26 12:27 ` [PATCH 04/25] mm: remove MEMORY_DEVICE_PUBLIC support Christoph Hellwig
                   ` (21 subsequent siblings)
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci,
	linux-kernel, John Hubbard, Michal Hocko

This function has never been used since it was first added to the kernel
more than a year and a half ago, and if we ever grow a consumer of the
MEMORY_DEVICE_PUBLIC infrastructure it can easily use devm_memremap_pages
directly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 include/linux/hmm.h |  3 ---
 mm/hmm.c            | 50 ---------------------------------------------
 2 files changed, 53 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 99765be3284d..5c46b0f603fd 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -722,9 +722,6 @@ struct hmm_devmem {
 struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 				  struct device *device,
 				  unsigned long size);
-struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
-					   struct device *device,
-					   struct resource *res);
 
 /*
  * hmm_devmem_page_set_drvdata - set per-page driver data field
diff --git a/mm/hmm.c b/mm/hmm.c
index 00cc642b3d7e..bd260a3b6b09 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1478,54 +1478,4 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	return devmem;
 }
 EXPORT_SYMBOL_GPL(hmm_devmem_add);
-
-struct hmm_devmem *hmm_devmem_add_resource(const struct hmm_devmem_ops *ops,
-					   struct device *device,
-					   struct resource *res)
-{
-	struct hmm_devmem *devmem;
-	void *result;
-	int ret;
-
-	if (res->desc != IORES_DESC_DEVICE_PUBLIC_MEMORY)
-		return ERR_PTR(-EINVAL);
-
-	dev_pagemap_get_ops();
-
-	devmem = devm_kzalloc(device, sizeof(*devmem), GFP_KERNEL);
-	if (!devmem)
-		return ERR_PTR(-ENOMEM);
-
-	init_completion(&devmem->completion);
-	devmem->pfn_first = -1UL;
-	devmem->pfn_last = -1UL;
-	devmem->resource = res;
-	devmem->device = device;
-	devmem->ops = ops;
-
-	ret = percpu_ref_init(&devmem->ref, &hmm_devmem_ref_release,
-			      0, GFP_KERNEL);
-	if (ret)
-		return ERR_PTR(ret);
-
-	devmem->pfn_first = devmem->resource->start >> PAGE_SHIFT;
-	devmem->pfn_last = devmem->pfn_first +
-			   (resource_size(devmem->resource) >> PAGE_SHIFT);
-	devmem->page_fault = hmm_devmem_fault;
-
-	devmem->pagemap.type = MEMORY_DEVICE_PUBLIC;
-	devmem->pagemap.res = *devmem->resource;
-	devmem->pagemap.page_free = hmm_devmem_free;
-	devmem->pagemap.altmap_valid = false;
-	devmem->pagemap.ref = &devmem->ref;
-	devmem->pagemap.data = devmem;
-	devmem->pagemap.kill = hmm_devmem_ref_kill;
-	devmem->pagemap.cleanup = hmm_devmem_ref_exit;
-
-	result = devm_memremap_pages(devmem->device, &devmem->pagemap);
-	if (IS_ERR(result))
-		return result;
-	return devmem;
-}
-EXPORT_SYMBOL_GPL(hmm_devmem_add_resource);
 #endif /* CONFIG_DEVICE_PRIVATE || CONFIG_DEVICE_PUBLIC */
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 04/25] mm: remove MEMORY_DEVICE_PUBLIC support
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (2 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 03/25] mm: remove hmm_devmem_add_resource Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 16:00   ` Dan Williams
  2019-06-26 12:27 ` [PATCH 05/25] mm: don't clear ->mapping in hmm_devmem_free Christoph Hellwig
                   ` (20 subsequent siblings)
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci,
	linux-kernel, Michal Hocko

The code hasn't been used since it was added to the tree, and doesn't
appear to actually be usable.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 include/linux/hmm.h      |  4 ++--
 include/linux/ioport.h   |  1 -
 include/linux/memremap.h |  8 --------
 include/linux/mm.h       | 12 ------------
 mm/Kconfig               | 11 -----------
 mm/gup.c                 |  7 -------
 mm/hmm.c                 |  4 ++--
 mm/memcontrol.c          | 11 +++++------
 mm/memory-failure.c      |  6 +-----
 mm/memory.c              | 34 ----------------------------------
 mm/migrate.c             | 26 +++-----------------------
 mm/swap.c                | 11 -----------
 12 files changed, 13 insertions(+), 122 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 5c46b0f603fd..44a5ac738bb5 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -584,7 +584,7 @@ static inline void hmm_mm_destroy(struct mm_struct *mm) {}
 static inline void hmm_mm_init(struct mm_struct *mm) {}
 #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
 
-#if IS_ENABLED(CONFIG_DEVICE_PRIVATE) ||  IS_ENABLED(CONFIG_DEVICE_PUBLIC)
+#if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
 struct hmm_devmem;
 
 struct page *hmm_vma_alloc_locked_page(struct vm_area_struct *vma,
@@ -748,7 +748,7 @@ static inline unsigned long hmm_devmem_page_get_drvdata(const struct page *page)
 {
 	return page->hmm_data;
 }
-#endif /* CONFIG_DEVICE_PRIVATE || CONFIG_DEVICE_PUBLIC */
+#endif /* CONFIG_DEVICE_PRIVATE */
 #else /* IS_ENABLED(CONFIG_HMM) */
 static inline void hmm_mm_destroy(struct mm_struct *mm) {}
 static inline void hmm_mm_init(struct mm_struct *mm) {}
diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index da0ebaec25f0..dd961882bc74 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -132,7 +132,6 @@ enum {
 	IORES_DESC_PERSISTENT_MEMORY		= 4,
 	IORES_DESC_PERSISTENT_MEMORY_LEGACY	= 5,
 	IORES_DESC_DEVICE_PRIVATE_MEMORY	= 6,
-	IORES_DESC_DEVICE_PUBLIC_MEMORY		= 7,
 };
 
 /* helpers to define resources */
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index 1732dea030b2..995c62c5a48b 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -37,13 +37,6 @@ struct vmem_altmap {
  * A more complete discussion of unaddressable memory may be found in
  * include/linux/hmm.h and Documentation/vm/hmm.rst.
  *
- * MEMORY_DEVICE_PUBLIC:
- * Device memory that is cache coherent from device and CPU point of view. This
- * is use on platform that have an advance system bus (like CAPI or CCIX). A
- * driver can hotplug the device memory using ZONE_DEVICE and with that memory
- * type. Any page of a process can be migrated to such memory. However no one
- * should be allow to pin such memory so that it can always be evicted.
- *
  * MEMORY_DEVICE_FS_DAX:
  * Host memory that has similar access semantics as System RAM i.e. DMA
  * coherent and supports page pinning. In support of coordinating page
@@ -58,7 +51,6 @@ struct vmem_altmap {
  */
 enum memory_type {
 	MEMORY_DEVICE_PRIVATE = 1,
-	MEMORY_DEVICE_PUBLIC,
 	MEMORY_DEVICE_FS_DAX,
 	MEMORY_DEVICE_PCI_P2PDMA,
 };
diff --git a/include/linux/mm.h b/include/linux/mm.h
index dd0b5f4e1e45..6e4b9be08b13 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -944,7 +944,6 @@ static inline bool put_devmap_managed_page(struct page *page)
 		return false;
 	switch (page->pgmap->type) {
 	case MEMORY_DEVICE_PRIVATE:
-	case MEMORY_DEVICE_PUBLIC:
 	case MEMORY_DEVICE_FS_DAX:
 		__put_devmap_managed_page(page);
 		return true;
@@ -960,12 +959,6 @@ static inline bool is_device_private_page(const struct page *page)
 		page->pgmap->type == MEMORY_DEVICE_PRIVATE;
 }
 
-static inline bool is_device_public_page(const struct page *page)
-{
-	return is_zone_device_page(page) &&
-		page->pgmap->type == MEMORY_DEVICE_PUBLIC;
-}
-
 #ifdef CONFIG_PCI_P2PDMA
 static inline bool is_pci_p2pdma_page(const struct page *page)
 {
@@ -998,11 +991,6 @@ static inline bool is_device_private_page(const struct page *page)
 	return false;
 }
 
-static inline bool is_device_public_page(const struct page *page)
-{
-	return false;
-}
-
 static inline bool is_pci_p2pdma_page(const struct page *page)
 {
 	return false;
diff --git a/mm/Kconfig b/mm/Kconfig
index 0d2ba7e1f43e..6f35b85b3052 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -718,17 +718,6 @@ config DEVICE_PRIVATE
 	  memory; i.e., memory that is only accessible from the device (or
 	  group of devices). You likely also want to select HMM_MIRROR.
 
-config DEVICE_PUBLIC
-	bool "Addressable device memory (like GPU memory)"
-	depends on ARCH_HAS_HMM
-	select HMM
-	select DEV_PAGEMAP_OPS
-
-	help
-	  Allows creation of struct pages to represent addressable device
-	  memory; i.e., memory that is accessible from both the device and
-	  the CPU
-
 config FRAME_VECTOR
 	bool
 
diff --git a/mm/gup.c b/mm/gup.c
index ddde097cf9e4..fe131d879c70 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -605,13 +605,6 @@ static int get_gate_page(struct mm_struct *mm, unsigned long address,
 		if ((gup_flags & FOLL_DUMP) || !is_zero_pfn(pte_pfn(*pte)))
 			goto unmap;
 		*page = pte_page(*pte);
-
-		/*
-		 * This should never happen (a device public page in the gate
-		 * area).
-		 */
-		if (is_device_public_page(*page))
-			goto unmap;
 	}
 	if (unlikely(!try_get_page(*page))) {
 		ret = -ENOMEM;
diff --git a/mm/hmm.c b/mm/hmm.c
index bd260a3b6b09..376159a769fb 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1331,7 +1331,7 @@ EXPORT_SYMBOL(hmm_range_dma_unmap);
 #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
 
 
-#if IS_ENABLED(CONFIG_DEVICE_PRIVATE) ||  IS_ENABLED(CONFIG_DEVICE_PUBLIC)
+#if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
 struct page *hmm_vma_alloc_locked_page(struct vm_area_struct *vma,
 				       unsigned long addr)
 {
@@ -1478,4 +1478,4 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	return devmem;
 }
 EXPORT_SYMBOL_GPL(hmm_devmem_add);
-#endif /* CONFIG_DEVICE_PRIVATE || CONFIG_DEVICE_PUBLIC */
+#endif /* CONFIG_DEVICE_PRIVATE  */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ba9138a4a1de..fa844ae85bce 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4994,8 +4994,8 @@ static int mem_cgroup_move_account(struct page *page,
  *   2(MC_TARGET_SWAP): if the swap entry corresponding to this pte is a
  *     target for charge migration. if @target is not NULL, the entry is stored
  *     in target->ent.
- *   3(MC_TARGET_DEVICE): like MC_TARGET_PAGE  but page is MEMORY_DEVICE_PUBLIC
- *     or MEMORY_DEVICE_PRIVATE (so ZONE_DEVICE page and thus not on the lru).
+ *   3(MC_TARGET_DEVICE): like MC_TARGET_PAGE  but page is MEMORY_DEVICE_PRIVATE
+ *     (so ZONE_DEVICE page and thus not on the lru).
  *     For now we such page is charge like a regular page would be as for all
  *     intent and purposes it is just special memory taking the place of a
  *     regular page.
@@ -5029,8 +5029,7 @@ static enum mc_target_type get_mctgt_type(struct vm_area_struct *vma,
 		 */
 		if (page->mem_cgroup == mc.from) {
 			ret = MC_TARGET_PAGE;
-			if (is_device_private_page(page) ||
-			    is_device_public_page(page))
+			if (is_device_private_page(page))
 				ret = MC_TARGET_DEVICE;
 			if (target)
 				target->page = page;
@@ -5101,8 +5100,8 @@ static int mem_cgroup_count_precharge_pte_range(pmd_t *pmd,
 	if (ptl) {
 		/*
 		 * Note their can not be MC_TARGET_DEVICE for now as we do not
-		 * support transparent huge page with MEMORY_DEVICE_PUBLIC or
-		 * MEMORY_DEVICE_PRIVATE but this might change.
+		 * support transparent huge page with MEMORY_DEVICE_PRIVATE but
+		 * this might change.
 		 */
 		if (get_mctgt_type_thp(vma, addr, *pmd, NULL) == MC_TARGET_PAGE)
 			mc.precharge += HPAGE_PMD_NR;
diff --git a/mm/memory-failure.c b/mm/memory-failure.c
index 8da0334b9ca0..d9fc1a8bdf6a 100644
--- a/mm/memory-failure.c
+++ b/mm/memory-failure.c
@@ -1177,16 +1177,12 @@ static int memory_failure_dev_pagemap(unsigned long pfn, int flags,
 		goto unlock;
 	}
 
-	switch (pgmap->type) {
-	case MEMORY_DEVICE_PRIVATE:
-	case MEMORY_DEVICE_PUBLIC:
+	if (pgmap->type == MEMORY_DEVICE_PRIVATE) {
 		/*
 		 * TODO: Handle HMM pages which may need coordination
 		 * with device-side memory.
 		 */
 		goto unlock;
-	default:
-		break;
 	}
 
 	/*
diff --git a/mm/memory.c b/mm/memory.c
index ddf20bd0c317..bd21e7063bf0 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -585,29 +585,6 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr,
 			return NULL;
 		if (is_zero_pfn(pfn))
 			return NULL;
-
-		/*
-		 * Device public pages are special pages (they are ZONE_DEVICE
-		 * pages but different from persistent memory). They behave
-		 * allmost like normal pages. The difference is that they are
-		 * not on the lru and thus should never be involve with any-
-		 * thing that involve lru manipulation (mlock, numa balancing,
-		 * ...).
-		 *
-		 * This is why we still want to return NULL for such page from
-		 * vm_normal_page() so that we do not have to special case all
-		 * call site of vm_normal_page().
-		 */
-		if (likely(pfn <= highest_memmap_pfn)) {
-			struct page *page = pfn_to_page(pfn);
-
-			if (is_device_public_page(page)) {
-				if (with_public_device)
-					return page;
-				return NULL;
-			}
-		}
-
 		if (pte_devmap(pte))
 			return NULL;
 
@@ -797,17 +774,6 @@ copy_one_pte(struct mm_struct *dst_mm, struct mm_struct *src_mm,
 		rss[mm_counter(page)]++;
 	} else if (pte_devmap(pte)) {
 		page = pte_page(pte);
-
-		/*
-		 * Cache coherent device memory behave like regular page and
-		 * not like persistent memory page. For more informations see
-		 * MEMORY_DEVICE_CACHE_COHERENT in memory_hotplug.h
-		 */
-		if (is_device_public_page(page)) {
-			get_page(page);
-			page_dup_rmap(page, false);
-			rss[mm_counter(page)]++;
-		}
 	}
 
 out_set_pte:
diff --git a/mm/migrate.c b/mm/migrate.c
index f2ecc2855a12..149c692d5f9b 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -246,8 +246,6 @@ static bool remove_migration_pte(struct page *page, struct vm_area_struct *vma,
 			if (is_device_private_page(new)) {
 				entry = make_device_private_entry(new, pte_write(pte));
 				pte = swp_entry_to_pte(entry);
-			} else if (is_device_public_page(new)) {
-				pte = pte_mkdevmap(pte);
 			}
 		}
 
@@ -381,7 +379,6 @@ static int expected_page_refs(struct address_space *mapping, struct page *page)
 	 * ZONE_DEVICE pages.
 	 */
 	expected_count += is_device_private_page(page);
-	expected_count += is_device_public_page(page);
 	if (mapping)
 		expected_count += hpage_nr_pages(page) + page_has_private(page);
 
@@ -994,10 +991,7 @@ static int move_to_new_page(struct page *newpage, struct page *page,
 		if (!PageMappingFlags(page))
 			page->mapping = NULL;
 
-		if (unlikely(is_zone_device_page(newpage))) {
-			if (is_device_public_page(newpage))
-				flush_dcache_page(newpage);
-		} else
+		if (likely(!is_zone_device_page(newpage)))
 			flush_dcache_page(newpage);
 
 	}
@@ -2406,16 +2400,7 @@ static bool migrate_vma_check_page(struct page *page)
 		 * FIXME proper solution is to rework migration_entry_wait() so
 		 * it does not need to take a reference on page.
 		 */
-		if (is_device_private_page(page))
-			return true;
-
-		/*
-		 * Only allow device public page to be migrated and account for
-		 * the extra reference count imply by ZONE_DEVICE pages.
-		 */
-		if (!is_device_public_page(page))
-			return false;
-		extra++;
+		return is_device_private_page(page);
 	}
 
 	/* For file back page */
@@ -2665,11 +2650,6 @@ static void migrate_vma_insert_page(struct migrate_vma *migrate,
 
 			swp_entry = make_device_private_entry(page, vma->vm_flags & VM_WRITE);
 			entry = swp_entry_to_pte(swp_entry);
-		} else if (is_device_public_page(page)) {
-			entry = pte_mkold(mk_pte(page, READ_ONCE(vma->vm_page_prot)));
-			if (vma->vm_flags & VM_WRITE)
-				entry = pte_mkwrite(pte_mkdirty(entry));
-			entry = pte_mkdevmap(entry);
 		}
 	} else {
 		entry = mk_pte(page, vma->vm_page_prot);
@@ -2789,7 +2769,7 @@ static void migrate_vma_pages(struct migrate_vma *migrate)
 					migrate->src[i] &= ~MIGRATE_PFN_MIGRATE;
 					continue;
 				}
-			} else if (!is_device_public_page(newpage)) {
+			} else {
 				/*
 				 * Other types of ZONE_DEVICE page are not
 				 * supported.
diff --git a/mm/swap.c b/mm/swap.c
index 7ede3eddc12a..83107410d29f 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -740,17 +740,6 @@ void release_pages(struct page **pages, int nr)
 		if (is_huge_zero_page(page))
 			continue;
 
-		/* Device public page can not be huge page */
-		if (is_device_public_page(page)) {
-			if (locked_pgdat) {
-				spin_unlock_irqrestore(&locked_pgdat->lru_lock,
-						       flags);
-				locked_pgdat = NULL;
-			}
-			put_devmap_managed_page(page);
-			continue;
-		}
-
 		page = compound_head(page);
 		if (!put_page_testzero(page))
 			continue;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 05/25] mm: don't clear ->mapping in hmm_devmem_free
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (3 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 04/25] mm: remove MEMORY_DEVICE_PUBLIC support Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 06/25] mm: export alloc_pages_vma Christoph Hellwig
                   ` (19 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci,
	linux-kernel, John Hubbard

->mapping isn't even used by HMM users, and the field at the same offset
in the zone_device part of the union is declared as pad.  (Which btw is
rather confusing, as DAX uses ->pgmap and ->mapping from two different
sides of the union, but DAX doesn't use hmm_devmem_free).

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/hmm.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/mm/hmm.c b/mm/hmm.c
index 376159a769fb..e7dd2ab8f9ab 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1383,8 +1383,6 @@ static void hmm_devmem_free(struct page *page, void *data)
 {
 	struct hmm_devmem *devmem = data;
 
-	page->mapping = NULL;
-
 	devmem->ops->free(devmem, page);
 }
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 06/25] mm: export alloc_pages_vma
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (4 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 05/25] mm: don't clear ->mapping in hmm_devmem_free Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:36   ` Michal Hocko
  2019-06-26 12:27 ` [PATCH 07/25] mm: factor out a devm_request_free_mem_region helper Christoph Hellwig
                   ` (18 subsequent siblings)
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci,
	linux-kernel, John Hubbard

nouveau is currently using this through an odd hmm wrapper, and I plan
to switch it to the real thing later in this series.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
---
 mm/mempolicy.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 01600d80ae01..f48569aa1863 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -2098,6 +2098,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
 out:
 	return page;
 }
+EXPORT_SYMBOL(alloc_pages_vma);
 
 /**
  * 	alloc_pages_current - Allocate pages.
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 07/25] mm: factor out a devm_request_free_mem_region helper
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (5 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 06/25] mm: export alloc_pages_vma Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 08/25] memremap: validate the pagemap type passed to devm_memremap_pages Christoph Hellwig
                   ` (17 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci,
	linux-kernel, John Hubbard

Keep the physical address allocation that hmm_add_device does with the
rest of the resource code, and allow future reuse of it without the hmm
wrapper.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: John Hubbard <jhubbard@nvidia.com>
---
 include/linux/ioport.h |  2 ++
 kernel/resource.c      | 39 +++++++++++++++++++++++++++++++++++++++
 mm/hmm.c               | 33 ++++-----------------------------
 3 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index dd961882bc74..a02b290ca08a 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -285,6 +285,8 @@ static inline bool resource_overlaps(struct resource *r1, struct resource *r2)
        return (r1->start <= r2->end && r1->end >= r2->start);
 }
 
+struct resource *devm_request_free_mem_region(struct device *dev,
+		struct resource *base, unsigned long size);
 
 #endif /* __ASSEMBLY__ */
 #endif	/* _LINUX_IOPORT_H */
diff --git a/kernel/resource.c b/kernel/resource.c
index 158f04ec1d4f..d22423e85cf8 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1628,6 +1628,45 @@ void resource_list_free(struct list_head *head)
 }
 EXPORT_SYMBOL(resource_list_free);
 
+#ifdef CONFIG_DEVICE_PRIVATE
+/**
+ * devm_request_free_mem_region - find free region for device private memory
+ *
+ * @dev: device struct to bind the resource to
+ * @size: size in bytes of the device memory to add
+ * @base: resource tree to look in
+ *
+ * This function tries to find an empty range of physical address big enough to
+ * contain the new resource, so that it can later be hotplugged as ZONE_DEVICE
+ * memory, which in turn allocates struct pages.
+ */
+struct resource *devm_request_free_mem_region(struct device *dev,
+		struct resource *base, unsigned long size)
+{
+	resource_size_t end, addr;
+	struct resource *res;
+
+	size = ALIGN(size, 1UL << PA_SECTION_SHIFT);
+	end = min_t(unsigned long, base->end, (1UL << MAX_PHYSMEM_BITS) - 1);
+	addr = end - size + 1UL;
+
+	for (; addr > size && addr >= base->start; addr -= size) {
+		if (region_intersects(addr, size, 0, IORES_DESC_NONE) !=
+				REGION_DISJOINT)
+			continue;
+
+		res = devm_request_mem_region(dev, addr, size, dev_name(dev));
+		if (!res)
+			return ERR_PTR(-ENOMEM);
+		res->desc = IORES_DESC_DEVICE_PRIVATE_MEMORY;
+		return res;
+	}
+
+	return ERR_PTR(-ERANGE);
+}
+EXPORT_SYMBOL_GPL(devm_request_free_mem_region);
+#endif /* CONFIG_DEVICE_PRIVATE */
+
 static int __init strict_iomem(char *str)
 {
 	if (strstr(str, "relaxed"))
diff --git a/mm/hmm.c b/mm/hmm.c
index e7dd2ab8f9ab..48574f8485bb 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -25,8 +25,6 @@
 #include <linux/mmu_notifier.h>
 #include <linux/memory_hotplug.h>
 
-#define PA_SECTION_SIZE (1UL << PA_SECTION_SHIFT)
-
 #if IS_ENABLED(CONFIG_HMM_MIRROR)
 static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
 
@@ -1408,7 +1406,6 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 				  unsigned long size)
 {
 	struct hmm_devmem *devmem;
-	resource_size_t addr;
 	void *result;
 	int ret;
 
@@ -1430,32 +1427,10 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	if (ret)
 		return ERR_PTR(ret);
 
-	size = ALIGN(size, PA_SECTION_SIZE);
-	addr = min((unsigned long)iomem_resource.end,
-		   (1UL << MAX_PHYSMEM_BITS) - 1);
-	addr = addr - size + 1UL;
-
-	/*
-	 * FIXME add a new helper to quickly walk resource tree and find free
-	 * range
-	 *
-	 * FIXME what about ioport_resource resource ?
-	 */
-	for (; addr > size && addr >= iomem_resource.start; addr -= size) {
-		ret = region_intersects(addr, size, 0, IORES_DESC_NONE);
-		if (ret != REGION_DISJOINT)
-			continue;
-
-		devmem->resource = devm_request_mem_region(device, addr, size,
-							   dev_name(device));
-		if (!devmem->resource)
-			return ERR_PTR(-ENOMEM);
-		break;
-	}
-	if (!devmem->resource)
-		return ERR_PTR(-ERANGE);
-
-	devmem->resource->desc = IORES_DESC_DEVICE_PRIVATE_MEMORY;
+	devmem->resource = devm_request_free_mem_region(device, &iomem_resource,
+			size);
+	if (IS_ERR(devmem->resource))
+		return ERR_CAST(devmem->resource);
 	devmem->pfn_first = devmem->resource->start >> PAGE_SHIFT;
 	devmem->pfn_last = devmem->pfn_first +
 			   (resource_size(devmem->resource) >> PAGE_SHIFT);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 08/25] memremap: validate the pagemap type passed to devm_memremap_pages
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (6 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 07/25] mm: factor out a devm_request_free_mem_region helper Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 18:01   ` Ira Weiny
  2019-06-26 12:27 ` [PATCH 09/25] memremap: move dev_pagemap callbacks into a separate structure Christoph Hellwig
                   ` (16 subsequent siblings)
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

Most pgmap types are only supported when certain config options are
enabled.  Check for a type that is valid for the current configuration
before setting up the pagemap.  For this the usage of the 0 type for
device dax gets replaced with an explicit MEMORY_DEVICE_DEVDAX type.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/dax/device.c     |  1 +
 include/linux/memremap.h |  8 ++++++++
 kernel/memremap.c        | 22 ++++++++++++++++++++++
 3 files changed, 31 insertions(+)

diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index 8465d12fecba..79014baa782d 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -468,6 +468,7 @@ int dev_dax_probe(struct device *dev)
 	dev_dax->pgmap.ref = &dev_dax->ref;
 	dev_dax->pgmap.kill = dev_dax_percpu_kill;
 	dev_dax->pgmap.cleanup = dev_dax_percpu_exit;
+	dev_dax->pgmap.type = MEMORY_DEVICE_DEVDAX;
 	addr = devm_memremap_pages(dev, &dev_dax->pgmap);
 	if (IS_ERR(addr))
 		return PTR_ERR(addr);
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index 995c62c5a48b..0c86f2c5ac9c 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -45,13 +45,21 @@ struct vmem_altmap {
  * wakeup is used to coordinate physical address space management (ex:
  * fs truncate/hole punch) vs pinned pages (ex: device dma).
  *
+ * MEMORY_DEVICE_DEVDAX:
+ * Host memory that has similar access semantics as System RAM i.e. DMA
+ * coherent and supports page pinning. In contrast to
+ * MEMORY_DEVICE_FS_DAX, this memory is access via a device-dax
+ * character device.
+ *
  * MEMORY_DEVICE_PCI_P2PDMA:
  * Device memory residing in a PCI BAR intended for use with Peer-to-Peer
  * transactions.
  */
 enum memory_type {
+	/* 0 is reserved to catch uninitialized type fields */
 	MEMORY_DEVICE_PRIVATE = 1,
 	MEMORY_DEVICE_FS_DAX,
+	MEMORY_DEVICE_DEVDAX,
 	MEMORY_DEVICE_PCI_P2PDMA,
 };
 
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 6e1970719dc2..abda62d1e5a3 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -157,6 +157,28 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 	pgprot_t pgprot = PAGE_KERNEL;
 	int error, nid, is_ram;
 
+	switch (pgmap->type) {
+	case MEMORY_DEVICE_PRIVATE:
+		if (!IS_ENABLED(CONFIG_DEVICE_PRIVATE)) {
+			WARN(1, "Device private memory not supported\n");
+			return ERR_PTR(-EINVAL);
+		}
+		break;
+	case MEMORY_DEVICE_FS_DAX:
+		if (!IS_ENABLED(CONFIG_ZONE_DEVICE) ||
+		    IS_ENABLED(CONFIG_FS_DAX_LIMITED)) {
+			WARN(1, "File system DAX not supported\n");
+			return ERR_PTR(-EINVAL);
+		}
+		break;
+	case MEMORY_DEVICE_DEVDAX:
+	case MEMORY_DEVICE_PCI_P2PDMA:
+		break;
+	default:
+		WARN(1, "Invalid pgmap type %d\n", pgmap->type);
+		break;
+	}
+
 	if (!pgmap->ref || !pgmap->kill || !pgmap->cleanup) {
 		WARN(1, "Missing reference count teardown definition\n");
 		return ERR_PTR(-EINVAL);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 09/25] memremap: move dev_pagemap callbacks into a separate structure
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (7 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 08/25] memremap: validate the pagemap type passed to devm_memremap_pages Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 10/25] memremap: pass a struct dev_pagemap to ->kill and ->cleanup Christoph Hellwig
                   ` (15 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci,
	linux-kernel, Logan Gunthorpe

The dev_pagemap is a growing too many callbacks.  Move them into a
separate ops structure so that they are not duplicated for multiple
instances, and an attacker can't easily overwrite them.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/dax/device.c              | 11 ++++++----
 drivers/dax/pmem/core.c           |  2 +-
 drivers/nvdimm/pmem.c             | 19 +++++++++-------
 drivers/pci/p2pdma.c              |  8 +++++--
 include/linux/memremap.h          | 36 +++++++++++++++++--------------
 kernel/memremap.c                 | 18 ++++++++--------
 mm/hmm.c                          | 10 ++++++---
 tools/testing/nvdimm/test/iomap.c |  7 +++---
 8 files changed, 65 insertions(+), 46 deletions(-)

diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index 79014baa782d..f390083a64d7 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -36,9 +36,8 @@ static void dev_dax_percpu_exit(struct percpu_ref *ref)
 	percpu_ref_exit(ref);
 }
 
-static void dev_dax_percpu_kill(struct percpu_ref *data)
+static void dev_dax_percpu_kill(struct percpu_ref *ref)
 {
-	struct percpu_ref *ref = data;
 	struct dev_dax *dev_dax = ref_to_dev_dax(ref);
 
 	dev_dbg(&dev_dax->dev, "%s\n", __func__);
@@ -442,6 +441,11 @@ static void dev_dax_kill(void *dev_dax)
 	kill_dev_dax(dev_dax);
 }
 
+static const struct dev_pagemap_ops dev_dax_pagemap_ops = {
+	.kill		= dev_dax_percpu_kill,
+	.cleanup	= dev_dax_percpu_exit,
+};
+
 int dev_dax_probe(struct device *dev)
 {
 	struct dev_dax *dev_dax = to_dev_dax(dev);
@@ -466,9 +470,8 @@ int dev_dax_probe(struct device *dev)
 		return rc;
 
 	dev_dax->pgmap.ref = &dev_dax->ref;
-	dev_dax->pgmap.kill = dev_dax_percpu_kill;
-	dev_dax->pgmap.cleanup = dev_dax_percpu_exit;
 	dev_dax->pgmap.type = MEMORY_DEVICE_DEVDAX;
+	dev_dax->pgmap.ops = &dev_dax_pagemap_ops;
 	addr = devm_memremap_pages(dev, &dev_dax->pgmap);
 	if (IS_ERR(addr))
 		return PTR_ERR(addr);
diff --git a/drivers/dax/pmem/core.c b/drivers/dax/pmem/core.c
index f9f51786d556..6eb6dfdf19bf 100644
--- a/drivers/dax/pmem/core.c
+++ b/drivers/dax/pmem/core.c
@@ -16,7 +16,7 @@ struct dev_dax *__dax_pmem_probe(struct device *dev, enum dev_dax_subsys subsys)
 	struct dev_dax *dev_dax;
 	struct nd_namespace_io *nsio;
 	struct dax_region *dax_region;
-	struct dev_pagemap pgmap = { 0 };
+	struct dev_pagemap pgmap = { };
 	struct nd_namespace_common *ndns;
 	struct nd_dax *nd_dax = to_nd_dax(dev);
 	struct nd_pfn *nd_pfn = &nd_dax->nd_pfn;
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 24d7fe7c74ed..c2449af2b388 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -303,7 +303,7 @@ static const struct attribute_group *pmem_attribute_groups[] = {
 	NULL,
 };
 
-static void __pmem_release_queue(struct percpu_ref *ref)
+static void pmem_pagemap_cleanup(struct percpu_ref *ref)
 {
 	struct request_queue *q;
 
@@ -313,10 +313,10 @@ static void __pmem_release_queue(struct percpu_ref *ref)
 
 static void pmem_release_queue(void *ref)
 {
-	__pmem_release_queue(ref);
+	pmem_pagemap_cleanup(ref);
 }
 
-static void pmem_freeze_queue(struct percpu_ref *ref)
+static void pmem_pagemap_kill(struct percpu_ref *ref)
 {
 	struct request_queue *q;
 
@@ -339,19 +339,24 @@ static void pmem_release_pgmap_ops(void *__pgmap)
 	dev_pagemap_put_ops();
 }
 
-static void fsdax_pagefree(struct page *page, void *data)
+static void pmem_pagemap_page_free(struct page *page, void *data)
 {
 	wake_up_var(&page->_refcount);
 }
 
+static const struct dev_pagemap_ops fsdax_pagemap_ops = {
+	.page_free		= pmem_pagemap_page_free,
+	.kill			= pmem_pagemap_kill,
+	.cleanup		= pmem_pagemap_cleanup,
+};
+
 static int setup_pagemap_fsdax(struct device *dev, struct dev_pagemap *pgmap)
 {
 	dev_pagemap_get_ops();
 	if (devm_add_action_or_reset(dev, pmem_release_pgmap_ops, pgmap))
 		return -ENOMEM;
 	pgmap->type = MEMORY_DEVICE_FS_DAX;
-	pgmap->page_free = fsdax_pagefree;
-
+	pgmap->ops = &fsdax_pagemap_ops;
 	return 0;
 }
 
@@ -409,8 +414,6 @@ static int pmem_attach_disk(struct device *dev,
 
 	pmem->pfn_flags = PFN_DEV;
 	pmem->pgmap.ref = &q->q_usage_counter;
-	pmem->pgmap.kill = pmem_freeze_queue;
-	pmem->pgmap.cleanup = __pmem_release_queue;
 	if (is_nd_pfn(dev)) {
 		if (setup_pagemap_fsdax(dev, &pmem->pgmap))
 			return -ENOMEM;
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index a98126ad9c3a..13f0380a8c7f 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -152,6 +152,11 @@ static int pci_p2pdma_setup(struct pci_dev *pdev)
 	return error;
 }
 
+static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
+	.kill		= pci_p2pdma_percpu_kill,
+	.cleanup	= pci_p2pdma_percpu_cleanup,
+};
+
 /**
  * pci_p2pdma_add_resource - add memory for use as p2p memory
  * @pdev: the device to add the memory to
@@ -207,8 +212,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
 	pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
 	pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
 		pci_resource_start(pdev, bar);
-	pgmap->kill = pci_p2pdma_percpu_kill;
-	pgmap->cleanup = pci_p2pdma_percpu_cleanup;
+	pgmap->ops = &pci_p2pdma_pagemap_ops;
 
 	addr = devm_memremap_pages(&pdev->dev, pgmap);
 	if (IS_ERR(addr)) {
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index 0c86f2c5ac9c..919755f48c7e 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -63,41 +63,45 @@ enum memory_type {
 	MEMORY_DEVICE_PCI_P2PDMA,
 };
 
-/*
- * Additional notes about MEMORY_DEVICE_PRIVATE may be found in
- * include/linux/hmm.h and Documentation/vm/hmm.rst. There is also a brief
- * explanation in include/linux/memory_hotplug.h.
- *
- * The page_free() callback is called once the page refcount reaches 1
- * (ZONE_DEVICE pages never reach 0 refcount unless there is a refcount bug.
- * This allows the device driver to implement its own memory management.)
- */
-typedef void (*dev_page_free_t)(struct page *page, void *data);
+struct dev_pagemap_ops {
+	/*
+	 * Called once the page refcount reaches 1.  (ZONE_DEVICE pages never
+	 * reach 0 refcount unless there is a refcount bug. This allows the
+	 * device driver to implement its own memory management.)
+	 */
+	void (*page_free)(struct page *page, void *data);
+
+	/*
+	 * Transition the refcount in struct dev_pagemap to the dead state.
+	 */
+	void (*kill)(struct percpu_ref *ref);
+
+	/*
+	 * Wait for refcount in struct dev_pagemap to be idle and reap it.
+	 */
+	void (*cleanup)(struct percpu_ref *ref);
+};
 
 /**
  * struct dev_pagemap - metadata for ZONE_DEVICE mappings
- * @page_free: free page callback when page refcount reaches 1
  * @altmap: pre-allocated/reserved memory for vmemmap allocations
  * @res: physical address range covered by @ref
  * @ref: reference count that pins the devm_memremap_pages() mapping
- * @kill: callback to transition @ref to the dead state
- * @cleanup: callback to wait for @ref to be idle and reap it
  * @dev: host device of the mapping for debug
  * @data: private data pointer for page_free()
  * @type: memory type: see MEMORY_* in memory_hotplug.h
+ * @ops: method table
  */
 struct dev_pagemap {
-	dev_page_free_t page_free;
 	struct vmem_altmap altmap;
 	bool altmap_valid;
 	struct resource res;
 	struct percpu_ref *ref;
-	void (*kill)(struct percpu_ref *ref);
-	void (*cleanup)(struct percpu_ref *ref);
 	struct device *dev;
 	void *data;
 	enum memory_type type;
 	u64 pci_p2pdma_bus_offset;
+	const struct dev_pagemap_ops *ops;
 };
 
 #ifdef CONFIG_ZONE_DEVICE
diff --git a/kernel/memremap.c b/kernel/memremap.c
index abda62d1e5a3..0824237ef979 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -92,10 +92,10 @@ static void devm_memremap_pages_release(void *data)
 	unsigned long pfn;
 	int nid;
 
-	pgmap->kill(pgmap->ref);
+	pgmap->ops->kill(pgmap->ref);
 	for_each_device_pfn(pfn, pgmap)
 		put_page(pfn_to_page(pfn));
-	pgmap->cleanup(pgmap->ref);
+	pgmap->ops->cleanup(pgmap->ref);
 
 	/* pages are dead and unused, undo the arch mapping */
 	align_start = res->start & ~(SECTION_SIZE - 1);
@@ -128,8 +128,8 @@ static void devm_memremap_pages_release(void *data)
  * @pgmap: pointer to a struct dev_pagemap
  *
  * Notes:
- * 1/ At a minimum the res, ref and type members of @pgmap must be initialized
- *    by the caller before passing it to this function
+ * 1/ At a minimum the res, ref and type and ops members of @pgmap must be
+ *    initialized by the caller before passing it to this function
  *
  * 2/ The altmap field may optionally be initialized, in which case altmap_valid
  *    must be set to true
@@ -179,7 +179,8 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 		break;
 	}
 
-	if (!pgmap->ref || !pgmap->kill || !pgmap->cleanup) {
+	if (!pgmap->ref || !pgmap->ops || !pgmap->ops->kill ||
+	    !pgmap->ops->cleanup) {
 		WARN(1, "Missing reference count teardown definition\n");
 		return ERR_PTR(-EINVAL);
 	}
@@ -293,9 +294,8 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
  err_pfn_remap:
 	pgmap_array_delete(res);
  err_array:
-	pgmap->kill(pgmap->ref);
-	pgmap->cleanup(pgmap->ref);
-
+	pgmap->ops->kill(pgmap->ref);
+	pgmap->ops->cleanup(pgmap->ref);
 	return ERR_PTR(error);
 }
 EXPORT_SYMBOL_GPL(devm_memremap_pages);
@@ -388,7 +388,7 @@ void __put_devmap_managed_page(struct page *page)
 
 		mem_cgroup_uncharge(page);
 
-		page->pgmap->page_free(page, page->pgmap->data);
+		page->pgmap->ops->page_free(page, page->pgmap->data);
 	} else if (!count)
 		__put_page(page);
 }
diff --git a/mm/hmm.c b/mm/hmm.c
index 48574f8485bb..583a02a16872 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1384,6 +1384,12 @@ static void hmm_devmem_free(struct page *page, void *data)
 	devmem->ops->free(devmem, page);
 }
 
+static const struct dev_pagemap_ops hmm_pagemap_ops = {
+	.page_free		= hmm_devmem_free,
+	.kill			= hmm_devmem_ref_kill,
+	.cleanup		= hmm_devmem_ref_exit,
+};
+
 /*
  * hmm_devmem_add() - hotplug ZONE_DEVICE memory for device memory
  *
@@ -1438,12 +1444,10 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 
 	devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
 	devmem->pagemap.res = *devmem->resource;
-	devmem->pagemap.page_free = hmm_devmem_free;
+	devmem->pagemap.ops = &hmm_pagemap_ops;
 	devmem->pagemap.altmap_valid = false;
 	devmem->pagemap.ref = &devmem->ref;
 	devmem->pagemap.data = devmem;
-	devmem->pagemap.kill = hmm_devmem_ref_kill;
-	devmem->pagemap.cleanup = hmm_devmem_ref_exit;
 
 	result = devm_memremap_pages(devmem->device, &devmem->pagemap);
 	if (IS_ERR(result))
diff --git a/tools/testing/nvdimm/test/iomap.c b/tools/testing/nvdimm/test/iomap.c
index 076df22e4bda..cf3f064a697d 100644
--- a/tools/testing/nvdimm/test/iomap.c
+++ b/tools/testing/nvdimm/test/iomap.c
@@ -100,9 +100,10 @@ static void nfit_test_kill(void *_pgmap)
 {
 	struct dev_pagemap *pgmap = _pgmap;
 
-	WARN_ON(!pgmap || !pgmap->ref || !pgmap->kill || !pgmap->cleanup);
-	pgmap->kill(pgmap->ref);
-	pgmap->cleanup(pgmap->ref);
+	WARN_ON(!pgmap || !pgmap->ref || !pgmap->ops || !pgmap->ops->kill ||
+		!pgmap->ops->cleanup);
+	pgmap->ops->kill(pgmap->ref);
+	pgmap->ops->cleanup(pgmap->ref);
 }
 
 void *__wrap_devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 10/25] memremap: pass a struct dev_pagemap to ->kill and ->cleanup
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (8 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 09/25] memremap: move dev_pagemap callbacks into a separate structure Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 11/25] memremap: lift the devmap_enable manipulation into devm_memremap_pages Christoph Hellwig
                   ` (14 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci,
	linux-kernel, Logan Gunthorpe

Passing the actual typed structure leads to more understandable code
vs just passing the ref member.

Reported-by: Logan Gunthorpe <logang@deltatee.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Logan Gunthorpe <logang@deltatee.com>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
Reviewed-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/dax/device.c              | 12 ++++++------
 drivers/nvdimm/pmem.c             | 18 +++++++++---------
 drivers/pci/p2pdma.c              |  9 +++++----
 include/linux/memremap.h          |  4 ++--
 kernel/memremap.c                 |  8 ++++----
 mm/hmm.c                          | 10 +++++-----
 tools/testing/nvdimm/test/iomap.c |  4 ++--
 7 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index f390083a64d7..b5257038c188 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -27,21 +27,21 @@ static void dev_dax_percpu_release(struct percpu_ref *ref)
 	complete(&dev_dax->cmp);
 }
 
-static void dev_dax_percpu_exit(struct percpu_ref *ref)
+static void dev_dax_percpu_exit(struct dev_pagemap *pgmap)
 {
-	struct dev_dax *dev_dax = ref_to_dev_dax(ref);
+	struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap);
 
 	dev_dbg(&dev_dax->dev, "%s\n", __func__);
 	wait_for_completion(&dev_dax->cmp);
-	percpu_ref_exit(ref);
+	percpu_ref_exit(pgmap->ref);
 }
 
-static void dev_dax_percpu_kill(struct percpu_ref *ref)
+static void dev_dax_percpu_kill(struct dev_pagemap *pgmap)
 {
-	struct dev_dax *dev_dax = ref_to_dev_dax(ref);
+	struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap);
 
 	dev_dbg(&dev_dax->dev, "%s\n", __func__);
-	percpu_ref_kill(ref);
+	percpu_ref_kill(pgmap->ref);
 }
 
 static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma,
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index c2449af2b388..9dac48359353 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -303,24 +303,24 @@ static const struct attribute_group *pmem_attribute_groups[] = {
 	NULL,
 };
 
-static void pmem_pagemap_cleanup(struct percpu_ref *ref)
+static void pmem_pagemap_cleanup(struct dev_pagemap *pgmap)
 {
-	struct request_queue *q;
+	struct request_queue *q =
+		container_of(pgmap->ref, struct request_queue, q_usage_counter);
 
-	q = container_of(ref, typeof(*q), q_usage_counter);
 	blk_cleanup_queue(q);
 }
 
-static void pmem_release_queue(void *ref)
+static void pmem_release_queue(void *pgmap)
 {
-	pmem_pagemap_cleanup(ref);
+	pmem_pagemap_cleanup(pgmap);
 }
 
-static void pmem_pagemap_kill(struct percpu_ref *ref)
+static void pmem_pagemap_kill(struct dev_pagemap *pgmap)
 {
-	struct request_queue *q;
+	struct request_queue *q =
+		container_of(pgmap->ref, struct request_queue, q_usage_counter);
 
-	q = container_of(ref, typeof(*q), q_usage_counter);
 	blk_freeze_queue_start(q);
 }
 
@@ -435,7 +435,7 @@ static int pmem_attach_disk(struct device *dev,
 		memcpy(&bb_res, &pmem->pgmap.res, sizeof(bb_res));
 	} else {
 		if (devm_add_action_or_reset(dev, pmem_release_queue,
-					&q->q_usage_counter))
+					&pmem->pgmap))
 			return -ENOMEM;
 		addr = devm_memremap(dev, pmem->phys_addr,
 				pmem->size, ARCH_MEMREMAP_PMEM);
diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index 13f0380a8c7f..ebd8ce3bba2e 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -90,14 +90,15 @@ static void pci_p2pdma_percpu_release(struct percpu_ref *ref)
 	complete(&p2p_pgmap->ref_done);
 }
 
-static void pci_p2pdma_percpu_kill(struct percpu_ref *ref)
+static void pci_p2pdma_percpu_kill(struct dev_pagemap *pgmap)
 {
-	percpu_ref_kill(ref);
+	percpu_ref_kill(pgmap->ref);
 }
 
-static void pci_p2pdma_percpu_cleanup(struct percpu_ref *ref)
+static void pci_p2pdma_percpu_cleanup(struct dev_pagemap *pgmap)
 {
-	struct p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(ref);
+	struct p2pdma_pagemap *p2p_pgmap =
+		container_of(pgmap, struct p2pdma_pagemap, pgmap);
 
 	wait_for_completion(&p2p_pgmap->ref_done);
 	percpu_ref_exit(&p2p_pgmap->ref);
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index 919755f48c7e..b8666a0d8665 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -74,12 +74,12 @@ struct dev_pagemap_ops {
 	/*
 	 * Transition the refcount in struct dev_pagemap to the dead state.
 	 */
-	void (*kill)(struct percpu_ref *ref);
+	void (*kill)(struct dev_pagemap *pgmap);
 
 	/*
 	 * Wait for refcount in struct dev_pagemap to be idle and reap it.
 	 */
-	void (*cleanup)(struct percpu_ref *ref);
+	void (*cleanup)(struct dev_pagemap *pgmap);
 };
 
 /**
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 0824237ef979..00c1ceb60c19 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -92,10 +92,10 @@ static void devm_memremap_pages_release(void *data)
 	unsigned long pfn;
 	int nid;
 
-	pgmap->ops->kill(pgmap->ref);
+	pgmap->ops->kill(pgmap);
 	for_each_device_pfn(pfn, pgmap)
 		put_page(pfn_to_page(pfn));
-	pgmap->ops->cleanup(pgmap->ref);
+	pgmap->ops->cleanup(pgmap);
 
 	/* pages are dead and unused, undo the arch mapping */
 	align_start = res->start & ~(SECTION_SIZE - 1);
@@ -294,8 +294,8 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
  err_pfn_remap:
 	pgmap_array_delete(res);
  err_array:
-	pgmap->ops->kill(pgmap->ref);
-	pgmap->ops->cleanup(pgmap->ref);
+	pgmap->ops->kill(pgmap);
+	pgmap->ops->cleanup(pgmap);
 	return ERR_PTR(error);
 }
 EXPORT_SYMBOL_GPL(devm_memremap_pages);
diff --git a/mm/hmm.c b/mm/hmm.c
index 583a02a16872..987793fba923 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1352,18 +1352,18 @@ static void hmm_devmem_ref_release(struct percpu_ref *ref)
 	complete(&devmem->completion);
 }
 
-static void hmm_devmem_ref_exit(struct percpu_ref *ref)
+static void hmm_devmem_ref_exit(struct dev_pagemap *pgmap)
 {
 	struct hmm_devmem *devmem;
 
-	devmem = container_of(ref, struct hmm_devmem, ref);
+	devmem = container_of(pgmap, struct hmm_devmem, pagemap);
 	wait_for_completion(&devmem->completion);
-	percpu_ref_exit(ref);
+	percpu_ref_exit(pgmap->ref);
 }
 
-static void hmm_devmem_ref_kill(struct percpu_ref *ref)
+static void hmm_devmem_ref_kill(struct dev_pagemap *pgmap)
 {
-	percpu_ref_kill(ref);
+	percpu_ref_kill(pgmap->ref);
 }
 
 static vm_fault_t hmm_devmem_fault(struct vm_area_struct *vma,
diff --git a/tools/testing/nvdimm/test/iomap.c b/tools/testing/nvdimm/test/iomap.c
index cf3f064a697d..82f901569e06 100644
--- a/tools/testing/nvdimm/test/iomap.c
+++ b/tools/testing/nvdimm/test/iomap.c
@@ -102,8 +102,8 @@ static void nfit_test_kill(void *_pgmap)
 
 	WARN_ON(!pgmap || !pgmap->ref || !pgmap->ops || !pgmap->ops->kill ||
 		!pgmap->ops->cleanup);
-	pgmap->ops->kill(pgmap->ref);
-	pgmap->ops->cleanup(pgmap->ref);
+	pgmap->ops->kill(pgmap);
+	pgmap->ops->cleanup(pgmap);
 }
 
 void *__wrap_devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 11/25] memremap: lift the devmap_enable manipulation into devm_memremap_pages
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (9 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 10/25] memremap: pass a struct dev_pagemap to ->kill and ->cleanup Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 19:04   ` Ira Weiny
  2019-06-26 12:27 ` [PATCH 12/25] memremap: add a migrate_to_ram method to struct dev_pagemap_ops Christoph Hellwig
                   ` (13 subsequent siblings)
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

Just check if there is a ->page_free operation set and take care of the
static key enable, as well as the put using device managed resources.
Also check that a ->page_free is provided for the pgmaps types that
require it, and check for a valid type as well while we are at it.

Note that this also fixes the fact that hmm never called
dev_pagemap_put_ops and thus would leave the slow path enabled forever,
even after a device driver unload or disable.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/nvdimm/pmem.c | 23 +++--------------
 include/linux/mm.h    | 10 --------
 kernel/memremap.c     | 59 +++++++++++++++++++++++++++----------------
 mm/hmm.c              |  2 --
 4 files changed, 41 insertions(+), 53 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 9dac48359353..48767171a4df 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -334,11 +334,6 @@ static void pmem_release_disk(void *__pmem)
 	put_disk(pmem->disk);
 }
 
-static void pmem_release_pgmap_ops(void *__pgmap)
-{
-	dev_pagemap_put_ops();
-}
-
 static void pmem_pagemap_page_free(struct page *page, void *data)
 {
 	wake_up_var(&page->_refcount);
@@ -350,16 +345,6 @@ static const struct dev_pagemap_ops fsdax_pagemap_ops = {
 	.cleanup		= pmem_pagemap_cleanup,
 };
 
-static int setup_pagemap_fsdax(struct device *dev, struct dev_pagemap *pgmap)
-{
-	dev_pagemap_get_ops();
-	if (devm_add_action_or_reset(dev, pmem_release_pgmap_ops, pgmap))
-		return -ENOMEM;
-	pgmap->type = MEMORY_DEVICE_FS_DAX;
-	pgmap->ops = &fsdax_pagemap_ops;
-	return 0;
-}
-
 static int pmem_attach_disk(struct device *dev,
 		struct nd_namespace_common *ndns)
 {
@@ -415,8 +400,8 @@ static int pmem_attach_disk(struct device *dev,
 	pmem->pfn_flags = PFN_DEV;
 	pmem->pgmap.ref = &q->q_usage_counter;
 	if (is_nd_pfn(dev)) {
-		if (setup_pagemap_fsdax(dev, &pmem->pgmap))
-			return -ENOMEM;
+		pmem->pgmap.type = MEMORY_DEVICE_FS_DAX;
+		pmem->pgmap.ops = &fsdax_pagemap_ops;
 		addr = devm_memremap_pages(dev, &pmem->pgmap);
 		pfn_sb = nd_pfn->pfn_sb;
 		pmem->data_offset = le64_to_cpu(pfn_sb->dataoff);
@@ -428,8 +413,8 @@ static int pmem_attach_disk(struct device *dev,
 	} else if (pmem_should_map_pages(dev)) {
 		memcpy(&pmem->pgmap.res, &nsio->res, sizeof(pmem->pgmap.res));
 		pmem->pgmap.altmap_valid = false;
-		if (setup_pagemap_fsdax(dev, &pmem->pgmap))
-			return -ENOMEM;
+		pmem->pgmap.type = MEMORY_DEVICE_FS_DAX;
+		pmem->pgmap.ops = &fsdax_pagemap_ops;
 		addr = devm_memremap_pages(dev, &pmem->pgmap);
 		pmem->pfn_flags |= PFN_MAP;
 		memcpy(&bb_res, &pmem->pgmap.res, sizeof(bb_res));
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 6e4b9be08b13..aa3970291cdf 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -932,8 +932,6 @@ static inline bool is_zone_device_page(const struct page *page)
 #endif
 
 #ifdef CONFIG_DEV_PAGEMAP_OPS
-void dev_pagemap_get_ops(void);
-void dev_pagemap_put_ops(void);
 void __put_devmap_managed_page(struct page *page);
 DECLARE_STATIC_KEY_FALSE(devmap_managed_key);
 static inline bool put_devmap_managed_page(struct page *page)
@@ -973,14 +971,6 @@ static inline bool is_pci_p2pdma_page(const struct page *page)
 #endif /* CONFIG_PCI_P2PDMA */
 
 #else /* CONFIG_DEV_PAGEMAP_OPS */
-static inline void dev_pagemap_get_ops(void)
-{
-}
-
-static inline void dev_pagemap_put_ops(void)
-{
-}
-
 static inline bool put_devmap_managed_page(struct page *page)
 {
 	return false;
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 00c1ceb60c19..3219a4c91d07 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -17,6 +17,35 @@ static DEFINE_XARRAY(pgmap_array);
 #define SECTION_MASK ~((1UL << PA_SECTION_SHIFT) - 1)
 #define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
 
+#ifdef CONFIG_DEV_PAGEMAP_OPS
+DEFINE_STATIC_KEY_FALSE(devmap_managed_key);
+EXPORT_SYMBOL(devmap_managed_key);
+static atomic_t devmap_managed_enable;
+
+static void devmap_managed_enable_put(void *data)
+{
+	if (atomic_dec_and_test(&devmap_managed_enable))
+		static_branch_disable(&devmap_managed_key);
+}
+
+static int devmap_managed_enable_get(struct device *dev, struct dev_pagemap *pgmap)
+{
+	if (!pgmap->ops->page_free) {
+		WARN(1, "Missing page_free method\n");
+		return -EINVAL;
+	}
+
+	if (atomic_inc_return(&devmap_managed_enable) == 1)
+		static_branch_enable(&devmap_managed_key);
+	return devm_add_action_or_reset(dev, devmap_managed_enable_put, NULL);
+}
+#else
+static int devmap_managed_enable_get(struct device *dev, struct dev_pagemap *pgmap)
+{
+	return -EINVAL;
+}
+#endif /* CONFIG_DEV_PAGEMAP_OPS */
+
 #if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
 vm_fault_t device_private_entry_fault(struct vm_area_struct *vma,
 		       unsigned long addr,
@@ -156,6 +185,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 	};
 	pgprot_t pgprot = PAGE_KERNEL;
 	int error, nid, is_ram;
+	bool need_devmap_managed = true;
 
 	switch (pgmap->type) {
 	case MEMORY_DEVICE_PRIVATE:
@@ -173,6 +203,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 		break;
 	case MEMORY_DEVICE_DEVDAX:
 	case MEMORY_DEVICE_PCI_P2PDMA:
+		need_devmap_managed = false;
 		break;
 	default:
 		WARN(1, "Invalid pgmap type %d\n", pgmap->type);
@@ -185,6 +216,12 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 		return ERR_PTR(-EINVAL);
 	}
 
+	if (need_devmap_managed) {
+		error = devmap_managed_enable_get(dev, pgmap);
+		if (error)
+			return ERR_PTR(error);
+	}
+
 	align_start = res->start & ~(SECTION_SIZE - 1);
 	align_size = ALIGN(res->start + resource_size(res), SECTION_SIZE)
 		- align_start;
@@ -351,28 +388,6 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn,
 EXPORT_SYMBOL_GPL(get_dev_pagemap);
 
 #ifdef CONFIG_DEV_PAGEMAP_OPS
-DEFINE_STATIC_KEY_FALSE(devmap_managed_key);
-EXPORT_SYMBOL(devmap_managed_key);
-static atomic_t devmap_enable;
-
-/*
- * Toggle the static key for ->page_free() callbacks when dev_pagemap
- * pages go idle.
- */
-void dev_pagemap_get_ops(void)
-{
-	if (atomic_inc_return(&devmap_enable) == 1)
-		static_branch_enable(&devmap_managed_key);
-}
-EXPORT_SYMBOL_GPL(dev_pagemap_get_ops);
-
-void dev_pagemap_put_ops(void)
-{
-	if (atomic_dec_and_test(&devmap_enable))
-		static_branch_disable(&devmap_managed_key);
-}
-EXPORT_SYMBOL_GPL(dev_pagemap_put_ops);
-
 void __put_devmap_managed_page(struct page *page)
 {
 	int count = page_ref_dec_return(page);
diff --git a/mm/hmm.c b/mm/hmm.c
index 987793fba923..5b0bd5f6a74f 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1415,8 +1415,6 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	void *result;
 	int ret;
 
-	dev_pagemap_get_ops();
-
 	devmem = devm_kzalloc(device, sizeof(*devmem), GFP_KERNEL);
 	if (!devmem)
 		return ERR_PTR(-ENOMEM);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 12/25] memremap: add a migrate_to_ram method to struct dev_pagemap_ops
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (10 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 11/25] memremap: lift the devmap_enable manipulation into devm_memremap_pages Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-27 16:29   ` Jason Gunthorpe
  2019-06-26 12:27 ` [PATCH 13/25] memremap: remove the data field in struct dev_pagemap Christoph Hellwig
                   ` (12 subsequent siblings)
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci,
	linux-kernel, Ralph Campbell

This replaces the hacky ->fault callback, which is currently directly
called from common code through a hmm specific data structure as an
exercise in layering violations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
---
 include/linux/hmm.h      |  6 ------
 include/linux/memremap.h |  6 ++++++
 include/linux/swapops.h  | 15 ---------------
 kernel/memremap.c        | 35 ++++-------------------------------
 mm/hmm.c                 | 13 +++++--------
 mm/memory.c              |  9 ++-------
 6 files changed, 17 insertions(+), 67 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 44a5ac738bb5..ba19c19e24ed 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -692,11 +692,6 @@ struct hmm_devmem_ops {
  * chunk, as an optimization. It must, however, prioritize the faulting address
  * over all the others.
  */
-typedef vm_fault_t (*dev_page_fault_t)(struct vm_area_struct *vma,
-				unsigned long addr,
-				const struct page *page,
-				unsigned int flags,
-				pmd_t *pmdp);
 
 struct hmm_devmem {
 	struct completion		completion;
@@ -707,7 +702,6 @@ struct hmm_devmem {
 	struct dev_pagemap		pagemap;
 	const struct hmm_devmem_ops	*ops;
 	struct percpu_ref		ref;
-	dev_page_fault_t		page_fault;
 };
 
 /*
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index b8666a0d8665..ac985bd03a7f 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -80,6 +80,12 @@ struct dev_pagemap_ops {
 	 * Wait for refcount in struct dev_pagemap to be idle and reap it.
 	 */
 	void (*cleanup)(struct dev_pagemap *pgmap);
+
+	/*
+	 * Used for private (un-addressable) device memory only.  Must migrate
+	 * the page back to a CPU accessible page.
+	 */
+	vm_fault_t (*migrate_to_ram)(struct vm_fault *vmf);
 };
 
 /**
diff --git a/include/linux/swapops.h b/include/linux/swapops.h
index 4d961668e5fc..15bdb6fe71e5 100644
--- a/include/linux/swapops.h
+++ b/include/linux/swapops.h
@@ -129,12 +129,6 @@ static inline struct page *device_private_entry_to_page(swp_entry_t entry)
 {
 	return pfn_to_page(swp_offset(entry));
 }
-
-vm_fault_t device_private_entry_fault(struct vm_area_struct *vma,
-		       unsigned long addr,
-		       swp_entry_t entry,
-		       unsigned int flags,
-		       pmd_t *pmdp);
 #else /* CONFIG_DEVICE_PRIVATE */
 static inline swp_entry_t make_device_private_entry(struct page *page, bool write)
 {
@@ -164,15 +158,6 @@ static inline struct page *device_private_entry_to_page(swp_entry_t entry)
 {
 	return NULL;
 }
-
-static inline vm_fault_t device_private_entry_fault(struct vm_area_struct *vma,
-				     unsigned long addr,
-				     swp_entry_t entry,
-				     unsigned int flags,
-				     pmd_t *pmdp)
-{
-	return VM_FAULT_SIGBUS;
-}
 #endif /* CONFIG_DEVICE_PRIVATE */
 
 #ifdef CONFIG_MIGRATION
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 3219a4c91d07..c06a5487dda7 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -11,7 +11,6 @@
 #include <linux/types.h>
 #include <linux/wait_bit.h>
 #include <linux/xarray.h>
-#include <linux/hmm.h>
 
 static DEFINE_XARRAY(pgmap_array);
 #define SECTION_MASK ~((1UL << PA_SECTION_SHIFT) - 1)
@@ -46,36 +45,6 @@ static int devmap_managed_enable_get(struct device *dev, struct dev_pagemap *pgm
 }
 #endif /* CONFIG_DEV_PAGEMAP_OPS */
 
-#if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
-vm_fault_t device_private_entry_fault(struct vm_area_struct *vma,
-		       unsigned long addr,
-		       swp_entry_t entry,
-		       unsigned int flags,
-		       pmd_t *pmdp)
-{
-	struct page *page = device_private_entry_to_page(entry);
-	struct hmm_devmem *devmem;
-
-	devmem = container_of(page->pgmap, typeof(*devmem), pagemap);
-
-	/*
-	 * The page_fault() callback must migrate page back to system memory
-	 * so that CPU can access it. This might fail for various reasons
-	 * (device issue, device was unsafely unplugged, ...). When such
-	 * error conditions happen, the callback must return VM_FAULT_SIGBUS.
-	 *
-	 * Note that because memory cgroup charges are accounted to the device
-	 * memory, this should never fail because of memory restrictions (but
-	 * allocation of regular system page might still fail because we are
-	 * out of memory).
-	 *
-	 * There is a more in-depth description of what that callback can and
-	 * cannot do, in include/linux/memremap.h
-	 */
-	return devmem->page_fault(vma, addr, page, flags, pmdp);
-}
-#endif /* CONFIG_DEVICE_PRIVATE */
-
 static void pgmap_array_delete(struct resource *res)
 {
 	xa_store_range(&pgmap_array, PHYS_PFN(res->start), PHYS_PFN(res->end),
@@ -193,6 +162,10 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 			WARN(1, "Device private memory not supported\n");
 			return ERR_PTR(-EINVAL);
 		}
+		if (!pgmap->ops || !pgmap->ops->migrate_to_ram) {
+			WARN(1, "Missing migrate_to_ram method\n");
+			return ERR_PTR(-EINVAL);
+		}
 		break;
 	case MEMORY_DEVICE_FS_DAX:
 		if (!IS_ENABLED(CONFIG_ZONE_DEVICE) ||
diff --git a/mm/hmm.c b/mm/hmm.c
index 5b0bd5f6a74f..96633ee066d8 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1366,15 +1366,12 @@ static void hmm_devmem_ref_kill(struct dev_pagemap *pgmap)
 	percpu_ref_kill(pgmap->ref);
 }
 
-static vm_fault_t hmm_devmem_fault(struct vm_area_struct *vma,
-			    unsigned long addr,
-			    const struct page *page,
-			    unsigned int flags,
-			    pmd_t *pmdp)
+static vm_fault_t hmm_devmem_migrate_to_ram(struct vm_fault *vmf)
 {
-	struct hmm_devmem *devmem = page->pgmap->data;
+	struct hmm_devmem *devmem = vmf->page->pgmap->data;
 
-	return devmem->ops->fault(devmem, vma, addr, page, flags, pmdp);
+	return devmem->ops->fault(devmem, vmf->vma, vmf->address, vmf->page,
+			vmf->flags, vmf->pmd);
 }
 
 static void hmm_devmem_free(struct page *page, void *data)
@@ -1388,6 +1385,7 @@ static const struct dev_pagemap_ops hmm_pagemap_ops = {
 	.page_free		= hmm_devmem_free,
 	.kill			= hmm_devmem_ref_kill,
 	.cleanup		= hmm_devmem_ref_exit,
+	.migrate_to_ram		= hmm_devmem_migrate_to_ram,
 };
 
 /*
@@ -1438,7 +1436,6 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	devmem->pfn_first = devmem->resource->start >> PAGE_SHIFT;
 	devmem->pfn_last = devmem->pfn_first +
 			   (resource_size(devmem->resource) >> PAGE_SHIFT);
-	devmem->page_fault = hmm_devmem_fault;
 
 	devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
 	devmem->pagemap.res = *devmem->resource;
diff --git a/mm/memory.c b/mm/memory.c
index bd21e7063bf0..293d2936fd6c 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -2748,13 +2748,8 @@ vm_fault_t do_swap_page(struct vm_fault *vmf)
 			migration_entry_wait(vma->vm_mm, vmf->pmd,
 					     vmf->address);
 		} else if (is_device_private_entry(entry)) {
-			/*
-			 * For un-addressable device memory we call the pgmap
-			 * fault handler callback. The callback must migrate
-			 * the page back to some CPU accessible page.
-			 */
-			ret = device_private_entry_fault(vma, vmf->address, entry,
-						 vmf->flags, vmf->pmd);
+			vmf->page = device_private_entry_to_page(entry);
+			ret = vmf->page->pgmap->ops->migrate_to_ram(vmf);
 		} else if (is_hwpoison_entry(entry)) {
 			ret = VM_FAULT_HWPOISON;
 		} else {
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 13/25] memremap: remove the data field in struct dev_pagemap
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (11 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 12/25] memremap: add a migrate_to_ram method to struct dev_pagemap_ops Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 14/25] memremap: replace the altmap_valid field with a PGMAP_ALTMAP_VALID flag Christoph Hellwig
                   ` (11 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

struct dev_pagemap is always embedded into a containing structure, so
there is no need to an additional private data field.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/nvdimm/pmem.c    | 2 +-
 include/linux/memremap.h | 3 +--
 kernel/memremap.c        | 2 +-
 mm/hmm.c                 | 9 +++++----
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 48767171a4df..093408ce40ad 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -334,7 +334,7 @@ static void pmem_release_disk(void *__pmem)
 	put_disk(pmem->disk);
 }
 
-static void pmem_pagemap_page_free(struct page *page, void *data)
+static void pmem_pagemap_page_free(struct page *page)
 {
 	wake_up_var(&page->_refcount);
 }
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index ac985bd03a7f..336eca601dad 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -69,7 +69,7 @@ struct dev_pagemap_ops {
 	 * reach 0 refcount unless there is a refcount bug. This allows the
 	 * device driver to implement its own memory management.)
 	 */
-	void (*page_free)(struct page *page, void *data);
+	void (*page_free)(struct page *page);
 
 	/*
 	 * Transition the refcount in struct dev_pagemap to the dead state.
@@ -104,7 +104,6 @@ struct dev_pagemap {
 	struct resource res;
 	struct percpu_ref *ref;
 	struct device *dev;
-	void *data;
 	enum memory_type type;
 	u64 pci_p2pdma_bus_offset;
 	const struct dev_pagemap_ops *ops;
diff --git a/kernel/memremap.c b/kernel/memremap.c
index c06a5487dda7..6c3dbb692037 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -376,7 +376,7 @@ void __put_devmap_managed_page(struct page *page)
 
 		mem_cgroup_uncharge(page);
 
-		page->pgmap->ops->page_free(page, page->pgmap->data);
+		page->pgmap->ops->page_free(page);
 	} else if (!count)
 		__put_page(page);
 }
diff --git a/mm/hmm.c b/mm/hmm.c
index 96633ee066d8..36e25cdbdac1 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1368,15 +1368,17 @@ static void hmm_devmem_ref_kill(struct dev_pagemap *pgmap)
 
 static vm_fault_t hmm_devmem_migrate_to_ram(struct vm_fault *vmf)
 {
-	struct hmm_devmem *devmem = vmf->page->pgmap->data;
+	struct hmm_devmem *devmem =
+		container_of(vmf->page->pgmap, struct hmm_devmem, pagemap);
 
 	return devmem->ops->fault(devmem, vmf->vma, vmf->address, vmf->page,
 			vmf->flags, vmf->pmd);
 }
 
-static void hmm_devmem_free(struct page *page, void *data)
+static void hmm_devmem_free(struct page *page)
 {
-	struct hmm_devmem *devmem = data;
+	struct hmm_devmem *devmem =
+		container_of(page->pgmap, struct hmm_devmem, pagemap);
 
 	devmem->ops->free(devmem, page);
 }
@@ -1442,7 +1444,6 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	devmem->pagemap.ops = &hmm_pagemap_ops;
 	devmem->pagemap.altmap_valid = false;
 	devmem->pagemap.ref = &devmem->ref;
-	devmem->pagemap.data = devmem;
 
 	result = devm_memremap_pages(devmem->device, &devmem->pagemap);
 	if (IS_ERR(result))
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 14/25] memremap: replace the altmap_valid field with a PGMAP_ALTMAP_VALID flag
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (12 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 13/25] memremap: remove the data field in struct dev_pagemap Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 21:13   ` Ira Weiny
  2019-06-26 12:27 ` [PATCH 15/25] memremap: provide an optional internal refcount in struct dev_pagemap Christoph Hellwig
                   ` (10 subsequent siblings)
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

Add a flags field to struct dev_pagemap to replace the altmap_valid
boolean to be a little more extensible.  Also add a pgmap_altmap() helper
to find the optional altmap and clean up the code using the altmap using
it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 arch/powerpc/mm/mem.c     | 10 +---------
 arch/x86/mm/init_64.c     |  8 ++------
 drivers/nvdimm/pfn_devs.c |  3 +--
 drivers/nvdimm/pmem.c     |  1 -
 include/linux/memremap.h  | 12 +++++++++++-
 kernel/memremap.c         | 26 ++++++++++----------------
 mm/hmm.c                  |  1 -
 mm/memory_hotplug.c       |  6 ++----
 mm/page_alloc.c           |  5 ++---
 9 files changed, 29 insertions(+), 43 deletions(-)

diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index cba29131bccc..f774d80df025 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -131,17 +131,9 @@ void __ref arch_remove_memory(int nid, u64 start, u64 size,
 {
 	unsigned long start_pfn = start >> PAGE_SHIFT;
 	unsigned long nr_pages = size >> PAGE_SHIFT;
-	struct page *page;
+	struct page *page = pfn_to_page(start_pfn) + vmem_altmap_offset(altmap);
 	int ret;
 
-	/*
-	 * If we have an altmap then we need to skip over any reserved PFNs
-	 * when querying the zone.
-	 */
-	page = pfn_to_page(start_pfn);
-	if (altmap)
-		page += vmem_altmap_offset(altmap);
-
 	__remove_pages(page_zone(page), start_pfn, nr_pages, altmap);
 
 	/* Remove htab bolted mappings for this section of memory */
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 693aaf28d5fe..3139e992ef9d 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -1211,13 +1211,9 @@ void __ref arch_remove_memory(int nid, u64 start, u64 size,
 {
 	unsigned long start_pfn = start >> PAGE_SHIFT;
 	unsigned long nr_pages = size >> PAGE_SHIFT;
-	struct page *page = pfn_to_page(start_pfn);
-	struct zone *zone;
+	struct page *page = pfn_to_page(start_pfn) + vmem_altmap_offset(altmap);
+	struct zone *zone = page_zone(page);
 
-	/* With altmap the first mapped page is offset from @start */
-	if (altmap)
-		page += vmem_altmap_offset(altmap);
-	zone = page_zone(page);
 	__remove_pages(zone, start_pfn, nr_pages, altmap);
 	kernel_physical_mapping_remove(start, start + size);
 }
diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
index 0f81fc56bbfd..55fb6b7433ed 100644
--- a/drivers/nvdimm/pfn_devs.c
+++ b/drivers/nvdimm/pfn_devs.c
@@ -622,7 +622,6 @@ static int __nvdimm_setup_pfn(struct nd_pfn *nd_pfn, struct dev_pagemap *pgmap)
 		if (offset < reserve)
 			return -EINVAL;
 		nd_pfn->npfns = le64_to_cpu(pfn_sb->npfns);
-		pgmap->altmap_valid = false;
 	} else if (nd_pfn->mode == PFN_MODE_PMEM) {
 		nd_pfn->npfns = PFN_SECTION_ALIGN_UP((resource_size(res)
 					- offset) / PAGE_SIZE);
@@ -634,7 +633,7 @@ static int __nvdimm_setup_pfn(struct nd_pfn *nd_pfn, struct dev_pagemap *pgmap)
 		memcpy(altmap, &__altmap, sizeof(*altmap));
 		altmap->free = PHYS_PFN(offset - reserve);
 		altmap->alloc = 0;
-		pgmap->altmap_valid = true;
+		pgmap->flags |= PGMAP_ALTMAP_VALID;
 	} else
 		return -ENXIO;
 
diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
index 093408ce40ad..e7d8cc9f41e8 100644
--- a/drivers/nvdimm/pmem.c
+++ b/drivers/nvdimm/pmem.c
@@ -412,7 +412,6 @@ static int pmem_attach_disk(struct device *dev,
 		bb_res.start += pmem->data_offset;
 	} else if (pmem_should_map_pages(dev)) {
 		memcpy(&pmem->pgmap.res, &nsio->res, sizeof(pmem->pgmap.res));
-		pmem->pgmap.altmap_valid = false;
 		pmem->pgmap.type = MEMORY_DEVICE_FS_DAX;
 		pmem->pgmap.ops = &fsdax_pagemap_ops;
 		addr = devm_memremap_pages(dev, &pmem->pgmap);
diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index 336eca601dad..e25685b878e9 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -88,6 +88,8 @@ struct dev_pagemap_ops {
 	vm_fault_t (*migrate_to_ram)(struct vm_fault *vmf);
 };
 
+#define PGMAP_ALTMAP_VALID	(1 << 0)
+
 /**
  * struct dev_pagemap - metadata for ZONE_DEVICE mappings
  * @altmap: pre-allocated/reserved memory for vmemmap allocations
@@ -96,19 +98,27 @@ struct dev_pagemap_ops {
  * @dev: host device of the mapping for debug
  * @data: private data pointer for page_free()
  * @type: memory type: see MEMORY_* in memory_hotplug.h
+ * @flags: PGMAP_* flags to specify defailed behavior
  * @ops: method table
  */
 struct dev_pagemap {
 	struct vmem_altmap altmap;
-	bool altmap_valid;
 	struct resource res;
 	struct percpu_ref *ref;
 	struct device *dev;
 	enum memory_type type;
+	unsigned int flags;
 	u64 pci_p2pdma_bus_offset;
 	const struct dev_pagemap_ops *ops;
 };
 
+static inline struct vmem_altmap *pgmap_altmap(struct dev_pagemap *pgmap)
+{
+	if (pgmap->flags & PGMAP_ALTMAP_VALID)
+		return &pgmap->altmap;
+	return NULL;
+}
+
 #ifdef CONFIG_ZONE_DEVICE
 void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap);
 void devm_memunmap_pages(struct device *dev, struct dev_pagemap *pgmap);
diff --git a/kernel/memremap.c b/kernel/memremap.c
index 6c3dbb692037..eee490e7d7e1 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -54,14 +54,8 @@ static void pgmap_array_delete(struct resource *res)
 
 static unsigned long pfn_first(struct dev_pagemap *pgmap)
 {
-	const struct resource *res = &pgmap->res;
-	struct vmem_altmap *altmap = &pgmap->altmap;
-	unsigned long pfn;
-
-	pfn = res->start >> PAGE_SHIFT;
-	if (pgmap->altmap_valid)
-		pfn += vmem_altmap_offset(altmap);
-	return pfn;
+	return (pgmap->res.start >> PAGE_SHIFT) +
+		vmem_altmap_offset(pgmap_altmap(pgmap));
 }
 
 static unsigned long pfn_end(struct dev_pagemap *pgmap)
@@ -109,7 +103,7 @@ static void devm_memremap_pages_release(void *data)
 				align_size >> PAGE_SHIFT, NULL);
 	} else {
 		arch_remove_memory(nid, align_start, align_size,
-				pgmap->altmap_valid ? &pgmap->altmap : NULL);
+				pgmap_altmap(pgmap));
 		kasan_remove_zero_shadow(__va(align_start), align_size);
 	}
 	mem_hotplug_done();
@@ -129,8 +123,8 @@ static void devm_memremap_pages_release(void *data)
  * 1/ At a minimum the res, ref and type and ops members of @pgmap must be
  *    initialized by the caller before passing it to this function
  *
- * 2/ The altmap field may optionally be initialized, in which case altmap_valid
- *    must be set to true
+ * 2/ The altmap field may optionally be initialized, in which case
+ *    PGMAP_ALTMAP_VALID must be set in pgmap->flags.
  *
  * 3/ pgmap->ref must be 'live' on entry and will be killed and reaped
  *    at devm_memremap_pages_release() time, or if this routine fails.
@@ -142,15 +136,13 @@ static void devm_memremap_pages_release(void *data)
 void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 {
 	resource_size_t align_start, align_size, align_end;
-	struct vmem_altmap *altmap = pgmap->altmap_valid ?
-			&pgmap->altmap : NULL;
 	struct resource *res = &pgmap->res;
 	struct dev_pagemap *conflict_pgmap;
 	struct mhp_restrictions restrictions = {
 		/*
 		 * We do not want any optional features only our own memmap
 		*/
-		.altmap = altmap,
+		.altmap = pgmap_altmap(pgmap),
 	};
 	pgprot_t pgprot = PAGE_KERNEL;
 	int error, nid, is_ram;
@@ -274,7 +266,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 
 		zone = &NODE_DATA(nid)->node_zones[ZONE_DEVICE];
 		move_pfn_range_to_zone(zone, align_start >> PAGE_SHIFT,
-				align_size >> PAGE_SHIFT, altmap);
+				align_size >> PAGE_SHIFT, pgmap_altmap(pgmap));
 	}
 
 	mem_hotplug_done();
@@ -319,7 +311,9 @@ EXPORT_SYMBOL_GPL(devm_memunmap_pages);
 unsigned long vmem_altmap_offset(struct vmem_altmap *altmap)
 {
 	/* number of pfns from base where pfn_to_page() is valid */
-	return altmap->reserve + altmap->free;
+	if (altmap)
+		return altmap->reserve + altmap->free;
+	return 0;
 }
 
 void vmem_altmap_free(struct vmem_altmap *altmap, unsigned long nr_pfns)
diff --git a/mm/hmm.c b/mm/hmm.c
index 36e25cdbdac1..e4470462298f 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1442,7 +1442,6 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
 	devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
 	devmem->pagemap.res = *devmem->resource;
 	devmem->pagemap.ops = &hmm_pagemap_ops;
-	devmem->pagemap.altmap_valid = false;
 	devmem->pagemap.ref = &devmem->ref;
 
 	result = devm_memremap_pages(devmem->device, &devmem->pagemap);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index e096c987d261..6166ba5a15f3 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -557,10 +557,8 @@ void __remove_pages(struct zone *zone, unsigned long phys_start_pfn,
 	int sections_to_remove;
 
 	/* In the ZONE_DEVICE case device driver owns the memory region */
-	if (is_dev_zone(zone)) {
-		if (altmap)
-			map_offset = vmem_altmap_offset(altmap);
-	}
+	if (is_dev_zone(zone))
+		map_offset = vmem_altmap_offset(altmap);
 
 	clear_zone_contiguous(zone);
 
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d66bc8abe0af..17a39d40a556 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5853,6 +5853,7 @@ void __ref memmap_init_zone_device(struct zone *zone,
 {
 	unsigned long pfn, end_pfn = start_pfn + size;
 	struct pglist_data *pgdat = zone->zone_pgdat;
+	struct vmem_altmap *altmap = pgmap_altmap(pgmap);
 	unsigned long zone_idx = zone_idx(zone);
 	unsigned long start = jiffies;
 	int nid = pgdat->node_id;
@@ -5865,9 +5866,7 @@ void __ref memmap_init_zone_device(struct zone *zone,
 	 * of the pages reserved for the memmap, so we can just jump to
 	 * the end of that region and start processing the device pages.
 	 */
-	if (pgmap->altmap_valid) {
-		struct vmem_altmap *altmap = &pgmap->altmap;
-
+	if (altmap) {
 		start_pfn = altmap->base_pfn + vmem_altmap_offset(altmap);
 		size = end_pfn - start_pfn;
 	}
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 15/25] memremap: provide an optional internal refcount in struct dev_pagemap
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (13 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 14/25] memremap: replace the altmap_valid field with a PGMAP_ALTMAP_VALID flag Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 21:47   ` Ira Weiny
  2019-06-26 12:27 ` [PATCH 16/25] device-dax: use the dev_pagemap internal refcount Christoph Hellwig
                   ` (9 subsequent siblings)
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

Provide an internal refcounting logic if no ->ref field is provided
in the pagemap passed into devm_memremap_pages so that callers don't
have to reinvent it poorly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/memremap.h          |  4 ++
 kernel/memremap.c                 | 64 ++++++++++++++++++++++++-------
 tools/testing/nvdimm/test/iomap.c | 58 ++++++++++++++++++++++------
 3 files changed, 101 insertions(+), 25 deletions(-)

diff --git a/include/linux/memremap.h b/include/linux/memremap.h
index e25685b878e9..f8a5b2a19945 100644
--- a/include/linux/memremap.h
+++ b/include/linux/memremap.h
@@ -95,6 +95,8 @@ struct dev_pagemap_ops {
  * @altmap: pre-allocated/reserved memory for vmemmap allocations
  * @res: physical address range covered by @ref
  * @ref: reference count that pins the devm_memremap_pages() mapping
+ * @internal_ref: internal reference if @ref is not provided by the caller
+ * @done: completion for @internal_ref
  * @dev: host device of the mapping for debug
  * @data: private data pointer for page_free()
  * @type: memory type: see MEMORY_* in memory_hotplug.h
@@ -105,6 +107,8 @@ struct dev_pagemap {
 	struct vmem_altmap altmap;
 	struct resource res;
 	struct percpu_ref *ref;
+	struct percpu_ref internal_ref;
+	struct completion done;
 	struct device *dev;
 	enum memory_type type;
 	unsigned int flags;
diff --git a/kernel/memremap.c b/kernel/memremap.c
index eee490e7d7e1..bea6f887adad 100644
--- a/kernel/memremap.c
+++ b/kernel/memremap.c
@@ -29,7 +29,7 @@ static void devmap_managed_enable_put(void *data)
 
 static int devmap_managed_enable_get(struct device *dev, struct dev_pagemap *pgmap)
 {
-	if (!pgmap->ops->page_free) {
+	if (!pgmap->ops || !pgmap->ops->page_free) {
 		WARN(1, "Missing page_free method\n");
 		return -EINVAL;
 	}
@@ -75,6 +75,24 @@ static unsigned long pfn_next(unsigned long pfn)
 #define for_each_device_pfn(pfn, map) \
 	for (pfn = pfn_first(map); pfn < pfn_end(map); pfn = pfn_next(pfn))
 
+static void dev_pagemap_kill(struct dev_pagemap *pgmap)
+{
+	if (pgmap->ops && pgmap->ops->kill)
+		pgmap->ops->kill(pgmap);
+	else
+		percpu_ref_kill(pgmap->ref);
+}
+
+static void dev_pagemap_cleanup(struct dev_pagemap *pgmap)
+{
+	if (pgmap->ops && pgmap->ops->cleanup) {
+		pgmap->ops->cleanup(pgmap);
+	} else {
+		wait_for_completion(&pgmap->done);
+		percpu_ref_exit(pgmap->ref);
+	}
+}
+
 static void devm_memremap_pages_release(void *data)
 {
 	struct dev_pagemap *pgmap = data;
@@ -84,10 +102,10 @@ static void devm_memremap_pages_release(void *data)
 	unsigned long pfn;
 	int nid;
 
-	pgmap->ops->kill(pgmap);
+	dev_pagemap_kill(pgmap);
 	for_each_device_pfn(pfn, pgmap)
 		put_page(pfn_to_page(pfn));
-	pgmap->ops->cleanup(pgmap);
+	dev_pagemap_cleanup(pgmap);
 
 	/* pages are dead and unused, undo the arch mapping */
 	align_start = res->start & ~(SECTION_SIZE - 1);
@@ -114,20 +132,29 @@ static void devm_memremap_pages_release(void *data)
 		      "%s: failed to free all reserved pages\n", __func__);
 }
 
+static void dev_pagemap_percpu_release(struct percpu_ref *ref)
+{
+	struct dev_pagemap *pgmap =
+		container_of(ref, struct dev_pagemap, internal_ref);
+
+	complete(&pgmap->done);
+}
+
 /**
  * devm_memremap_pages - remap and provide memmap backing for the given resource
  * @dev: hosting device for @res
  * @pgmap: pointer to a struct dev_pagemap
  *
  * Notes:
- * 1/ At a minimum the res, ref and type and ops members of @pgmap must be
- *    initialized by the caller before passing it to this function
+ * 1/ At a minimum the res and type members of @pgmap must be initialized
+ *    by the caller before passing it to this function
  *
  * 2/ The altmap field may optionally be initialized, in which case
  *    PGMAP_ALTMAP_VALID must be set in pgmap->flags.
  *
- * 3/ pgmap->ref must be 'live' on entry and will be killed and reaped
- *    at devm_memremap_pages_release() time, or if this routine fails.
+ * 3/ The ref field may optionally be provided, in which pgmap->ref must be
+ *    'live' on entry and will be killed and reaped at
+ *    devm_memremap_pages_release() time, or if this routine fails.
  *
  * 4/ res is expected to be a host memory range that could feasibly be
  *    treated as a "System RAM" range, i.e. not a device mmio range, but
@@ -175,10 +202,21 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 		break;
 	}
 
-	if (!pgmap->ref || !pgmap->ops || !pgmap->ops->kill ||
-	    !pgmap->ops->cleanup) {
-		WARN(1, "Missing reference count teardown definition\n");
-		return ERR_PTR(-EINVAL);
+	if (!pgmap->ref) {
+		if (pgmap->ops && (pgmap->ops->kill || pgmap->ops->cleanup))
+			return ERR_PTR(-EINVAL);
+
+		init_completion(&pgmap->done);
+		error = percpu_ref_init(&pgmap->internal_ref,
+				dev_pagemap_percpu_release, 0, GFP_KERNEL);
+		if (error)
+			return ERR_PTR(error);
+		pgmap->ref = &pgmap->internal_ref;
+	} else {
+		if (!pgmap->ops || !pgmap->ops->kill || !pgmap->ops->cleanup) {
+			WARN(1, "Missing reference count teardown definition\n");
+			return ERR_PTR(-EINVAL);
+		}
 	}
 
 	if (need_devmap_managed) {
@@ -296,8 +334,8 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
  err_pfn_remap:
 	pgmap_array_delete(res);
  err_array:
-	pgmap->ops->kill(pgmap);
-	pgmap->ops->cleanup(pgmap);
+	dev_pagemap_kill(pgmap);
+	dev_pagemap_cleanup(pgmap);
 	return ERR_PTR(error);
 }
 EXPORT_SYMBOL_GPL(devm_memremap_pages);
diff --git a/tools/testing/nvdimm/test/iomap.c b/tools/testing/nvdimm/test/iomap.c
index 82f901569e06..cd040b5abffe 100644
--- a/tools/testing/nvdimm/test/iomap.c
+++ b/tools/testing/nvdimm/test/iomap.c
@@ -100,26 +100,60 @@ static void nfit_test_kill(void *_pgmap)
 {
 	struct dev_pagemap *pgmap = _pgmap;
 
-	WARN_ON(!pgmap || !pgmap->ref || !pgmap->ops || !pgmap->ops->kill ||
-		!pgmap->ops->cleanup);
-	pgmap->ops->kill(pgmap);
-	pgmap->ops->cleanup(pgmap);
+	WARN_ON(!pgmap || !pgmap->ref);
+
+	if (pgmap->ops && pgmap->ops->kill)
+		pgmap->ops->kill(pgmap);
+	else
+		percpu_ref_kill(pgmap->ref);
+
+	if (pgmap->ops && pgmap->ops->cleanup) {
+		pgmap->ops->cleanup(pgmap);
+	} else {
+		wait_for_completion(&pgmap->done);
+		percpu_ref_exit(pgmap->ref);
+	}
+}
+
+static void dev_pagemap_percpu_release(struct percpu_ref *ref)
+{
+	struct dev_pagemap *pgmap =
+		container_of(ref, struct dev_pagemap, internal_ref);
+
+	complete(&pgmap->done);
 }
 
 void *__wrap_devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
 {
+	int error;
 	resource_size_t offset = pgmap->res.start;
 	struct nfit_test_resource *nfit_res = get_nfit_res(offset);
 
-	if (nfit_res) {
-		int rc;
-
-		rc = devm_add_action_or_reset(dev, nfit_test_kill, pgmap);
-		if (rc)
-			return ERR_PTR(rc);
-		return nfit_res->buf + offset - nfit_res->res.start;
+	if (!nfit_res)
+		return devm_memremap_pages(dev, pgmap);
+
+	pgmap->dev = dev;
+	if (!pgmap->ref) {
+		if (pgmap->ops && (pgmap->ops->kill || pgmap->ops->cleanup))
+			return ERR_PTR(-EINVAL);
+
+		init_completion(&pgmap->done);
+		error = percpu_ref_init(&pgmap->internal_ref,
+				dev_pagemap_percpu_release, 0, GFP_KERNEL);
+		if (error)
+			return ERR_PTR(error);
+		pgmap->ref = &pgmap->internal_ref;
+	} else {
+		if (!pgmap->ops || !pgmap->ops->kill || !pgmap->ops->cleanup) {
+			WARN(1, "Missing reference count teardown definition\n");
+			return ERR_PTR(-EINVAL);
+		}
 	}
-	return devm_memremap_pages(dev, pgmap);
+
+	error = devm_add_action_or_reset(dev, nfit_test_kill, pgmap);
+	if (error)
+		return ERR_PTR(error);
+	return nfit_res->buf + offset - nfit_res->res.start;
 }
 EXPORT_SYMBOL_GPL(__wrap_devm_memremap_pages);
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (14 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 15/25] memremap: provide an optional internal refcount in struct dev_pagemap Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 21:48   ` Ira Weiny
  2019-06-28 15:38   ` Jason Gunthorpe
  2019-06-26 12:27 ` [PATCH 17/25] PCI/P2PDMA: " Christoph Hellwig
                   ` (8 subsequent siblings)
  24 siblings, 2 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

The functionality is identical to the one currently open coded in
device-dax.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/dax/dax-private.h |  4 ----
 drivers/dax/device.c      | 43 ---------------------------------------
 2 files changed, 47 deletions(-)

diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h
index b4177aafbbd1..c915889d1769 100644
--- a/drivers/dax/dax-private.h
+++ b/drivers/dax/dax-private.h
@@ -43,8 +43,6 @@ struct dax_region {
  * @target_node: effective numa node if dev_dax memory range is onlined
  * @dev - device core
  * @pgmap - pgmap for memmap setup / lifetime (driver owned)
- * @ref: pgmap reference count (driver owned)
- * @cmp: @ref final put completion (driver owned)
  */
 struct dev_dax {
 	struct dax_region *region;
@@ -52,8 +50,6 @@ struct dev_dax {
 	int target_node;
 	struct device dev;
 	struct dev_pagemap pgmap;
-	struct percpu_ref ref;
-	struct completion cmp;
 };
 
 static inline struct dev_dax *to_dev_dax(struct device *dev)
diff --git a/drivers/dax/device.c b/drivers/dax/device.c
index b5257038c188..1af823b2fe6b 100644
--- a/drivers/dax/device.c
+++ b/drivers/dax/device.c
@@ -14,36 +14,6 @@
 #include "dax-private.h"
 #include "bus.h"
 
-static struct dev_dax *ref_to_dev_dax(struct percpu_ref *ref)
-{
-	return container_of(ref, struct dev_dax, ref);
-}
-
-static void dev_dax_percpu_release(struct percpu_ref *ref)
-{
-	struct dev_dax *dev_dax = ref_to_dev_dax(ref);
-
-	dev_dbg(&dev_dax->dev, "%s\n", __func__);
-	complete(&dev_dax->cmp);
-}
-
-static void dev_dax_percpu_exit(struct dev_pagemap *pgmap)
-{
-	struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap);
-
-	dev_dbg(&dev_dax->dev, "%s\n", __func__);
-	wait_for_completion(&dev_dax->cmp);
-	percpu_ref_exit(pgmap->ref);
-}
-
-static void dev_dax_percpu_kill(struct dev_pagemap *pgmap)
-{
-	struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap);
-
-	dev_dbg(&dev_dax->dev, "%s\n", __func__);
-	percpu_ref_kill(pgmap->ref);
-}
-
 static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma,
 		const char *func)
 {
@@ -441,11 +411,6 @@ static void dev_dax_kill(void *dev_dax)
 	kill_dev_dax(dev_dax);
 }
 
-static const struct dev_pagemap_ops dev_dax_pagemap_ops = {
-	.kill		= dev_dax_percpu_kill,
-	.cleanup	= dev_dax_percpu_exit,
-};
-
 int dev_dax_probe(struct device *dev)
 {
 	struct dev_dax *dev_dax = to_dev_dax(dev);
@@ -463,15 +428,7 @@ int dev_dax_probe(struct device *dev)
 		return -EBUSY;
 	}
 
-	init_completion(&dev_dax->cmp);
-	rc = percpu_ref_init(&dev_dax->ref, dev_dax_percpu_release, 0,
-			GFP_KERNEL);
-	if (rc)
-		return rc;
-
-	dev_dax->pgmap.ref = &dev_dax->ref;
 	dev_dax->pgmap.type = MEMORY_DEVICE_DEVDAX;
-	dev_dax->pgmap.ops = &dev_dax_pagemap_ops;
 	addr = devm_memremap_pages(dev, &dev_dax->pgmap);
 	if (IS_ERR(addr))
 		return PTR_ERR(addr);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 17/25] PCI/P2PDMA: use the dev_pagemap internal refcount
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (15 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 16/25] device-dax: use the dev_pagemap internal refcount Christoph Hellwig
@ 2019-06-26 12:27 ` " Christoph Hellwig
  2019-06-26 21:49   ` Ira Weiny
  2019-06-27 18:49   ` Logan Gunthorpe
  2019-06-26 12:27 ` [PATCH 18/25] nouveau: use alloc_page_vma directly Christoph Hellwig
                   ` (7 subsequent siblings)
  24 siblings, 2 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

The functionality is identical to the one currently open coded in
p2pdma.c.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/pci/p2pdma.c | 57 ++++----------------------------------------
 1 file changed, 4 insertions(+), 53 deletions(-)

diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
index ebd8ce3bba2e..608f84df604a 100644
--- a/drivers/pci/p2pdma.c
+++ b/drivers/pci/p2pdma.c
@@ -24,12 +24,6 @@ struct pci_p2pdma {
 	bool p2pmem_published;
 };
 
-struct p2pdma_pagemap {
-	struct dev_pagemap pgmap;
-	struct percpu_ref ref;
-	struct completion ref_done;
-};
-
 static ssize_t size_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
@@ -78,32 +72,6 @@ static const struct attribute_group p2pmem_group = {
 	.name = "p2pmem",
 };
 
-static struct p2pdma_pagemap *to_p2p_pgmap(struct percpu_ref *ref)
-{
-	return container_of(ref, struct p2pdma_pagemap, ref);
-}
-
-static void pci_p2pdma_percpu_release(struct percpu_ref *ref)
-{
-	struct p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(ref);
-
-	complete(&p2p_pgmap->ref_done);
-}
-
-static void pci_p2pdma_percpu_kill(struct dev_pagemap *pgmap)
-{
-	percpu_ref_kill(pgmap->ref);
-}
-
-static void pci_p2pdma_percpu_cleanup(struct dev_pagemap *pgmap)
-{
-	struct p2pdma_pagemap *p2p_pgmap =
-		container_of(pgmap, struct p2pdma_pagemap, pgmap);
-
-	wait_for_completion(&p2p_pgmap->ref_done);
-	percpu_ref_exit(&p2p_pgmap->ref);
-}
-
 static void pci_p2pdma_release(void *data)
 {
 	struct pci_dev *pdev = data;
@@ -153,11 +121,6 @@ static int pci_p2pdma_setup(struct pci_dev *pdev)
 	return error;
 }
 
-static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
-	.kill		= pci_p2pdma_percpu_kill,
-	.cleanup	= pci_p2pdma_percpu_cleanup,
-};
-
 /**
  * pci_p2pdma_add_resource - add memory for use as p2p memory
  * @pdev: the device to add the memory to
@@ -171,7 +134,6 @@ static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
 int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
 			    u64 offset)
 {
-	struct p2pdma_pagemap *p2p_pgmap;
 	struct dev_pagemap *pgmap;
 	void *addr;
 	int error;
@@ -194,26 +156,15 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
 			return error;
 	}
 
-	p2p_pgmap = devm_kzalloc(&pdev->dev, sizeof(*p2p_pgmap), GFP_KERNEL);
-	if (!p2p_pgmap)
+	pgmap = devm_kzalloc(&pdev->dev, sizeof(*pgmap), GFP_KERNEL);
+	if (!pgmap)
 		return -ENOMEM;
-
-	init_completion(&p2p_pgmap->ref_done);
-	error = percpu_ref_init(&p2p_pgmap->ref,
-			pci_p2pdma_percpu_release, 0, GFP_KERNEL);
-	if (error)
-		goto pgmap_free;
-
-	pgmap = &p2p_pgmap->pgmap;
-
 	pgmap->res.start = pci_resource_start(pdev, bar) + offset;
 	pgmap->res.end = pgmap->res.start + size - 1;
 	pgmap->res.flags = pci_resource_flags(pdev, bar);
-	pgmap->ref = &p2p_pgmap->ref;
 	pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
 	pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
 		pci_resource_start(pdev, bar);
-	pgmap->ops = &pci_p2pdma_pagemap_ops;
 
 	addr = devm_memremap_pages(&pdev->dev, pgmap);
 	if (IS_ERR(addr)) {
@@ -224,7 +175,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
 	error = gen_pool_add_owner(pdev->p2pdma->pool, (unsigned long)addr,
 			pci_bus_address(pdev, bar) + offset,
 			resource_size(&pgmap->res), dev_to_node(&pdev->dev),
-			&p2p_pgmap->ref);
+			pgmap->ref);
 	if (error)
 		goto pages_free;
 
@@ -236,7 +187,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
 pages_free:
 	devm_memunmap_pages(&pdev->dev, pgmap);
 pgmap_free:
-	devm_kfree(&pdev->dev, p2p_pgmap);
+	devm_kfree(&pdev->dev, pgmap);
 	return error;
 }
 EXPORT_SYMBOL_GPL(pci_p2pdma_add_resource);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 18/25] nouveau: use alloc_page_vma directly
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (16 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 17/25] PCI/P2PDMA: " Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 19/25] nouveau: use devm_memremap_pages directly Christoph Hellwig
                   ` (6 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

hmm_vma_alloc_locked_page is scheduled to go away, use the proper
mm function directly.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/gpu/drm/nouveau/nouveau_dmem.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index 40c47d6a7d78..a50f6fd2fe24 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -148,11 +148,12 @@ nouveau_dmem_fault_alloc_and_copy(struct vm_area_struct *vma,
 		if (!spage || !(src_pfns[i] & MIGRATE_PFN_MIGRATE))
 			continue;
 
-		dpage = hmm_vma_alloc_locked_page(vma, addr);
+		dpage = alloc_page_vma(GFP_HIGHUSER, vma, addr);
 		if (!dpage) {
 			dst_pfns[i] = MIGRATE_PFN_ERROR;
 			continue;
 		}
+		lock_page(dpage);
 
 		dst_pfns[i] = migrate_pfn(page_to_pfn(dpage)) |
 			      MIGRATE_PFN_LOCKED;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 19/25] nouveau: use devm_memremap_pages directly
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (17 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 18/25] nouveau: use alloc_page_vma directly Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 20/25] mm: remove hmm_vma_alloc_locked_page Christoph Hellwig
                   ` (5 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

Just use devm_memremap_pages instead of hmm_devmem_add pages to allow
killing that wrapper which doesn't provide a whole lot of benefits.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/nouveau/nouveau_dmem.c | 82 ++++++++++++--------------
 1 file changed, 38 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index a50f6fd2fe24..0fb7a44b8bc4 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -72,7 +72,8 @@ struct nouveau_dmem_migrate {
 };
 
 struct nouveau_dmem {
-	struct hmm_devmem *devmem;
+	struct nouveau_drm *drm;
+	struct dev_pagemap pagemap;
 	struct nouveau_dmem_migrate migrate;
 	struct list_head chunk_free;
 	struct list_head chunk_full;
@@ -80,6 +81,11 @@ struct nouveau_dmem {
 	struct mutex mutex;
 };
 
+static inline struct nouveau_dmem *page_to_dmem(struct page *page)
+{
+	return container_of(page->pgmap, struct nouveau_dmem, pagemap);
+}
+
 struct nouveau_dmem_fault {
 	struct nouveau_drm *drm;
 	struct nouveau_fence *fence;
@@ -96,8 +102,7 @@ struct nouveau_migrate {
 	unsigned long dma_nr;
 };
 
-static void
-nouveau_dmem_free(struct hmm_devmem *devmem, struct page *page)
+static void nouveau_dmem_page_free(struct page *page)
 {
 	struct nouveau_dmem_chunk *chunk;
 	unsigned long idx;
@@ -260,29 +265,21 @@ static const struct migrate_vma_ops nouveau_dmem_fault_migrate_ops = {
 	.finalize_and_map	= nouveau_dmem_fault_finalize_and_map,
 };
 
-static vm_fault_t
-nouveau_dmem_fault(struct hmm_devmem *devmem,
-		   struct vm_area_struct *vma,
-		   unsigned long addr,
-		   const struct page *page,
-		   unsigned int flags,
-		   pmd_t *pmdp)
+static vm_fault_t nouveau_dmem_migrate_to_ram(struct vm_fault *vmf)
 {
-	struct drm_device *drm_dev = dev_get_drvdata(devmem->device);
+	struct nouveau_dmem *dmem = page_to_dmem(vmf->page);
 	unsigned long src[1] = {0}, dst[1] = {0};
-	struct nouveau_dmem_fault fault = {0};
+	struct nouveau_dmem_fault fault = { .drm = dmem->drm };
 	int ret;
 
-
-
 	/*
 	 * FIXME what we really want is to find some heuristic to migrate more
 	 * than just one page on CPU fault. When such fault happens it is very
 	 * likely that more surrounding page will CPU fault too.
 	 */
-	fault.drm = nouveau_drm(drm_dev);
-	ret = migrate_vma(&nouveau_dmem_fault_migrate_ops, vma, addr,
-			  addr + PAGE_SIZE, src, dst, &fault);
+	ret = migrate_vma(&nouveau_dmem_fault_migrate_ops, vmf->vma,
+			vmf->address, vmf->address + PAGE_SIZE,
+			src, dst, &fault);
 	if (ret)
 		return VM_FAULT_SIGBUS;
 
@@ -292,10 +289,9 @@ nouveau_dmem_fault(struct hmm_devmem *devmem,
 	return 0;
 }
 
-static const struct hmm_devmem_ops
-nouveau_dmem_devmem_ops = {
-	.free = nouveau_dmem_free,
-	.fault = nouveau_dmem_fault,
+static const struct dev_pagemap_ops nouveau_dmem_pagemap_ops = {
+	.page_free		= nouveau_dmem_page_free,
+	.migrate_to_ram		= nouveau_dmem_migrate_to_ram,
 };
 
 static int
@@ -581,7 +577,8 @@ void
 nouveau_dmem_init(struct nouveau_drm *drm)
 {
 	struct device *device = drm->dev->dev;
-	unsigned long i, size;
+	struct resource *res;
+	unsigned long i, size, pfn_first;
 	int ret;
 
 	/* This only make sense on PASCAL or newer */
@@ -591,6 +588,7 @@ nouveau_dmem_init(struct nouveau_drm *drm)
 	if (!(drm->dmem = kzalloc(sizeof(*drm->dmem), GFP_KERNEL)))
 		return;
 
+	drm->dmem->drm = drm;
 	mutex_init(&drm->dmem->mutex);
 	INIT_LIST_HEAD(&drm->dmem->chunk_free);
 	INIT_LIST_HEAD(&drm->dmem->chunk_full);
@@ -600,11 +598,8 @@ nouveau_dmem_init(struct nouveau_drm *drm)
 
 	/* Initialize migration dma helpers before registering memory */
 	ret = nouveau_dmem_migrate_init(drm);
-	if (ret) {
-		kfree(drm->dmem);
-		drm->dmem = NULL;
-		return;
-	}
+	if (ret)
+		goto out_free;
 
 	/*
 	 * FIXME we need some kind of policy to decide how much VRAM we
@@ -612,14 +607,16 @@ nouveau_dmem_init(struct nouveau_drm *drm)
 	 * and latter if we want to do thing like over commit then we
 	 * could revisit this.
 	 */
-	drm->dmem->devmem = hmm_devmem_add(&nouveau_dmem_devmem_ops,
-					   device, size);
-	if (IS_ERR(drm->dmem->devmem)) {
-		kfree(drm->dmem);
-		drm->dmem = NULL;
-		return;
-	}
-
+	res = devm_request_free_mem_region(device, &iomem_resource, size);
+	if (IS_ERR(res))
+		goto out_free;
+	drm->dmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
+	drm->dmem->pagemap.res = *res;
+	drm->dmem->pagemap.ops = &nouveau_dmem_pagemap_ops;
+	if (IS_ERR(devm_memremap_pages(device, &drm->dmem->pagemap)))
+		goto out_free;
+
+	pfn_first = res->start >> PAGE_SHIFT;
 	for (i = 0; i < (size / DMEM_CHUNK_SIZE); ++i) {
 		struct nouveau_dmem_chunk *chunk;
 		struct page *page;
@@ -632,8 +629,7 @@ nouveau_dmem_init(struct nouveau_drm *drm)
 		}
 
 		chunk->drm = drm;
-		chunk->pfn_first = drm->dmem->devmem->pfn_first;
-		chunk->pfn_first += (i * DMEM_CHUNK_NPAGES);
+		chunk->pfn_first = pfn_first + (i * DMEM_CHUNK_NPAGES);
 		list_add_tail(&chunk->list, &drm->dmem->chunk_empty);
 
 		page = pfn_to_page(chunk->pfn_first);
@@ -643,6 +639,10 @@ nouveau_dmem_init(struct nouveau_drm *drm)
 	}
 
 	NV_INFO(drm, "DMEM: registered %ldMB of device memory\n", size >> 20);
+	return;
+out_free:
+	kfree(drm->dmem);
+	drm->dmem = NULL;
 }
 
 static void
@@ -833,13 +833,7 @@ nouveau_dmem_migrate_vma(struct nouveau_drm *drm,
 static inline bool
 nouveau_dmem_page(struct nouveau_drm *drm, struct page *page)
 {
-	if (!is_device_private_page(page))
-		return false;
-
-	if (drm->dmem->devmem != page->pgmap->data)
-		return false;
-
-	return true;
+	return is_device_private_page(page) && drm->dmem == page_to_dmem(page);
 }
 
 void
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 20/25] mm: remove hmm_vma_alloc_locked_page
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (18 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 19/25] nouveau: use devm_memremap_pages directly Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-27 16:26   ` Jason Gunthorpe
  2019-06-26 12:27 ` [PATCH 21/25] mm: remove hmm_devmem_add Christoph Hellwig
                   ` (4 subsequent siblings)
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

The only user of it has just been removed, and there wasn't really any need
to wrap a basic memory allocator to start with.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/hmm.h |  3 ---
 mm/hmm.c            | 14 --------------
 2 files changed, 17 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index ba19c19e24ed..1d55b7ea2da6 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -587,9 +587,6 @@ static inline void hmm_mm_init(struct mm_struct *mm) {}
 #if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
 struct hmm_devmem;
 
-struct page *hmm_vma_alloc_locked_page(struct vm_area_struct *vma,
-				       unsigned long addr);
-
 /*
  * struct hmm_devmem_ops - callback for ZONE_DEVICE memory events
  *
diff --git a/mm/hmm.c b/mm/hmm.c
index e4470462298f..fdbd48771292 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1330,20 +1330,6 @@ EXPORT_SYMBOL(hmm_range_dma_unmap);
 
 
 #if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
-struct page *hmm_vma_alloc_locked_page(struct vm_area_struct *vma,
-				       unsigned long addr)
-{
-	struct page *page;
-
-	page = alloc_page_vma(GFP_HIGHUSER, vma, addr);
-	if (!page)
-		return NULL;
-	lock_page(page);
-	return page;
-}
-EXPORT_SYMBOL(hmm_vma_alloc_locked_page);
-
-
 static void hmm_devmem_ref_release(struct percpu_ref *ref)
 {
 	struct hmm_devmem *devmem;
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 21/25] mm: remove hmm_devmem_add
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (19 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 20/25] mm: remove hmm_vma_alloc_locked_page Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 22/25] mm: simplify ZONE_DEVICE page private data Christoph Hellwig
                   ` (3 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

There isn't really much value add in the hmm_devmem_add wrapper and
more, as using devm_memremap_pages directly now is just as simple.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
---
 Documentation/vm/hmm.rst |  26 --------
 include/linux/hmm.h      | 129 ---------------------------------------
 mm/hmm.c                 | 110 ---------------------------------
 3 files changed, 265 deletions(-)

diff --git a/Documentation/vm/hmm.rst b/Documentation/vm/hmm.rst
index 7cdf7282e022..50e1380950a9 100644
--- a/Documentation/vm/hmm.rst
+++ b/Documentation/vm/hmm.rst
@@ -329,32 +329,6 @@ directly using struct page for device memory which left most kernel code paths
 unaware of the difference. We only need to make sure that no one ever tries to
 map those pages from the CPU side.
 
-HMM provides a set of helpers to register and hotplug device memory as a new
-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::
-
- struct hmm_devmem_ops {
-     void (*free)(struct hmm_devmem *devmem, struct page *page);
-     int (*fault)(struct hmm_devmem *devmem,
-                  struct vm_area_struct *vma,
-                  unsigned long addr,
-                  struct page *page,
-                  unsigned flags,
-                  pmd_t *pmdp);
- };
-
-The first callback (free()) happens when the last reference on a device page is
-dropped. This means the device page is now free and no longer used by anyone.
-The second callback happens whenever the CPU tries to access a device page
-which it cannot do. This second callback must trigger a migration back to
-system memory.
-
 
 Migration to and from device memory
 ===================================
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 1d55b7ea2da6..86aa4ec3404c 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -585,135 +585,6 @@ static inline void hmm_mm_init(struct mm_struct *mm) {}
 #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
 
 #if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
-struct hmm_devmem;
-
-/*
- * struct hmm_devmem_ops - callback for ZONE_DEVICE memory events
- *
- * @free: call when refcount on page reach 1 and thus is no longer use
- * @fault: call when there is a page fault to unaddressable memory
- *
- * Both callback happens from page_free() and page_fault() callback of struct
- * dev_pagemap respectively. See include/linux/memremap.h for more details on
- * those.
- *
- * The hmm_devmem_ops callback are just here to provide a coherent and
- * uniq API to device driver and device driver should not register their
- * own page_free() or page_fault() but rely on the hmm_devmem_ops call-
- * back.
- */
-struct hmm_devmem_ops {
-	/*
-	 * free() - free a device page
-	 * @devmem: device memory structure (see struct hmm_devmem)
-	 * @page: pointer to struct page being freed
-	 *
-	 * Call back occurs whenever a device page refcount reach 1 which
-	 * means that no one is holding any reference on the page anymore
-	 * (ZONE_DEVICE page have an elevated refcount of 1 as default so
-	 * that they are not release to the general page allocator).
-	 *
-	 * Note that callback has exclusive ownership of the page (as no
-	 * one is holding any reference).
-	 */
-	void (*free)(struct hmm_devmem *devmem, struct page *page);
-	/*
-	 * fault() - CPU page fault or get user page (GUP)
-	 * @devmem: device memory structure (see struct hmm_devmem)
-	 * @vma: virtual memory area containing the virtual address
-	 * @addr: virtual address that faulted or for which there is a GUP
-	 * @page: pointer to struct page backing virtual address (unreliable)
-	 * @flags: FAULT_FLAG_* (see include/linux/mm.h)
-	 * @pmdp: page middle directory
-	 * Returns: VM_FAULT_MINOR/MAJOR on success or one of VM_FAULT_ERROR
-	 *   on error
-	 *
-	 * The callback occurs whenever there is a CPU page fault or GUP on a
-	 * virtual address. This means that the device driver must migrate the
-	 * page back to regular memory (CPU accessible).
-	 *
-	 * The device driver is free to migrate more than one page from the
-	 * fault() callback as an optimization. However if device decide to
-	 * migrate more than one page it must always priotirize the faulting
-	 * address over the others.
-	 *
-	 * The struct page pointer is only given as an hint to allow quick
-	 * lookup of internal device driver data. A concurrent migration
-	 * might have already free that page and the virtual address might
-	 * not longer be back by it. So it should not be modified by the
-	 * callback.
-	 *
-	 * Note that mmap semaphore is held in read mode at least when this
-	 * callback occurs, hence the vma is valid upon callback entry.
-	 */
-	vm_fault_t (*fault)(struct hmm_devmem *devmem,
-		     struct vm_area_struct *vma,
-		     unsigned long addr,
-		     const struct page *page,
-		     unsigned int flags,
-		     pmd_t *pmdp);
-};
-
-/*
- * struct hmm_devmem - track device memory
- *
- * @completion: completion object for device memory
- * @pfn_first: first pfn for this resource (set by hmm_devmem_add())
- * @pfn_last: last pfn for this resource (set by hmm_devmem_add())
- * @resource: IO resource reserved for this chunk of memory
- * @pagemap: device page map for that chunk
- * @device: device to bind resource to
- * @ops: memory operations callback
- * @ref: per CPU refcount
- * @page_fault: callback when CPU fault on an unaddressable device page
- *
- * This an helper structure for device drivers that do not wish to implement
- * the gory details related to hotplugging new memoy and allocating struct
- * pages.
- *
- * Device drivers can directly use ZONE_DEVICE memory on their own if they
- * wish to do so.
- *
- * The page_fault() callback must migrate page back, from device memory to
- * system memory, so that the CPU can access it. This might fail for various
- * reasons (device issues,  device have been unplugged, ...). When such error
- * conditions happen, the page_fault() callback must return VM_FAULT_SIGBUS and
- * set the CPU page table entry to "poisoned".
- *
- * Note that because memory cgroup charges are transferred to the device memory,
- * this should never fail due to memory restrictions. However, allocation
- * of a regular system page might still fail because we are out of memory. If
- * that happens, the page_fault() callback must return VM_FAULT_OOM.
- *
- * The page_fault() callback can also try to migrate back multiple pages in one
- * chunk, as an optimization. It must, however, prioritize the faulting address
- * over all the others.
- */
-
-struct hmm_devmem {
-	struct completion		completion;
-	unsigned long			pfn_first;
-	unsigned long			pfn_last;
-	struct resource			*resource;
-	struct device			*device;
-	struct dev_pagemap		pagemap;
-	const struct hmm_devmem_ops	*ops;
-	struct percpu_ref		ref;
-};
-
-/*
- * To add (hotplug) device memory, HMM assumes that there is no real resource
- * that reserves a range in the physical address space (this is intended to be
- * use by unaddressable device memory). It will reserve a physical range big
- * enough and allocate struct page for it.
- *
- * The device driver can wrap the hmm_devmem struct inside a private device
- * driver struct.
- */
-struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
-				  struct device *device,
-				  unsigned long size);
-
 /*
  * hmm_devmem_page_set_drvdata - set per-page driver data field
  *
diff --git a/mm/hmm.c b/mm/hmm.c
index fdbd48771292..90ca0cdab9db 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -1327,113 +1327,3 @@ long hmm_range_dma_unmap(struct hmm_range *range,
 }
 EXPORT_SYMBOL(hmm_range_dma_unmap);
 #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
-
-
-#if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
-static void hmm_devmem_ref_release(struct percpu_ref *ref)
-{
-	struct hmm_devmem *devmem;
-
-	devmem = container_of(ref, struct hmm_devmem, ref);
-	complete(&devmem->completion);
-}
-
-static void hmm_devmem_ref_exit(struct dev_pagemap *pgmap)
-{
-	struct hmm_devmem *devmem;
-
-	devmem = container_of(pgmap, struct hmm_devmem, pagemap);
-	wait_for_completion(&devmem->completion);
-	percpu_ref_exit(pgmap->ref);
-}
-
-static void hmm_devmem_ref_kill(struct dev_pagemap *pgmap)
-{
-	percpu_ref_kill(pgmap->ref);
-}
-
-static vm_fault_t hmm_devmem_migrate_to_ram(struct vm_fault *vmf)
-{
-	struct hmm_devmem *devmem =
-		container_of(vmf->page->pgmap, struct hmm_devmem, pagemap);
-
-	return devmem->ops->fault(devmem, vmf->vma, vmf->address, vmf->page,
-			vmf->flags, vmf->pmd);
-}
-
-static void hmm_devmem_free(struct page *page)
-{
-	struct hmm_devmem *devmem =
-		container_of(page->pgmap, struct hmm_devmem, pagemap);
-
-	devmem->ops->free(devmem, page);
-}
-
-static const struct dev_pagemap_ops hmm_pagemap_ops = {
-	.page_free		= hmm_devmem_free,
-	.kill			= hmm_devmem_ref_kill,
-	.cleanup		= hmm_devmem_ref_exit,
-	.migrate_to_ram		= hmm_devmem_migrate_to_ram,
-};
-
-/*
- * hmm_devmem_add() - hotplug ZONE_DEVICE memory for device memory
- *
- * @ops: memory event device driver callback (see struct hmm_devmem_ops)
- * @device: device struct to bind the resource too
- * @size: size in bytes of the device memory to add
- * Returns: pointer to new hmm_devmem struct ERR_PTR otherwise
- *
- * This function first finds an empty range of physical address big enough to
- * contain the new resource, and then hotplugs it as ZONE_DEVICE memory, which
- * in turn allocates struct pages. It does not do anything beyond that; all
- * events affecting the memory will go through the various callbacks provided
- * by hmm_devmem_ops struct.
- *
- * Device driver should call this function during device initialization and
- * is then responsible of memory management. HMM only provides helpers.
- */
-struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
-				  struct device *device,
-				  unsigned long size)
-{
-	struct hmm_devmem *devmem;
-	void *result;
-	int ret;
-
-	devmem = devm_kzalloc(device, sizeof(*devmem), GFP_KERNEL);
-	if (!devmem)
-		return ERR_PTR(-ENOMEM);
-
-	init_completion(&devmem->completion);
-	devmem->pfn_first = -1UL;
-	devmem->pfn_last = -1UL;
-	devmem->resource = NULL;
-	devmem->device = device;
-	devmem->ops = ops;
-
-	ret = percpu_ref_init(&devmem->ref, &hmm_devmem_ref_release,
-			      0, GFP_KERNEL);
-	if (ret)
-		return ERR_PTR(ret);
-
-	devmem->resource = devm_request_free_mem_region(device, &iomem_resource,
-			size);
-	if (IS_ERR(devmem->resource))
-		return ERR_CAST(devmem->resource);
-	devmem->pfn_first = devmem->resource->start >> PAGE_SHIFT;
-	devmem->pfn_last = devmem->pfn_first +
-			   (resource_size(devmem->resource) >> PAGE_SHIFT);
-
-	devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
-	devmem->pagemap.res = *devmem->resource;
-	devmem->pagemap.ops = &hmm_pagemap_ops;
-	devmem->pagemap.ref = &devmem->ref;
-
-	result = devm_memremap_pages(devmem->device, &devmem->pagemap);
-	if (IS_ERR(result))
-		return result;
-	return devmem;
-}
-EXPORT_SYMBOL_GPL(hmm_devmem_add);
-#endif /* CONFIG_DEVICE_PRIVATE  */
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 22/25] mm: simplify ZONE_DEVICE page private data
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (20 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 21/25] mm: remove hmm_devmem_add Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 23/25] mm: sort out the DEVICE_PRIVATE Kconfig mess Christoph Hellwig
                   ` (2 subsequent siblings)
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

Remove the clumsy hmm_devmem_page_{get,set}_drvdata helpers, and
instead just access the page directly.  Also make the page data
a void pointer, and thus much easier to use.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/nouveau/nouveau_dmem.c | 18 ++++++---------
 include/linux/hmm.h                    | 32 --------------------------
 include/linux/mm_types.h               |  2 +-
 mm/page_alloc.c                        |  8 +++----
 4 files changed, 12 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_dmem.c b/drivers/gpu/drm/nouveau/nouveau_dmem.c
index 0fb7a44b8bc4..42c026010938 100644
--- a/drivers/gpu/drm/nouveau/nouveau_dmem.c
+++ b/drivers/gpu/drm/nouveau/nouveau_dmem.c
@@ -104,11 +104,8 @@ struct nouveau_migrate {
 
 static void nouveau_dmem_page_free(struct page *page)
 {
-	struct nouveau_dmem_chunk *chunk;
-	unsigned long idx;
-
-	chunk = (void *)hmm_devmem_page_get_drvdata(page);
-	idx = page_to_pfn(page) - chunk->pfn_first;
+	struct nouveau_dmem_chunk *chunk = page->zone_device_data;
+	unsigned long idx = page_to_pfn(page) - chunk->pfn_first;
 
 	/*
 	 * FIXME:
@@ -200,7 +197,7 @@ nouveau_dmem_fault_alloc_and_copy(struct vm_area_struct *vma,
 
 		dst_addr = fault->dma[fault->npages++];
 
-		chunk = (void *)hmm_devmem_page_get_drvdata(spage);
+		chunk = spage->zone_device_data;
 		src_addr = page_to_pfn(spage) - chunk->pfn_first;
 		src_addr = (src_addr << PAGE_SHIFT) + chunk->bo->bo.offset;
 
@@ -633,9 +630,8 @@ nouveau_dmem_init(struct nouveau_drm *drm)
 		list_add_tail(&chunk->list, &drm->dmem->chunk_empty);
 
 		page = pfn_to_page(chunk->pfn_first);
-		for (j = 0; j < DMEM_CHUNK_NPAGES; ++j, ++page) {
-			hmm_devmem_page_set_drvdata(page, (long)chunk);
-		}
+		for (j = 0; j < DMEM_CHUNK_NPAGES; ++j, ++page)
+			page->zone_device_data = chunk;
 	}
 
 	NV_INFO(drm, "DMEM: registered %ldMB of device memory\n", size >> 20);
@@ -698,7 +694,7 @@ nouveau_dmem_migrate_alloc_and_copy(struct vm_area_struct *vma,
 		if (!dpage || dst_pfns[i] == MIGRATE_PFN_ERROR)
 			continue;
 
-		chunk = (void *)hmm_devmem_page_get_drvdata(dpage);
+		chunk = dpage->zone_device_data;
 		dst_addr = page_to_pfn(dpage) - chunk->pfn_first;
 		dst_addr = (dst_addr << PAGE_SHIFT) + chunk->bo->bo.offset;
 
@@ -862,7 +858,7 @@ nouveau_dmem_convert_pfn(struct nouveau_drm *drm,
 			continue;
 		}
 
-		chunk = (void *)hmm_devmem_page_get_drvdata(page);
+		chunk = page->zone_device_data;
 		addr = page_to_pfn(page) - chunk->pfn_first;
 		addr = (addr + chunk->bo->bo.mem.start) << PAGE_SHIFT;
 
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 86aa4ec3404c..3d00e9550e77 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -584,36 +584,4 @@ static inline void hmm_mm_destroy(struct mm_struct *mm) {}
 static inline void hmm_mm_init(struct mm_struct *mm) {}
 #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
 
-#if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
-/*
- * hmm_devmem_page_set_drvdata - set per-page driver data field
- *
- * @page: pointer to struct page
- * @data: driver data value to set
- *
- * Because page can not be on lru we have an unsigned long that driver can use
- * to store a per page field. This just a simple helper to do that.
- */
-static inline void hmm_devmem_page_set_drvdata(struct page *page,
-					       unsigned long data)
-{
-	page->hmm_data = data;
-}
-
-/*
- * hmm_devmem_page_get_drvdata - get per page driver data field
- *
- * @page: pointer to struct page
- * Return: driver data value
- */
-static inline unsigned long hmm_devmem_page_get_drvdata(const struct page *page)
-{
-	return page->hmm_data;
-}
-#endif /* CONFIG_DEVICE_PRIVATE */
-#else /* IS_ENABLED(CONFIG_HMM) */
-static inline void hmm_mm_destroy(struct mm_struct *mm) {}
-static inline void hmm_mm_init(struct mm_struct *mm) {}
-#endif /* IS_ENABLED(CONFIG_HMM) */
-
 #endif /* LINUX_HMM_H */
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 8ec38b11b361..f33a1289c101 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -158,7 +158,7 @@ struct page {
 		struct {	/* ZONE_DEVICE pages */
 			/** @pgmap: Points to the hosting device page map. */
 			struct dev_pagemap *pgmap;
-			unsigned long hmm_data;
+			void *zone_device_data;
 			unsigned long _zd_pad_1;	/* uses mapping */
 		};
 
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 17a39d40a556..c0e031c52db5 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -5886,12 +5886,12 @@ void __ref memmap_init_zone_device(struct zone *zone,
 		__SetPageReserved(page);
 
 		/*
-		 * ZONE_DEVICE pages union ->lru with a ->pgmap back
-		 * pointer and hmm_data.  It is a bug if a ZONE_DEVICE
-		 * page is ever freed or placed on a driver-private list.
+		 * ZONE_DEVICE pages union ->lru with a ->pgmap back pointer
+		 * and zone_device_data.  It is a bug if a ZONE_DEVICE page is
+		 * ever freed or placed on a driver-private list.
 		 */
 		page->pgmap = pgmap;
-		page->hmm_data = 0;
+		page->zone_device_data = NULL;
 
 		/*
 		 * Mark the block movable so that blocks are reserved for
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 23/25] mm: sort out the DEVICE_PRIVATE Kconfig mess
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (21 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 22/25] mm: simplify ZONE_DEVICE page private data Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 21:36   ` Ira Weiny
  2019-06-26 12:27 ` [PATCH 24/25] mm: remove the HMM config option Christoph Hellwig
  2019-06-26 12:27 ` [PATCH 25/25] mm: don't select MIGRATE_VMA_HELPER from HMM_MIRROR Christoph Hellwig
  24 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

The ZONE_DEVICE support doesn't depend on anything HMM related, just on
various bits of arch support as indicated by the architecture.  Also
don't select the option from nouveau as it isn't present in many setups,
and depend on it instead.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/nouveau/Kconfig | 2 +-
 mm/Kconfig                      | 5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index dba2613f7180..6303d203ab1d 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -85,10 +85,10 @@ config DRM_NOUVEAU_BACKLIGHT
 config DRM_NOUVEAU_SVM
 	bool "(EXPERIMENTAL) Enable SVM (Shared Virtual Memory) support"
 	depends on ARCH_HAS_HMM
+	depends on DEVICE_PRIVATE
 	depends on DRM_NOUVEAU
 	depends on STAGING
 	select HMM_MIRROR
-	select DEVICE_PRIVATE
 	default n
 	help
 	  Say Y here if you want to enable experimental support for
diff --git a/mm/Kconfig b/mm/Kconfig
index 6f35b85b3052..eecf037a54b3 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -677,13 +677,13 @@ config ARCH_HAS_HMM_MIRROR
 
 config ARCH_HAS_HMM
 	bool
-	default y
 	depends on (X86_64 || PPC64)
 	depends on ZONE_DEVICE
 	depends on MMU && 64BIT
 	depends on MEMORY_HOTPLUG
 	depends on MEMORY_HOTREMOVE
 	depends on SPARSEMEM_VMEMMAP
+	default y
 
 config MIGRATE_VMA_HELPER
 	bool
@@ -709,8 +709,7 @@ config HMM_MIRROR
 
 config DEVICE_PRIVATE
 	bool "Unaddressable device memory (GPU memory, ...)"
-	depends on ARCH_HAS_HMM
-	select HMM
+	depends on ZONE_DEVICE
 	select DEV_PAGEMAP_OPS
 
 	help
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 24/25] mm: remove the HMM config option
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (22 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 23/25] mm: sort out the DEVICE_PRIVATE Kconfig mess Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  2019-06-26 21:38   ` Ira Weiny
  2019-06-27 16:29   ` Jason Gunthorpe
  2019-06-26 12:27 ` [PATCH 25/25] mm: don't select MIGRATE_VMA_HELPER from HMM_MIRROR Christoph Hellwig
  24 siblings, 2 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

All the mm/hmm.c code is better keyed off HMM_MIRROR.  Also let nouveau
depend on it instead of the mix of a dummy dependency symbol plus the
actually selected one.  Drop various odd dependencies, as the code is
pretty portable.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/gpu/drm/nouveau/Kconfig |  3 +--
 include/linux/hmm.h             |  5 +----
 include/linux/mm_types.h        |  2 +-
 mm/Kconfig                      | 27 ++++-----------------------
 mm/Makefile                     |  2 +-
 mm/hmm.c                        |  2 --
 6 files changed, 8 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 6303d203ab1d..66c839d8e9d1 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -84,11 +84,10 @@ config DRM_NOUVEAU_BACKLIGHT
 
 config DRM_NOUVEAU_SVM
 	bool "(EXPERIMENTAL) Enable SVM (Shared Virtual Memory) support"
-	depends on ARCH_HAS_HMM
 	depends on DEVICE_PRIVATE
 	depends on DRM_NOUVEAU
+	depends on HMM_MIRROR
 	depends on STAGING
-	select HMM_MIRROR
 	default n
 	help
 	  Say Y here if you want to enable experimental support for
diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index 3d00e9550e77..b697496e85ba 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -62,7 +62,7 @@
 #include <linux/kconfig.h>
 #include <asm/pgtable.h>
 
-#if IS_ENABLED(CONFIG_HMM)
+#ifdef CONFIG_HMM_MIRROR
 
 #include <linux/device.h>
 #include <linux/migrate.h>
@@ -332,9 +332,6 @@ static inline uint64_t hmm_pfn_from_pfn(const struct hmm_range *range,
 	return hmm_device_entry_from_pfn(range, pfn);
 }
 
-
-
-#if IS_ENABLED(CONFIG_HMM_MIRROR)
 /*
  * Mirroring: how to synchronize device page table with CPU page table.
  *
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index f33a1289c101..8d37182f8dbe 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -501,7 +501,7 @@ struct mm_struct {
 #endif
 		struct work_struct async_put_work;
 
-#if IS_ENABLED(CONFIG_HMM)
+#ifdef CONFIG_HMM_MIRROR
 		/* HMM needs to track a few things per mm */
 		struct hmm *hmm;
 #endif
diff --git a/mm/Kconfig b/mm/Kconfig
index eecf037a54b3..1e426c26b1d6 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -669,37 +669,18 @@ config ZONE_DEVICE
 
 	  If FS_DAX is enabled, then say Y.
 
-config ARCH_HAS_HMM_MIRROR
-	bool
-	default y
-	depends on (X86_64 || PPC64)
-	depends on MMU && 64BIT
-
-config ARCH_HAS_HMM
-	bool
-	depends on (X86_64 || PPC64)
-	depends on ZONE_DEVICE
-	depends on MMU && 64BIT
-	depends on MEMORY_HOTPLUG
-	depends on MEMORY_HOTREMOVE
-	depends on SPARSEMEM_VMEMMAP
-	default y
-
 config MIGRATE_VMA_HELPER
 	bool
 
 config DEV_PAGEMAP_OPS
 	bool
 
-config HMM
-	bool
-	select MMU_NOTIFIER
-	select MIGRATE_VMA_HELPER
-
 config HMM_MIRROR
 	bool "HMM mirror CPU page table into a device page table"
-	depends on ARCH_HAS_HMM
-	select HMM
+	depends on (X86_64 || PPC64)
+	depends on MMU && 64BIT
+	select MMU_NOTIFIER
+	select MIGRATE_VMA_HELPER
 	help
 	  Select HMM_MIRROR if you want to mirror range of the CPU page table of a
 	  process into a device page table. Here, mirror means "keep synchronized".
diff --git a/mm/Makefile b/mm/Makefile
index ac5e5ba78874..91c99040065c 100644
--- a/mm/Makefile
+++ b/mm/Makefile
@@ -102,5 +102,5 @@ obj-$(CONFIG_FRAME_VECTOR) += frame_vector.o
 obj-$(CONFIG_DEBUG_PAGE_REF) += debug_page_ref.o
 obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o
 obj-$(CONFIG_PERCPU_STATS) += percpu-stats.o
-obj-$(CONFIG_HMM) += hmm.o
+obj-$(CONFIG_HMM_MIRROR) += hmm.o
 obj-$(CONFIG_MEMFD_CREATE) += memfd.o
diff --git a/mm/hmm.c b/mm/hmm.c
index 90ca0cdab9db..d62ce64d6bca 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -25,7 +25,6 @@
 #include <linux/mmu_notifier.h>
 #include <linux/memory_hotplug.h>
 
-#if IS_ENABLED(CONFIG_HMM_MIRROR)
 static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
 
 static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
@@ -1326,4 +1325,3 @@ long hmm_range_dma_unmap(struct hmm_range *range,
 	return cpages;
 }
 EXPORT_SYMBOL(hmm_range_dma_unmap);
-#endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* [PATCH 25/25] mm: don't select MIGRATE_VMA_HELPER from HMM_MIRROR
  2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
                   ` (23 preceding siblings ...)
  2019-06-26 12:27 ` [PATCH 24/25] mm: remove the HMM config option Christoph Hellwig
@ 2019-06-26 12:27 ` Christoph Hellwig
  24 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-26 12:27 UTC (permalink / raw)
  To: Dan Williams, Jérôme Glisse, Jason Gunthorpe, Ben Skeggs
  Cc: linux-mm, nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

The migrate_vma helper is only used by noveau to migrate device private
pages around.  Other HMM_MIRROR users like amdgpu or infiniband don't
need it.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
---
 drivers/gpu/drm/nouveau/Kconfig | 1 +
 mm/Kconfig                      | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
index 66c839d8e9d1..96b9814e6d06 100644
--- a/drivers/gpu/drm/nouveau/Kconfig
+++ b/drivers/gpu/drm/nouveau/Kconfig
@@ -88,6 +88,7 @@ config DRM_NOUVEAU_SVM
 	depends on DRM_NOUVEAU
 	depends on HMM_MIRROR
 	depends on STAGING
+	select MIGRATE_VMA_HELPER
 	default n
 	help
 	  Say Y here if you want to enable experimental support for
diff --git a/mm/Kconfig b/mm/Kconfig
index 1e426c26b1d6..40cf0562412d 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -680,7 +680,6 @@ config HMM_MIRROR
 	depends on (X86_64 || PPC64)
 	depends on MMU && 64BIT
 	select MMU_NOTIFIER
-	select MIGRATE_VMA_HELPER
 	help
 	  Select HMM_MIRROR if you want to mirror range of the CPU page table of a
 	  process into a device page table. Here, mirror means "keep synchronized".
-- 
2.20.1


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 06/25] mm: export alloc_pages_vma
  2019-06-26 12:27 ` [PATCH 06/25] mm: export alloc_pages_vma Christoph Hellwig
@ 2019-06-26 12:36   ` Michal Hocko
  0 siblings, 0 replies; 59+ messages in thread
From: Michal Hocko @ 2019-06-26 12:36 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, linux-mm, nouveau, dri-devel, linux-nvdimm,
	linux-pci, linux-kernel, John Hubbard

On Wed 26-06-19 14:27:05, Christoph Hellwig wrote:
> nouveau is currently using this through an odd hmm wrapper, and I plan
> to switch it to the real thing later in this series.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/mempolicy.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 01600d80ae01..f48569aa1863 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -2098,6 +2098,7 @@ alloc_pages_vma(gfp_t gfp, int order, struct vm_area_struct *vma,
>  out:
>  	return page;
>  }
> +EXPORT_SYMBOL(alloc_pages_vma);
>  
>  /**
>   * 	alloc_pages_current - Allocate pages.
> -- 
> 2.20.1

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 04/25] mm: remove MEMORY_DEVICE_PUBLIC support
  2019-06-26 12:27 ` [PATCH 04/25] mm: remove MEMORY_DEVICE_PUBLIC support Christoph Hellwig
@ 2019-06-26 16:00   ` Dan Williams
  2019-06-26 17:14     ` Ira Weiny
  0 siblings, 1 reply; 59+ messages in thread
From: Dan Williams @ 2019-06-26 16:00 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jérôme Glisse, Jason Gunthorpe, Ben Skeggs, Linux MM,
	nouveau, Maling list - DRI developers, linux-nvdimm, linux-pci,
	Linux Kernel Mailing List, Michal Hocko, Weiny, Ira

[ add Ira ]

On Wed, Jun 26, 2019 at 5:27 AM Christoph Hellwig <hch@lst.de> wrote:
>
> The code hasn't been used since it was added to the tree, and doesn't
> appear to actually be usable.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
[..]
> diff --git a/mm/swap.c b/mm/swap.c
> index 7ede3eddc12a..83107410d29f 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -740,17 +740,6 @@ void release_pages(struct page **pages, int nr)
>                 if (is_huge_zero_page(page))
>                         continue;
>
> -               /* Device public page can not be huge page */
> -               if (is_device_public_page(page)) {
> -                       if (locked_pgdat) {
> -                               spin_unlock_irqrestore(&locked_pgdat->lru_lock,
> -                                                      flags);
> -                               locked_pgdat = NULL;
> -                       }
> -                       put_devmap_managed_page(page);
> -                       continue;
> -               }
> -

This collides with Ira's bug fix [1]. The MEMORY_DEVICE_FSDAX case
needs this to be converted to be independent of "public" pages.
Perhaps it should be pulled out of -mm and incorporated in this
series.

[1]: https://lore.kernel.org/lkml/20190605214922.17684-1-ira.weiny@intel.com/

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 04/25] mm: remove MEMORY_DEVICE_PUBLIC support
  2019-06-26 16:00   ` Dan Williams
@ 2019-06-26 17:14     ` Ira Weiny
  2019-06-26 18:49       ` Ira Weiny
  0 siblings, 1 reply; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 17:14 UTC (permalink / raw)
  To: Dan Williams
  Cc: Christoph Hellwig, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, Linux MM, nouveau, Maling list - DRI developers,
	linux-nvdimm, linux-pci, Linux Kernel Mailing List, Michal Hocko

On Wed, Jun 26, 2019 at 09:00:47AM -0700, Dan Williams wrote:
> [ add Ira ]
> 
> On Wed, Jun 26, 2019 at 5:27 AM Christoph Hellwig <hch@lst.de> wrote:
> >
> > The code hasn't been used since it was added to the tree, and doesn't
> > appear to actually be usable.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> > Acked-by: Michal Hocko <mhocko@suse.com>
> [..]
> > diff --git a/mm/swap.c b/mm/swap.c
> > index 7ede3eddc12a..83107410d29f 100644
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -740,17 +740,6 @@ void release_pages(struct page **pages, int nr)
> >                 if (is_huge_zero_page(page))
> >                         continue;
> >
> > -               /* Device public page can not be huge page */
> > -               if (is_device_public_page(page)) {
> > -                       if (locked_pgdat) {
> > -                               spin_unlock_irqrestore(&locked_pgdat->lru_lock,
> > -                                                      flags);
> > -                               locked_pgdat = NULL;
> > -                       }
> > -                       put_devmap_managed_page(page);
> > -                       continue;
> > -               }
> > -
> 
> This collides with Ira's bug fix [1]. The MEMORY_DEVICE_FSDAX case
> needs this to be converted to be independent of "public" pages.
> Perhaps it should be pulled out of -mm and incorporated in this
> series.
> 
> [1]: https://lore.kernel.org/lkml/20190605214922.17684-1-ira.weiny@intel.com/

Agreed and Andrew picked the first 2 versions of it, mmotm commits:

3eed114b5b6b mm-swap-fix-release_pages-when-releasing-devmap-pages-v2
9b7d8d0f572f mm/swap.c: fix release_pages() when releasing devmap pages

I don't see v3 but there were no objections...

Ira


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 08/25] memremap: validate the pagemap type passed to devm_memremap_pages
  2019-06-26 12:27 ` [PATCH 08/25] memremap: validate the pagemap type passed to devm_memremap_pages Christoph Hellwig
@ 2019-06-26 18:01   ` Ira Weiny
  0 siblings, 0 replies; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 18:01 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, linux-nvdimm, linux-pci, linux-kernel, dri-devel,
	linux-mm, nouveau

On Wed, Jun 26, 2019 at 02:27:07PM +0200, Christoph Hellwig wrote:
> Most pgmap types are only supported when certain config options are
> enabled.  Check for a type that is valid for the current configuration
> before setting up the pagemap.  For this the usage of the 0 type for
> device dax gets replaced with an explicit MEMORY_DEVICE_DEVDAX type.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/dax/device.c     |  1 +
>  include/linux/memremap.h |  8 ++++++++
>  kernel/memremap.c        | 22 ++++++++++++++++++++++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index 8465d12fecba..79014baa782d 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -468,6 +468,7 @@ int dev_dax_probe(struct device *dev)
>  	dev_dax->pgmap.ref = &dev_dax->ref;
>  	dev_dax->pgmap.kill = dev_dax_percpu_kill;
>  	dev_dax->pgmap.cleanup = dev_dax_percpu_exit;
> +	dev_dax->pgmap.type = MEMORY_DEVICE_DEVDAX;
>  	addr = devm_memremap_pages(dev, &dev_dax->pgmap);
>  	if (IS_ERR(addr))
>  		return PTR_ERR(addr);
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index 995c62c5a48b..0c86f2c5ac9c 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -45,13 +45,21 @@ struct vmem_altmap {
>   * wakeup is used to coordinate physical address space management (ex:
>   * fs truncate/hole punch) vs pinned pages (ex: device dma).
>   *
> + * MEMORY_DEVICE_DEVDAX:
> + * Host memory that has similar access semantics as System RAM i.e. DMA
> + * coherent and supports page pinning. In contrast to
> + * MEMORY_DEVICE_FS_DAX, this memory is access via a device-dax
> + * character device.
> + *
>   * MEMORY_DEVICE_PCI_P2PDMA:
>   * Device memory residing in a PCI BAR intended for use with Peer-to-Peer
>   * transactions.
>   */
>  enum memory_type {
> +	/* 0 is reserved to catch uninitialized type fields */
>  	MEMORY_DEVICE_PRIVATE = 1,
>  	MEMORY_DEVICE_FS_DAX,
> +	MEMORY_DEVICE_DEVDAX,
>  	MEMORY_DEVICE_PCI_P2PDMA,
>  };
>  
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index 6e1970719dc2..abda62d1e5a3 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -157,6 +157,28 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  	pgprot_t pgprot = PAGE_KERNEL;
>  	int error, nid, is_ram;
>  
> +	switch (pgmap->type) {
> +	case MEMORY_DEVICE_PRIVATE:
> +		if (!IS_ENABLED(CONFIG_DEVICE_PRIVATE)) {
> +			WARN(1, "Device private memory not supported\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +		break;
> +	case MEMORY_DEVICE_FS_DAX:
> +		if (!IS_ENABLED(CONFIG_ZONE_DEVICE) ||
> +		    IS_ENABLED(CONFIG_FS_DAX_LIMITED)) {
> +			WARN(1, "File system DAX not supported\n");
> +			return ERR_PTR(-EINVAL);
> +		}
> +		break;
> +	case MEMORY_DEVICE_DEVDAX:
> +	case MEMORY_DEVICE_PCI_P2PDMA:
> +		break;
> +	default:
> +		WARN(1, "Invalid pgmap type %d\n", pgmap->type);
> +		break;
> +	}
> +
>  	if (!pgmap->ref || !pgmap->kill || !pgmap->cleanup) {
>  		WARN(1, "Missing reference count teardown definition\n");
>  		return ERR_PTR(-EINVAL);
> -- 
> 2.20.1
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 04/25] mm: remove MEMORY_DEVICE_PUBLIC support
  2019-06-26 17:14     ` Ira Weiny
@ 2019-06-26 18:49       ` Ira Weiny
  0 siblings, 0 replies; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 18:49 UTC (permalink / raw)
  To: Dan Williams
  Cc: Christoph Hellwig, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, Linux MM, nouveau, Maling list - DRI developers,
	linux-nvdimm, linux-pci, Linux Kernel Mailing List, Michal Hocko

On Wed, Jun 26, 2019 at 10:14:45AM -0700, 'Ira Weiny' wrote:
> On Wed, Jun 26, 2019 at 09:00:47AM -0700, Dan Williams wrote:
> > [ add Ira ]
> > 
> > On Wed, Jun 26, 2019 at 5:27 AM Christoph Hellwig <hch@lst.de> wrote:
> > >
> > > The code hasn't been used since it was added to the tree, and doesn't
> > > appear to actually be usable.
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> > > Acked-by: Michal Hocko <mhocko@suse.com>
> > [..]
> > > diff --git a/mm/swap.c b/mm/swap.c
> > > index 7ede3eddc12a..83107410d29f 100644
> > > --- a/mm/swap.c
> > > +++ b/mm/swap.c
> > > @@ -740,17 +740,6 @@ void release_pages(struct page **pages, int nr)
> > >                 if (is_huge_zero_page(page))
> > >                         continue;
> > >
> > > -               /* Device public page can not be huge page */
> > > -               if (is_device_public_page(page)) {
> > > -                       if (locked_pgdat) {
> > > -                               spin_unlock_irqrestore(&locked_pgdat->lru_lock,
> > > -                                                      flags);
> > > -                               locked_pgdat = NULL;
> > > -                       }
> > > -                       put_devmap_managed_page(page);
> > > -                       continue;
> > > -               }
> > > -
> > 
> > This collides with Ira's bug fix [1]. The MEMORY_DEVICE_FSDAX case
> > needs this to be converted to be independent of "public" pages.
> > Perhaps it should be pulled out of -mm and incorporated in this
> > series.
> > 
> > [1]: https://lore.kernel.org/lkml/20190605214922.17684-1-ira.weiny@intel.com/
> 
> Agreed and Andrew picked the first 2 versions of it, mmotm commits:
> 
> 3eed114b5b6b mm-swap-fix-release_pages-when-releasing-devmap-pages-v2
> 9b7d8d0f572f mm/swap.c: fix release_pages() when releasing devmap pages
> 
> I don't see v3 but there were no objections...

Ok somehow I can't fetch mmotm right now...

Dan had and updated mmotm tree and it does have my v4 patch.

Does anyone else have issues with git://git.cmpxchg.org/linux-mmotm.git or is
it just me?  FWIW I have checked proxies etc... and can get to linus and other
sites just fine, so it looks like an issue there.  Although the web page is
fine...

Sorry,
Ira


^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 11/25] memremap: lift the devmap_enable manipulation into devm_memremap_pages
  2019-06-26 12:27 ` [PATCH 11/25] memremap: lift the devmap_enable manipulation into devm_memremap_pages Christoph Hellwig
@ 2019-06-26 19:04   ` Ira Weiny
  2019-06-27  8:50     ` Christoph Hellwig
  0 siblings, 1 reply; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 19:04 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, linux-nvdimm, linux-pci, linux-kernel, dri-devel,
	linux-mm, nouveau

On Wed, Jun 26, 2019 at 02:27:10PM +0200, Christoph Hellwig wrote:
> Just check if there is a ->page_free operation set and take care of the
> static key enable, as well as the put using device managed resources.
> Also check that a ->page_free is provided for the pgmaps types that
> require it, and check for a valid type as well while we are at it.
> 
> Note that this also fixes the fact that hmm never called
> dev_pagemap_put_ops and thus would leave the slow path enabled forever,
> even after a device driver unload or disable.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/nvdimm/pmem.c | 23 +++--------------
>  include/linux/mm.h    | 10 --------
>  kernel/memremap.c     | 59 +++++++++++++++++++++++++++----------------
>  mm/hmm.c              |  2 --
>  4 files changed, 41 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 9dac48359353..48767171a4df 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -334,11 +334,6 @@ static void pmem_release_disk(void *__pmem)
>  	put_disk(pmem->disk);
>  }
>  
> -static void pmem_release_pgmap_ops(void *__pgmap)
> -{
> -	dev_pagemap_put_ops();
> -}
> -
>  static void pmem_pagemap_page_free(struct page *page, void *data)
>  {
>  	wake_up_var(&page->_refcount);
> @@ -350,16 +345,6 @@ static const struct dev_pagemap_ops fsdax_pagemap_ops = {
>  	.cleanup		= pmem_pagemap_cleanup,
>  };
>  
> -static int setup_pagemap_fsdax(struct device *dev, struct dev_pagemap *pgmap)
> -{
> -	dev_pagemap_get_ops();
> -	if (devm_add_action_or_reset(dev, pmem_release_pgmap_ops, pgmap))
> -		return -ENOMEM;
> -	pgmap->type = MEMORY_DEVICE_FS_DAX;
> -	pgmap->ops = &fsdax_pagemap_ops;
> -	return 0;
> -}
> -
>  static int pmem_attach_disk(struct device *dev,
>  		struct nd_namespace_common *ndns)
>  {
> @@ -415,8 +400,8 @@ static int pmem_attach_disk(struct device *dev,
>  	pmem->pfn_flags = PFN_DEV;
>  	pmem->pgmap.ref = &q->q_usage_counter;
>  	if (is_nd_pfn(dev)) {
> -		if (setup_pagemap_fsdax(dev, &pmem->pgmap))
> -			return -ENOMEM;
> +		pmem->pgmap.type = MEMORY_DEVICE_FS_DAX;
> +		pmem->pgmap.ops = &fsdax_pagemap_ops;
>  		addr = devm_memremap_pages(dev, &pmem->pgmap);
>  		pfn_sb = nd_pfn->pfn_sb;
>  		pmem->data_offset = le64_to_cpu(pfn_sb->dataoff);
> @@ -428,8 +413,8 @@ static int pmem_attach_disk(struct device *dev,
>  	} else if (pmem_should_map_pages(dev)) {
>  		memcpy(&pmem->pgmap.res, &nsio->res, sizeof(pmem->pgmap.res));
>  		pmem->pgmap.altmap_valid = false;
> -		if (setup_pagemap_fsdax(dev, &pmem->pgmap))
> -			return -ENOMEM;
> +		pmem->pgmap.type = MEMORY_DEVICE_FS_DAX;
> +		pmem->pgmap.ops = &fsdax_pagemap_ops;
>  		addr = devm_memremap_pages(dev, &pmem->pgmap);
>  		pmem->pfn_flags |= PFN_MAP;
>  		memcpy(&bb_res, &pmem->pgmap.res, sizeof(bb_res));
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 6e4b9be08b13..aa3970291cdf 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -932,8 +932,6 @@ static inline bool is_zone_device_page(const struct page *page)
>  #endif
>  
>  #ifdef CONFIG_DEV_PAGEMAP_OPS
> -void dev_pagemap_get_ops(void);
> -void dev_pagemap_put_ops(void);
>  void __put_devmap_managed_page(struct page *page);
>  DECLARE_STATIC_KEY_FALSE(devmap_managed_key);
>  static inline bool put_devmap_managed_page(struct page *page)
> @@ -973,14 +971,6 @@ static inline bool is_pci_p2pdma_page(const struct page *page)
>  #endif /* CONFIG_PCI_P2PDMA */
>  
>  #else /* CONFIG_DEV_PAGEMAP_OPS */
> -static inline void dev_pagemap_get_ops(void)
> -{
> -}
> -
> -static inline void dev_pagemap_put_ops(void)
> -{
> -}
> -
>  static inline bool put_devmap_managed_page(struct page *page)
>  {
>  	return false;
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index 00c1ceb60c19..3219a4c91d07 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -17,6 +17,35 @@ static DEFINE_XARRAY(pgmap_array);
>  #define SECTION_MASK ~((1UL << PA_SECTION_SHIFT) - 1)
>  #define SECTION_SIZE (1UL << PA_SECTION_SHIFT)
>  
> +#ifdef CONFIG_DEV_PAGEMAP_OPS
> +DEFINE_STATIC_KEY_FALSE(devmap_managed_key);
> +EXPORT_SYMBOL(devmap_managed_key);
> +static atomic_t devmap_managed_enable;
> +
> +static void devmap_managed_enable_put(void *data)
> +{
> +	if (atomic_dec_and_test(&devmap_managed_enable))
> +		static_branch_disable(&devmap_managed_key);
> +}
> +
> +static int devmap_managed_enable_get(struct device *dev, struct dev_pagemap *pgmap)
> +{
> +	if (!pgmap->ops->page_free) {

NIT: later on you add the check for pgmap->ops...  it should probably be here.

But not sure that bisection will be an issue here.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> +		WARN(1, "Missing page_free method\n");
> +		return -EINVAL;
> +	}
> +
> +	if (atomic_inc_return(&devmap_managed_enable) == 1)
> +		static_branch_enable(&devmap_managed_key);
> +	return devm_add_action_or_reset(dev, devmap_managed_enable_put, NULL);
> +}
> +#else
> +static int devmap_managed_enable_get(struct device *dev, struct dev_pagemap *pgmap)
> +{
> +	return -EINVAL;
> +}
> +#endif /* CONFIG_DEV_PAGEMAP_OPS */
> +
>  #if IS_ENABLED(CONFIG_DEVICE_PRIVATE)
>  vm_fault_t device_private_entry_fault(struct vm_area_struct *vma,
>  		       unsigned long addr,
> @@ -156,6 +185,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  	};
>  	pgprot_t pgprot = PAGE_KERNEL;
>  	int error, nid, is_ram;
> +	bool need_devmap_managed = true;
>  
>  	switch (pgmap->type) {
>  	case MEMORY_DEVICE_PRIVATE:
> @@ -173,6 +203,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  		break;
>  	case MEMORY_DEVICE_DEVDAX:
>  	case MEMORY_DEVICE_PCI_P2PDMA:
> +		need_devmap_managed = false;
>  		break;
>  	default:
>  		WARN(1, "Invalid pgmap type %d\n", pgmap->type);
> @@ -185,6 +216,12 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> +	if (need_devmap_managed) {
> +		error = devmap_managed_enable_get(dev, pgmap);
> +		if (error)
> +			return ERR_PTR(error);
> +	}
> +
>  	align_start = res->start & ~(SECTION_SIZE - 1);
>  	align_size = ALIGN(res->start + resource_size(res), SECTION_SIZE)
>  		- align_start;
> @@ -351,28 +388,6 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn,
>  EXPORT_SYMBOL_GPL(get_dev_pagemap);
>  
>  #ifdef CONFIG_DEV_PAGEMAP_OPS
> -DEFINE_STATIC_KEY_FALSE(devmap_managed_key);
> -EXPORT_SYMBOL(devmap_managed_key);
> -static atomic_t devmap_enable;
> -
> -/*
> - * Toggle the static key for ->page_free() callbacks when dev_pagemap
> - * pages go idle.
> - */
> -void dev_pagemap_get_ops(void)
> -{
> -	if (atomic_inc_return(&devmap_enable) == 1)
> -		static_branch_enable(&devmap_managed_key);
> -}
> -EXPORT_SYMBOL_GPL(dev_pagemap_get_ops);
> -
> -void dev_pagemap_put_ops(void)
> -{
> -	if (atomic_dec_and_test(&devmap_enable))
> -		static_branch_disable(&devmap_managed_key);
> -}
> -EXPORT_SYMBOL_GPL(dev_pagemap_put_ops);
> -
>  void __put_devmap_managed_page(struct page *page)
>  {
>  	int count = page_ref_dec_return(page);
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 987793fba923..5b0bd5f6a74f 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -1415,8 +1415,6 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
>  	void *result;
>  	int ret;
>  
> -	dev_pagemap_get_ops();
> -
>  	devmem = devm_kzalloc(device, sizeof(*devmem), GFP_KERNEL);
>  	if (!devmem)
>  		return ERR_PTR(-ENOMEM);
> -- 
> 2.20.1
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 14/25] memremap: replace the altmap_valid field with a PGMAP_ALTMAP_VALID flag
  2019-06-26 12:27 ` [PATCH 14/25] memremap: replace the altmap_valid field with a PGMAP_ALTMAP_VALID flag Christoph Hellwig
@ 2019-06-26 21:13   ` Ira Weiny
  0 siblings, 0 replies; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 21:13 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, linux-nvdimm, linux-pci, linux-kernel, dri-devel,
	linux-mm, nouveau

On Wed, Jun 26, 2019 at 02:27:13PM +0200, Christoph Hellwig wrote:
> Add a flags field to struct dev_pagemap to replace the altmap_valid
> boolean to be a little more extensible.  Also add a pgmap_altmap() helper
> to find the optional altmap and clean up the code using the altmap using
> it.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  arch/powerpc/mm/mem.c     | 10 +---------
>  arch/x86/mm/init_64.c     |  8 ++------
>  drivers/nvdimm/pfn_devs.c |  3 +--
>  drivers/nvdimm/pmem.c     |  1 -
>  include/linux/memremap.h  | 12 +++++++++++-
>  kernel/memremap.c         | 26 ++++++++++----------------
>  mm/hmm.c                  |  1 -
>  mm/memory_hotplug.c       |  6 ++----
>  mm/page_alloc.c           |  5 ++---
>  9 files changed, 29 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
> index cba29131bccc..f774d80df025 100644
> --- a/arch/powerpc/mm/mem.c
> +++ b/arch/powerpc/mm/mem.c
> @@ -131,17 +131,9 @@ void __ref arch_remove_memory(int nid, u64 start, u64 size,
>  {
>  	unsigned long start_pfn = start >> PAGE_SHIFT;
>  	unsigned long nr_pages = size >> PAGE_SHIFT;
> -	struct page *page;
> +	struct page *page = pfn_to_page(start_pfn) + vmem_altmap_offset(altmap);
>  	int ret;
>  
> -	/*
> -	 * If we have an altmap then we need to skip over any reserved PFNs
> -	 * when querying the zone.
> -	 */
> -	page = pfn_to_page(start_pfn);
> -	if (altmap)
> -		page += vmem_altmap_offset(altmap);
> -
>  	__remove_pages(page_zone(page), start_pfn, nr_pages, altmap);
>  
>  	/* Remove htab bolted mappings for this section of memory */
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 693aaf28d5fe..3139e992ef9d 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -1211,13 +1211,9 @@ void __ref arch_remove_memory(int nid, u64 start, u64 size,
>  {
>  	unsigned long start_pfn = start >> PAGE_SHIFT;
>  	unsigned long nr_pages = size >> PAGE_SHIFT;
> -	struct page *page = pfn_to_page(start_pfn);
> -	struct zone *zone;
> +	struct page *page = pfn_to_page(start_pfn) + vmem_altmap_offset(altmap);
> +	struct zone *zone = page_zone(page);
>  
> -	/* With altmap the first mapped page is offset from @start */
> -	if (altmap)
> -		page += vmem_altmap_offset(altmap);
> -	zone = page_zone(page);
>  	__remove_pages(zone, start_pfn, nr_pages, altmap);
>  	kernel_physical_mapping_remove(start, start + size);
>  }
> diff --git a/drivers/nvdimm/pfn_devs.c b/drivers/nvdimm/pfn_devs.c
> index 0f81fc56bbfd..55fb6b7433ed 100644
> --- a/drivers/nvdimm/pfn_devs.c
> +++ b/drivers/nvdimm/pfn_devs.c
> @@ -622,7 +622,6 @@ static int __nvdimm_setup_pfn(struct nd_pfn *nd_pfn, struct dev_pagemap *pgmap)
>  		if (offset < reserve)
>  			return -EINVAL;
>  		nd_pfn->npfns = le64_to_cpu(pfn_sb->npfns);
> -		pgmap->altmap_valid = false;
>  	} else if (nd_pfn->mode == PFN_MODE_PMEM) {
>  		nd_pfn->npfns = PFN_SECTION_ALIGN_UP((resource_size(res)
>  					- offset) / PAGE_SIZE);
> @@ -634,7 +633,7 @@ static int __nvdimm_setup_pfn(struct nd_pfn *nd_pfn, struct dev_pagemap *pgmap)
>  		memcpy(altmap, &__altmap, sizeof(*altmap));
>  		altmap->free = PHYS_PFN(offset - reserve);
>  		altmap->alloc = 0;
> -		pgmap->altmap_valid = true;
> +		pgmap->flags |= PGMAP_ALTMAP_VALID;
>  	} else
>  		return -ENXIO;
>  
> diff --git a/drivers/nvdimm/pmem.c b/drivers/nvdimm/pmem.c
> index 093408ce40ad..e7d8cc9f41e8 100644
> --- a/drivers/nvdimm/pmem.c
> +++ b/drivers/nvdimm/pmem.c
> @@ -412,7 +412,6 @@ static int pmem_attach_disk(struct device *dev,
>  		bb_res.start += pmem->data_offset;
>  	} else if (pmem_should_map_pages(dev)) {
>  		memcpy(&pmem->pgmap.res, &nsio->res, sizeof(pmem->pgmap.res));
> -		pmem->pgmap.altmap_valid = false;
>  		pmem->pgmap.type = MEMORY_DEVICE_FS_DAX;
>  		pmem->pgmap.ops = &fsdax_pagemap_ops;
>  		addr = devm_memremap_pages(dev, &pmem->pgmap);
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index 336eca601dad..e25685b878e9 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -88,6 +88,8 @@ struct dev_pagemap_ops {
>  	vm_fault_t (*migrate_to_ram)(struct vm_fault *vmf);
>  };
>  
> +#define PGMAP_ALTMAP_VALID	(1 << 0)
> +
>  /**
>   * struct dev_pagemap - metadata for ZONE_DEVICE mappings
>   * @altmap: pre-allocated/reserved memory for vmemmap allocations
> @@ -96,19 +98,27 @@ struct dev_pagemap_ops {
>   * @dev: host device of the mapping for debug
>   * @data: private data pointer for page_free()
>   * @type: memory type: see MEMORY_* in memory_hotplug.h
> + * @flags: PGMAP_* flags to specify defailed behavior
>   * @ops: method table
>   */
>  struct dev_pagemap {
>  	struct vmem_altmap altmap;
> -	bool altmap_valid;
>  	struct resource res;
>  	struct percpu_ref *ref;
>  	struct device *dev;
>  	enum memory_type type;
> +	unsigned int flags;
>  	u64 pci_p2pdma_bus_offset;
>  	const struct dev_pagemap_ops *ops;
>  };
>  
> +static inline struct vmem_altmap *pgmap_altmap(struct dev_pagemap *pgmap)
> +{
> +	if (pgmap->flags & PGMAP_ALTMAP_VALID)
> +		return &pgmap->altmap;
> +	return NULL;
> +}
> +
>  #ifdef CONFIG_ZONE_DEVICE
>  void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap);
>  void devm_memunmap_pages(struct device *dev, struct dev_pagemap *pgmap);
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index 6c3dbb692037..eee490e7d7e1 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -54,14 +54,8 @@ static void pgmap_array_delete(struct resource *res)
>  
>  static unsigned long pfn_first(struct dev_pagemap *pgmap)
>  {
> -	const struct resource *res = &pgmap->res;
> -	struct vmem_altmap *altmap = &pgmap->altmap;
> -	unsigned long pfn;
> -
> -	pfn = res->start >> PAGE_SHIFT;
> -	if (pgmap->altmap_valid)
> -		pfn += vmem_altmap_offset(altmap);
> -	return pfn;
> +	return (pgmap->res.start >> PAGE_SHIFT) +
> +		vmem_altmap_offset(pgmap_altmap(pgmap));
>  }
>  
>  static unsigned long pfn_end(struct dev_pagemap *pgmap)
> @@ -109,7 +103,7 @@ static void devm_memremap_pages_release(void *data)
>  				align_size >> PAGE_SHIFT, NULL);
>  	} else {
>  		arch_remove_memory(nid, align_start, align_size,
> -				pgmap->altmap_valid ? &pgmap->altmap : NULL);
> +				pgmap_altmap(pgmap));
>  		kasan_remove_zero_shadow(__va(align_start), align_size);
>  	}
>  	mem_hotplug_done();
> @@ -129,8 +123,8 @@ static void devm_memremap_pages_release(void *data)
>   * 1/ At a minimum the res, ref and type and ops members of @pgmap must be
>   *    initialized by the caller before passing it to this function
>   *
> - * 2/ The altmap field may optionally be initialized, in which case altmap_valid
> - *    must be set to true
> + * 2/ The altmap field may optionally be initialized, in which case
> + *    PGMAP_ALTMAP_VALID must be set in pgmap->flags.
>   *
>   * 3/ pgmap->ref must be 'live' on entry and will be killed and reaped
>   *    at devm_memremap_pages_release() time, or if this routine fails.
> @@ -142,15 +136,13 @@ static void devm_memremap_pages_release(void *data)
>  void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  {
>  	resource_size_t align_start, align_size, align_end;
> -	struct vmem_altmap *altmap = pgmap->altmap_valid ?
> -			&pgmap->altmap : NULL;
>  	struct resource *res = &pgmap->res;
>  	struct dev_pagemap *conflict_pgmap;
>  	struct mhp_restrictions restrictions = {
>  		/*
>  		 * We do not want any optional features only our own memmap
>  		*/
> -		.altmap = altmap,
> +		.altmap = pgmap_altmap(pgmap),
>  	};
>  	pgprot_t pgprot = PAGE_KERNEL;
>  	int error, nid, is_ram;
> @@ -274,7 +266,7 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  
>  		zone = &NODE_DATA(nid)->node_zones[ZONE_DEVICE];
>  		move_pfn_range_to_zone(zone, align_start >> PAGE_SHIFT,
> -				align_size >> PAGE_SHIFT, altmap);
> +				align_size >> PAGE_SHIFT, pgmap_altmap(pgmap));
>  	}
>  
>  	mem_hotplug_done();
> @@ -319,7 +311,9 @@ EXPORT_SYMBOL_GPL(devm_memunmap_pages);
>  unsigned long vmem_altmap_offset(struct vmem_altmap *altmap)
>  {
>  	/* number of pfns from base where pfn_to_page() is valid */
> -	return altmap->reserve + altmap->free;
> +	if (altmap)
> +		return altmap->reserve + altmap->free;
> +	return 0;
>  }
>  
>  void vmem_altmap_free(struct vmem_altmap *altmap, unsigned long nr_pfns)
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 36e25cdbdac1..e4470462298f 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -1442,7 +1442,6 @@ struct hmm_devmem *hmm_devmem_add(const struct hmm_devmem_ops *ops,
>  	devmem->pagemap.type = MEMORY_DEVICE_PRIVATE;
>  	devmem->pagemap.res = *devmem->resource;
>  	devmem->pagemap.ops = &hmm_pagemap_ops;
> -	devmem->pagemap.altmap_valid = false;
>  	devmem->pagemap.ref = &devmem->ref;
>  
>  	result = devm_memremap_pages(devmem->device, &devmem->pagemap);
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index e096c987d261..6166ba5a15f3 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -557,10 +557,8 @@ void __remove_pages(struct zone *zone, unsigned long phys_start_pfn,
>  	int sections_to_remove;
>  
>  	/* In the ZONE_DEVICE case device driver owns the memory region */
> -	if (is_dev_zone(zone)) {
> -		if (altmap)
> -			map_offset = vmem_altmap_offset(altmap);
> -	}
> +	if (is_dev_zone(zone))
> +		map_offset = vmem_altmap_offset(altmap);
>  
>  	clear_zone_contiguous(zone);
>  
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d66bc8abe0af..17a39d40a556 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -5853,6 +5853,7 @@ void __ref memmap_init_zone_device(struct zone *zone,
>  {
>  	unsigned long pfn, end_pfn = start_pfn + size;
>  	struct pglist_data *pgdat = zone->zone_pgdat;
> +	struct vmem_altmap *altmap = pgmap_altmap(pgmap);
>  	unsigned long zone_idx = zone_idx(zone);
>  	unsigned long start = jiffies;
>  	int nid = pgdat->node_id;
> @@ -5865,9 +5866,7 @@ void __ref memmap_init_zone_device(struct zone *zone,
>  	 * of the pages reserved for the memmap, so we can just jump to
>  	 * the end of that region and start processing the device pages.
>  	 */
> -	if (pgmap->altmap_valid) {
> -		struct vmem_altmap *altmap = &pgmap->altmap;
> -
> +	if (altmap) {
>  		start_pfn = altmap->base_pfn + vmem_altmap_offset(altmap);
>  		size = end_pfn - start_pfn;
>  	}
> -- 
> 2.20.1
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 23/25] mm: sort out the DEVICE_PRIVATE Kconfig mess
  2019-06-26 12:27 ` [PATCH 23/25] mm: sort out the DEVICE_PRIVATE Kconfig mess Christoph Hellwig
@ 2019-06-26 21:36   ` Ira Weiny
  0 siblings, 0 replies; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 21:36 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, linux-mm, nouveau, dri-devel, linux-nvdimm,
	linux-pci, linux-kernel

On Wed, Jun 26, 2019 at 02:27:22PM +0200, Christoph Hellwig wrote:
> The ZONE_DEVICE support doesn't depend on anything HMM related, just on
> various bits of arch support as indicated by the architecture.  Also
> don't select the option from nouveau as it isn't present in many setups,
> and depend on it instead.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/gpu/drm/nouveau/Kconfig | 2 +-
>  mm/Kconfig                      | 5 ++---
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> index dba2613f7180..6303d203ab1d 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -85,10 +85,10 @@ config DRM_NOUVEAU_BACKLIGHT
>  config DRM_NOUVEAU_SVM
>  	bool "(EXPERIMENTAL) Enable SVM (Shared Virtual Memory) support"
>  	depends on ARCH_HAS_HMM
> +	depends on DEVICE_PRIVATE
>  	depends on DRM_NOUVEAU
>  	depends on STAGING
>  	select HMM_MIRROR
> -	select DEVICE_PRIVATE
>  	default n
>  	help
>  	  Say Y here if you want to enable experimental support for
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 6f35b85b3052..eecf037a54b3 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -677,13 +677,13 @@ config ARCH_HAS_HMM_MIRROR
>  
>  config ARCH_HAS_HMM
>  	bool
> -	default y
>  	depends on (X86_64 || PPC64)
>  	depends on ZONE_DEVICE
>  	depends on MMU && 64BIT
>  	depends on MEMORY_HOTPLUG
>  	depends on MEMORY_HOTREMOVE
>  	depends on SPARSEMEM_VMEMMAP
> +	default y
>  
>  config MIGRATE_VMA_HELPER
>  	bool
> @@ -709,8 +709,7 @@ config HMM_MIRROR
>  
>  config DEVICE_PRIVATE
>  	bool "Unaddressable device memory (GPU memory, ...)"
> -	depends on ARCH_HAS_HMM
> -	select HMM
> +	depends on ZONE_DEVICE
>  	select DEV_PAGEMAP_OPS
>  
>  	help
> -- 
> 2.20.1
> 

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 24/25] mm: remove the HMM config option
  2019-06-26 12:27 ` [PATCH 24/25] mm: remove the HMM config option Christoph Hellwig
@ 2019-06-26 21:38   ` Ira Weiny
  2019-06-27 16:29   ` Jason Gunthorpe
  1 sibling, 0 replies; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 21:38 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, linux-nvdimm, linux-pci, linux-kernel, dri-devel,
	linux-mm, nouveau

On Wed, Jun 26, 2019 at 02:27:23PM +0200, Christoph Hellwig wrote:
> All the mm/hmm.c code is better keyed off HMM_MIRROR.  Also let nouveau
> depend on it instead of the mix of a dummy dependency symbol plus the
> actually selected one.  Drop various odd dependencies, as the code is
> pretty portable.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Seems reasonable to me.

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/gpu/drm/nouveau/Kconfig |  3 +--
>  include/linux/hmm.h             |  5 +----
>  include/linux/mm_types.h        |  2 +-
>  mm/Kconfig                      | 27 ++++-----------------------
>  mm/Makefile                     |  2 +-
>  mm/hmm.c                        |  2 --
>  6 files changed, 8 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/nouveau/Kconfig b/drivers/gpu/drm/nouveau/Kconfig
> index 6303d203ab1d..66c839d8e9d1 100644
> --- a/drivers/gpu/drm/nouveau/Kconfig
> +++ b/drivers/gpu/drm/nouveau/Kconfig
> @@ -84,11 +84,10 @@ config DRM_NOUVEAU_BACKLIGHT
>  
>  config DRM_NOUVEAU_SVM
>  	bool "(EXPERIMENTAL) Enable SVM (Shared Virtual Memory) support"
> -	depends on ARCH_HAS_HMM
>  	depends on DEVICE_PRIVATE
>  	depends on DRM_NOUVEAU
> +	depends on HMM_MIRROR
>  	depends on STAGING
> -	select HMM_MIRROR
>  	default n
>  	help
>  	  Say Y here if you want to enable experimental support for
> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> index 3d00e9550e77..b697496e85ba 100644
> --- a/include/linux/hmm.h
> +++ b/include/linux/hmm.h
> @@ -62,7 +62,7 @@
>  #include <linux/kconfig.h>
>  #include <asm/pgtable.h>
>  
> -#if IS_ENABLED(CONFIG_HMM)
> +#ifdef CONFIG_HMM_MIRROR
>  
>  #include <linux/device.h>
>  #include <linux/migrate.h>
> @@ -332,9 +332,6 @@ static inline uint64_t hmm_pfn_from_pfn(const struct hmm_range *range,
>  	return hmm_device_entry_from_pfn(range, pfn);
>  }
>  
> -
> -
> -#if IS_ENABLED(CONFIG_HMM_MIRROR)
>  /*
>   * Mirroring: how to synchronize device page table with CPU page table.
>   *
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index f33a1289c101..8d37182f8dbe 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -501,7 +501,7 @@ struct mm_struct {
>  #endif
>  		struct work_struct async_put_work;
>  
> -#if IS_ENABLED(CONFIG_HMM)
> +#ifdef CONFIG_HMM_MIRROR
>  		/* HMM needs to track a few things per mm */
>  		struct hmm *hmm;
>  #endif
> diff --git a/mm/Kconfig b/mm/Kconfig
> index eecf037a54b3..1e426c26b1d6 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -669,37 +669,18 @@ config ZONE_DEVICE
>  
>  	  If FS_DAX is enabled, then say Y.
>  
> -config ARCH_HAS_HMM_MIRROR
> -	bool
> -	default y
> -	depends on (X86_64 || PPC64)
> -	depends on MMU && 64BIT
> -
> -config ARCH_HAS_HMM
> -	bool
> -	depends on (X86_64 || PPC64)
> -	depends on ZONE_DEVICE
> -	depends on MMU && 64BIT
> -	depends on MEMORY_HOTPLUG
> -	depends on MEMORY_HOTREMOVE
> -	depends on SPARSEMEM_VMEMMAP
> -	default y
> -
>  config MIGRATE_VMA_HELPER
>  	bool
>  
>  config DEV_PAGEMAP_OPS
>  	bool
>  
> -config HMM
> -	bool
> -	select MMU_NOTIFIER
> -	select MIGRATE_VMA_HELPER
> -
>  config HMM_MIRROR
>  	bool "HMM mirror CPU page table into a device page table"
> -	depends on ARCH_HAS_HMM
> -	select HMM
> +	depends on (X86_64 || PPC64)
> +	depends on MMU && 64BIT
> +	select MMU_NOTIFIER
> +	select MIGRATE_VMA_HELPER
>  	help
>  	  Select HMM_MIRROR if you want to mirror range of the CPU page table of a
>  	  process into a device page table. Here, mirror means "keep synchronized".
> diff --git a/mm/Makefile b/mm/Makefile
> index ac5e5ba78874..91c99040065c 100644
> --- a/mm/Makefile
> +++ b/mm/Makefile
> @@ -102,5 +102,5 @@ obj-$(CONFIG_FRAME_VECTOR) += frame_vector.o
>  obj-$(CONFIG_DEBUG_PAGE_REF) += debug_page_ref.o
>  obj-$(CONFIG_HARDENED_USERCOPY) += usercopy.o
>  obj-$(CONFIG_PERCPU_STATS) += percpu-stats.o
> -obj-$(CONFIG_HMM) += hmm.o
> +obj-$(CONFIG_HMM_MIRROR) += hmm.o
>  obj-$(CONFIG_MEMFD_CREATE) += memfd.o
> diff --git a/mm/hmm.c b/mm/hmm.c
> index 90ca0cdab9db..d62ce64d6bca 100644
> --- a/mm/hmm.c
> +++ b/mm/hmm.c
> @@ -25,7 +25,6 @@
>  #include <linux/mmu_notifier.h>
>  #include <linux/memory_hotplug.h>
>  
> -#if IS_ENABLED(CONFIG_HMM_MIRROR)
>  static const struct mmu_notifier_ops hmm_mmu_notifier_ops;
>  
>  static inline struct hmm *mm_get_hmm(struct mm_struct *mm)
> @@ -1326,4 +1325,3 @@ long hmm_range_dma_unmap(struct hmm_range *range,
>  	return cpages;
>  }
>  EXPORT_SYMBOL(hmm_range_dma_unmap);
> -#endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
> -- 
> 2.20.1
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 15/25] memremap: provide an optional internal refcount in struct dev_pagemap
  2019-06-26 12:27 ` [PATCH 15/25] memremap: provide an optional internal refcount in struct dev_pagemap Christoph Hellwig
@ 2019-06-26 21:47   ` Ira Weiny
  2019-06-27  8:51     ` Christoph Hellwig
  0 siblings, 1 reply; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 21:47 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, linux-nvdimm, linux-pci, linux-kernel, dri-devel,
	linux-mm, nouveau

On Wed, Jun 26, 2019 at 02:27:14PM +0200, Christoph Hellwig wrote:
> Provide an internal refcounting logic if no ->ref field is provided
> in the pagemap passed into devm_memremap_pages so that callers don't
> have to reinvent it poorly.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/memremap.h          |  4 ++
>  kernel/memremap.c                 | 64 ++++++++++++++++++++++++-------
>  tools/testing/nvdimm/test/iomap.c | 58 ++++++++++++++++++++++------
>  3 files changed, 101 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/memremap.h b/include/linux/memremap.h
> index e25685b878e9..f8a5b2a19945 100644
> --- a/include/linux/memremap.h
> +++ b/include/linux/memremap.h
> @@ -95,6 +95,8 @@ struct dev_pagemap_ops {
>   * @altmap: pre-allocated/reserved memory for vmemmap allocations
>   * @res: physical address range covered by @ref
>   * @ref: reference count that pins the devm_memremap_pages() mapping
> + * @internal_ref: internal reference if @ref is not provided by the caller
> + * @done: completion for @internal_ref
>   * @dev: host device of the mapping for debug
>   * @data: private data pointer for page_free()
>   * @type: memory type: see MEMORY_* in memory_hotplug.h
> @@ -105,6 +107,8 @@ struct dev_pagemap {
>  	struct vmem_altmap altmap;
>  	struct resource res;
>  	struct percpu_ref *ref;
> +	struct percpu_ref internal_ref;
> +	struct completion done;
>  	struct device *dev;
>  	enum memory_type type;
>  	unsigned int flags;
> diff --git a/kernel/memremap.c b/kernel/memremap.c
> index eee490e7d7e1..bea6f887adad 100644
> --- a/kernel/memremap.c
> +++ b/kernel/memremap.c
> @@ -29,7 +29,7 @@ static void devmap_managed_enable_put(void *data)
>  
>  static int devmap_managed_enable_get(struct device *dev, struct dev_pagemap *pgmap)
>  {
> -	if (!pgmap->ops->page_free) {
> +	if (!pgmap->ops || !pgmap->ops->page_free) {
>  		WARN(1, "Missing page_free method\n");
>  		return -EINVAL;
>  	}
> @@ -75,6 +75,24 @@ static unsigned long pfn_next(unsigned long pfn)
>  #define for_each_device_pfn(pfn, map) \
>  	for (pfn = pfn_first(map); pfn < pfn_end(map); pfn = pfn_next(pfn))
>  
> +static void dev_pagemap_kill(struct dev_pagemap *pgmap)
> +{
> +	if (pgmap->ops && pgmap->ops->kill)
> +		pgmap->ops->kill(pgmap);
> +	else
> +		percpu_ref_kill(pgmap->ref);
> +}
> +
> +static void dev_pagemap_cleanup(struct dev_pagemap *pgmap)
> +{
> +	if (pgmap->ops && pgmap->ops->cleanup) {
> +		pgmap->ops->cleanup(pgmap);
> +	} else {
> +		wait_for_completion(&pgmap->done);
> +		percpu_ref_exit(pgmap->ref);
> +	}
> +}
> +
>  static void devm_memremap_pages_release(void *data)
>  {
>  	struct dev_pagemap *pgmap = data;
> @@ -84,10 +102,10 @@ static void devm_memremap_pages_release(void *data)
>  	unsigned long pfn;
>  	int nid;
>  
> -	pgmap->ops->kill(pgmap);
> +	dev_pagemap_kill(pgmap);
>  	for_each_device_pfn(pfn, pgmap)
>  		put_page(pfn_to_page(pfn));
> -	pgmap->ops->cleanup(pgmap);
> +	dev_pagemap_cleanup(pgmap);
>  
>  	/* pages are dead and unused, undo the arch mapping */
>  	align_start = res->start & ~(SECTION_SIZE - 1);
> @@ -114,20 +132,29 @@ static void devm_memremap_pages_release(void *data)
>  		      "%s: failed to free all reserved pages\n", __func__);
>  }
>  
> +static void dev_pagemap_percpu_release(struct percpu_ref *ref)
> +{
> +	struct dev_pagemap *pgmap =
> +		container_of(ref, struct dev_pagemap, internal_ref);
> +
> +	complete(&pgmap->done);
> +}
> +
>  /**
>   * devm_memremap_pages - remap and provide memmap backing for the given resource
>   * @dev: hosting device for @res
>   * @pgmap: pointer to a struct dev_pagemap
>   *
>   * Notes:
> - * 1/ At a minimum the res, ref and type and ops members of @pgmap must be
> - *    initialized by the caller before passing it to this function
> + * 1/ At a minimum the res and type members of @pgmap must be initialized
> + *    by the caller before passing it to this function
>   *
>   * 2/ The altmap field may optionally be initialized, in which case
>   *    PGMAP_ALTMAP_VALID must be set in pgmap->flags.
>   *
> - * 3/ pgmap->ref must be 'live' on entry and will be killed and reaped
> - *    at devm_memremap_pages_release() time, or if this routine fails.
> + * 3/ The ref field may optionally be provided, in which pgmap->ref must be
> + *    'live' on entry and will be killed and reaped at
> + *    devm_memremap_pages_release() time, or if this routine fails.
>   *
>   * 4/ res is expected to be a host memory range that could feasibly be
>   *    treated as a "System RAM" range, i.e. not a device mmio range, but
> @@ -175,10 +202,21 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  		break;
>  	}
>  
> -	if (!pgmap->ref || !pgmap->ops || !pgmap->ops->kill ||
> -	    !pgmap->ops->cleanup) {
> -		WARN(1, "Missing reference count teardown definition\n");
> -		return ERR_PTR(-EINVAL);
> +	if (!pgmap->ref) {
> +		if (pgmap->ops && (pgmap->ops->kill || pgmap->ops->cleanup))
> +			return ERR_PTR(-EINVAL);
> +
> +		init_completion(&pgmap->done);
> +		error = percpu_ref_init(&pgmap->internal_ref,
> +				dev_pagemap_percpu_release, 0, GFP_KERNEL);
> +		if (error)
> +			return ERR_PTR(error);
> +		pgmap->ref = &pgmap->internal_ref;
> +	} else {
> +		if (!pgmap->ops || !pgmap->ops->kill || !pgmap->ops->cleanup) {
> +			WARN(1, "Missing reference count teardown definition\n");
> +			return ERR_PTR(-EINVAL);
> +		}

After this series are there any users who continue to supply their own
reference object and these callbacks?

As it stands:

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

>  	}
>  
>  	if (need_devmap_managed) {
> @@ -296,8 +334,8 @@ void *devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>   err_pfn_remap:
>  	pgmap_array_delete(res);
>   err_array:
> -	pgmap->ops->kill(pgmap);
> -	pgmap->ops->cleanup(pgmap);
> +	dev_pagemap_kill(pgmap);
> +	dev_pagemap_cleanup(pgmap);
>  	return ERR_PTR(error);
>  }
>  EXPORT_SYMBOL_GPL(devm_memremap_pages);
> diff --git a/tools/testing/nvdimm/test/iomap.c b/tools/testing/nvdimm/test/iomap.c
> index 82f901569e06..cd040b5abffe 100644
> --- a/tools/testing/nvdimm/test/iomap.c
> +++ b/tools/testing/nvdimm/test/iomap.c
> @@ -100,26 +100,60 @@ static void nfit_test_kill(void *_pgmap)
>  {
>  	struct dev_pagemap *pgmap = _pgmap;
>  
> -	WARN_ON(!pgmap || !pgmap->ref || !pgmap->ops || !pgmap->ops->kill ||
> -		!pgmap->ops->cleanup);
> -	pgmap->ops->kill(pgmap);
> -	pgmap->ops->cleanup(pgmap);
> +	WARN_ON(!pgmap || !pgmap->ref);
> +
> +	if (pgmap->ops && pgmap->ops->kill)
> +		pgmap->ops->kill(pgmap);
> +	else
> +		percpu_ref_kill(pgmap->ref);
> +
> +	if (pgmap->ops && pgmap->ops->cleanup) {
> +		pgmap->ops->cleanup(pgmap);
> +	} else {
> +		wait_for_completion(&pgmap->done);
> +		percpu_ref_exit(pgmap->ref);
> +	}
> +}
> +
> +static void dev_pagemap_percpu_release(struct percpu_ref *ref)
> +{
> +	struct dev_pagemap *pgmap =
> +		container_of(ref, struct dev_pagemap, internal_ref);
> +
> +	complete(&pgmap->done);
>  }
>  
>  void *__wrap_devm_memremap_pages(struct device *dev, struct dev_pagemap *pgmap)
>  {
> +	int error;
>  	resource_size_t offset = pgmap->res.start;
>  	struct nfit_test_resource *nfit_res = get_nfit_res(offset);
>  
> -	if (nfit_res) {
> -		int rc;
> -
> -		rc = devm_add_action_or_reset(dev, nfit_test_kill, pgmap);
> -		if (rc)
> -			return ERR_PTR(rc);
> -		return nfit_res->buf + offset - nfit_res->res.start;
> +	if (!nfit_res)
> +		return devm_memremap_pages(dev, pgmap);
> +
> +	pgmap->dev = dev;
> +	if (!pgmap->ref) {
> +		if (pgmap->ops && (pgmap->ops->kill || pgmap->ops->cleanup))
> +			return ERR_PTR(-EINVAL);
> +
> +		init_completion(&pgmap->done);
> +		error = percpu_ref_init(&pgmap->internal_ref,
> +				dev_pagemap_percpu_release, 0, GFP_KERNEL);
> +		if (error)
> +			return ERR_PTR(error);
> +		pgmap->ref = &pgmap->internal_ref;
> +	} else {
> +		if (!pgmap->ops || !pgmap->ops->kill || !pgmap->ops->cleanup) {
> +			WARN(1, "Missing reference count teardown definition\n");
> +			return ERR_PTR(-EINVAL);
> +		}
>  	}
> -	return devm_memremap_pages(dev, pgmap);
> +
> +	error = devm_add_action_or_reset(dev, nfit_test_kill, pgmap);
> +	if (error)
> +		return ERR_PTR(error);
> +	return nfit_res->buf + offset - nfit_res->res.start;
>  }
>  EXPORT_SYMBOL_GPL(__wrap_devm_memremap_pages);
>  
> -- 
> 2.20.1
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-26 12:27 ` [PATCH 16/25] device-dax: use the dev_pagemap internal refcount Christoph Hellwig
@ 2019-06-26 21:48   ` Ira Weiny
  2019-06-28 15:38   ` Jason Gunthorpe
  1 sibling, 0 replies; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 21:48 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, linux-nvdimm, linux-pci, linux-kernel, dri-devel,
	linux-mm, nouveau

On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote:
> The functionality is identical to the one currently open coded in
> device-dax.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/dax/dax-private.h |  4 ----
>  drivers/dax/device.c      | 43 ---------------------------------------
>  2 files changed, 47 deletions(-)
> 
> diff --git a/drivers/dax/dax-private.h b/drivers/dax/dax-private.h
> index b4177aafbbd1..c915889d1769 100644
> --- a/drivers/dax/dax-private.h
> +++ b/drivers/dax/dax-private.h
> @@ -43,8 +43,6 @@ struct dax_region {
>   * @target_node: effective numa node if dev_dax memory range is onlined
>   * @dev - device core
>   * @pgmap - pgmap for memmap setup / lifetime (driver owned)
> - * @ref: pgmap reference count (driver owned)
> - * @cmp: @ref final put completion (driver owned)
>   */
>  struct dev_dax {
>  	struct dax_region *region;
> @@ -52,8 +50,6 @@ struct dev_dax {
>  	int target_node;
>  	struct device dev;
>  	struct dev_pagemap pgmap;
> -	struct percpu_ref ref;
> -	struct completion cmp;
>  };
>  
>  static inline struct dev_dax *to_dev_dax(struct device *dev)
> diff --git a/drivers/dax/device.c b/drivers/dax/device.c
> index b5257038c188..1af823b2fe6b 100644
> --- a/drivers/dax/device.c
> +++ b/drivers/dax/device.c
> @@ -14,36 +14,6 @@
>  #include "dax-private.h"
>  #include "bus.h"
>  
> -static struct dev_dax *ref_to_dev_dax(struct percpu_ref *ref)
> -{
> -	return container_of(ref, struct dev_dax, ref);
> -}
> -
> -static void dev_dax_percpu_release(struct percpu_ref *ref)
> -{
> -	struct dev_dax *dev_dax = ref_to_dev_dax(ref);
> -
> -	dev_dbg(&dev_dax->dev, "%s\n", __func__);
> -	complete(&dev_dax->cmp);
> -}
> -
> -static void dev_dax_percpu_exit(struct dev_pagemap *pgmap)
> -{
> -	struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap);
> -
> -	dev_dbg(&dev_dax->dev, "%s\n", __func__);
> -	wait_for_completion(&dev_dax->cmp);
> -	percpu_ref_exit(pgmap->ref);
> -}
> -
> -static void dev_dax_percpu_kill(struct dev_pagemap *pgmap)
> -{
> -	struct dev_dax *dev_dax = container_of(pgmap, struct dev_dax, pgmap);
> -
> -	dev_dbg(&dev_dax->dev, "%s\n", __func__);
> -	percpu_ref_kill(pgmap->ref);
> -}
> -
>  static int check_vma(struct dev_dax *dev_dax, struct vm_area_struct *vma,
>  		const char *func)
>  {
> @@ -441,11 +411,6 @@ static void dev_dax_kill(void *dev_dax)
>  	kill_dev_dax(dev_dax);
>  }
>  
> -static const struct dev_pagemap_ops dev_dax_pagemap_ops = {
> -	.kill		= dev_dax_percpu_kill,
> -	.cleanup	= dev_dax_percpu_exit,
> -};
> -
>  int dev_dax_probe(struct device *dev)
>  {
>  	struct dev_dax *dev_dax = to_dev_dax(dev);
> @@ -463,15 +428,7 @@ int dev_dax_probe(struct device *dev)
>  		return -EBUSY;
>  	}
>  
> -	init_completion(&dev_dax->cmp);
> -	rc = percpu_ref_init(&dev_dax->ref, dev_dax_percpu_release, 0,
> -			GFP_KERNEL);
> -	if (rc)
> -		return rc;
> -
> -	dev_dax->pgmap.ref = &dev_dax->ref;
>  	dev_dax->pgmap.type = MEMORY_DEVICE_DEVDAX;
> -	dev_dax->pgmap.ops = &dev_dax_pagemap_ops;
>  	addr = devm_memremap_pages(dev, &dev_dax->pgmap);
>  	if (IS_ERR(addr))
>  		return PTR_ERR(addr);
> -- 
> 2.20.1
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 17/25] PCI/P2PDMA: use the dev_pagemap internal refcount
  2019-06-26 12:27 ` [PATCH 17/25] PCI/P2PDMA: " Christoph Hellwig
@ 2019-06-26 21:49   ` Ira Weiny
  2019-06-27 18:49   ` Logan Gunthorpe
  1 sibling, 0 replies; 59+ messages in thread
From: Ira Weiny @ 2019-06-26 21:49 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Jason Gunthorpe,
	Ben Skeggs, linux-nvdimm, linux-pci, linux-kernel, dri-devel,
	linux-mm, nouveau

On Wed, Jun 26, 2019 at 02:27:16PM +0200, Christoph Hellwig wrote:
> The functionality is identical to the one currently open coded in
> p2pdma.c.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Ira Weiny <ira.weiny@intel.com>

> ---
>  drivers/pci/p2pdma.c | 57 ++++----------------------------------------
>  1 file changed, 4 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index ebd8ce3bba2e..608f84df604a 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -24,12 +24,6 @@ struct pci_p2pdma {
>  	bool p2pmem_published;
>  };
>  
> -struct p2pdma_pagemap {
> -	struct dev_pagemap pgmap;
> -	struct percpu_ref ref;
> -	struct completion ref_done;
> -};
> -
>  static ssize_t size_show(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> @@ -78,32 +72,6 @@ static const struct attribute_group p2pmem_group = {
>  	.name = "p2pmem",
>  };
>  
> -static struct p2pdma_pagemap *to_p2p_pgmap(struct percpu_ref *ref)
> -{
> -	return container_of(ref, struct p2pdma_pagemap, ref);
> -}
> -
> -static void pci_p2pdma_percpu_release(struct percpu_ref *ref)
> -{
> -	struct p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(ref);
> -
> -	complete(&p2p_pgmap->ref_done);
> -}
> -
> -static void pci_p2pdma_percpu_kill(struct dev_pagemap *pgmap)
> -{
> -	percpu_ref_kill(pgmap->ref);
> -}
> -
> -static void pci_p2pdma_percpu_cleanup(struct dev_pagemap *pgmap)
> -{
> -	struct p2pdma_pagemap *p2p_pgmap =
> -		container_of(pgmap, struct p2pdma_pagemap, pgmap);
> -
> -	wait_for_completion(&p2p_pgmap->ref_done);
> -	percpu_ref_exit(&p2p_pgmap->ref);
> -}
> -
>  static void pci_p2pdma_release(void *data)
>  {
>  	struct pci_dev *pdev = data;
> @@ -153,11 +121,6 @@ static int pci_p2pdma_setup(struct pci_dev *pdev)
>  	return error;
>  }
>  
> -static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
> -	.kill		= pci_p2pdma_percpu_kill,
> -	.cleanup	= pci_p2pdma_percpu_cleanup,
> -};
> -
>  /**
>   * pci_p2pdma_add_resource - add memory for use as p2p memory
>   * @pdev: the device to add the memory to
> @@ -171,7 +134,6 @@ static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
>  int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  			    u64 offset)
>  {
> -	struct p2pdma_pagemap *p2p_pgmap;
>  	struct dev_pagemap *pgmap;
>  	void *addr;
>  	int error;
> @@ -194,26 +156,15 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  			return error;
>  	}
>  
> -	p2p_pgmap = devm_kzalloc(&pdev->dev, sizeof(*p2p_pgmap), GFP_KERNEL);
> -	if (!p2p_pgmap)
> +	pgmap = devm_kzalloc(&pdev->dev, sizeof(*pgmap), GFP_KERNEL);
> +	if (!pgmap)
>  		return -ENOMEM;
> -
> -	init_completion(&p2p_pgmap->ref_done);
> -	error = percpu_ref_init(&p2p_pgmap->ref,
> -			pci_p2pdma_percpu_release, 0, GFP_KERNEL);
> -	if (error)
> -		goto pgmap_free;
> -
> -	pgmap = &p2p_pgmap->pgmap;
> -
>  	pgmap->res.start = pci_resource_start(pdev, bar) + offset;
>  	pgmap->res.end = pgmap->res.start + size - 1;
>  	pgmap->res.flags = pci_resource_flags(pdev, bar);
> -	pgmap->ref = &p2p_pgmap->ref;
>  	pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
>  	pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
>  		pci_resource_start(pdev, bar);
> -	pgmap->ops = &pci_p2pdma_pagemap_ops;
>  
>  	addr = devm_memremap_pages(&pdev->dev, pgmap);
>  	if (IS_ERR(addr)) {
> @@ -224,7 +175,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  	error = gen_pool_add_owner(pdev->p2pdma->pool, (unsigned long)addr,
>  			pci_bus_address(pdev, bar) + offset,
>  			resource_size(&pgmap->res), dev_to_node(&pdev->dev),
> -			&p2p_pgmap->ref);
> +			pgmap->ref);
>  	if (error)
>  		goto pages_free;
>  
> @@ -236,7 +187,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  pages_free:
>  	devm_memunmap_pages(&pdev->dev, pgmap);
>  pgmap_free:
> -	devm_kfree(&pdev->dev, p2p_pgmap);
> +	devm_kfree(&pdev->dev, pgmap);
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(pci_p2pdma_add_resource);
> -- 
> 2.20.1
> 
> _______________________________________________
> Linux-nvdimm mailing list
> Linux-nvdimm@lists.01.org
> https://lists.01.org/mailman/listinfo/linux-nvdimm

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 11/25] memremap: lift the devmap_enable manipulation into devm_memremap_pages
  2019-06-26 19:04   ` Ira Weiny
@ 2019-06-27  8:50     ` Christoph Hellwig
  0 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-27  8:50 UTC (permalink / raw)
  To: Ira Weiny
  Cc: Christoph Hellwig, Dan Williams, Jérôme Glisse,
	Jason Gunthorpe, Ben Skeggs, linux-nvdimm, linux-pci,
	linux-kernel, dri-devel, linux-mm, nouveau

On Wed, Jun 26, 2019 at 12:04:46PM -0700, Ira Weiny wrote:
> > +static int devmap_managed_enable_get(struct device *dev, struct dev_pagemap *pgmap)
> > +{
> > +	if (!pgmap->ops->page_free) {
> 
> NIT: later on you add the check for pgmap->ops...  it should probably be here.
> 
> But not sure that bisection will be an issue here.

At this point we do not allow a NULL ops pointer.  That only becomes
a valid option one the internal refcount is added.  Until then a NULL
->ops pointer leads to an error return from devm_memremap_pages.

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 15/25] memremap: provide an optional internal refcount in struct dev_pagemap
  2019-06-26 21:47   ` Ira Weiny
@ 2019-06-27  8:51     ` Christoph Hellwig
  0 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-27  8:51 UTC (permalink / raw)
  To: Ira Weiny
  Cc: Christoph Hellwig, Dan Williams, Jérôme Glisse,
	Jason Gunthorpe, Ben Skeggs, linux-nvdimm, linux-pci,
	linux-kernel, dri-devel, linux-mm, nouveau

On Wed, Jun 26, 2019 at 02:47:50PM -0700, Ira Weiny wrote:
> > +
> > +		init_completion(&pgmap->done);
> > +		error = percpu_ref_init(&pgmap->internal_ref,
> > +				dev_pagemap_percpu_release, 0, GFP_KERNEL);
> > +		if (error)
> > +			return ERR_PTR(error);
> > +		pgmap->ref = &pgmap->internal_ref;
> > +	} else {
> > +		if (!pgmap->ops || !pgmap->ops->kill || !pgmap->ops->cleanup) {
> > +			WARN(1, "Missing reference count teardown definition\n");
> > +			return ERR_PTR(-EINVAL);
> > +		}
> 
> After this series are there any users who continue to supply their own
> reference object and these callbacks?

Yes, fsdax uses the block layer request_queue reference count.

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 03/25] mm: remove hmm_devmem_add_resource
  2019-06-26 12:27 ` [PATCH 03/25] mm: remove hmm_devmem_add_resource Christoph Hellwig
@ 2019-06-27 16:18   ` Jason Gunthorpe
  2019-06-27 16:54     ` Christoph Hellwig
  0 siblings, 1 reply; 59+ messages in thread
From: Jason Gunthorpe @ 2019-06-27 16:18 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel,
	John Hubbard, Michal Hocko

On Wed, Jun 26, 2019 at 02:27:02PM +0200, Christoph Hellwig wrote:
> This function has never been used since it was first added to the kernel
> more than a year and a half ago, and if we ever grow a consumer of the
> MEMORY_DEVICE_PUBLIC infrastructure it can easily use devm_memremap_pages
> directly.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> ---
>  include/linux/hmm.h |  3 ---
>  mm/hmm.c            | 50 ---------------------------------------------
>  2 files changed, 53 deletions(-)

This should be squashed to the new earlier patch?

Jason

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 20/25] mm: remove hmm_vma_alloc_locked_page
  2019-06-26 12:27 ` [PATCH 20/25] mm: remove hmm_vma_alloc_locked_page Christoph Hellwig
@ 2019-06-27 16:26   ` Jason Gunthorpe
  0 siblings, 0 replies; 59+ messages in thread
From: Jason Gunthorpe @ 2019-06-27 16:26 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

On Wed, Jun 26, 2019 at 02:27:19PM +0200, Christoph Hellwig wrote:
> The only user of it has just been removed, and there wasn't really any need
> to wrap a basic memory allocator to start with.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  include/linux/hmm.h |  3 ---
>  mm/hmm.c            | 14 --------------
>  2 files changed, 17 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>

Jason

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 12/25] memremap: add a migrate_to_ram method to struct dev_pagemap_ops
  2019-06-26 12:27 ` [PATCH 12/25] memremap: add a migrate_to_ram method to struct dev_pagemap_ops Christoph Hellwig
@ 2019-06-27 16:29   ` Jason Gunthorpe
  2019-06-27 16:53     ` Christoph Hellwig
  0 siblings, 1 reply; 59+ messages in thread
From: Jason Gunthorpe @ 2019-06-27 16:29 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel,
	Ralph Campbell

On Wed, Jun 26, 2019 at 02:27:11PM +0200, Christoph Hellwig wrote:
> This replaces the hacky ->fault callback, which is currently directly
> called from common code through a hmm specific data structure as an
> exercise in layering violations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Ralph Campbell <rcampbell@nvidia.com>
> ---
>  include/linux/hmm.h      |  6 ------
>  include/linux/memremap.h |  6 ++++++
>  include/linux/swapops.h  | 15 ---------------
>  kernel/memremap.c        | 35 ++++-------------------------------
>  mm/hmm.c                 | 13 +++++--------
>  mm/memory.c              |  9 ++-------
>  6 files changed, 17 insertions(+), 67 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
 
I'ver heard there are some other use models for fault() here beyond
migrate to ram, but we can rename it if we ever see them.

> +static vm_fault_t hmm_devmem_migrate_to_ram(struct vm_fault *vmf)
>  {
> -	struct hmm_devmem *devmem = page->pgmap->data;
> +	struct hmm_devmem *devmem = vmf->page->pgmap->data;
>  
> -	return devmem->ops->fault(devmem, vma, addr, page, flags, pmdp);
> +	return devmem->ops->fault(devmem, vmf->vma, vmf->address, vmf->page,
> +			vmf->flags, vmf->pmd);
>  }

Next cycle we should probably rename this fault to migrate_to_ram as
well and pass in the vmf..

Jason

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 24/25] mm: remove the HMM config option
  2019-06-26 12:27 ` [PATCH 24/25] mm: remove the HMM config option Christoph Hellwig
  2019-06-26 21:38   ` Ira Weiny
@ 2019-06-27 16:29   ` Jason Gunthorpe
  1 sibling, 0 replies; 59+ messages in thread
From: Jason Gunthorpe @ 2019-06-27 16:29 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

On Wed, Jun 26, 2019 at 02:27:23PM +0200, Christoph Hellwig wrote:
> All the mm/hmm.c code is better keyed off HMM_MIRROR.  Also let nouveau
> depend on it instead of the mix of a dummy dependency symbol plus the
> actually selected one.  Drop various odd dependencies, as the code is
> pretty portable.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  drivers/gpu/drm/nouveau/Kconfig |  3 +--
>  include/linux/hmm.h             |  5 +----
>  include/linux/mm_types.h        |  2 +-
>  mm/Kconfig                      | 27 ++++-----------------------
>  mm/Makefile                     |  2 +-
>  mm/hmm.c                        |  2 --
>  6 files changed, 8 insertions(+), 33 deletions(-)

Makes more sense to me too

Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>

Jason

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 12/25] memremap: add a migrate_to_ram method to struct dev_pagemap_ops
  2019-06-27 16:29   ` Jason Gunthorpe
@ 2019-06-27 16:53     ` Christoph Hellwig
  0 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-27 16:53 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Christoph Hellwig, Dan Williams, Jérôme Glisse,
	Ben Skeggs, linux-mm, nouveau, dri-devel, linux-nvdimm,
	linux-pci, linux-kernel, Ralph Campbell

On Thu, Jun 27, 2019 at 04:29:45PM +0000, Jason Gunthorpe wrote:
> I'ver heard there are some other use models for fault() here beyond
> migrate to ram, but we can rename it if we ever see them.

Well, it absolutely needs to migrate to some piece of addressable
and coherent memory, so ram might be a nice shortcut for that.

> > +static vm_fault_t hmm_devmem_migrate_to_ram(struct vm_fault *vmf)
> >  {
> > -	struct hmm_devmem *devmem = page->pgmap->data;
> > +	struct hmm_devmem *devmem = vmf->page->pgmap->data;
> >  
> > -	return devmem->ops->fault(devmem, vma, addr, page, flags, pmdp);
> > +	return devmem->ops->fault(devmem, vmf->vma, vmf->address, vmf->page,
> > +			vmf->flags, vmf->pmd);
> >  }
> 
> Next cycle we should probably rename this fault to migrate_to_ram as
> well and pass in the vmf..

That ->fault op goes away entirely in one of the next patches in the
series.

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 03/25] mm: remove hmm_devmem_add_resource
  2019-06-27 16:18   ` Jason Gunthorpe
@ 2019-06-27 16:54     ` Christoph Hellwig
  0 siblings, 0 replies; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-27 16:54 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Christoph Hellwig, Dan Williams, Jérôme Glisse,
	Ben Skeggs, linux-mm, nouveau, dri-devel, linux-nvdimm,
	linux-pci, linux-kernel, John Hubbard, Michal Hocko

On Thu, Jun 27, 2019 at 04:18:22PM +0000, Jason Gunthorpe wrote:
> On Wed, Jun 26, 2019 at 02:27:02PM +0200, Christoph Hellwig wrote:
> > This function has never been used since it was first added to the kernel
> > more than a year and a half ago, and if we ever grow a consumer of the
> > MEMORY_DEVICE_PUBLIC infrastructure it can easily use devm_memremap_pages
> > directly.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Jason Gunthorpe <jgg@mellanox.com>
> > Reviewed-by: John Hubbard <jhubbard@nvidia.com>
> > Acked-by: Michal Hocko <mhocko@suse.com>
> > ---
> >  include/linux/hmm.h |  3 ---
> >  mm/hmm.c            | 50 ---------------------------------------------
> >  2 files changed, 53 deletions(-)
> 
> This should be squashed to the new earlier patch?

We could do that.  Do you just want to do that when you apply it?

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 17/25] PCI/P2PDMA: use the dev_pagemap internal refcount
  2019-06-26 12:27 ` [PATCH 17/25] PCI/P2PDMA: " Christoph Hellwig
  2019-06-26 21:49   ` Ira Weiny
@ 2019-06-27 18:49   ` Logan Gunthorpe
  1 sibling, 0 replies; 59+ messages in thread
From: Logan Gunthorpe @ 2019-06-27 18:49 UTC (permalink / raw)
  To: Christoph Hellwig, Dan Williams, Jérôme Glisse,
	Jason Gunthorpe, Ben Skeggs
  Cc: linux-nvdimm, linux-pci, linux-kernel, dri-devel, linux-mm, nouveau



On 2019-06-26 6:27 a.m., Christoph Hellwig wrote:
> The functionality is identical to the one currently open coded in
> p2pdma.c.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Reviewed-by: Logan Gunthorpe <logang@deltatee.com>

Also, for the P2PDMA changes in this series:

Tested-by: Logan Gunthorpe <logang@deltatee.com>

I've ran this series through my simple P2PDMA tests.

Logan

> ---
>  drivers/pci/p2pdma.c | 57 ++++----------------------------------------
>  1 file changed, 4 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/pci/p2pdma.c b/drivers/pci/p2pdma.c
> index ebd8ce3bba2e..608f84df604a 100644
> --- a/drivers/pci/p2pdma.c
> +++ b/drivers/pci/p2pdma.c
> @@ -24,12 +24,6 @@ struct pci_p2pdma {
>  	bool p2pmem_published;
>  };
>  
> -struct p2pdma_pagemap {
> -	struct dev_pagemap pgmap;
> -	struct percpu_ref ref;
> -	struct completion ref_done;
> -};
> -
>  static ssize_t size_show(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> @@ -78,32 +72,6 @@ static const struct attribute_group p2pmem_group = {
>  	.name = "p2pmem",
>  };
>  
> -static struct p2pdma_pagemap *to_p2p_pgmap(struct percpu_ref *ref)
> -{
> -	return container_of(ref, struct p2pdma_pagemap, ref);
> -}
> -
> -static void pci_p2pdma_percpu_release(struct percpu_ref *ref)
> -{
> -	struct p2pdma_pagemap *p2p_pgmap = to_p2p_pgmap(ref);
> -
> -	complete(&p2p_pgmap->ref_done);
> -}
> -
> -static void pci_p2pdma_percpu_kill(struct dev_pagemap *pgmap)
> -{
> -	percpu_ref_kill(pgmap->ref);
> -}
> -
> -static void pci_p2pdma_percpu_cleanup(struct dev_pagemap *pgmap)
> -{
> -	struct p2pdma_pagemap *p2p_pgmap =
> -		container_of(pgmap, struct p2pdma_pagemap, pgmap);
> -
> -	wait_for_completion(&p2p_pgmap->ref_done);
> -	percpu_ref_exit(&p2p_pgmap->ref);
> -}
> -
>  static void pci_p2pdma_release(void *data)
>  {
>  	struct pci_dev *pdev = data;
> @@ -153,11 +121,6 @@ static int pci_p2pdma_setup(struct pci_dev *pdev)
>  	return error;
>  }
>  
> -static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
> -	.kill		= pci_p2pdma_percpu_kill,
> -	.cleanup	= pci_p2pdma_percpu_cleanup,
> -};
> -
>  /**
>   * pci_p2pdma_add_resource - add memory for use as p2p memory
>   * @pdev: the device to add the memory to
> @@ -171,7 +134,6 @@ static const struct dev_pagemap_ops pci_p2pdma_pagemap_ops = {
>  int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  			    u64 offset)
>  {
> -	struct p2pdma_pagemap *p2p_pgmap;
>  	struct dev_pagemap *pgmap;
>  	void *addr;
>  	int error;
> @@ -194,26 +156,15 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  			return error;
>  	}
>  
> -	p2p_pgmap = devm_kzalloc(&pdev->dev, sizeof(*p2p_pgmap), GFP_KERNEL);
> -	if (!p2p_pgmap)
> +	pgmap = devm_kzalloc(&pdev->dev, sizeof(*pgmap), GFP_KERNEL);
> +	if (!pgmap)
>  		return -ENOMEM;
> -
> -	init_completion(&p2p_pgmap->ref_done);
> -	error = percpu_ref_init(&p2p_pgmap->ref,
> -			pci_p2pdma_percpu_release, 0, GFP_KERNEL);
> -	if (error)
> -		goto pgmap_free;
> -
> -	pgmap = &p2p_pgmap->pgmap;
> -
>  	pgmap->res.start = pci_resource_start(pdev, bar) + offset;
>  	pgmap->res.end = pgmap->res.start + size - 1;
>  	pgmap->res.flags = pci_resource_flags(pdev, bar);
> -	pgmap->ref = &p2p_pgmap->ref;
>  	pgmap->type = MEMORY_DEVICE_PCI_P2PDMA;
>  	pgmap->pci_p2pdma_bus_offset = pci_bus_address(pdev, bar) -
>  		pci_resource_start(pdev, bar);
> -	pgmap->ops = &pci_p2pdma_pagemap_ops;
>  
>  	addr = devm_memremap_pages(&pdev->dev, pgmap);
>  	if (IS_ERR(addr)) {
> @@ -224,7 +175,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  	error = gen_pool_add_owner(pdev->p2pdma->pool, (unsigned long)addr,
>  			pci_bus_address(pdev, bar) + offset,
>  			resource_size(&pgmap->res), dev_to_node(&pdev->dev),
> -			&p2p_pgmap->ref);
> +			pgmap->ref);
>  	if (error)
>  		goto pages_free;
>  
> @@ -236,7 +187,7 @@ int pci_p2pdma_add_resource(struct pci_dev *pdev, int bar, size_t size,
>  pages_free:
>  	devm_memunmap_pages(&pdev->dev, pgmap);
>  pgmap_free:
> -	devm_kfree(&pdev->dev, p2p_pgmap);
> +	devm_kfree(&pdev->dev, pgmap);
>  	return error;
>  }
>  EXPORT_SYMBOL_GPL(pci_p2pdma_add_resource);
> 

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-26 12:27 ` [PATCH 16/25] device-dax: use the dev_pagemap internal refcount Christoph Hellwig
  2019-06-26 21:48   ` Ira Weiny
@ 2019-06-28 15:38   ` Jason Gunthorpe
  2019-06-28 16:27     ` Dan Williams
  1 sibling, 1 reply; 59+ messages in thread
From: Jason Gunthorpe @ 2019-06-28 15:38 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Dan Williams, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote:
> The functionality is identical to the one currently open coded in
> device-dax.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> ---
>  drivers/dax/dax-private.h |  4 ----
>  drivers/dax/device.c      | 43 ---------------------------------------
>  2 files changed, 47 deletions(-)

DanW: I think this series has reached enough review, did you want
to ack/test any further?

This needs to land in hmm.git soon to make the merge window.

Thanks,
Jason

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 15:38   ` Jason Gunthorpe
@ 2019-06-28 16:27     ` Dan Williams
  2019-06-28 17:02       ` Jason Gunthorpe
  0 siblings, 1 reply; 59+ messages in thread
From: Dan Williams @ 2019-06-28 16:27 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Christoph Hellwig, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote:
> > The functionality is identical to the one currently open coded in
> > device-dax.
> >
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > ---
> >  drivers/dax/dax-private.h |  4 ----
> >  drivers/dax/device.c      | 43 ---------------------------------------
> >  2 files changed, 47 deletions(-)
>
> DanW: I think this series has reached enough review, did you want
> to ack/test any further?
>
> This needs to land in hmm.git soon to make the merge window.

I was awaiting a decision about resolving the collision with Ira's
patch before testing the final result again [1]. You can go ahead and
add my reviewed-by for the series, but my tested-by should be on the
final state of the series.

[1]: https://lore.kernel.org/lkml/CAPcyv4gTOf+EWzSGrFrh2GrTZt5HVR=e+xicUKEpiy57px8J+w@mail.gmail.com/

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 16:27     ` Dan Williams
@ 2019-06-28 17:02       ` Jason Gunthorpe
  2019-06-28 17:08         ` Dan Williams
  0 siblings, 1 reply; 59+ messages in thread
From: Jason Gunthorpe @ 2019-06-28 17:02 UTC (permalink / raw)
  To: Dan Williams
  Cc: Christoph Hellwig, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel

On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote:
> On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> >
> > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote:
> > > The functionality is identical to the one currently open coded in
> > > device-dax.
> > >
> > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > >  drivers/dax/dax-private.h |  4 ----
> > >  drivers/dax/device.c      | 43 ---------------------------------------
> > >  2 files changed, 47 deletions(-)
> >
> > DanW: I think this series has reached enough review, did you want
> > to ack/test any further?
> >
> > This needs to land in hmm.git soon to make the merge window.
> 
> I was awaiting a decision about resolving the collision with Ira's
> patch before testing the final result again [1]. You can go ahead and
> add my reviewed-by for the series, but my tested-by should be on the
> final state of the series.

The conflict looks OK to me, I think we can let Andrew and Linus
resolve it.

Thanks,
Jason

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 17:02       ` Jason Gunthorpe
@ 2019-06-28 17:08         ` Dan Williams
  2019-06-28 17:10           ` Dan Williams
  0 siblings, 1 reply; 59+ messages in thread
From: Dan Williams @ 2019-06-28 17:08 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Christoph Hellwig, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel,
	Andrew Morton

On Fri, Jun 28, 2019 at 10:02 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote:
> > On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> > >
> > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote:
> > > > The functionality is identical to the one currently open coded in
> > > > device-dax.
> > > >
> > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > > >  drivers/dax/dax-private.h |  4 ----
> > > >  drivers/dax/device.c      | 43 ---------------------------------------
> > > >  2 files changed, 47 deletions(-)
> > >
> > > DanW: I think this series has reached enough review, did you want
> > > to ack/test any further?
> > >
> > > This needs to land in hmm.git soon to make the merge window.
> >
> > I was awaiting a decision about resolving the collision with Ira's
> > patch before testing the final result again [1]. You can go ahead and
> > add my reviewed-by for the series, but my tested-by should be on the
> > final state of the series.
>
> The conflict looks OK to me, I think we can let Andrew and Linus
> resolve it.
>

Andrew's tree effectively always rebases since it's a quilt series.
I'd recommend pulling Ira's patch out of -mm and applying it with the
rest of hmm reworks. Any other git tree I'd agree with just doing the
late conflict resolution, but I'm not clear on what's the best
practice when conflicting with -mm.

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 17:08         ` Dan Williams
@ 2019-06-28 17:10           ` Dan Williams
  2019-06-28 18:29             ` Jason Gunthorpe
  0 siblings, 1 reply; 59+ messages in thread
From: Dan Williams @ 2019-06-28 17:10 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Christoph Hellwig, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel,
	Andrew Morton

On Fri, Jun 28, 2019 at 10:08 AM Dan Williams <dan.j.williams@intel.com> wrote:
>
> On Fri, Jun 28, 2019 at 10:02 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> >
> > On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote:
> > > On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> > > >
> > > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote:
> > > > > The functionality is identical to the one currently open coded in
> > > > > device-dax.
> > > > >
> > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > > > >  drivers/dax/dax-private.h |  4 ----
> > > > >  drivers/dax/device.c      | 43 ---------------------------------------
> > > > >  2 files changed, 47 deletions(-)
> > > >
> > > > DanW: I think this series has reached enough review, did you want
> > > > to ack/test any further?
> > > >
> > > > This needs to land in hmm.git soon to make the merge window.
> > >
> > > I was awaiting a decision about resolving the collision with Ira's
> > > patch before testing the final result again [1]. You can go ahead and
> > > add my reviewed-by for the series, but my tested-by should be on the
> > > final state of the series.
> >
> > The conflict looks OK to me, I think we can let Andrew and Linus
> > resolve it.
> >
>
> Andrew's tree effectively always rebases since it's a quilt series.
> I'd recommend pulling Ira's patch out of -mm and applying it with the
> rest of hmm reworks. Any other git tree I'd agree with just doing the
> late conflict resolution, but I'm not clear on what's the best
> practice when conflicting with -mm.

Regardless the patch is buggy. If you want to do the conflict
resolution it should be because the DEVICE_PUBLIC removal effectively
does the same fix otherwise we're knowingly leaving a broken point in
the history.

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 17:10           ` Dan Williams
@ 2019-06-28 18:29             ` Jason Gunthorpe
  2019-06-28 18:44               ` Dan Williams
  0 siblings, 1 reply; 59+ messages in thread
From: Jason Gunthorpe @ 2019-06-28 18:29 UTC (permalink / raw)
  To: Dan Williams
  Cc: Christoph Hellwig, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel,
	Andrew Morton

On Fri, Jun 28, 2019 at 10:10:12AM -0700, Dan Williams wrote:
> On Fri, Jun 28, 2019 at 10:08 AM Dan Williams <dan.j.williams@intel.com> wrote:
> >
> > On Fri, Jun 28, 2019 at 10:02 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> > >
> > > On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote:
> > > > On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> > > > >
> > > > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote:
> > > > > > The functionality is identical to the one currently open coded in
> > > > > > device-dax.
> > > > > >
> > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > > > > >  drivers/dax/dax-private.h |  4 ----
> > > > > >  drivers/dax/device.c      | 43 ---------------------------------------
> > > > > >  2 files changed, 47 deletions(-)
> > > > >
> > > > > DanW: I think this series has reached enough review, did you want
> > > > > to ack/test any further?
> > > > >
> > > > > This needs to land in hmm.git soon to make the merge window.
> > > >
> > > > I was awaiting a decision about resolving the collision with Ira's
> > > > patch before testing the final result again [1]. You can go ahead and
> > > > add my reviewed-by for the series, but my tested-by should be on the
> > > > final state of the series.
> > >
> > > The conflict looks OK to me, I think we can let Andrew and Linus
> > > resolve it.
> >
> > Andrew's tree effectively always rebases since it's a quilt series.
> > I'd recommend pulling Ira's patch out of -mm and applying it with the
> > rest of hmm reworks. Any other git tree I'd agree with just doing the
> > late conflict resolution, but I'm not clear on what's the best
> > practice when conflicting with -mm.

What happens depends on timing as things arrive to Linus. I promised
to send hmm.git early, so I understand that Andrew will quilt rebase
his tree to Linus's and fix the conflict in Ira's patch before he
sends it.

> Regardless the patch is buggy. If you want to do the conflict
> resolution it should be because the DEVICE_PUBLIC removal effectively
> does the same fix otherwise we're knowingly leaving a broken point in
> the history.

I'm not sure I understand your concern, is there something wrong with
CH's series as it stands? hmm is a non-rebasing git tree, so as long
as the series is correct *when I apply it* there is no broken history.

I assumed the conflict resolution for Ira's patch was to simply take
the deletion of the if block from CH's series - right?

If we do need to take Ira's patch into hmm.git it will go after CH's
series (and Ira will have to rebase/repost it), so I think there is
nothing to do at this moment - unless you are saying there is a
problem with the series in CH's git tree?

Regards,
Jason

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 18:29             ` Jason Gunthorpe
@ 2019-06-28 18:44               ` Dan Williams
  2019-06-28 18:51                 ` Christoph Hellwig
  0 siblings, 1 reply; 59+ messages in thread
From: Dan Williams @ 2019-06-28 18:44 UTC (permalink / raw)
  To: Jason Gunthorpe
  Cc: Christoph Hellwig, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel,
	Andrew Morton

On Fri, Jun 28, 2019 at 11:29 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
>
> On Fri, Jun 28, 2019 at 10:10:12AM -0700, Dan Williams wrote:
> > On Fri, Jun 28, 2019 at 10:08 AM Dan Williams <dan.j.williams@intel.com> wrote:
> > >
> > > On Fri, Jun 28, 2019 at 10:02 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> > > >
> > > > On Fri, Jun 28, 2019 at 09:27:44AM -0700, Dan Williams wrote:
> > > > > On Fri, Jun 28, 2019 at 8:39 AM Jason Gunthorpe <jgg@mellanox.com> wrote:
> > > > > >
> > > > > > On Wed, Jun 26, 2019 at 02:27:15PM +0200, Christoph Hellwig wrote:
> > > > > > > The functionality is identical to the one currently open coded in
> > > > > > > device-dax.
> > > > > > >
> > > > > > > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > > > > > > Reviewed-by: Ira Weiny <ira.weiny@intel.com>
> > > > > > >  drivers/dax/dax-private.h |  4 ----
> > > > > > >  drivers/dax/device.c      | 43 ---------------------------------------
> > > > > > >  2 files changed, 47 deletions(-)
> > > > > >
> > > > > > DanW: I think this series has reached enough review, did you want
> > > > > > to ack/test any further?
> > > > > >
> > > > > > This needs to land in hmm.git soon to make the merge window.
> > > > >
> > > > > I was awaiting a decision about resolving the collision with Ira's
> > > > > patch before testing the final result again [1]. You can go ahead and
> > > > > add my reviewed-by for the series, but my tested-by should be on the
> > > > > final state of the series.
> > > >
> > > > The conflict looks OK to me, I think we can let Andrew and Linus
> > > > resolve it.
> > >
> > > Andrew's tree effectively always rebases since it's a quilt series.
> > > I'd recommend pulling Ira's patch out of -mm and applying it with the
> > > rest of hmm reworks. Any other git tree I'd agree with just doing the
> > > late conflict resolution, but I'm not clear on what's the best
> > > practice when conflicting with -mm.
>
> What happens depends on timing as things arrive to Linus. I promised
> to send hmm.git early, so I understand that Andrew will quilt rebase
> his tree to Linus's and fix the conflict in Ira's patch before he
> sends it.
>
> > Regardless the patch is buggy. If you want to do the conflict
> > resolution it should be because the DEVICE_PUBLIC removal effectively
> > does the same fix otherwise we're knowingly leaving a broken point in
> > the history.
>
> I'm not sure I understand your concern, is there something wrong with
> CH's series as it stands? hmm is a non-rebasing git tree, so as long
> as the series is correct *when I apply it* there is no broken history.
>
> I assumed the conflict resolution for Ira's patch was to simply take
> the deletion of the if block from CH's series - right?
>
> If we do need to take Ira's patch into hmm.git it will go after CH's
> series (and Ira will have to rebase/repost it), so I think there is
> nothing to do at this moment - unless you are saying there is a
> problem with the series in CH's git tree?

There is a problem with the series in CH's tree. It removes the
->page_free() callback from the release_pages() path because it goes
too far and removes the put_devmap_managed_page() call.

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 18:44               ` Dan Williams
@ 2019-06-28 18:51                 ` Christoph Hellwig
  2019-06-28 18:59                   ` Dan Williams
  0 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-28 18:51 UTC (permalink / raw)
  To: Dan Williams
  Cc: Jason Gunthorpe, Christoph Hellwig, Jérôme Glisse,
	Ben Skeggs, linux-mm, nouveau, dri-devel, linux-nvdimm,
	linux-pci, linux-kernel, Andrew Morton

On Fri, Jun 28, 2019 at 11:44:35AM -0700, Dan Williams wrote:
> There is a problem with the series in CH's tree. It removes the
> ->page_free() callback from the release_pages() path because it goes
> too far and removes the put_devmap_managed_page() call.

release_pages only called put_devmap_managed_page for device public
pages.  So I can't see how that is in any way a problem.

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 18:51                 ` Christoph Hellwig
@ 2019-06-28 18:59                   ` Dan Williams
  2019-06-28 19:02                     ` Christoph Hellwig
  0 siblings, 1 reply; 59+ messages in thread
From: Dan Williams @ 2019-06-28 18:59 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jason Gunthorpe, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel,
	Andrew Morton

On Fri, Jun 28, 2019 at 11:52 AM Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Jun 28, 2019 at 11:44:35AM -0700, Dan Williams wrote:
> > There is a problem with the series in CH's tree. It removes the
> > ->page_free() callback from the release_pages() path because it goes
> > too far and removes the put_devmap_managed_page() call.
>
> release_pages only called put_devmap_managed_page for device public
> pages.  So I can't see how that is in any way a problem.

It's a bug that the call to put_devmap_managed_page() was gated by
MEMORY_DEVICE_PUBLIC in release_pages(). That path is also applicable
to MEMORY_DEVICE_FSDAX because it needs to trigger the ->page_free()
callback to wake up wait_on_var() via fsdax_pagefree().

So I guess you could argue that the MEMORY_DEVICE_PUBLIC removal patch
left the original bug in place. In that sense we're no worse off, but
since we know about the bug, the fix and the patches have not been
applied yet, why not fix it now?

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 18:59                   ` Dan Williams
@ 2019-06-28 19:02                     ` Christoph Hellwig
  2019-06-28 19:14                       ` Dan Williams
  0 siblings, 1 reply; 59+ messages in thread
From: Christoph Hellwig @ 2019-06-28 19:02 UTC (permalink / raw)
  To: Dan Williams
  Cc: Christoph Hellwig, Jason Gunthorpe, Jérôme Glisse,
	Ben Skeggs, linux-mm, nouveau, dri-devel, linux-nvdimm,
	linux-pci, linux-kernel, Andrew Morton

On Fri, Jun 28, 2019 at 11:59:19AM -0700, Dan Williams wrote:
> It's a bug that the call to put_devmap_managed_page() was gated by
> MEMORY_DEVICE_PUBLIC in release_pages(). That path is also applicable
> to MEMORY_DEVICE_FSDAX because it needs to trigger the ->page_free()
> callback to wake up wait_on_var() via fsdax_pagefree().
> 
> So I guess you could argue that the MEMORY_DEVICE_PUBLIC removal patch
> left the original bug in place. In that sense we're no worse off, but
> since we know about the bug, the fix and the patches have not been
> applied yet, why not fix it now?

The fix it now would simply be to apply Ira original patch now, but
given that we are at -rc6 is this really a good time?  And if we don't
apply it now based on the quilt based -mm worflow it just seems a lot
easier to apply it after my series.  Unless we want to include it in
the series, in which case I can do a quick rebase, we'd just need to
make sure Andrew pulls it from -mm.

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 19:02                     ` Christoph Hellwig
@ 2019-06-28 19:14                       ` Dan Williams
  2019-07-02 22:35                         ` Andrew Morton
  0 siblings, 1 reply; 59+ messages in thread
From: Dan Williams @ 2019-06-28 19:14 UTC (permalink / raw)
  To: Christoph Hellwig
  Cc: Jason Gunthorpe, Jérôme Glisse, Ben Skeggs, linux-mm,
	nouveau, dri-devel, linux-nvdimm, linux-pci, linux-kernel,
	Andrew Morton

On Fri, Jun 28, 2019 at 12:02 PM Christoph Hellwig <hch@lst.de> wrote:
>
> On Fri, Jun 28, 2019 at 11:59:19AM -0700, Dan Williams wrote:
> > It's a bug that the call to put_devmap_managed_page() was gated by
> > MEMORY_DEVICE_PUBLIC in release_pages(). That path is also applicable
> > to MEMORY_DEVICE_FSDAX because it needs to trigger the ->page_free()
> > callback to wake up wait_on_var() via fsdax_pagefree().
> >
> > So I guess you could argue that the MEMORY_DEVICE_PUBLIC removal patch
> > left the original bug in place. In that sense we're no worse off, but
> > since we know about the bug, the fix and the patches have not been
> > applied yet, why not fix it now?
>
> The fix it now would simply be to apply Ira original patch now, but
> given that we are at -rc6 is this really a good time?  And if we don't
> apply it now based on the quilt based -mm worflow it just seems a lot
> easier to apply it after my series.  Unless we want to include it in
> the series, in which case I can do a quick rebase, we'd just need to
> make sure Andrew pulls it from -mm.

I believe -mm auto drops patches when they appear in the -next
baseline. So it should "just work" to pull it into the series and send
it along for -next inclusion.

^ permalink raw reply	[flat|nested] 59+ messages in thread

* Re: [PATCH 16/25] device-dax: use the dev_pagemap internal refcount
  2019-06-28 19:14                       ` Dan Williams
@ 2019-07-02 22:35                         ` Andrew Morton
  0 siblings, 0 replies; 59+ messages in thread
From: Andrew Morton @ 2019-07-02 22:35 UTC (permalink / raw)
  To: Dan Williams
  Cc: Christoph Hellwig, Jason Gunthorpe, Jérôme Glisse,
	Ben Skeggs, linux-mm, nouveau, dri-devel, linux-nvdimm,
	linux-pci, linux-kernel

On Fri, 28 Jun 2019 12:14:44 -0700 Dan Williams <dan.j.williams@intel.com> wrote:

> I believe -mm auto drops patches when they appear in the -next
> baseline. So it should "just work" to pull it into the series and send
> it along for -next inclusion.

Yup.  Although it isn't very "auto" - I manually check that the patch
which turned up in -next was identical to the version which I had.  If
not, I go find out why...


^ permalink raw reply	[flat|nested] 59+ messages in thread

end of thread, back to index

Thread overview: 59+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-26 12:26 dev_pagemap related cleanups v3 Christoph Hellwig
2019-06-26 12:27 ` [PATCH 01/25] mm: remove the unused ARCH_HAS_HMM_DEVICE Kconfig option Christoph Hellwig
2019-06-26 12:27 ` [PATCH 02/25] mm: remove the struct hmm_device infrastructure Christoph Hellwig
2019-06-26 12:27 ` [PATCH 03/25] mm: remove hmm_devmem_add_resource Christoph Hellwig
2019-06-27 16:18   ` Jason Gunthorpe
2019-06-27 16:54     ` Christoph Hellwig
2019-06-26 12:27 ` [PATCH 04/25] mm: remove MEMORY_DEVICE_PUBLIC support Christoph Hellwig
2019-06-26 16:00   ` Dan Williams
2019-06-26 17:14     ` Ira Weiny
2019-06-26 18:49       ` Ira Weiny
2019-06-26 12:27 ` [PATCH 05/25] mm: don't clear ->mapping in hmm_devmem_free Christoph Hellwig
2019-06-26 12:27 ` [PATCH 06/25] mm: export alloc_pages_vma Christoph Hellwig
2019-06-26 12:36   ` Michal Hocko
2019-06-26 12:27 ` [PATCH 07/25] mm: factor out a devm_request_free_mem_region helper Christoph Hellwig
2019-06-26 12:27 ` [PATCH 08/25] memremap: validate the pagemap type passed to devm_memremap_pages Christoph Hellwig
2019-06-26 18:01   ` Ira Weiny
2019-06-26 12:27 ` [PATCH 09/25] memremap: move dev_pagemap callbacks into a separate structure Christoph Hellwig
2019-06-26 12:27 ` [PATCH 10/25] memremap: pass a struct dev_pagemap to ->kill and ->cleanup Christoph Hellwig
2019-06-26 12:27 ` [PATCH 11/25] memremap: lift the devmap_enable manipulation into devm_memremap_pages Christoph Hellwig
2019-06-26 19:04   ` Ira Weiny
2019-06-27  8:50     ` Christoph Hellwig
2019-06-26 12:27 ` [PATCH 12/25] memremap: add a migrate_to_ram method to struct dev_pagemap_ops Christoph Hellwig
2019-06-27 16:29   ` Jason Gunthorpe
2019-06-27 16:53     ` Christoph Hellwig
2019-06-26 12:27 ` [PATCH 13/25] memremap: remove the data field in struct dev_pagemap Christoph Hellwig
2019-06-26 12:27 ` [PATCH 14/25] memremap: replace the altmap_valid field with a PGMAP_ALTMAP_VALID flag Christoph Hellwig
2019-06-26 21:13   ` Ira Weiny
2019-06-26 12:27 ` [PATCH 15/25] memremap: provide an optional internal refcount in struct dev_pagemap Christoph Hellwig
2019-06-26 21:47   ` Ira Weiny
2019-06-27  8:51     ` Christoph Hellwig
2019-06-26 12:27 ` [PATCH 16/25] device-dax: use the dev_pagemap internal refcount Christoph Hellwig
2019-06-26 21:48   ` Ira Weiny
2019-06-28 15:38   ` Jason Gunthorpe
2019-06-28 16:27     ` Dan Williams
2019-06-28 17:02       ` Jason Gunthorpe
2019-06-28 17:08         ` Dan Williams
2019-06-28 17:10           ` Dan Williams
2019-06-28 18:29             ` Jason Gunthorpe
2019-06-28 18:44               ` Dan Williams
2019-06-28 18:51                 ` Christoph Hellwig
2019-06-28 18:59                   ` Dan Williams
2019-06-28 19:02                     ` Christoph Hellwig
2019-06-28 19:14                       ` Dan Williams
2019-07-02 22:35                         ` Andrew Morton
2019-06-26 12:27 ` [PATCH 17/25] PCI/P2PDMA: " Christoph Hellwig
2019-06-26 21:49   ` Ira Weiny
2019-06-27 18:49   ` Logan Gunthorpe
2019-06-26 12:27 ` [PATCH 18/25] nouveau: use alloc_page_vma directly Christoph Hellwig
2019-06-26 12:27 ` [PATCH 19/25] nouveau: use devm_memremap_pages directly Christoph Hellwig
2019-06-26 12:27 ` [PATCH 20/25] mm: remove hmm_vma_alloc_locked_page Christoph Hellwig
2019-06-27 16:26   ` Jason Gunthorpe
2019-06-26 12:27 ` [PATCH 21/25] mm: remove hmm_devmem_add Christoph Hellwig
2019-06-26 12:27 ` [PATCH 22/25] mm: simplify ZONE_DEVICE page private data Christoph Hellwig
2019-06-26 12:27 ` [PATCH 23/25] mm: sort out the DEVICE_PRIVATE Kconfig mess Christoph Hellwig
2019-06-26 21:36   ` Ira Weiny
2019-06-26 12:27 ` [PATCH 24/25] mm: remove the HMM config option Christoph Hellwig
2019-06-26 21:38   ` Ira Weiny
2019-06-27 16:29   ` Jason Gunthorpe
2019-06-26 12:27 ` [PATCH 25/25] mm: don't select MIGRATE_VMA_HELPER from HMM_MIRROR Christoph Hellwig

Linux-PCI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-pci/0 linux-pci/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-pci linux-pci/ https://lore.kernel.org/linux-pci \
		linux-pci@vger.kernel.org linux-pci@archiver.kernel.org
	public-inbox-index linux-pci

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-pci


AGPL code for this site: git clone https://public-inbox.org/ public-inbox