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

[David Hildenbrand <david@redhat.com>]

>>> +#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