On Thu, 5 Sep 2019, Vlastimil Babka wrote:
> >> - failing order-0 watermark checks in memory compaction does not account
> >> for how far below the watermarks the zone actually is: to enable
> >> migration, there must be *some* free memory available. Per the above,
> >> watermarks are not always suffficient if isolate_freepages() cannot
> >> find the free memory but it could require hundreds of MBs of reclaim to
> >> even reach this threshold (read: potentially very expensive reclaim with
> >> no indication compaction can be successful), and
>
> I doubt it's hundreds of MBs for a 2MB hugepage.
>
I'm not sure how you presume to know, we certainly have incidents where
compaction is skipped because free pages are are 100MB+ under low
watermarks.
> >> For hugepage allocations, these are quite substantial drawbacks because
> >> these are very high order allocations (order-9 on x86) and falling back to
> >> doing reclaim can potentially be *very* expensive without any indication
> >> that compaction would even be successful.
>
> You seem to lump together hugetlbfs and THP here, by saying "hugepage",
> but these are very different things - hugetlbfs reservations are
> expected to be potentially expensive.
>
Mike Kravetz followed up and I can make a simple change to this fix to
only run the new logic if the allocation is not using __GFP_RETRY_MAYFAIL
which would exclude hugetlb allocations and include transparent hugepage
allocations.
> >> Reclaim itself is unlikely to free entire pageblocks and certainly no
> >> reliance should be put on it to do so in isolation (recall lumpy reclaim).
> >> This means we should avoid reclaim and simply fail hugepage allocation if
> >> compaction is deferred.
>
> It is however possible that reclaim frees enough to make even a
> previously deferred compaction succeed.
>
This is another way that the return value that we get from memory
compaction can be improved since right now we only check
compaction_deferred() at the priorities we care about. This discussion
has revealed several areas where we can get more reliable and actionable
return values from memory compaction to implement a sane default policy in
the page allocator that works for everybody.
> >> It is also not helpful to thrash a zone by doing excessive reclaim if
> >> compaction may not be able to access that memory. If order-0 watermarks
> >> fail and the allocation order is sufficiently large, it is likely better
> >> to fail the allocation rather than thrashing the zone.
> >>
> >> Signed-off-by: David Rientjes <rientjes@xxxxxxxxxx>
> >> ---
> >> mm/page_alloc.c | 22 ++++++++++++++++++++++
> >> 1 file changed, 22 insertions(+)
> >>
> >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >> --- a/mm/page_alloc.c
> >> +++ b/mm/page_alloc.c
> >> @@ -4458,6 +4458,28 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >> if (page)
> >> goto got_pg;
> >>
> >> + if (order >= pageblock_order && (gfp_mask & __GFP_IO)) {
> >> + /*
> >> + * If allocating entire pageblock(s) and compaction
> >> + * failed because all zones are below low watermarks
> >> + * or is prohibited because it recently failed at this
> >> + * order, fail immediately.
> >> + *
> >> + * Reclaim is
> >> + * - potentially very expensive because zones are far
> >> + * below their low watermarks or this is part of very
> >> + * bursty high order allocations,
> >> + * - not guaranteed to help because isolate_freepages()
> >> + * may not iterate over freed pages as part of its
> >> + * linear scan, and
> >> + * - unlikely to make entire pageblocks free on its
> >> + * own.
> >> + */
> >> + if (compact_result == COMPACT_SKIPPED ||
> >> + compact_result == COMPACT_DEFERRED)
> >> + goto nopage;
>
> As I said, I expect this will make hugetlbfs reservations fail
> prematurely - Mike can probably confirm or disprove that.
> I think it also addresses consequences, not the primary problem, IMHO.
> I believe the primary problem is that we reclaim something even if
> there's enough memory for compaction. This won't change with your patch,
> as compact_result won't be SKIPPED in that case.
I'm relying only on Andrea's one line feedback saying that this would
address the swap storms that he is reporting, more details on why it
doesn't, if it doesn't, would definitely be helpful.
> Then we continue
> through to __alloc_pages_direct_reclaim(), shrink_zones() which will
> call compaction_ready(), which will only return true and skip reclaim of
> the zone, if there's high_watermark (!!!) + compact_gap() pages.
Interesting find, that heuristic certainly doesn't appear consistent.
Another thing to add to the list for how the memory compaction, direct
reclaim, and page allocator feedback loop can be improved to provide sane
default behavior for everybody.
If you'd like to send a patch to address this issue specifically, that
would be very helpful! I'm hoping Andrea can test it.
