Re: [HMM-CDM 5/5] mm/hmm: simplify kconfig and enable HMM and DEVICE_PUBLIC for ppc64

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 06/14/2017 01:11 PM, Jérôme Glisse wrote:
This just simplify kconfig and allow HMM and DEVICE_PUBLIC to be
selected for ppc64 once ZONE_DEVICE is allowed on ppc64 (different
patchset).

Signed-off-by: Jérôme Glisse <jglisse@xxxxxxxxxx>
Signed-off-by: John Hubbard <jhubbard@xxxxxxxxxx>
Cc: Balbir Singh <balbirs@xxxxxxxxxxx>
Cc: Aneesh Kumar <aneesh.kumar@xxxxxxxxxxxxxxxxxx>
Cc: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
Cc: Benjamin Herrenschmidt <benh@xxxxxxxxxxxxxxxxxxx>
---
  include/linux/hmm.h |  4 ++--
  mm/Kconfig          | 27 ++++++---------------------
  mm/hmm.c            |  4 ++--
  3 files changed, 10 insertions(+), 25 deletions(-)

diff --git a/include/linux/hmm.h b/include/linux/hmm.h
index f6713b2..720d18c 100644
--- a/include/linux/hmm.h
+++ b/include/linux/hmm.h
@@ -327,7 +327,7 @@ int hmm_vma_fault(struct vm_area_struct *vma,
  #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
-#if IS_ENABLED(CONFIG_HMM_DEVMEM)
+#if IS_ENABLED(CONFIG_DEVICE_PRIVATE) || IS_ENABLED(CONFIG_DEVICE_PUBLIC)
  struct hmm_devmem;
struct page *hmm_vma_alloc_locked_page(struct vm_area_struct *vma,
@@ -456,7 +456,7 @@ struct hmm_device {
   */
  struct hmm_device *hmm_device_new(void *drvdata);
  void hmm_device_put(struct hmm_device *hmm_device);
-#endif /* IS_ENABLED(CONFIG_HMM_DEVMEM) */
+#endif /* CONFIG_DEVICE_PRIVATE || CONFIG_DEVICE_PUBLIC */
/* Below are for HMM internal use only! Not to be used by device driver! */
diff --git a/mm/Kconfig b/mm/Kconfig
index ad082b9..7de939a 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -265,7 +265,7 @@ config ARCH_ENABLE_HUGEPAGE_MIGRATION
  config ARCH_HAS_HMM
  	bool
  	default y
-	depends on X86_64
+	depends on X86_64 || PPC64
  	depends on ZONE_DEVICE
  	depends on MMU && 64BIT
  	depends on MEMORY_HOTPLUG
@@ -277,7 +277,7 @@ config HMM
config HMM_MIRROR
  	bool "HMM mirror CPU page table into a device page table"
-	depends on ARCH_HAS_HMM
+	depends on ARCH_HAS_HMM && X86_64
  	select MMU_NOTIFIER
  	select HMM
  	help

Hi Jerome,

There are still some problems with using this configuration. First and foremost, it is still possible (and likely, given the complete dissimilarity in naming, and difference in location on the screen) to choose HMM_MIRROR, and *not* to choose either DEVICE_PRIVATE or DEVICE_PUBLIC. And then we end up with a swath of important page fault handling code being ifdef'd out, and one ends up having to investigate why.

As for solutions, at least for the x86 (DEVICE_PRIVATE)case, we could do this:

diff --git a/mm/Kconfig b/mm/Kconfig
index 7de939a29466..f64182d7b956 100644
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -279,6 +279,7 @@ config HMM_MIRROR
        bool "HMM mirror CPU page table into a device page table"
        depends on ARCH_HAS_HMM && X86_64
        select MMU_NOTIFIER
+       select DEVICE_PRIVATE
        select HMM
        help
          Select HMM_MIRROR if you want to mirror range of the CPU page table of a

...and that is better than the other direction (having HMM_MIRROR depend on DEVICE_PRIVATE), because in the latter case, HMM_MIRROR will disappear (and it's several lines above) until you select DEVICE_PRIVATE. That is hard to work with for the user.

The user will tend to select HMM_MIRROR, but it is *not* obvious that he/she should also select DEVICE_PRIVATE. So Kconfig should do it for them.

In fact, I'm not even sure if the DEVICE_PRIVATE and DEVICE_PUBLIC actually need Kconfig protection, but if they don't, then life would be easier for whoever is configuring their kernel.


@@ -287,15 +287,6 @@ config HMM_MIRROR
  	  page tables (at PAGE_SIZE granularity), and must be able to recover from
  	  the resulting potential page faults.
-config HMM_DEVMEM
-	bool "HMM device memory helpers (to leverage ZONE_DEVICE)"
-	depends on ARCH_HAS_HMM
-	select HMM
-	help
-	  HMM devmem is a set of helper routines to leverage the ZONE_DEVICE
-	  feature. This is just to avoid having device drivers to replicating a lot
-	  of boiler plate code.  See Documentation/vm/hmm.txt.
-

Yes, probably good to remove HMM_DEVMEM as a separate conig choice.

  config PHYS_ADDR_T_64BIT
  	def_bool 64BIT || ARCH_PHYS_ADDR_T_64BIT
@@ -720,11 +711,8 @@ config ZONE_DEVICE config DEVICE_PRIVATE
  	bool "Unaddressable device memory (GPU memory, ...)"
-	depends on X86_64
-	depends on ZONE_DEVICE
-	depends on MEMORY_HOTPLUG
-	depends on MEMORY_HOTREMOVE
-	depends on SPARSEMEM_VMEMMAP
+	depends on ARCH_HAS_HMM && X86_64
+	select HMM
help
  	  Allows creation of struct pages to represent unaddressable device
@@ -733,11 +721,8 @@ config DEVICE_PRIVATE
config DEVICE_PUBLIC
  	bool "Unaddressable device memory (GPU memory, ...)"

Typo: this is a copy-and-paste from DEVICE_PRIVATE, but the "Unaddressable" part wasn't changed, so you'll end up with two identical-looking lines in `make menuconfig`.

Maybe "Directly addressable device memory"? And make the line less identical to DEVICE_PRIVATE?

thanks,
--
John Hubbard
NVIDIA

-	depends on X86_64
-	depends on ZONE_DEVICE
-	depends on MEMORY_HOTPLUG
-	depends on MEMORY_HOTREMOVE
-	depends on SPARSEMEM_VMEMMAP
+	depends on ARCH_HAS_HMM
+	select HMM
help
  	  Allows creation of struct pages to represent addressable device
diff --git a/mm/hmm.c b/mm/hmm.c
index aed110e..085cc06 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -747,7 +747,7 @@ EXPORT_SYMBOL(hmm_vma_fault);
  #endif /* IS_ENABLED(CONFIG_HMM_MIRROR) */
-#if IS_ENABLED(CONFIG_HMM_DEVMEM)
+#if IS_ENABLED(CONFIG_DEVICE_PRIVATE) || IS_ENABLED(CONFIG_DEVICE_PUBLIC)
  struct page *hmm_vma_alloc_locked_page(struct vm_area_struct *vma,
  				       unsigned long addr)
  {
@@ -1306,4 +1306,4 @@ static int __init hmm_init(void)
  }
device_initcall(hmm_init);
-#endif /* IS_ENABLED(CONFIG_HMM_DEVMEM) */
+#endif /* CONFIG_DEVICE_PRIVATE || CONFIG_DEVICE_PUBLIC */
--
2.9.3


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]
  Powered by Linux