Message ID | 20230519123959.77335-3-hannes@cmpxchg.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp1204652vqo; Fri, 19 May 2023 05:44:40 -0700 (PDT) X-Google-Smtp-Source: ACHHUZ4pZ6BRWWNR6ZV5WThipKHYur1iIybhuHwxbraigG68elF9vslgFlG81FPWQk/TNyhv/ZS9 X-Received: by 2002:a05:6a20:7f86:b0:103:b436:aef7 with SMTP id d6-20020a056a207f8600b00103b436aef7mr3301898pzj.16.1684500280552; Fri, 19 May 2023 05:44:40 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1684500280; cv=none; d=google.com; s=arc-20160816; b=omwMwHWuqgZ8ZbXjBYp4mrFnhlm15yE2kw5sytLBzAaBM4fxqi1JoA14+aLpYzB70J sJdy2iQV0cyShMP0M0VeZnSC8v2/kIjvgAICtEeyxaD+m5FD73r48uT7OMMvXj/pGCrZ 8W3H7fuhhiebXE5+cerEaBvTNPost+xJi9CMEkJHj58EAwNPrHmNVIT2mJmnheeXso7W 3ExZySsMedbtrqPtCXyAKf9C6tOvxI8xhrWLdF7WHRXXze0KFew2N69L1CQP5r2D2thh 48itZud0+6fDY5UfBMf+MWfLhXkgpAHNKTd2mfhEUHrE/8vdTz4xaDsxZtpYTCw+gvmm j8bw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=KcIbsIikfDMuopSmhbHndV34wIxjpvvu7t5RPKLo+Ds=; b=Gg5dCd7aO7T+VV+imNhDroCXt+GfZ7UDJUuzNNdYi9vsIT/TMNgD9/wC5HF/lOax6c 2RQIhhYU9cjqL9qf9sXWceCEdfb58cdmYBuBRfFAp2dnR3mQcx+Cu/WJxd7S34hgHVS0 VM34/gFnpn7baJ8luu4MYAAKiE8gqZQaMPTeIgWSZnOD88Y3jWDz0grTRX/xS3iJg9ub mYdV3zjyGIoDstf80ZhUTcFVLm5R5rAjvwXzbqyA7+BqOJZH0vP4Z/yyCxb5DV2tmwjE PB8oCJjwnteGITM8w1Dd3wDw8JO9njLgfIxk7m5IuEZkUAbtHC+26m0G71G0gWOO7nfL Qm2Q== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@cmpxchg-org.20221208.gappssmtp.com header.s=20221208 header.b=sb6jjHiP; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=cmpxchg.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t29-20020a63955d000000b005346bc0f749si2752270pgn.236.2023.05.19.05.44.24; Fri, 19 May 2023 05:44:40 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@cmpxchg-org.20221208.gappssmtp.com header.s=20221208 header.b=sb6jjHiP; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=cmpxchg.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231400AbjESMkX (ORCPT <rfc822;wlfightup@gmail.com> + 99 others); Fri, 19 May 2023 08:40:23 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54450 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229586AbjESMkM (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 19 May 2023 08:40:12 -0400 Received: from mail-ej1-x62b.google.com (mail-ej1-x62b.google.com [IPv6:2a00:1450:4864:20::62b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D0A94102 for <linux-kernel@vger.kernel.org>; Fri, 19 May 2023 05:40:10 -0700 (PDT) Received: by mail-ej1-x62b.google.com with SMTP id a640c23a62f3a-96aadfb19d7so609384466b.2 for <linux-kernel@vger.kernel.org>; Fri, 19 May 2023 05:40:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20221208.gappssmtp.com; s=20221208; t=1684500009; x=1687092009; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=KcIbsIikfDMuopSmhbHndV34wIxjpvvu7t5RPKLo+Ds=; b=sb6jjHiPTREDcXdqa2JjB543aE4BffIZF/xpQ81XMOPbM5HDi7RIw/E2nSt+nx319g gPJgXd5jWMC7y6+vkLqrHE+0zbFS3gbHUeppbPpBCiBAohfGYKUfPFayPpzAob9UXhxE QEegswrzo+Jdcji9zBexb7zusPe6L0fKPR2rbjXa/EhMVS7NUITvoive3AA6Xwbeno+A CwED3HrTiR3swlcHMBb0od8FoLayohcwFMLmKVO2c2Y2qhgfs87pyMfAMwlsVGKgzU0o AYN8sqSYl+atdFoHRcFi2lu+/nsFdkF/r6TrTSzeUZT2EACdDjtMk4pC+XdWoe8U+5xo fkVQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1684500009; x=1687092009; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=KcIbsIikfDMuopSmhbHndV34wIxjpvvu7t5RPKLo+Ds=; b=Rvup7eSVESUeeeXTPL/mbT3ijKTm66o04ititcczos2YrhCLJec+DGA0nIEHKbmrIR Vr6Nt4uduNU0UmRbrAQgTEXx+YQheHacOtCOFcd/tuDMxpViotICTAw7/pTV4wxYmfYb Wro96y21MfR1+f0xScDBermbSBWpquc/4Pgoy++YM4+xzRvg+8HsU6GAuE28huqJv1sD Dt6KEV518TMTbCzChG/cQCwYPmpQN0TTH6aFP/wb/rs7btHynab+doVLjcdSX3bw39aj UjU2p35Dj5CBkiX1PcMPIO6htt/mYk6cZ6/XREaqYGk5zv9rfEW4p90hiKp6Y75pDwOf c83w== X-Gm-Message-State: AC+VfDzWO0/TlGc11WuMBBkUZX4J3PT1q9ocNMP6+2uDIqiULbKgZvv5 5G/3Wgl191X3+rOjrAnRlzml8g== X-Received: by 2002:a17:907:1c0e:b0:969:dda1:3896 with SMTP id nc14-20020a1709071c0e00b00969dda13896mr2224646ejc.0.1684500009025; Fri, 19 May 2023 05:40:09 -0700 (PDT) Received: from localhost ([2a02:8070:6389:7d40:e266:3092:9afb:a7b1]) by smtp.gmail.com with ESMTPSA id l18-20020a170906a41200b00965a52d2bf6sm2253368ejz.88.2023.05.19.05.40.08 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 19 May 2023 05:40:08 -0700 (PDT) From: Johannes Weiner <hannes@cmpxchg.org> To: Andrew Morton <akpm@linux-foundation.org> Cc: Mel Gorman <mgorman@techsingularity.net>, Vlastimil Babka <vbabka@suse.cz>, Michal Hocko <mhocko@suse.com>, linux-mm@kvack.org, linux-kernel@vger.kernel.org, kernel-team@fb.com Subject: [PATCH 2/5] mm: compaction: simplify should_compact_retry() Date: Fri, 19 May 2023 14:39:56 +0200 Message-Id: <20230519123959.77335-3-hannes@cmpxchg.org> X-Mailer: git-send-email 2.40.0 In-Reply-To: <20230519123959.77335-1-hannes@cmpxchg.org> References: <20230519123959.77335-1-hannes@cmpxchg.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1766326566058997171?= X-GMAIL-MSGID: =?utf-8?q?1766326566058997171?= |
Series |
mm: compaction: cleanups & simplifications
|
|
Commit Message
Johannes Weiner
May 19, 2023, 12:39 p.m. UTC
The different branches for retry are unnecessarily complicated. There
are really only three outcomes: progress (retry n times), skipped
(retry if reclaim can help), failed (retry with higher priority).
Rearrange the branches and the retry counter to make it simpler.
v2:
- fix trace point build (Mel)
- fix max_retries logic for costly allocs (Huang)
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
mm/page_alloc.c | 53 +++++++++++++++----------------------------------
1 file changed, 16 insertions(+), 37 deletions(-)
Comments
On 5/19/23 14:39, Johannes Weiner wrote: > The different branches for retry are unnecessarily complicated. There > are really only three outcomes: progress (retry n times), skipped > (retry if reclaim can help), failed (retry with higher priority). > > Rearrange the branches and the retry counter to make it simpler. > > v2: > - fix trace point build (Mel) > - fix max_retries logic for costly allocs (Huang) > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > --- > mm/page_alloc.c | 53 +++++++++++++++---------------------------------- > 1 file changed, 16 insertions(+), 37 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 5a84a0bebc37..72660e924b95 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3772,16 +3772,22 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > * Compaction managed to coalesce some page blocks, but the > * allocation failed presumably due to a race. Retry some. > */ > - if (compact_result == COMPACT_SUCCESS) > - (*compaction_retries)++; > + if (compact_result == COMPACT_SUCCESS) { > + /* > + * !costly requests are much more important than > + * __GFP_RETRY_MAYFAIL costly ones because they are de > + * facto nofail and invoke OOM killer to move on while > + * costly can fail and users are ready to cope with > + * that. 1/4 retries is rather arbitrary but we would > + * need much more detailed feedback from compaction to > + * make a better decision. > + */ > + if (order > PAGE_ALLOC_COSTLY_ORDER) > + max_retries /= 4; > > - /* > - * All zones were scanned completely and still no result. It > - * doesn't really make much sense to retry except when the > - * failure could be caused by insufficient priority > - */ > - if (compact_result == COMPACT_COMPLETE) > - goto check_priority; > + ret = ++(*compaction_retries) <= max_retries; > + goto out; I think you simplified this part too much, so now once it runs out of retries, it will return false, while previously it would increase the priority. > + } > > /* > * Compaction was skipped due to a lack of free order-0 > @@ -3793,35 +3799,8 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > } > > /* > - * If compaction backed due to being deferred, due to > - * contended locks in async mode, or due to scanners meeting > - * after a partial scan, retry with increased priority. > - */ > - if (compact_result == COMPACT_DEFERRED || > - compact_result == COMPACT_CONTENDED || > - compact_result == COMPACT_PARTIAL_SKIPPED) > - goto check_priority; > - > - /* > - * !costly requests are much more important than __GFP_RETRY_MAYFAIL > - * costly ones because they are de facto nofail and invoke OOM > - * killer to move on while costly can fail and users are ready > - * to cope with that. 1/4 retries is rather arbitrary but we > - * would need much more detailed feedback from compaction to > - * make a better decision. > - */ > - if (order > PAGE_ALLOC_COSTLY_ORDER) > - max_retries /= 4; > - if (*compaction_retries <= max_retries) { > - ret = true; > - goto out; > - } > - > - /* > - * Make sure there are attempts at the highest priority if we exhausted > - * all retries or failed at the lower priorities. > + * Compaction failed. Retry with increasing priority. > */ > -check_priority: > min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ? > MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY; >
On Mon, May 29, 2023 at 03:03:52PM +0200, Vlastimil Babka wrote: > On 5/19/23 14:39, Johannes Weiner wrote: > > The different branches for retry are unnecessarily complicated. There > > are really only three outcomes: progress (retry n times), skipped > > (retry if reclaim can help), failed (retry with higher priority). > > > > Rearrange the branches and the retry counter to make it simpler. > > > > v2: > > - fix trace point build (Mel) > > - fix max_retries logic for costly allocs (Huang) > > > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > --- > > mm/page_alloc.c | 53 +++++++++++++++---------------------------------- > > 1 file changed, 16 insertions(+), 37 deletions(-) > > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > index 5a84a0bebc37..72660e924b95 100644 > > --- a/mm/page_alloc.c > > +++ b/mm/page_alloc.c > > @@ -3772,16 +3772,22 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > > * Compaction managed to coalesce some page blocks, but the > > * allocation failed presumably due to a race. Retry some. > > */ > > - if (compact_result == COMPACT_SUCCESS) > > - (*compaction_retries)++; > > + if (compact_result == COMPACT_SUCCESS) { > > + /* > > + * !costly requests are much more important than > > + * __GFP_RETRY_MAYFAIL costly ones because they are de > > + * facto nofail and invoke OOM killer to move on while > > + * costly can fail and users are ready to cope with > > + * that. 1/4 retries is rather arbitrary but we would > > + * need much more detailed feedback from compaction to > > + * make a better decision. > > + */ > > + if (order > PAGE_ALLOC_COSTLY_ORDER) > > + max_retries /= 4; > > > > - /* > > - * All zones were scanned completely and still no result. It > > - * doesn't really make much sense to retry except when the > > - * failure could be caused by insufficient priority > > - */ > > - if (compact_result == COMPACT_COMPLETE) > > - goto check_priority; > > + ret = ++(*compaction_retries) <= max_retries; > > + goto out; > > I think you simplified this part too much, so now once it runs out of > retries, it will return false, while previously it would increase the priority. Oops, I'll send a delta fix to Andrew tomorrow. Thanks!
On Mon, May 29, 2023 at 12:38:07PM -0400, Johannes Weiner wrote: > On Mon, May 29, 2023 at 03:03:52PM +0200, Vlastimil Babka wrote: > > I think you simplified this part too much, so now once it runs out of > > retries, it will return false, while previously it would increase the priority. Here is the delta fix. If this looks good to everybody, can you please fold this into the patch you have in tree? Thanks! --- From 4b9429f9ef04fcb7bb5ffae0db8ea113b26d097b Mon Sep 17 00:00:00 2001 From: Johannes Weiner <hannes@cmpxchg.org> Date: Fri, 2 Jun 2023 16:02:37 +0200 Subject: [PATCH] mm: compaction: simplify should_compact_retry() fix Vlastimil points out an unintended change. Previously when hitting max_retries we'd bump the priority level and restart the loop. Now we bail out and fail instead. Restore the original behavior. Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> --- mm/page_alloc.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 72660e924b95..e7d7db36582b 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3768,6 +3768,15 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, if (fatal_signal_pending(current)) return false; + /* + * Compaction was skipped due to a lack of free order-0 + * migration targets. Continue if reclaim can help. + */ + if (compact_result == COMPACT_SKIPPED) { + ret = compaction_zonelist_suitable(ac, order, alloc_flags); + goto out; + } + /* * Compaction managed to coalesce some page blocks, but the * allocation failed presumably due to a race. Retry some. @@ -3785,17 +3794,10 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, if (order > PAGE_ALLOC_COSTLY_ORDER) max_retries /= 4; - ret = ++(*compaction_retries) <= max_retries; - goto out; - } - - /* - * Compaction was skipped due to a lack of free order-0 - * migration targets. Continue if reclaim can help. - */ - if (compact_result == COMPACT_SKIPPED) { - ret = compaction_zonelist_suitable(ac, order, alloc_flags); - goto out; + if (++(*compaction_retries) <= max_retries) { + ret = true; + goto out; + } } /*
On 6/2/23 16:47, Johannes Weiner wrote: > On Mon, May 29, 2023 at 12:38:07PM -0400, Johannes Weiner wrote: >> On Mon, May 29, 2023 at 03:03:52PM +0200, Vlastimil Babka wrote: >> > I think you simplified this part too much, so now once it runs out of >> > retries, it will return false, while previously it would increase the priority. > > Here is the delta fix. If this looks good to everybody, can you please > fold this into the patch you have in tree? Thanks! > > --- > From 4b9429f9ef04fcb7bb5ffae0db8ea113b26d097b Mon Sep 17 00:00:00 2001 > From: Johannes Weiner <hannes@cmpxchg.org> > Date: Fri, 2 Jun 2023 16:02:37 +0200 > Subject: [PATCH] mm: compaction: simplify should_compact_retry() fix > > Vlastimil points out an unintended change. Previously when hitting > max_retries we'd bump the priority level and restart the loop. Now we > bail out and fail instead. Restore the original behavior. > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> For the 2/5 +fix Acked-by: Vlastimil Babka <vbabka@suse.cz> > --- > mm/page_alloc.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index 72660e924b95..e7d7db36582b 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3768,6 +3768,15 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > if (fatal_signal_pending(current)) > return false; > > + /* > + * Compaction was skipped due to a lack of free order-0 > + * migration targets. Continue if reclaim can help. > + */ > + if (compact_result == COMPACT_SKIPPED) { > + ret = compaction_zonelist_suitable(ac, order, alloc_flags); > + goto out; > + } > + > /* > * Compaction managed to coalesce some page blocks, but the > * allocation failed presumably due to a race. Retry some. > @@ -3785,17 +3794,10 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, > if (order > PAGE_ALLOC_COSTLY_ORDER) > max_retries /= 4; > > - ret = ++(*compaction_retries) <= max_retries; > - goto out; > - } > - > - /* > - * Compaction was skipped due to a lack of free order-0 > - * migration targets. Continue if reclaim can help. > - */ > - if (compact_result == COMPACT_SKIPPED) { > - ret = compaction_zonelist_suitable(ac, order, alloc_flags); > - goto out; > + if (++(*compaction_retries) <= max_retries) { > + ret = true; > + goto out; > + } > } > > /*
diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 5a84a0bebc37..72660e924b95 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3772,16 +3772,22 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, * Compaction managed to coalesce some page blocks, but the * allocation failed presumably due to a race. Retry some. */ - if (compact_result == COMPACT_SUCCESS) - (*compaction_retries)++; + if (compact_result == COMPACT_SUCCESS) { + /* + * !costly requests are much more important than + * __GFP_RETRY_MAYFAIL costly ones because they are de + * facto nofail and invoke OOM killer to move on while + * costly can fail and users are ready to cope with + * that. 1/4 retries is rather arbitrary but we would + * need much more detailed feedback from compaction to + * make a better decision. + */ + if (order > PAGE_ALLOC_COSTLY_ORDER) + max_retries /= 4; - /* - * All zones were scanned completely and still no result. It - * doesn't really make much sense to retry except when the - * failure could be caused by insufficient priority - */ - if (compact_result == COMPACT_COMPLETE) - goto check_priority; + ret = ++(*compaction_retries) <= max_retries; + goto out; + } /* * Compaction was skipped due to a lack of free order-0 @@ -3793,35 +3799,8 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags, } /* - * If compaction backed due to being deferred, due to - * contended locks in async mode, or due to scanners meeting - * after a partial scan, retry with increased priority. - */ - if (compact_result == COMPACT_DEFERRED || - compact_result == COMPACT_CONTENDED || - compact_result == COMPACT_PARTIAL_SKIPPED) - goto check_priority; - - /* - * !costly requests are much more important than __GFP_RETRY_MAYFAIL - * costly ones because they are de facto nofail and invoke OOM - * killer to move on while costly can fail and users are ready - * to cope with that. 1/4 retries is rather arbitrary but we - * would need much more detailed feedback from compaction to - * make a better decision. - */ - if (order > PAGE_ALLOC_COSTLY_ORDER) - max_retries /= 4; - if (*compaction_retries <= max_retries) { - ret = true; - goto out; - } - - /* - * Make sure there are attempts at the highest priority if we exhausted - * all retries or failed at the lower priorities. + * Compaction failed. Retry with increasing priority. */ -check_priority: min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ? MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;