mbox series

[0/3] add folio_headpage() macro

Message ID 20230106174028.151384-1-sj@kernel.org
Headers
Series add folio_headpage() macro |

Message

SeongJae Park Jan. 6, 2023, 5:40 p.m. UTC 
  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(-)

Comments

Matthew Wilcox Jan. 6, 2023, 7:21 p.m. UTC | #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.
SeongJae Park Jan. 6, 2023, 7:45 p.m. UTC | #2 
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
Xiubo Li Jan. 9, 2023, 11:35 a.m. UTC | #3 
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(-)
>