lkml.org 
[lkml]   [2017]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [patch 2/2] mm, compaction: persistently skip hugetlbfs pageblocks
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.

\
 
 \ /
  Last update: 2017-09-11 23:12    [W:0.049 / U:5.740 seconds]
©2003-2017 Jasper Spaans. hosted at Digital OceanAdvertise on this site