On Wed 25-03-15 02:17:11, Johannes Weiner wrote:
> The should_alloc_retry() function was meant to encapsulate retry
> conditions of the allocator slowpath, but there are still checks
> remaining in the main function, and much of how the retrying is
> performed also depends on the OOM killer progress. The physical
> separation of those conditions make the code hard to follow.
>
> Inline the should_alloc_retry() checks. Notes:
>
> - The __GFP_NOFAIL check is already done in __alloc_pages_may_oom(),
> replace it with looping on OOM killer progress
>
> - The pm_suspended_storage() check is meant to skip the OOM killer
> when reclaim has no IO available, move to __alloc_pages_may_oom()
>
> - The order < PAGE_ALLOC_COSTLY order is re-united with its original
> counterpart of checking whether reclaim actually made any progress
it should be order <= PAGE_ALLOC_COSTLY
> Signed-off-by: Johannes Weiner <hannes@xxxxxxxxxxx>
The resulting code looks much better and logical.
After the COSTLY check is fixed.
Acked-by: Michal Hocko <mhocko@xxxxxxx>
> ---
> mm/page_alloc.c | 104 +++++++++++++++++---------------------------------------
> 1 file changed, 32 insertions(+), 72 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 9ebc760187ac..c1224ba45548 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2329,48 +2329,6 @@ void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...)
> show_mem(filter);
> }
>
> -static inline int
> -should_alloc_retry(gfp_t gfp_mask, unsigned int order,
> - unsigned long did_some_progress,
> - unsigned long pages_reclaimed)
> -{
> - /* Do not loop if specifically requested */
> - if (gfp_mask & __GFP_NORETRY)
> - return 0;
> -
> - /* Always retry if specifically requested */
> - if (gfp_mask & __GFP_NOFAIL)
> - return 1;
> -
> - /*
> - * Suspend converts GFP_KERNEL to __GFP_WAIT which can prevent reclaim
> - * making forward progress without invoking OOM. Suspend also disables
> - * storage devices so kswapd will not help. Bail if we are suspending.
> - */
> - if (!did_some_progress && pm_suspended_storage())
> - return 0;
> -
> - /*
> - * In this implementation, order <= PAGE_ALLOC_COSTLY_ORDER
> - * means __GFP_NOFAIL, but that may not be true in other
> - * implementations.
> - */
> - if (order <= PAGE_ALLOC_COSTLY_ORDER)
> - return 1;
> -
> - /*
> - * For order > PAGE_ALLOC_COSTLY_ORDER, if __GFP_REPEAT is
> - * specified, then we retry until we no longer reclaim any pages
> - * (above), or we've reclaimed an order of pages at least as
> - * large as the allocation's order. In both cases, if the
> - * allocation still fails, we stop retrying.
> - */
> - if (gfp_mask & __GFP_REPEAT && pages_reclaimed < (1 << order))
> - return 1;
> -
> - return 0;
> -}
> -
> static inline struct page *
> __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
> const struct alloc_context *ac, unsigned long *did_some_progress)
> @@ -2409,16 +2367,18 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
> /* The OOM killer does not needlessly kill tasks for lowmem */
> if (ac->high_zoneidx < ZONE_NORMAL)
> goto out;
> - /* The OOM killer does not compensate for light reclaim */
> + /* The OOM killer does not compensate for IO-less reclaim */
> if (!(gfp_mask & __GFP_FS)) {
> /*
> * XXX: Page reclaim didn't yield anything,
> * and the OOM killer can't be invoked, but
> - * keep looping as per should_alloc_retry().
> + * keep looping as per tradition.
> */
> *did_some_progress = 1;
> goto out;
> }
> + if (pm_suspended_storage())
> + goto out;
> /* The OOM killer may not free memory on a specific node */
> if (gfp_mask & __GFP_THISNODE)
> goto out;
> @@ -2801,40 +2761,40 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> if (page)
> goto got_pg;
>
> - /* Check if we should retry the allocation */
> + /* Do not loop if specifically requested */
> + if (gfp_mask & __GFP_NORETRY)
> + goto noretry;
> +
> + /* Keep reclaiming pages as long as there is reasonable progress */
> pages_reclaimed += did_some_progress;
> - if (should_alloc_retry(gfp_mask, order, did_some_progress,
> - pages_reclaimed)) {
> - /*
> - * If we fail to make progress by freeing individual
> - * pages, but the allocation wants us to keep going,
> - * start OOM killing tasks.
> - */
> - if (!did_some_progress) {
> - page = __alloc_pages_may_oom(gfp_mask, order, ac,
> - &did_some_progress);
> - if (page)
> - goto got_pg;
> - if (!did_some_progress)
> - goto nopage;
> - }
> + if ((did_some_progress && order < PAGE_ALLOC_COSTLY_ORDER) ||
> + ((gfp_mask & __GFP_REPEAT) && pages_reclaimed < (1 << order))) {
> /* Wait for some write requests to complete then retry */
> wait_iff_congested(ac->preferred_zone, BLK_RW_ASYNC, HZ/50);
> goto retry;
> - } else {
> - /*
> - * High-order allocations do not necessarily loop after
> - * direct reclaim and reclaim/compaction depends on compaction
> - * being called after reclaim so call directly if necessary
> - */
> - page = __alloc_pages_direct_compact(gfp_mask, order,
> - alloc_flags, ac, migration_mode,
> - &contended_compaction,
> - &deferred_compaction);
> - if (page)
> - goto got_pg;
> }
>
> + /* Reclaim has failed us, start killing things */
> + page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
> + if (page)
> + goto got_pg;
> +
> + /* Retry as long as the OOM killer is making progress */
> + if (did_some_progress)
> + goto retry;
> +
> +noretry:
> + /*
> + * High-order allocations do not necessarily loop after
> + * direct reclaim and reclaim/compaction depends on compaction
> + * being called after reclaim so call directly if necessary
> + */
> + page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
> + ac, migration_mode,
> + &contended_compaction,
> + &deferred_compaction);
> + if (page)
> + goto got_pg;
> nopage:
> warn_alloc_failed(gfp_mask, order, NULL);
> got_pg:
> --
> 2.3.3
>
--
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
