* [PATCH 1/3] include/linux/page-flags: add folio_headpage()
2023-01-06 17:40 [PATCH 0/3] add folio_headpage() macro SeongJae Park
@ 2023-01-06 17:40 ` SeongJae Park
2023-01-06 17:40 ` [PATCH 2/3] mm: use folio_headpage() instead of folio_page() SeongJae Park
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: SeongJae Park @ 2023-01-06 17:40 UTC (permalink / raw)
To: Andrew Morton; +Cc: SeongJae Park, willy, linux-mm, linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/3] mm: use folio_headpage() instead of folio_page()
2023-01-06 17:40 [PATCH 0/3] add folio_headpage() macro SeongJae Park
2023-01-06 17:40 ` [PATCH 1/3] include/linux/page-flags: add folio_headpage() SeongJae Park
@ 2023-01-06 17:40 ` SeongJae Park
2023-01-06 19:37 ` Matthew Wilcox
2023-01-06 17:40 ` [PATCH 3/3] fs/ceph/addr: " SeongJae Park
` (2 subsequent siblings)
4 siblings, 1 reply; 9+ messages in thread
From: SeongJae Park @ 2023-01-06 17:40 UTC (permalink / raw)
To: Andrew Morton; +Cc: SeongJae Park, willy, linux-mm, linux-kernel
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] mm: use folio_headpage() instead of folio_page()
2023-01-06 17:40 ` [PATCH 2/3] mm: use folio_headpage() instead of folio_page() SeongJae Park
@ 2023-01-06 19:37 ` Matthew Wilcox
0 siblings, 0 replies; 9+ messages in thread
From: Matthew Wilcox @ 2023-01-06 19:37 UTC (permalink / raw)
To: SeongJae Park; +Cc: Andrew Morton, linux-mm, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] fs/ceph/addr: use folio_headpage() instead of folio_page()
2023-01-06 17:40 [PATCH 0/3] add folio_headpage() macro SeongJae Park
2023-01-06 17:40 ` [PATCH 1/3] include/linux/page-flags: add folio_headpage() SeongJae Park
2023-01-06 17:40 ` [PATCH 2/3] mm: use folio_headpage() instead of folio_page() SeongJae Park
@ 2023-01-06 17:40 ` SeongJae Park
2023-01-06 19:39 ` Matthew Wilcox
2023-01-06 19:21 ` [PATCH 0/3] add folio_headpage() macro Matthew Wilcox
2023-01-09 11:35 ` Xiubo Li
4 siblings, 1 reply; 9+ messages in thread
From: SeongJae Park @ 2023-01-06 17:40 UTC (permalink / raw)
To: Andrew Morton
Cc: willy, Xiubo Li, Ilya Dryomov, Jeff Layton, ceph-devel, linux-mm,
linux-kernel, 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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] fs/ceph/addr: use folio_headpage() instead of folio_page()
2023-01-06 17:40 ` [PATCH 3/3] fs/ceph/addr: " SeongJae Park
@ 2023-01-06 19:39 ` Matthew Wilcox
0 siblings, 0 replies; 9+ messages in thread
From: Matthew Wilcox @ 2023-01-06 19:39 UTC (permalink / raw)
To: SeongJae Park
Cc: Andrew Morton, Xiubo Li, Ilya Dryomov, Jeff Layton, ceph-devel,
linux-mm, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] add folio_headpage() macro
2023-01-06 17:40 [PATCH 0/3] add folio_headpage() macro SeongJae Park
` (2 preceding siblings ...)
2023-01-06 17:40 ` [PATCH 3/3] fs/ceph/addr: " SeongJae Park
@ 2023-01-06 19:21 ` Matthew Wilcox
2023-01-06 19:45 ` SeongJae Park
2023-01-09 11:35 ` Xiubo Li
4 siblings, 1 reply; 9+ messages in thread
From: Matthew Wilcox @ 2023-01-06 19:21 UTC (permalink / raw)
To: SeongJae Park
Cc: Andrew Morton, Xiubo Li, Ilya Dryomov, Jeff Layton, linux-mm,
ceph-devel, linux-kernel
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.
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH 0/3] add folio_headpage() macro
2023-01-06 19:21 ` [PATCH 0/3] add folio_headpage() macro Matthew Wilcox
@ 2023-01-06 19:45 ` SeongJae Park
0 siblings, 0 replies; 9+ messages in thread
From: SeongJae Park @ 2023-01-06 19:45 UTC (permalink / raw)
To: Matthew Wilcox
Cc: SeongJae Park, Andrew Morton, Xiubo Li, Ilya Dryomov,
Jeff Layton, linux-mm, ceph-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 0/3] add folio_headpage() macro
2023-01-06 17:40 [PATCH 0/3] add folio_headpage() macro SeongJae Park
` (3 preceding siblings ...)
2023-01-06 19:21 ` [PATCH 0/3] add folio_headpage() macro Matthew Wilcox
@ 2023-01-09 11:35 ` Xiubo Li
4 siblings, 0 replies; 9+ messages in thread
From: Xiubo Li @ 2023-01-09 11:35 UTC (permalink / raw)
To: SeongJae Park, Andrew Morton
Cc: willy, Ilya Dryomov, Jeff Layton, linux-mm, ceph-devel, linux-kernel
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
^ permalink raw reply [flat|nested] 9+ messages in thread