[PATCH 0/3] add folio_headpage() macro

[PATCH 0/3] add folio_headpage() macro

[SeongJae Park]

The standard idiom for getting head page of a given folio is
'&folio->page'.  It is efficient and safe even if the folio is NULL,
because the offset of page field in folio is zero.  However, it makes
the code not that easy to understand at the first glance, especially the
NULL safety.  Also, sometimes people forget the idiom and use
'folio_page(folio, 0)' instead.  To make it easier to read and remember,
add a new macro function called 'folio_headpage()' with the NULL case
explanation.  Then, replace the 'folio_page(folio, 0)' calls with
'folio_headpage(folio)'.


SeongJae Park (3):
  include/linux/page-flags: add folio_headpage()
  mm: use folio_headpage() instead of folio_page()
  fs/ceph/addr: use folio_headpage() instead of folio_page()

 fs/ceph/addr.c             | 2 +-
 include/linux/page-flags.h | 8 ++++++++
 mm/shmem.c                 | 4 ++--
 mm/slab.c                  | 6 +++---
 mm/slab_common.c           | 4 ++--
 mm/slub.c                  | 4 ++--
 6 files changed, 18 insertions(+), 10 deletions(-)

-- 
2.25.1

[PATCH 1/3] include/linux/page-flags: add folio_headpage()

[SeongJae Park]

The standard idiom for getting head page of a given folio is
'&folio->page'.  It is efficient and safe even if the folio is NULL,
because the offset of page field in folio is zero.  However, it makes
the code not that easy to understand at the first glance, especially the
NULL safety.  Also, sometimes people forget the idiom and use
'folio_page(folio, 0)' instead.  To make it easier to read and remember,
add a new macro function called 'folio_headpage()' with the NULL case
explanation.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 include/linux/page-flags.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 69e93a0c1277..5a22bd823a5d 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -285,6 +285,14 @@ static inline unsigned long _compound_head(const struct page *page)
  */
 #define folio_page(folio, n)	nth_page(&(folio)->page, n)
 
