From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-15.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 21B6DC433DB for ; Mon, 25 Jan 2021 16:49:57 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B465322795 for ; Mon, 25 Jan 2021 16:49:56 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B465322795 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 3F8F88D0016; Mon, 25 Jan 2021 11:49:56 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 3CF9D8D0001; Mon, 25 Jan 2021 11:49:56 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2E6CC8D0016; Mon, 25 Jan 2021 11:49:56 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 179E68D0001 for ; Mon, 25 Jan 2021 11:49:56 -0500 (EST) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id A77E73649 for ; Mon, 25 Jan 2021 16:49:55 +0000 (UTC) X-FDA: 77744884350.01.spy23_2a02d4127587 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin01.hostedemail.com (Postfix) with ESMTP id 83A2E1004535C for ; Mon, 25 Jan 2021 16:49:55 +0000 (UTC) X-HE-Tag: spy23_2a02d4127587 X-Filterd-Recvd-Size: 3874 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf22.hostedemail.com (Postfix) with ESMTP for ; Mon, 25 Jan 2021 16:49:55 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id D1404ACFB; Mon, 25 Jan 2021 16:49:53 +0000 (UTC) To: Wonhyuk Yang , Andrew Morton Cc: Mel Gorman , linux-mm@kvack.org References: <20210123154320.24278-1-vvghjk1234@gmail.com> From: Vlastimil Babka Subject: Re: [PATCH] mm/compactoin: Fix edge case of fast_find_migrateblock() Message-ID: <0ed60e73-9c4f-5a62-754d-b3c864239941@suse.cz> Date: Mon, 25 Jan 2021 17:49:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.1 MIME-Version: 1.0 In-Reply-To: <20210123154320.24278-1-vvghjk1234@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 1/23/21 4:43 PM, Wonhyuk Yang wrote: > In the fast_find_migrateblock(), It iterate freelist to find > proper pageblock. But there are two edge cases. First, if the page > we found is equal to cc->migrate_pfn, it is considered that we > didn't found suitable pageblock. Second, if the loop was terminated > because order is less than PAGE_ALLOC_COSTLY_ORDER, it could > be considered that we found suitable one. >=20 > Fixes: 70b44595eafe9 ("mm, compaction: use free lists to quickly locate > a migration source") >=20 > Signed-off-by: Wonhyuk Yang Seems correct, although it's quite subtle code and it's been a long time.= .. Acked-by: Vlastimil Babka Maybe instead of replacing one magic value of 'pfn' with another (cc->migrate_pfn with high_pfn) I would just add a "bool found" and use i= t appropriately. Would make the function less subtle perhaps. While reviewing I found more potentially questionable parts, if you're in= terested: - if we go through "if (get_pageblock_skip(freepage))... continue;" we ar= e increasing nr_scanned++; but not checking the limit. - the "if (list_is_last(freelist, &freepage->lru)) break;" part seems unneccesary? if we are on the last page and just "continue;" the list_for_each_entry() iteration should stop anyway. > --- > mm/compaction.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) >=20 > diff --git a/mm/compaction.c b/mm/compaction.c > index e5acb9714436..46f49e6b7d1a 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -1742,8 +1742,8 @@ static unsigned long fast_find_migrateblock(struc= t compact_control *cc) > distance >>=3D 2; > high_pfn =3D pageblock_start_pfn(cc->migrate_pfn + distance); > =20 > - for (order =3D cc->order - 1; > - order >=3D PAGE_ALLOC_COSTLY_ORDER && pfn =3D=3D cc->migrate_pfn= && nr_scanned < limit; > + for (order =3D cc->order - 1, pfn =3D high_pfn; > + order >=3D PAGE_ALLOC_COSTLY_ORDER && pfn =3D=3D high_pfn && nr_= scanned < limit; > order--) { > struct free_area *area =3D &cc->zone->free_area[order]; > struct list_head *freelist; > @@ -1785,7 +1785,6 @@ static unsigned long fast_find_migrateblock(struc= t compact_control *cc) > } > =20 > if (nr_scanned >=3D limit) { > - cc->fast_search_fail++; > move_freelist_tail(freelist, freepage); > break; > } > @@ -1799,9 +1798,10 @@ static unsigned long fast_find_migrateblock(stru= ct compact_control *cc) > * If fast scanning failed then use a cached entry for a page block > * that had free pages as the basis for starting a linear scan. > */ > - if (pfn =3D=3D cc->migrate_pfn) > + if (pfn =3D=3D high_pfn) { > + cc->fast_search_fail++; > pfn =3D reinit_migrate_pfn(cc); > - > + } > return pfn; > } > =20 >=20