[RFC] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
Message ID | 20231129104530.63787-1-v-songbaohua@oppo.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:a5a7:0:b0:403:3b70:6f57 with SMTP id d7csp250675vqn; Wed, 29 Nov 2023 02:46:39 -0800 (PST) X-Google-Smtp-Source: AGHT+IEulXraDlS2ht8ORL7PEM9Q5qXxuz9sDyT05gaeYMbs7W834Dpd6nSN+IZOEWKVVblUYCBY X-Received: by 2002:a9d:63c9:0:b0:6d7:e8fd:bc75 with SMTP id e9-20020a9d63c9000000b006d7e8fdbc75mr18225358otl.34.1701254799038; Wed, 29 Nov 2023 02:46:39 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701254799; cv=none; d=google.com; s=arc-20160816; b=B/AxyCRkPqoxrSOnhqN2J2hOdfHq/vEjVNV6hYHwCzAisFTBAY4z/sRlNuJ0xJAiV3 Z1dZAYVWsB6/Ssv4dhBdFVLhOc9XGzlLUd1nfiygQUT+EP4rT/wk/vH5RVaZy0oE/JEg 64Xkj5rzCLlPY00WEA+NK7tM88E12jfA1T4gp8Tnp4zSt+J6hQEu3x10vcCmSiJ3VFw2 RBSWYkeW/w6Ik9DBzs8t5dbI9uPDLCkWGI1C/jns4eBNbIAz5s2lNYcv8n2tTIxQjgTg muMFdIbYpeoLWeOamSab5UFftq0xZvO+WepsNbPYkEPzRBu3wRA1QPEmlHoTM4+cWQeU OFgQ== 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 :message-id:date:subject:cc:to:from:dkim-signature; bh=0WUGmqoJPaeTzET/O78MEEgB8PQJztuihLmKtgPXGxM=; fh=HoTU46Jxi7Al99DmANcxYRnK7FK9awaO/x3AR3UqVFY=; b=O+YfCxNB+3a7HXfoEq+5+Uokg0HYXkFFQfOoyUBY4HRVCFehBAHA0ztCbibEB/JQ03 DTsITq7kLjHA10XaAgeXOf7buqcP3rIivRkmDDa2S/J6Q1AyJUh4IHxy/PiJEnhW5Nyq 3NDMmrcYjAi569mVopuTSbZy1+/1bDDx8XNXwxkO62vU8DlMdBRzcXYLFWEmBLx7MYkj SbL+r/2mVEB+7vCidEhwHTy6j/q//3nv45ywt83rpboZvw53EsH+owSab7tAdQeIcIBv fsA2vk+eOrEeEcDIQx7upELEHPtahE0EE8pkMZodmf0/GM+SNl+8hp3UsV6dMntynXpR jd5A== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=cevsIwfU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from snail.vger.email (snail.vger.email. [2620:137:e000::3:7]) by mx.google.com with ESMTPS id f25-20020a635119000000b005c2124e421esi14179015pgb.156.2023.11.29.02.46.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 29 Nov 2023 02:46:38 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=cevsIwfU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by snail.vger.email (Postfix) with ESMTP id 8B414807C64C; Wed, 29 Nov 2023 02:45:56 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at snail.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229477AbjK2Kpr (ORCPT <rfc822;toshivichauhan@gmail.com> + 99 others); Wed, 29 Nov 2023 05:45:47 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:39250 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231308AbjK2Kpm (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 29 Nov 2023 05:45:42 -0500 Received: from mail-pf1-x42c.google.com (mail-pf1-x42c.google.com [IPv6:2607:f8b0:4864:20::42c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 98B0A10D3 for <linux-kernel@vger.kernel.org>; Wed, 29 Nov 2023 02:45:48 -0800 (PST) Received: by mail-pf1-x42c.google.com with SMTP id d2e1a72fcca58-6cbe5b6ec62so5351980b3a.1 for <linux-kernel@vger.kernel.org>; Wed, 29 Nov 2023 02:45:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1701254748; x=1701859548; darn=vger.kernel.org; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=0WUGmqoJPaeTzET/O78MEEgB8PQJztuihLmKtgPXGxM=; b=cevsIwfUjDSpF4Nq9v5TmQfzRVoIbtgaoQDeATM5qX0YASQFaAUY15y+F+F3JBpii0 SI6mU9r4BrkqZlVgHsCBM8KxfqP1EzBm2RJB3hjaLmzX8Zkl7iwR0ScMgw3EAu/uVpyd yLbQ1C5ZmDgW2Hj/OxRZCvzfNguglyfbSRxhmBhzbWJBe26DAMyLXndeniFzYL7Zx7WN i+bd1vlWTI3WUmgBpd4FATF309aLm5510FLog+ukZACZc+CF7bnQNgTRRsusqxwOAJR2 2E1NMQp/Hovr7vafYeBFCOy7wEQu+5rssjUdZkaRZgHkO8n7zMNZfxCgnZOXWH9PYB0g I+Zg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1701254748; x=1701859548; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=0WUGmqoJPaeTzET/O78MEEgB8PQJztuihLmKtgPXGxM=; b=kS+5s7Q/aRP/q3O0Zcgtn1pTZUkN6IVZmoLirIvPwBqgwuJaDKdVSNUIjIfp5P2qoY h4f+lHW14JZp33KR5eYSE6nJmZsHUSJJ22PjoyC5aB6kK0/6UBHDzaC8edCT8208nfU1 guvi5sYN3SEChqFO7FhPBetvPYykmNMvWyqfRnfwBEGCu18hElN61C6HGVdSlAVLSnYi qMix/xlGphBAp1/8D4kdhHFiEYV9Z3pPcXEbv2TfFr38JrIs+v/FZuJHzbMzgjVg4F/1 UUI1gd1+vg5Bl/EW6SqGJJibYUbPsVF60b+khWOpq6n/NX/tDdWk4YyPD3RPgIBzPvlx 92lA== X-Gm-Message-State: AOJu0YwrySSPPKJ9e6lCQ69QxKF/v/hse9IANUduSBzoi1QPaoB5a95M gH+G3fe8KAPTyvktyZqYN4w= X-Received: by 2002:a05:6a20:a10a:b0:18a:e176:87e9 with SMTP id q10-20020a056a20a10a00b0018ae17687e9mr15899897pzk.15.1701254747980; Wed, 29 Nov 2023 02:45:47 -0800 (PST) Received: from barry-desktop.. (47-72-151-223.dsl.dyn.ihug.co.nz. [47.72.151.223]) by smtp.gmail.com with ESMTPSA id 18-20020aa79112000000b0068fcb70ccafsm10425917pfh.129.2023.11.29.02.45.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 29 Nov 2023 02:45:47 -0800 (PST) From: Barry Song <21cnbao@gmail.com> X-Google-Original-From: Barry Song <v-songbaohua@oppo.com> To: akpm@linux-foundation.org, linux-mm@kvack.org Cc: david@redhat.com, shikemeng@huaweicloud.com, willy@infradead.org, mgorman@techsingularity.net, hannes@cmpxchg.org, baolin.wang@linux.alibaba.com, linux-kernel@vger.kernel.org, Barry Song <v-songbaohua@oppo.com>, Zhanyuan Hu <huzhanyuan@oppo.com> Subject: [RFC PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock Date: Wed, 29 Nov 2023 23:45:30 +1300 Message-Id: <20231129104530.63787-1-v-songbaohua@oppo.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM, RCVD_IN_DNSWL_BLOCKED,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-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Wed, 29 Nov 2023 02:45:56 -0800 (PST) X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1783894952369681351 X-GMAIL-MSGID: 1783894952369681351 |
Series |
[RFC] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
|
|
Commit Message
Barry Song
Nov. 29, 2023, 10:45 a.m. UTC
Testing shows fast_isolate_freepages can blindly choose an unsuitable
pageblock from time to time particularly while the min mark is used
from XXX path:
if (!page) {
cc->fast_search_fail++;
if (scan_start) {
/*
* Use the highest PFN found above min. If one was
* not found, be pessimistic for direct compaction
* and use the min mark.
*/
if (highest >= min_pfn) {
page = pfn_to_page(highest);
cc->free_pfn = highest;
} else {
if (cc->direct_compaction && pfn_valid(min_pfn)) { /* XXX */
page = pageblock_pfn_to_page(min_pfn,
min(pageblock_end_pfn(min_pfn),
zone_end_pfn(cc->zone)),
cc->zone);
cc->free_pfn = min_pfn;
}
}
}
}
In contrast, slow path is skipping unsuitable pageblocks in a decent way.
I don't know if it is an intended design or just an oversight. But
it seems more sensible to skip unsuitable pageblock.
Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
mm/compaction.c | 6 ++++++
1 file changed, 6 insertions(+)
Comments
On 11/29/2023 6:45 PM, Barry Song wrote: > Testing shows fast_isolate_freepages can blindly choose an unsuitable > pageblock from time to time particularly while the min mark is used > from XXX path: > if (!page) { > cc->fast_search_fail++; > if (scan_start) { > /* > * Use the highest PFN found above min. If one was > * not found, be pessimistic for direct compaction > * and use the min mark. > */ > if (highest >= min_pfn) { > page = pfn_to_page(highest); > cc->free_pfn = highest; > } else { > if (cc->direct_compaction && pfn_valid(min_pfn)) { /* XXX */ > page = pageblock_pfn_to_page(min_pfn, > min(pageblock_end_pfn(min_pfn), > zone_end_pfn(cc->zone)), > cc->zone); > cc->free_pfn = min_pfn; > } > } > } > } Yes, the min_pfn can be an unsuitable migration target. But I think we can just add the suitable_migration_target() validation into 'min_pfn' case? Since other cases must be suitable target which found from MIGRATE_MOVABLE free list. Something like below: diff --git a/mm/compaction.c b/mm/compaction.c index 01ba298739dd..4e8eb4571909 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -1611,6 +1611,8 @@ static void fast_isolate_freepages(struct compact_control *cc) min(pageblock_end_pfn(min_pfn), zone_end_pfn(cc->zone)), cc->zone); + if (!suitable_migration_target(cc, page)) + page = NULL; cc->free_pfn = min_pfn; } } By the way, I wonder if this patch can improve the efficiency of compaction in your test case? > In contrast, slow path is skipping unsuitable pageblocks in a decent way. > > I don't know if it is an intended design or just an oversight. But > it seems more sensible to skip unsuitable pageblock. > > Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com> > Signed-off-by: Barry Song <v-songbaohua@oppo.com> > --- > mm/compaction.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/mm/compaction.c b/mm/compaction.c > index 01ba298739dd..98c485a25614 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1625,6 +1625,12 @@ static void fast_isolate_freepages(struct compact_control *cc) > cc->total_free_scanned += nr_scanned; > if (!page) > return; > + /* > + * Otherwise, we can blindly choose an improper pageblock especially > + * while using the min mark > + */ > + if (!suitable_migration_target(cc, page)) > + return; > > low_pfn = page_to_pfn(page); > fast_isolate_around(cc, low_pfn);
On Wed, Dec 6, 2023 at 10:54 PM Baolin Wang <baolin.wang@linux.alibaba.com> wrote: > > > > On 11/29/2023 6:45 PM, Barry Song wrote: > > Testing shows fast_isolate_freepages can blindly choose an unsuitable > > pageblock from time to time particularly while the min mark is used > > from XXX path: > > if (!page) { > > cc->fast_search_fail++; > > if (scan_start) { > > /* > > * Use the highest PFN found above min. If one was > > * not found, be pessimistic for direct compaction > > * and use the min mark. > > */ > > if (highest >= min_pfn) { > > page = pfn_to_page(highest); > > cc->free_pfn = highest; > > } else { > > if (cc->direct_compaction && pfn_valid(min_pfn)) { /* XXX */ > > page = pageblock_pfn_to_page(min_pfn, > > min(pageblock_end_pfn(min_pfn), > > zone_end_pfn(cc->zone)), > > cc->zone); > > cc->free_pfn = min_pfn; > > } > > } > > } > > } > > Yes, the min_pfn can be an unsuitable migration target. But I think we > can just add the suitable_migration_target() validation into 'min_pfn' > case? Since other cases must be suitable target which found from > MIGRATE_MOVABLE free list. Something like below: > > diff --git a/mm/compaction.c b/mm/compaction.c > index 01ba298739dd..4e8eb4571909 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1611,6 +1611,8 @@ static void fast_isolate_freepages(struct > compact_control *cc) > > min(pageblock_end_pfn(min_pfn), > > zone_end_pfn(cc->zone)), > cc->zone); > + if > (!suitable_migration_target(cc, page)) > + page = NULL; > cc->free_pfn = min_pfn; > } > } > yes. this makes more senses. > By the way, I wonder if this patch can improve the efficiency of > compaction in your test case? This happens not quite often. when running 25 machines for one night, most of them can hit this unexpected code path. but the frequency isn't many times in one second. it might be one time in a couple of hours. so it is very difficult to measure the visible performance impact in my machines though the affection of choosing the unsuitable migration_target should be negative. I feel like it's worth fixing this to at least make the code theoretically self-explanatory? as it is quite odd unsuitable_migration_target can be still migration_target? > > > In contrast, slow path is skipping unsuitable pageblocks in a decent way. > > > > I don't know if it is an intended design or just an oversight. But > > it seems more sensible to skip unsuitable pageblock. > > > > Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com> > > Signed-off-by: Barry Song <v-songbaohua@oppo.com> > > --- > > mm/compaction.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/mm/compaction.c b/mm/compaction.c > > index 01ba298739dd..98c485a25614 100644 > > --- a/mm/compaction.c > > +++ b/mm/compaction.c > > @@ -1625,6 +1625,12 @@ static void fast_isolate_freepages(struct compact_control *cc) > > cc->total_free_scanned += nr_scanned; > > if (!page) > > return; > > + /* > > + * Otherwise, we can blindly choose an improper pageblock especially > > + * while using the min mark > > + */ > > + if (!suitable_migration_target(cc, page)) > > + return; > > > > low_pfn = page_to_pfn(page); > > fast_isolate_around(cc, low_pfn); Thanks Barry
On 12/6/2023 6:18 PM, Barry Song wrote: > On Wed, Dec 6, 2023 at 10:54 PM Baolin Wang > <baolin.wang@linux.alibaba.com> wrote: >> >> >> >> On 11/29/2023 6:45 PM, Barry Song wrote: >>> Testing shows fast_isolate_freepages can blindly choose an unsuitable >>> pageblock from time to time particularly while the min mark is used >>> from XXX path: >>> if (!page) { >>> cc->fast_search_fail++; >>> if (scan_start) { >>> /* >>> * Use the highest PFN found above min. If one was >>> * not found, be pessimistic for direct compaction >>> * and use the min mark. >>> */ >>> if (highest >= min_pfn) { >>> page = pfn_to_page(highest); >>> cc->free_pfn = highest; >>> } else { >>> if (cc->direct_compaction && pfn_valid(min_pfn)) { /* XXX */ >>> page = pageblock_pfn_to_page(min_pfn, >>> min(pageblock_end_pfn(min_pfn), >>> zone_end_pfn(cc->zone)), >>> cc->zone); >>> cc->free_pfn = min_pfn; >>> } >>> } >>> } >>> } >> >> Yes, the min_pfn can be an unsuitable migration target. But I think we >> can just add the suitable_migration_target() validation into 'min_pfn' >> case? Since other cases must be suitable target which found from >> MIGRATE_MOVABLE free list. Something like below: >> >> diff --git a/mm/compaction.c b/mm/compaction.c >> index 01ba298739dd..4e8eb4571909 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -1611,6 +1611,8 @@ static void fast_isolate_freepages(struct >> compact_control *cc) >> >> min(pageblock_end_pfn(min_pfn), >> >> zone_end_pfn(cc->zone)), >> cc->zone); >> + if >> (!suitable_migration_target(cc, page)) >> + page = NULL; >> cc->free_pfn = min_pfn; >> } >> } >> > > yes. this makes more senses. > >> By the way, I wonder if this patch can improve the efficiency of >> compaction in your test case? > > This happens not quite often. when running 25 machines for > one night, most of them can hit this unexpected code path. > but the frequency isn't many times in one second. it might > be one time in a couple of hours. > > so it is very difficult to measure the visible performance impact > in my machines though the affection of choosing the unsuitable > migration_target should be negative. OK. Fair enough. > > I feel like it's worth fixing this to at least make the code theoretically > self-explanatory? as it is quite odd unsuitable_migration_target can > be still migration_target? > >> >>> In contrast, slow path is skipping unsuitable pageblocks in a decent way. >>> >>> I don't know if it is an intended design or just an oversight. But >>> it seems more sensible to skip unsuitable pageblock. >>> >>> Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com> >>> Signed-off-by: Barry Song <v-songbaohua@oppo.com> >>> --- >>> mm/compaction.c | 6 ++++++ >>> 1 file changed, 6 insertions(+) >>> >>> diff --git a/mm/compaction.c b/mm/compaction.c >>> index 01ba298739dd..98c485a25614 100644 >>> --- a/mm/compaction.c >>> +++ b/mm/compaction.c >>> @@ -1625,6 +1625,12 @@ static void fast_isolate_freepages(struct compact_control *cc) >>> cc->total_free_scanned += nr_scanned; >>> if (!page) >>> return; >>> + /* >>> + * Otherwise, we can blindly choose an improper pageblock especially >>> + * while using the min mark >>> + */ >>> + if (!suitable_migration_target(cc, page)) >>> + return; >>> >>> low_pfn = page_to_pfn(page); >>> fast_isolate_around(cc, low_pfn); > > Thanks > Barry
diff --git a/mm/compaction.c b/mm/compaction.c index 01ba298739dd..98c485a25614 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -1625,6 +1625,12 @@ static void fast_isolate_freepages(struct compact_control *cc) cc->total_free_scanned += nr_scanned; if (!page) return; + /* + * Otherwise, we can blindly choose an improper pageblock especially + * while using the min mark + */ + if (!suitable_migration_target(cc, page)) + return; low_pfn = page_to_pfn(page); fast_isolate_around(cc, low_pfn);