Re: [PATCH net-next -v5 3/4] mm: introduce __get_page() and __put_page()

[John Hubbard <jhubbard@nvidia.com>]

On 10/9/21 02:37, Yunsheng Lin wrote:
> Introduce __get_page() and __put_page() to operate on the
> base page or head of a compound page for the cases when a
> page is known to be a base page or head of a compound page.

Hi,

I wonder if you are aware of a much larger, 137-patch seriesto do that:
folio/pageset [1]?

The naming you are proposing here does not really improve clarity. There
is nothing about __get_page() that makes it clear that it's meant only
for head/base pages, while get_page() tail pages as well. And the
well-known and widely used get_page() and put_page() get their meaning
shifted.

This area is hard to get right, and that's why there have been 15
versions, and a lot of contention associated with [1]. If you have an
alternate approach, I think it would be better in its own separate
series, with a cover letter that, at a minimum, explains how it compares
to folios/pagesets.

So in case it's not clear, I'd like to request that you drop this one
patch from your series.


[1] https://lore.kernel.org/r/YSPwmNNuuQhXNToQ@casper.infradead.org

thanks,
-- 
John Hubbard
NVIDIA

> 
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> ---
>   include/linux/mm.h | 21 ++++++++++++++-------
>   mm/swap.c          |  6 +++---
>   2 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 73a52aba448f..5683313c3e9d 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -902,7 +902,7 @@ static inline struct page *virt_to_head_page(const void *x)
>   	return compound_head(page);
>   }
>   
> -void __put_page(struct page *page);
> +void __put_single_or_compound_page(struct page *page);
>   
>   void put_pages_list(struct list_head *pages);
>   
> @@ -1203,9 +1203,8 @@ static inline bool is_pci_p2pdma_page(const struct page *page)
>   #define page_ref_zero_or_close_to_overflow(page) \
>   	((unsigned int) page_ref_count(page) + 127u <= 127u)
>   
> -static inline void get_page(struct page *page)
> +static inline void __get_page(struct page *page)
>   {
> -	page = compound_head(page);
>   	/*
>   	 * Getting a normal page or the head of a compound page
>   	 * requires to already have an elevated page->_refcount.
> @@ -1214,6 +1213,11 @@ static inline void get_page(struct page *page)
>   	page_ref_inc(page);
>   }
>   
> +static inline void get_page(struct page *page)
> +{
> +	__get_page(compound_head(page));
> +}
> +
>   bool __must_check try_grab_page(struct page *page, unsigned int flags);
>   struct page *try_grab_compound_head(struct page *page, int refs,
>   				    unsigned int flags);
> @@ -1228,10 +1232,8 @@ static inline __must_check bool try_get_page(struct page *page)
>   	return true;
>   }
>   
> -static inline void put_page(struct page *page)
> +static inline void __put_page(struct page *page)
>   {
> -	page = compound_head(page);
> -
>   	/*
>   	 * For devmap managed pages we need to catch refcount transition from
>   	 * 2 to 1, when refcount reach one it means the page is free and we
> @@ -1244,7 +1246,12 @@ static inline void put_page(struct page *page)
>   	}
>   
>   	if (put_page_testzero(page))
> -		__put_page(page);
> +		__put_single_or_compound_page(page);
> +}
> +
> +static inline void put_page(struct page *page)
> +{
> +	__put_page(compound_head(page));
>   }
>   
>   /*
> diff --git a/mm/swap.c b/mm/swap.c
> index af3cad4e5378..565cbde1caea 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -111,7 +111,7 @@ static void __put_compound_page(struct page *page)
>   	destroy_compound_page(page);
>   }
>   
> -void __put_page(struct page *page)
> +void __put_single_or_compound_page(struct page *page)
>   {
>   	if (is_zone_device_page(page)) {
>   		put_dev_pagemap(page->pgmap);
> @@ -128,7 +128,7 @@ void __put_page(struct page *page)
>   	else
>   		__put_single_page(page);
>   }
> -EXPORT_SYMBOL(__put_page);
> +EXPORT_SYMBOL(__put_single_or_compound_page);
>   
>   /**
>    * put_pages_list() - release a list of pages
> @@ -1153,7 +1153,7 @@ void put_devmap_managed_page(struct page *page)
>   	if (count == 1)
>   		free_devmap_managed_page(page);
>   	else if (!count)
> -		__put_page(page);
> +		__put_single_or_compound_page(page);
>   }
>   EXPORT_SYMBOL(put_devmap_managed_page);
>   #endif
>