+/**
+ * folio_headpage - Return the head page from a folio.
+ * @folio: The pointer to the folio.
+ *
+ * Return: The head page of the folio, or NULL if the folio is NULL.
+ */
+#define folio_headpage(folio)	(&(folio)->page)
+
 static __always_inline int PageTail(struct page *page)
 {
 	return READ_ONCE(page->compound_head) & 1 || page_is_fake_head(page);
-- 
2.25.1

[PATCH 2/3] mm: use folio_headpage() instead of folio_page()

[SeongJae Park]

Several code in mm is using 'folio_page(folio, 0)' for getting the head
pages of folios.  It's not the standard idiom and inefficient.  Replace
the calls to 'folio_headpage()'.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/shmem.c       | 4 ++--
 mm/slab.c        | 6 +++---
 mm/slab_common.c | 4 ++--
 mm/slub.c        | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index bc5c156ef470..8ae73973a7fc 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -3211,7 +3211,7 @@ static const char *shmem_get_link(struct dentry *dentry,
 		folio = filemap_get_folio(inode->i_mapping, 0);
 		if (!folio)
 			return ERR_PTR(-ECHILD);
-		if (PageHWPoison(folio_page(folio, 0)) ||
+		if (PageHWPoison(folio_headpage(folio)) ||
 		    !folio_test_uptodate(folio)) {
 			folio_put(folio);
 			return ERR_PTR(-ECHILD);
@@ -3222,7 +3222,7 @@ static const char *shmem_get_link(struct dentry *dentry,
 			return ERR_PTR(error);
 		if (!folio)
 			return ERR_PTR(-ECHILD);
-		if (PageHWPoison(folio_page(folio, 0))) {
+		if (PageHWPoison(folio_headpage(folio))) {
 			folio_unlock(folio);
 			folio_put(folio);
 			return ERR_PTR(-ECHILD);
diff --git a/mm/slab.c b/mm/slab.c
index 7a269db050ee..a6f8f95678c9 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1373,7 +1373,7 @@ static struct slab *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
 	/* Make the flag visible before any changes to folio->mapping */
 	smp_wmb();
 	/* Record if ALLOC_NO_WATERMARKS was set when allocating the slab */
-	if (sk_memalloc_socks() && page_is_pfmemalloc(folio_page(folio, 0)))
+	if (sk_memalloc_socks() && page_is_pfmemalloc(folio_headpage(folio)))
 		slab_set_pfmemalloc(slab);
 
 	return slab;
@@ -1389,7 +1389,7 @@ static void kmem_freepages(struct kmem_cache *cachep, struct slab *slab)
 
 	BUG_ON(!folio_test_slab(folio));
 	__slab_clear_pfmemalloc(slab);
-	page_mapcount_reset(folio_page(folio, 0));
+	page_mapcount_reset(folio_headpage(folio));
 	folio->mapping = NULL;
 	/* Make the mapping reset visible before clearing the flag */
 	smp_wmb();
@@ -1398,7 +1398,7 @@ static void kmem_freepages(struct kmem_cache *cachep, struct slab *slab)
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += 1 << order;
 	unaccount_slab(slab, order, cachep);
-	__free_pages(folio_page(folio, 0), order);
+	__free_pages(folio_headpage(folio), order);
 }
 
 static void kmem_rcu_free(struct rcu_head *head)
diff --git a/mm/slab_common.c b/mm/slab_common.c
index bf4e777cfe90..34a0b9988d12 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -939,9 +939,9 @@ void free_large_kmalloc(struct folio *folio, void *object)
 	kasan_kfree_large(object);
 	kmsan_kfree_large(object);
 
-	mod_lruvec_page_state(folio_page(folio, 0), NR_SLAB_UNRECLAIMABLE_B,
+	mod_lruvec_page_state(folio_headpage(folio), NR_SLAB_UNRECLAIMABLE_B,
 			      -(PAGE_SIZE << order));
-	__free_pages(folio_page(folio, 0), order);
+	__free_pages(folio_headpage(folio), order);
 }
 
 static void *__kmalloc_large_node(size_t size, gfp_t flags, int node);
diff --git a/mm/slub.c b/mm/slub.c
index 13459c69095a..1f0cbb4c2288 100644
--- a/mm/slub.c
+++ b/mm/slub.c
@@ -1859,7 +1859,7 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node,
 	__folio_set_slab(folio);
 	/* Make the flag visible before any changes to folio->mapping */
 	smp_wmb();
-	if (page_is_pfmemalloc(folio_page(folio, 0)))
+	if (page_is_pfmemalloc(folio_headpage(folio)))
 		slab_set_pfmemalloc(slab);
 
 	return slab;
@@ -2066,7 +2066,7 @@ static void __free_slab(struct kmem_cache *s, struct slab *slab)
 	if (current->reclaim_state)
 		current->reclaim_state->reclaimed_slab += pages;
 	unaccount_slab(slab, order, s);
-	__free_pages(folio_page(folio, 0), order);
+	__free_pages(folio_headpage(folio), order);
 }
 
 static void rcu_free_slab(struct rcu_head *h)
-- 
2.25.1

[PATCH 3/3] fs/ceph/addr: use folio_headpage() instead of folio_page()

[SeongJae Park]

Using 'folio_page(folio, 0)' for getting the head page of a folios is
not the standard idiom and inefficient.  Replace the call in fs/ceph/ to
'folio_headpage()'.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 fs/ceph/addr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/ceph/addr.c b/fs/ceph/addr.c
index 8c74871e37c9..b76e94152b21 100644
--- a/fs/ceph/addr.c
+++ b/fs/ceph/addr.c
@@ -1290,7 +1290,7 @@ static int ceph_netfs_check_write_begin(struct file *file, loff_t pos, unsigned
 	struct ceph_inode_info *ci = ceph_inode(inode);
 	struct ceph_snap_context *snapc;
 
-	snapc = ceph_find_incompatible(folio_page(*foliop, 0));
+	snapc = ceph_find_incompatible(folio_headpage(*foliop));
 	if (snapc) {
 		int r;
 
-- 
2.25.1

Re: [PATCH 0/3] add folio_headpage() macro

[Matthew Wilcox]

On Fri, Jan 06, 2023 at 05:40:25PM +0000, SeongJae Park wrote:
> The standard idiom for getting head page of a given folio is
> '&folio->page'.  It is efficient and safe even if the folio is NULL,
> because the offset of page field in folio is zero.  However, it makes
> the code not that easy to understand at the first glance, especially the
> NULL safety.  Also, sometimes people forget the idiom and use
> 'folio_page(folio, 0)' instead.  To make it easier to read and remember,
> add a new macro function called 'folio_headpage()' with the NULL case
> explanation.  Then, replace the 'folio_page(folio, 0)' calls with
> 'folio_headpage(folio)'.

No.  Everywhere that uses &folio->page is a place that needs to be fixed.
It shouldn't have a nice convenience macro.  It should make you mildly
uncomfortable.

Re: [PATCH 2/3] mm: use folio_headpage() instead of folio_page()

[Matthew Wilcox]

On Fri, Jan 06, 2023 at 05:40:27PM +0000, SeongJae Park wrote:
> diff --git a/mm/shmem.c b/mm/shmem.c
> index bc5c156ef470..8ae73973a7fc 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -3211,7 +3211,7 @@ static const char *shmem_get_link(struct dentry *dentry,
>  		folio = filemap_get_folio(inode->i_mapping, 0);
>  		if (!folio)
>  			return ERR_PTR(-ECHILD);
> -		if (PageHWPoison(folio_page(folio, 0)) ||
> +		if (PageHWPoison(folio_headpage(folio)) ||

This is actually incorrect.  We don't want the head page, we want the
page at index 0.  It's a subtle but important difference later on.

> @@ -3222,7 +3222,7 @@ static const char *shmem_get_link(struct dentry *dentry,
>  			return ERR_PTR(error);
>  		if (!folio)
>  			return ERR_PTR(-ECHILD);
> -		if (PageHWPoison(folio_page(folio, 0))) {
> +		if (PageHWPoison(folio_headpage(folio))) {

Same here.

> +++ b/mm/slab.c
> @@ -1373,7 +1373,7 @@ static struct slab *kmem_getpages(struct kmem_cache *cachep, gfp_t flags,
>  	/* Make the flag visible before any changes to folio->mapping */
>  	smp_wmb();
>  	/* Record if ALLOC_NO_WATERMARKS was set when allocating the slab */
> -	if (sk_memalloc_socks() && page_is_pfmemalloc(folio_page(folio, 0)))
> +	if (sk_memalloc_socks() && page_is_pfmemalloc(folio_headpage(folio)))

We should have a folio_is_pfmemalloc().

> @@ -1389,7 +1389,7 @@ static void kmem_freepages(struct kmem_cache *cachep, struct slab *slab)
>  
>  	BUG_ON(!folio_test_slab(folio));
>  	__slab_clear_pfmemalloc(slab);
> -	page_mapcount_reset(folio_page(folio, 0));
> +	page_mapcount_reset(folio_headpage(folio));

This one should be &folio->page.

> @@ -1398,7 +1398,7 @@ static void kmem_freepages(struct kmem_cache *cachep, struct slab *slab)
>  	if (current->reclaim_state)
>  		current->reclaim_state->reclaimed_slab += 1 << order;
>  	unaccount_slab(slab, order, cachep);
> -	__free_pages(folio_page(folio, 0), order);
> +	__free_pages(folio_headpage(folio), order);

&folio->page.

> @@ -939,9 +939,9 @@ void free_large_kmalloc(struct folio *folio, void *object)
>  	kasan_kfree_large(object);
>  	kmsan_kfree_large(object);
>  
> -	mod_lruvec_page_state(folio_page(folio, 0), NR_SLAB_UNRECLAIMABLE_B,
> +	mod_lruvec_page_state(folio_headpage(folio), NR_SLAB_UNRECLAIMABLE_B,
>  			      -(PAGE_SIZE << order));

	lruvec_stat_mod_folio(folio, NR_SLAB_UNRECLAIMABLE_B, ...

> -	__free_pages(folio_page(folio, 0), order);
> +	__free_pages(folio_headpage(folio), order);

&folio->page.

> +++ b/mm/slub.c
> @@ -1859,7 +1859,7 @@ static inline struct slab *alloc_slab_page(gfp_t flags, int node,
>  	__folio_set_slab(folio);
>  	/* Make the flag visible before any changes to folio->mapping */
>  	smp_wmb();
> -	if (page_is_pfmemalloc(folio_page(folio, 0)))
> +	if (page_is_pfmemalloc(folio_headpage(folio)))

folio_is_pfmemalloc()

> @@ -2066,7 +2066,7 @@ static void __free_slab(struct kmem_cache *s, struct slab *slab)
>  	if (current->reclaim_state)
>  		current->reclaim_state->reclaimed_slab += pages;
>  	unaccount_slab(slab, order, s);
> -	__free_pages(folio_page(folio, 0), order);
> +	__free_pages(folio_headpage(folio), order);

&folio->page.

Re: [PATCH 3/3] fs/ceph/addr: use folio_headpage() instead of folio_page()

[Matthew Wilcox]

On Fri, Jan 06, 2023 at 05:40:28PM +0000, SeongJae Park wrote:
> -	snapc = ceph_find_incompatible(folio_page(*foliop, 0));
> +	snapc = ceph_find_incompatible(folio_headpage(*foliop));

ceph_find_incompatible() should take a folio.

Re: [PATCH 0/3] add folio_headpage() macro

[SeongJae Park]

On Fri, 6 Jan 2023 19:21:40 +0000 Matthew Wilcox <willy@infradead.org> wrote:

> On Fri, Jan 06, 2023 at 05:40:25PM +0000, SeongJae Park wrote:
> > The standard idiom for getting head page of a given folio is
> > '&folio->page'.  It is efficient and safe even if the folio is NULL,
> > because the offset of page field in folio is zero.  However, it makes
> > the code not that easy to understand at the first glance, especially the
> > NULL safety.  Also, sometimes people forget the idiom and use
> > 'folio_page(folio, 0)' instead.  To make it easier to read and remember,
> > add a new macro function called 'folio_headpage()' with the NULL case
> > explanation.  Then, replace the 'folio_page(folio, 0)' calls with
> > 'folio_headpage(folio)'.
> 
> No.  Everywhere that uses &folio->page is a place that needs to be fixed.
> It shouldn't have a nice convenience macro.  It should make you mildly
> uncomfortable.

It's true that it's just a mild uncomfortableness.  I will respect your opinion
here.  Thanks for the input.


Thanks,
SJ

Re: [PATCH 0/3] add folio_headpage() macro

[Xiubo Li]

Sorry I didn't see the [1/3] and [2/3] patches in my inbox, it seems you 
didn't CCed the ceph-devel@ mail list.

Thanks

On 07/01/2023 01:40, SeongJae Park wrote:
> The standard idiom for getting head page of a given folio is
> '&folio->page'.  It is efficient and safe even if the folio is NULL,
> because the offset of page field in folio is zero.  However, it makes
> the code not that easy to understand at the first glance, especially the
> NULL safety.  Also, sometimes people forget the idiom and use
> 'folio_page(folio, 0)' instead.  To make it easier to read and remember,
> add a new macro function called 'folio_headpage()' with the NULL case
> explanation.  Then, replace the 'folio_page(folio, 0)' calls with
> 'folio_headpage(folio)'.
>
>
> SeongJae Park (3):
>    include/linux/page-flags: add folio_headpage()
>    mm: use folio_headpage() instead of folio_page()
>    fs/ceph/addr: use folio_headpage() instead of folio_page()
>
>   fs/ceph/addr.c             | 2 +-
>   include/linux/page-flags.h | 8 ++++++++
>   mm/shmem.c                 | 4 ++--
>   mm/slab.c                  | 6 +++---
>   mm/slab_common.c           | 4 ++--
>   mm/slub.c                  | 4 ++--
>   6 files changed, 18 insertions(+), 10 deletions(-)
>
-- 
Best Regards,

Xiubo Li (李秀波)

Email: xiubli@redhat.com/xiubli@ibm.com
Slack: @Xiubo Li