Re: [PATCH v4 1/4] mm/page_isolation: make page isolation a standalone bit.

[David Hildenbrand]

> > > +#ifdef CONFIG_MEMORY_ISOLATION
> > > +	if (flags & PB_migrate_isolate_bit)
> > > +		return MIGRATE_ISOLATE;
> > > +#endif
> >
> > If you call get_pfnblock_flags_mask() with MIGRATETYPE_MASK, how could you ever get PB_migrate_isolate_bit?
>
> MIGRATETYPE_MASK is ((BIT(PB_migratetype_bits) - 1) | PB_migrate_isolate_bit),
> so it gets PB_migrate_isolate_bit.

Oh ... that's confusing.

> > I think what we should do is
> >
> > 1) Rename get_pfnblock_flags_mask() to get_pfnblock_flags()
> >
> > 2) Remove the mask parameter
> >
> > 3) Perform the masking in all callers.
>
> get_pfnblock_flags_mask() is also used by get_pageblock_skip() to
> get PB_migrate_skip. I do not think we want to include PB_migrate_skip
> in the mask to confuse readers.

The masking will be handled in the caller.

So get_pageblock_skip() would essentially do a

return get_pfnblock_flags() & PB_migrate_skip_bit;

etc.

> > Maybe, we should convert set_pfnblock_flags_mask() to
> >
> > void set_clear_pfnblock_flags(struct page *page, unsigned long
> > 			      set_flags, unsigned long clear_flags);
> >
> > And better, splitting it up (or providing helpers)
> >
> > set_pfnblock_flags(struct page *page, unsigned long flags);
> > clear_pfnblock_flags(struct page *page, unsigned long flags);
> >
> >
> > This implies some more code cleanups first that make the code easier to extend.
>
> The same due to PB_migrate_skip.
>
> Based on your suggestion, we could make {set,get}_pfnblock_flags_mask()
> internal APIs by prepending "__". They are only used by the new
> {get, set, clear}_pfnblock_flags() and {get, set, clear}_pageblock_{skip, isolate}().
> Then use {get, set, clear}_pfnblock_flags() for all migratetype operations.
>
> WDYT?

In general, lgtm. I just hope we can avoid the "_mask" part and just 
handle it in these functions directly?

> > > +	return flags;
> > >    }
> > >     /**
> > > @@ -402,8 +423,14 @@ void set_pfnblock_flags_mask(struct page *page, unsigned long flags,
> > >    	unsigned long bitidx, word_bitidx;
> > >    	unsigned long word;
> > >   +#ifdef CONFIG_MEMORY_ISOLATION
> > > +	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 8);
> > > +	/* extra one for MIGRATE_ISOLATE */
> > > +	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits) + 1);
> > > +#else
> > >    	BUILD_BUG_ON(NR_PAGEBLOCK_BITS != 4);
> > >    	BUILD_BUG_ON(MIGRATE_TYPES > (1 << PB_migratetype_bits));
> > > +#endif
> > >     	bitmap = get_pageblock_bitmap(page, pfn);
> > >    	bitidx = pfn_to_bitidx(page, pfn);
> > > @@ -426,6 +453,13 @@ void set_pageblock_migratetype(struct page *page, int migratetype)
> > >    		     migratetype < MIGRATE_PCPTYPES))
> > >    		migratetype = MIGRATE_UNMOVABLE;
> > >   +#ifdef CONFIG_MEMORY_ISOLATION
> > > +	if (migratetype == MIGRATE_ISOLATE) {
> > > +		set_pfnblock_flags_mask(page, PB_migrate_isolate_bit,
> > > +				page_to_pfn(page), PB_migrate_isolate_bit);
> > > +		return;
> > > +	}
> > > +#endif
> > >    	set_pfnblock_flags_mask(page, (unsigned long)migratetype,
> > >    				page_to_pfn(page), MIGRATETYPE_MASK);
> > >    }
> > > diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> > > index b2fc5266e3d2..751e21f6d85e 100644
> > > --- a/mm/page_isolation.c
> > > +++ b/mm/page_isolation.c
> > > @@ -15,6 +15,17 @@
> > >    #define CREATE_TRACE_POINTS
> > >    #include <trace/events/page_isolation.h>
> > >   +static inline bool __maybe_unused get_pageblock_isolate(struct page *page)
> > > +{
> > > +	return get_pfnblock_flags_mask(page, page_to_pfn(page),
> > > +			PB_migrate_isolate_bit);
> > > +}
> > > +static inline void clear_pageblock_isolate(struct page *page)
> > > +{
> > > +	set_pfnblock_flags_mask(page, 0, page_to_pfn(page),
> > > +			PB_migrate_isolate_bit);
> > > +}
> >
> > Should these reside in include/linux/pageblock-flags.h, just like the
> > CONFIG_COMPACTION "skip" variants?
>
> They are only used inside mm/page_isolation.c, so I would leave them
> here until other users come out.

get_pageblock_skip() and friends are also only used in mm/compaction.c.

Having these simple wrapper as inline functions in the same header 
should make it consistent.

... and avoid tricks like "__maybe_unused" here :)


--
Cheers,

David / dhildenb