lkml.org 
[lkml]   [2016]   [Apr]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers
    From
    Date
    On 04/05/2016 01:25 PM, Michal Hocko wrote:
    > From: Michal Hocko <mhocko@suse.com>
    >
    > Compaction can provide a wild variation of feedback to the caller. Many
    > of them are implementation specific and the caller of the compaction
    > (especially the page allocator) shouldn't be bound to specifics of the
    > current implementation.
    >
    > This patch abstracts the feedback into three basic types:
    > - compaction_made_progress - compaction was active and made some
    > progress.
    > - compaction_failed - compaction failed and further attempts to
    > invoke it would most probably fail and therefore it is not
    > worth retrying
    > - compaction_withdrawn - compaction wasn't invoked for an
    > implementation specific reasons. In the current implementation
    > it means that the compaction was deferred, contended or the
    > page scanners met too early without any progress. Retrying is
    > still worthwhile.
    >
    > Signed-off-by: Michal Hocko <mhocko@suse.com>
    > ---
    > include/linux/compaction.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++
    > mm/page_alloc.c | 25 ++++------------
    > 2 files changed, 80 insertions(+), 19 deletions(-)
    >
    > diff --git a/include/linux/compaction.h b/include/linux/compaction.h
    > index a7b9091ff349..512db9c3f0ed 100644
    > --- a/include/linux/compaction.h
    > +++ b/include/linux/compaction.h
    > @@ -78,6 +78,70 @@ extern void compaction_defer_reset(struct zone *zone, int order,
    > bool alloc_success);
    > extern bool compaction_restarting(struct zone *zone, int order);
    >
    > +/* Compaction has made some progress and retrying makes sense */
    > +static inline bool compaction_made_progress(enum compact_result result)
    > +{
    > + /*
    > + * Even though this might sound confusing this in fact tells us
    > + * that the compaction successfully isolated and migrated some
    > + * pageblocks.
    > + */
    > + if (result == COMPACT_PARTIAL)
    > + return true;
    > +
    > + return false;
    > +}
    > +
    > +/* Compaction has failed and it doesn't make much sense to keep retrying. */
    > +static inline bool compaction_failed(enum compact_result result)
    > +{
    > + /* All zones where scanned completely and still not result. */

    Hmm given that try_to_compact_pages() uses a max() on results, then in
    fact it takes only one zone to get this. Others could have been also
    SKIPPED or DEFERRED. Is that what you want?

    > + if (result == COMPACT_COMPLETE)
    > + return true;
    > +
    > + return false;
    > +}
    > +
    > +/*
    > + * Compaction has backed off for some reason. It might be throttling or
    > + * lock contention. Retrying is still worthwhile.
    > + */
    > +static inline bool compaction_withdrawn(enum compact_result result)
    > +{
    > + /*
    > + * Compaction backed off due to watermark checks for order-0
    > + * so the regular reclaim has to try harder and reclaim something.
    > + */
    > + if (result == COMPACT_SKIPPED)
    > + return true;
    > +
    > + /*
    > + * If compaction is deferred for high-order allocations, it is
    > + * because sync compaction recently failed. If this is the case
    > + * and the caller requested a THP allocation, we do not want
    > + * to heavily disrupt the system, so we fail the allocation
    > + * instead of entering direct reclaim.
    > + */
    > + if (result == COMPACT_DEFERRED)
    > + return true;
    > +
    > + /*
    > + * If compaction in async mode encounters contention or blocks higher
    > + * priority task we back off early rather than cause stalls.
    > + */
    > + if (result == COMPACT_CONTENDED)
    > + return true;
    > +
    > + /*
    > + * Page scanners have met but we haven't scanned full zones so this
    > + * is a back off in fact.
    > + */
    > + if (result == COMPACT_PARTIAL_SKIPPED)
    > + return true;
    > +
    > + return false;
    > +}
    > +

    [...]

    > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
    > index c37e6d1ad643..c05de84c8157 100644
    > --- a/mm/page_alloc.c
    > +++ b/mm/page_alloc.c
    > @@ -3362,25 +3362,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
    > if (page)
    > goto got_pg;
    >
    > - /* Checks for THP-specific high-order allocations */
    > - if (is_thp_gfp_mask(gfp_mask)) {
    > - /*
    > - * If compaction is deferred for high-order allocations, it is
    > - * because sync compaction recently failed. If this is the case
    > - * and the caller requested a THP allocation, we do not want
    > - * to heavily disrupt the system, so we fail the allocation
    > - * instead of entering direct reclaim.
    > - */
    > - if (compact_result == COMPACT_DEFERRED)
    > - goto nopage;
    > -
    > - /*
    > - * Compaction is contended so rather back off than cause
    > - * excessive stalls.
    > - */
    > - if(compact_result == COMPACT_CONTENDED)
    > - goto nopage;
    > - }
    > + /*
    > + * Checks for THP-specific high-order allocations and back off
    > + * if the the compaction backed off
    > + */
    > + if (is_thp_gfp_mask(gfp_mask) && compaction_withdrawn(compact_result))
    > + goto nopage;

    The change of semantics for THP is not trivial here and should at least
    be discussed in changelog. CONTENDED and DEFERRED is only subset of
    compaction_withdrawn() as seen above. Why is it useful to back off due
    to COMPACT_PARTIAL_SKIPPED (we were just unlucky in our starting
    position), but not due to COMPACT_COMPLETE (we have seen the whole zone
    but failed anyway)? Why back off due to COMPACT_SKIPPED (not enough
    order-0 pages) without trying reclaim at least once, and then another
    async compaction, like before?

    >
    > /*
    > * It can become very expensive to allocate transparent hugepages at
    >

    \
     
     \ /
      Last update: 2016-04-11 17:01    [W:3.525 / U:0.032 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site