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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>
