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
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 <[email protected]>
---
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
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.
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.
On Fri, 6 Jan 2023 19:21:40 +0000 Matthew Wilcox <[email protected]> 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
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: [email protected]/[email protected]
Slack: @Xiubo Li