[PATCH v2 2/5] mmzone: Introduce folio_migratetype()

Vishal Moola (Oracle) posted 5 patches 2 years, 7 months ago
[PATCH v2 2/5] mmzone: Introduce folio_migratetype()
Posted by Vishal Moola (Oracle) 2 years, 7 months ago
Introduce folio_migratetype() as a folio equivalent for
get_pageblock_migratetype(). This function intends to return the
migratetype the folio is located in, hence the name choice.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 include/linux/mmzone.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 744bf32e48a8..b58c76e68ac7 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -105,6 +105,9 @@ extern int page_group_by_mobility_disabled;
 #define get_pageblock_migratetype(page)					\
 	get_pfnblock_flags_mask(page, page_to_pfn(page), MIGRATETYPE_MASK)
 
+#define folio_migratetype(folio)				\
+	get_pfnblock_flags_mask(&folio->page, folio_pfn(folio),		\
+			MIGRATETYPE_MASK)
 struct free_area {
 	struct list_head	free_list[MIGRATE_TYPES];
 	unsigned long		nr_free;
-- 
2.40.1
Re: [PATCH v2 2/5] mmzone: Introduce folio_migratetype()
Posted by Andrew Morton 2 years, 7 months ago
On Tue, 13 Jun 2023 19:13:09 -0700 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote:

> Introduce folio_migratetype() as a folio equivalent for
> get_pageblock_migratetype(). This function intends to return the
> migratetype the folio is located in, hence the name choice.
> 
> ...
>
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -105,6 +105,9 @@ extern int page_group_by_mobility_disabled;
>  #define get_pageblock_migratetype(page)					\
>  	get_pfnblock_flags_mask(page, page_to_pfn(page), MIGRATETYPE_MASK)
>  
> +#define folio_migratetype(folio)				\
> +	get_pfnblock_flags_mask(&folio->page, folio_pfn(folio),		\
> +			MIGRATETYPE_MASK)

Theoretically this is risky because it evaluates its argument more than
once.  Although folio_migratetype(folio++) seems an unlikely thing to do.

An inlined C function is always preferable.  My quick attempt at that
reveals that the header files are All Messed Up As Usual.
Re: [PATCH v2 2/5] mmzone: Introduce folio_migratetype()
Posted by Matthew Wilcox 2 years, 7 months ago
On Wed, Jun 14, 2023 at 01:13:05PM -0700, Andrew Morton wrote:
> On Tue, 13 Jun 2023 19:13:09 -0700 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote:
> 
> > Introduce folio_migratetype() as a folio equivalent for
> > get_pageblock_migratetype(). This function intends to return the
> > migratetype the folio is located in, hence the name choice.
> > 
> > ...
> >
> > --- a/include/linux/mmzone.h
> > +++ b/include/linux/mmzone.h
> > @@ -105,6 +105,9 @@ extern int page_group_by_mobility_disabled;
> >  #define get_pageblock_migratetype(page)					\
> >  	get_pfnblock_flags_mask(page, page_to_pfn(page), MIGRATETYPE_MASK)
> >  
> > +#define folio_migratetype(folio)				\
> > +	get_pfnblock_flags_mask(&folio->page, folio_pfn(folio),		\
> > +			MIGRATETYPE_MASK)
> 
> Theoretically this is risky because it evaluates its argument more than
> once.  Although folio_migratetype(folio++) seems an unlikely thing to do.

folio++ is always an unsafe thing to do.  folios are not consecutive
in memory (unless we know they're order-0).

> An inlined C function is always preferable.  My quick attempt at that
> reveals that the header files are All Messed Up As Usual.

The page-equivalent of this also evaluates its arguments more than once,
so it doesn't see too risky for now?
Re: [PATCH v2 2/5] mmzone: Introduce folio_migratetype()
Posted by Andrew Morton 2 years, 7 months ago
On Wed, 14 Jun 2023 21:17:34 +0100 Matthew Wilcox <willy@infradead.org> wrote:

> On Wed, Jun 14, 2023 at 01:13:05PM -0700, Andrew Morton wrote:
> > On Tue, 13 Jun 2023 19:13:09 -0700 "Vishal Moola (Oracle)" <vishal.moola@gmail.com> wrote:
> > 
> > > Introduce folio_migratetype() as a folio equivalent for
> > > get_pageblock_migratetype(). This function intends to return the
> > > migratetype the folio is located in, hence the name choice.
> > > 
> > > ...
> > >
> > > --- a/include/linux/mmzone.h
> > > +++ b/include/linux/mmzone.h
> > > @@ -105,6 +105,9 @@ extern int page_group_by_mobility_disabled;
> > >  #define get_pageblock_migratetype(page)					\
> > >  	get_pfnblock_flags_mask(page, page_to_pfn(page), MIGRATETYPE_MASK)
> > >  
> > > +#define folio_migratetype(folio)				\
> > > +	get_pfnblock_flags_mask(&folio->page, folio_pfn(folio),		\
> > > +			MIGRATETYPE_MASK)
> > 
> > Theoretically this is risky because it evaluates its argument more than
> > once.  Although folio_migratetype(folio++) seems an unlikely thing to do.
> 
> folio++ is always an unsafe thing to do.  folios are not consecutive
> in memory (unless we know they're order-0).

OK, folio_migratetype(expensive_function_which_returns_a_folio()) or
folio_migratetype(function_with_side_effects_which_returns_a_folio()). 
There are many failure modes.

> > An inlined C function is always preferable.  My quick attempt at that
> > reveals that the header files are All Messed Up As Usual.
> 
> The page-equivalent of this also evaluates its arguments more than once,
> so it doesn't see too risky for now?

We got lucky.   It's just bad practice.