From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751762AbcGRLsd (ORCPT ); Mon, 18 Jul 2016 07:48:33 -0400 Received: from mx2.suse.de ([195.135.220.15]:46419 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751398AbcGRLsb (ORCPT ); Mon, 18 Jul 2016 07:48:31 -0400 Subject: Re: [PATCH v3 17/17] mm, vmscan: make compaction_ready() more accurate and readable To: Joonsoo Kim References: <20160624095437.16385-1-vbabka@suse.cz> <20160624095437.16385-18-vbabka@suse.cz> <20160706055550.GG23627@js1304-P5Q-DELUXE> Cc: Andrew Morton , linux-kernel@vger.kernel.org, linux-mm@kvack.org, Michal Hocko , Mel Gorman , David Rientjes , Rik van Riel From: Vlastimil Babka Message-ID: <934f0092-c9e9-5e38-cb2b-b09de5bd91ef@suse.cz> Date: Mon, 18 Jul 2016 13:48:28 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.1.1 MIME-Version: 1.0 In-Reply-To: <20160706055550.GG23627@js1304-P5Q-DELUXE> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/06/2016 07:55 AM, Joonsoo Kim wrote: > On Fri, Jun 24, 2016 at 11:54:37AM +0200, Vlastimil Babka wrote: >> The compaction_ready() is used during direct reclaim for costly order >> allocations to skip reclaim for zones where compaction should be attempted >> instead. It's combining the standard compaction_suitable() check with its own >> watermark check based on high watermark with extra gap, and the result is >> confusing at best. >> >> This patch attempts to better structure and document the checks involved. >> First, compaction_suitable() can determine that the allocation should either >> succeed already, or that compaction doesn't have enough free pages to proceed. >> The third possibility is that compaction has enough free pages, but we still >> decide to reclaim first - unless we are already above the high watermark with >> gap. This does not mean that the reclaim will actually reach this watermark >> during single attempt, this is rather an over-reclaim protection. So document >> the code as such. The check for compaction_deferred() is removed completely, as >> it in fact had no proper role here. >> >> The result after this patch is mainly a less confusing code. We also skip some >> over-reclaim in cases where the allocation should already succed. >> >> Signed-off-by: Vlastimil Babka >> Acked-by: Michal Hocko >> --- >> mm/vmscan.c | 43 ++++++++++++++++++++----------------------- >> 1 file changed, 20 insertions(+), 23 deletions(-) >> >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index 484ff05d5a8f..724131661f0c 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -2462,40 +2462,37 @@ static bool shrink_zone(struct zone *zone, struct scan_control *sc, >> } >> >> /* >> - * Returns true if compaction should go ahead for a high-order request, or >> - * the high-order allocation would succeed without compaction. >> + * Returns true if compaction should go ahead for a costly-order request, or >> + * the allocation would already succeed without compaction. Return false if we >> + * should reclaim first. >> */ >> static inline bool compaction_ready(struct zone *zone, int order, int classzone_idx) >> { >> unsigned long balance_gap, watermark; >> - bool watermark_ok; >> + enum compact_result suitable; >> + >> + suitable = compaction_suitable(zone, order, 0, classzone_idx); >> + if (suitable == COMPACT_PARTIAL) >> + /* Allocation should succeed already. Don't reclaim. */ >> + return true; >> + if (suitable == COMPACT_SKIPPED) >> + /* Compaction cannot yet proceed. Do reclaim. */ >> + return false; >> >> /* >> - * Compaction takes time to run and there are potentially other >> - * callers using the pages just freed. Continue reclaiming until >> - * there is a buffer of free pages available to give compaction >> - * a reasonable chance of completing and allocating the page >> + * Compaction is already possible, but it takes time to run and there >> + * are potentially other callers using the pages just freed. So proceed >> + * with reclaim to make a buffer of free pages available to give >> + * compaction a reasonable chance of completing and allocating the page. >> + * Note that we won't actually reclaim the whole buffer in one attempt >> + * as the target watermark in should_continue_reclaim() is lower. But if >> + * we are already above the high+gap watermark, don't reclaim at all. >> */ >> balance_gap = min(low_wmark_pages(zone), DIV_ROUND_UP( >> zone->managed_pages, KSWAPD_ZONE_BALANCE_GAP_RATIO)); >> watermark = high_wmark_pages(zone) + balance_gap + compact_gap(order); >> - watermark_ok = zone_watermark_ok_safe(zone, 0, watermark, classzone_idx); > > Hmm... it doesn't explain why both high_wmark_pages and balance_gap > are needed. If we want to make a buffer, one of them would work. Mel's series removed KSWAPD_ZONE_BALANCE_GAP_RATIO meanwhile, so that should be fine.