Re: [PATCH v11 04/25] mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages

On Mon, Dec 16, 2019 at 02:25:16PM -0800, John Hubbard wrote:

An upcoming patch changes and complicates the refcounting and especially the "put page" aspects of it. In order to keep everything clean, refactor the devmap page release routines:

• Rename put_devmap_managed_page() to page_is_devmap_managed(), and limit the functionality to "read only": return a bool, with no side effects.

• Add a new routine, put_devmap_managed_page(), to handle checking what kind of page it is, and what kind of refcount handling it requires.

• Rename __put_devmap_managed_page() to free_devmap_managed_page(), and limit the functionality to unconditionally freeing a devmap page.

What the reason to separate put_devmap_managed_page() from free_devmap_managed_page() if free_devmap_managed_page() has exacly one caller? Is it preparation for the next patches?

This is originally based on a separate patch by Ira Weiny, which applied to an early version of the put_user_page() experiments. Since then, Jérôme Glisse suggested the refactoring described above.

Cc: Christoph Hellwig hch@lst.de Suggested-by: Jérôme Glisse jglisse@redhat.com Reviewed-by: Dan Williams dan.j.williams@intel.com Reviewed-by: Jan Kara jack@suse.cz Signed-off-by: Ira Weiny ira.weiny@intel.com Signed-off-by: John Hubbard jhubbard@nvidia.com

---

include/linux/mm.h | 17 +++++++++++++---- mm/memremap.c | 16 ++-------------- mm/swap.c | 24 ++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 18 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h index c97ea3b694e6..77a4df06c8a7 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -952,9 +952,10 @@ static inline bool is_zone_device_page(const struct page *page) #endif #ifdef CONFIG_DEV_PAGEMAP_OPS -void __put_devmap_managed_page(struct page *page); +void free_devmap_managed_page(struct page *page); DECLARE_STATIC_KEY_FALSE(devmap_managed_key); -static inline bool put_devmap_managed_page(struct page *page)

+static inline bool page_is_devmap_managed(struct page *page) { if (!static_branch_unlikely(&devmap_managed_key)) return false; @@ -963,7 +964,6 @@ static inline bool put_devmap_managed_page(struct page *page) switch (page->pgmap->type) { case MEMORY_DEVICE_PRIVATE: case MEMORY_DEVICE_FS_DAX:

• __put_devmap_managed_page(page);
  

  return true; default: break;

@@ -971,7 +971,14 @@ static inline bool put_devmap_managed_page(struct page *page) return false; } +bool put_devmap_managed_page(struct page *page);

#else /* CONFIG_DEV_PAGEMAP_OPS */ +static inline bool page_is_devmap_managed(struct page *page) +{

• return false;

+}

static inline bool put_devmap_managed_page(struct page *page) { return false; @@ -1028,8 +1035,10 @@ static inline void put_page(struct page *page) * need to inform the device driver through callback. See * include/linux/memremap.h and HMM for details. */

• if (put_devmap_managed_page(page))

• if (page_is_devmap_managed(page)) {

• put_devmap_managed_page(page);
  

put_devmap_managed_page() has yet another page_is_devmap_managed() check inside. It looks strange.

return;

• }

if (put_page_testzero(page)) __put_page(page); diff --git a/mm/memremap.c b/mm/memremap.c index e899fa876a62..2ba773859031 100644 --- a/mm/memremap.c +++ b/mm/memremap.c @@ -411,20 +411,8 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn, EXPORT_SYMBOL_GPL(get_dev_pagemap); #ifdef CONFIG_DEV_PAGEMAP_OPS -void __put_devmap_managed_page(struct page *page) +void free_devmap_managed_page(struct page *page) {

• int count = page_ref_dec_return(page);
• /* still busy */
• if (count > 1)

• return;
  

• /* only triggered by the dev_pagemap shutdown path */
• if (count == 0) {

• __put_page(page);
  

• return;
  

• }
• /* notify page idle for dax */ if (!is_device_private_page(page)) { wake_up_var(&page->_refcount);

@@ -461,5 +449,5 @@ void __put_devmap_managed_page(struct page *page) page->mapping = NULL; page->pgmap->ops->page_free(page); } -EXPORT_SYMBOL(__put_devmap_managed_page); +EXPORT_SYMBOL(free_devmap_managed_page); #endif /* CONFIG_DEV_PAGEMAP_OPS */ diff --git a/mm/swap.c b/mm/swap.c index 5341ae93861f..49f7c2eea0ba 100644 --- a/mm/swap.c +++ b/mm/swap.c @@ -1102,3 +1102,27 @@ void __init swap_setup(void) * _really_ don't want to cluster much more */ }

+#ifdef CONFIG_DEV_PAGEMAP_OPS +bool put_devmap_managed_page(struct page *page) +{

• bool is_devmap = page_is_devmap_managed(page);
• if (is_devmap) {

Reversing the condition would save you an indentation level.

• int count = page_ref_dec_return(page);
  

• /*
  

•  * devmap page refcounts are 1-based, rather than 0-based: if
  

•  * refcount is 1, then the page is free and the refcount is
  

•  * stable because nobody holds a reference on the page.
  

•  */
  

• if (count == 1)
  

• 	free_devmap_managed_page(page);
  

• else if (!count)
  

• 	__put_page(page);
  

• }
• return is_devmap;

+} +EXPORT_SYMBOL(put_devmap_managed_page);

+#endif

2.24.1