Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1730103 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [patch 2/2] mm, compaction: persistently skip hugetlbfs  pageblocks David Rientjes <rientjes@google.com> - 2017-09-11 03:20 +0200
    Re: [patch 2/2] mm, compaction: persistently skip hugetlbfs  pageblocks Vlastimil Babka <vbabka@suse.cz> - 2017-09-11 09:00 +0200
      Re: [patch 2/2] mm, compaction: persistently skip hugetlbfs  pageblocks David Rientjes <rientjes@google.com> - 2017-09-11 23:20 +0200

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

On Wed, 23 Aug 2017, Vlastimil Babka wrote:

> > diff --git a/mm/compaction.c b/mm/compaction.c
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -217,6 +217,20 @@ static void reset_cached_positions(struct zone *zone)
> >  				pageblock_start_pfn(zone_end_pfn(zone) - 1);
> >  }
> >  
> > +/*
> > + * Hugetlbfs pages should consistenly be skipped until updated by the hugetlb
> > + * subsystem.  It is always pointless to compact pages of pageblock_order and
> > + * the free scanner can reconsider when no longer huge.
> > + */
> > +static bool pageblock_skip_persistent(struct page *page, unsigned int order)
> > +{
> > +	if (!PageHuge(page))
> > +		return false;
> > +	if (order != pageblock_order)
> > +		return false;
> > +	return true;
> 
> Why just HugeTLBfs? There's also no point in migrating/finding free
> pages in THPs. Actually, any compound page of pageblock order?
> 

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.

> > +}
> > +
> >  /*
> >   * This function is called to clear all cached information on pageblocks that
> >   * should be skipped for page isolation when the migrate and free page scanner
> > @@ -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().

[toc] | [next] | [standalone]

#1730169

On 09/11/2017 03:12 AM, David Rientjes wrote:
> On Wed, 23 Aug 2017, Vlastimil Babka wrote:
> 
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -217,6 +217,20 @@ static void reset_cached_positions(struct zone *zone)
>>>  				pageblock_start_pfn(zone_end_pfn(zone) - 1);
>>>  }
>>>  
>>> +/*
>>> + * Hugetlbfs pages should consistenly be skipped until updated by the hugetlb
>>> + * subsystem.  It is always pointless to compact pages of pageblock_order and
>>> + * the free scanner can reconsider when no longer huge.
>>> + */
>>> +static bool pageblock_skip_persistent(struct page *page, unsigned int order)
>>> +{
>>> +	if (!PageHuge(page))
>>> +		return false;
>>> +	if (order != pageblock_order)
>>> +		return false;
>>> +	return true;
>>
>> Why just HugeTLBfs? There's also no point in migrating/finding free
>> pages in THPs. Actually, any compound page of pageblock order?
>>
> 
> 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.

>>> +}
>>> +
>>>  /*
>>>   * This function is called to clear all cached information on pageblocks that
>>>   * should be skipped for page isolation when the migrate and free page scanner
>>> @@ -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.

[toc] | [prev] | [next] | [standalone]

#1730557

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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web