Re: [patch 2/2] mm, compaction: persistently skip hugetlbfs pageblocks

[David Rientjes]

On Mon, 11 Sep 2017, Vlastimil Babka wrote:

> > Yes, any page where compound_order(page) == pageblock_order would probably 
> > benefit from the same treatment.  I haven't encountered such an issue, 
> > however, so I thought it was best to restrict it only to hugetlb: hugetlb 
> > memory usually sits in the hugetlb free pool and seldom gets freed under 
> > normal conditions even when unmapped whereas thp is much more likely to be 
> > unmapped and split.  I wasn't sure that it was worth the pageblock skip.
> 
> Well, my thinking is that once we start checking page properties when
> resetting the skip bits, we might as well try to get the most of it, as
> there's no additional cost.
> 

There's no additional cost, but I have doubts of how persistent the 
conditions you're checking really are.  I know that hugetlb memory 
normally sits in a hugetlb free pool when unmapped by a user process, very 
different from thp memory that can always be unmapped and split.  I would 
consider PageHuge() to be inferred as a more persistent condition than thp 
memory.

> >>> @@ -241,6 +255,8 @@ static void __reset_isolation_suitable(struct zone *zone)
> >>>  			continue;
> >>>  		if (zone != page_zone(page))
> >>>  			continue;
> >>> +		if (pageblock_skip_persistent(page, compound_order(page)))
> >>> +			continue;
> >>
> >> I like the idea of how persistency is achieved by rechecking in the reset.
> >>
> >>>  
> >>>  		clear_pageblock_skip(page);
> >>>  	}
> >>> @@ -448,13 +464,15 @@ static unsigned long isolate_freepages_block(struct compact_control *cc,
> >>>  		 * and the only danger is skipping too much.
> >>>  		 */
> >>>  		if (PageCompound(page)) {
> >>> -			unsigned int comp_order = compound_order(page);
> >>> -
> >>> -			if (likely(comp_order < MAX_ORDER)) {
> >>> -				blockpfn += (1UL << comp_order) - 1;
> >>> -				cursor += (1UL << comp_order) - 1;
> >>> +			const unsigned int order = compound_order(page);
> >>> +
> >>> +			if (pageblock_skip_persistent(page, order)) {
> >>> +				set_pageblock_skip(page);
> >>> +				blockpfn = end_pfn;
> >>> +			} else if (likely(order < MAX_ORDER)) {
> >>> +				blockpfn += (1UL << order) - 1;
> >>> +				cursor += (1UL << order) - 1;
> >>>  			}
> >>
> >> Is this new code (and below) really necessary? The existing code should
> >> already lead to skip bit being set via update_pageblock_skip()?
> >>
> > 
> > I wanted to set the persistent pageblock skip regardless of 
> > cc->ignore_skip_hint without a local change to update_pageblock_skip().
> 
> After the first patch, there are no ignore_skip_hint users where it
> would make that much difference overriding the flag for some pageblocks
> (which this effectively does) at the cost of more complicated code.
> 

No objection to a patch that sets the skip only as part of 
update_pageblock_skip(), but that is not combined with changing the
pageblock_skip_persistent() logic, which is a separate issue.