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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id D159AC28B2B for ; Wed, 17 Aug 2022 03:11:00 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 14AD48D0002; Tue, 16 Aug 2022 23:11:00 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0FAF48D0001; Tue, 16 Aug 2022 23:11:00 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EDD608D0002; Tue, 16 Aug 2022 23:10:59 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id DBCBB8D0001 for ; Tue, 16 Aug 2022 23:10:59 -0400 (EDT) Received: from smtpin03.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay09.hostedemail.com (Postfix) with ESMTP id 97B0D805F2 for ; Wed, 17 Aug 2022 03:10:59 +0000 (UTC) X-FDA: 79807607838.03.E953AB5 Received: from mail-pl1-f179.google.com (mail-pl1-f179.google.com [209.85.214.179]) by imf30.hostedemail.com (Postfix) with ESMTP id 46D7680063 for ; Wed, 17 Aug 2022 03:10:58 +0000 (UTC) Received: by mail-pl1-f179.google.com with SMTP id p18so10894446plr.8 for ; Tue, 16 Aug 2022 20:10:57 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance-com.20210112.gappssmtp.com; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to:from:subject :user-agent:mime-version:date:message-id:from:to:cc; bh=NP/Ah3LgdfnASmfV06u0hEiNxg+0eNNBbdR3/5igwBg=; b=Pmka1HMkeOE4OsGKFH8d1K7F+9iA8hC9DZjD6luNI/cNp+62kLTHX6ApMoD/U9gH+l byGFDsbkSEgEtkSmI+X95AuR+opRp4Bfe/XM3b/H1/ZjNJ2iIUpJbJn7ZoNuuX+TZVFA Znkc3NSLYpT/VEydffqyCx6Vlly7XQQrpOtR9/65tknwhcmNX8AAXdGxdAmQr22yj3MS 8zNAXSPCOuHEtvrZzRL0SXWc6vpg/6AuKYK/lvffWkfoYZz08ce1Sqpa9+0aLf6d+/Wb svBhGB5aet6ER70WZqM1L0js8VYTw0L+PaNoQCgoLJEAeeI8TZ9Elgfta1mfJ41pQj5q xm7w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:references:cc:to:from:subject :user-agent:mime-version:date:message-id:x-gm-message-state:from:to :cc; bh=NP/Ah3LgdfnASmfV06u0hEiNxg+0eNNBbdR3/5igwBg=; b=CqDHa1kW+t3uQQhQaVgrNZ38Gq4lTYbGcsf9Bt2/K0KaFTzUXedRNFv5N1FfKHmEYD B17MJ8vj6HhSzIj48wS0BbUe6HVYEiHM8+u3d+fJsDcjdgGHrWqbrbiJMdIw3w+x1F9K S79ywxdT66nQcOfSuhl097O1xGAXNHsRnasd2w36utlNsjLrDPbJ/XDVXKkYyf4IjTiR G/vgB8mfttnhOTHVuISVC0YOcoKI8erKzcLFI9jxnOGda0Rp+JLDvRZNvhfFOOHHiG0f WAtNfv3KL4RyJnY/ej5IGccXbYHR4abS6/OAZ3ctjU9KecvC4TvSTZv9R0jPuyXfMgLV rWbQ== X-Gm-Message-State: ACgBeo02J2U/vMuNjD8Ui1PpzF/wf7vc/494nhYOd9gP7+IolTnk1fAO gdWYv0EDv1ZjYokfQKhLvVsf2w== X-Google-Smtp-Source: AA6agR5oFuHVgsuP+tqebCxeizLOrcGq2XUGDZUiLM8kqxk/7GtfU5BUzaneBQy4Y/i6Jasyc9LVAg== X-Received: by 2002:a17:90b:4c8d:b0:1f5:409b:b017 with SMTP id my13-20020a17090b4c8d00b001f5409bb017mr1733109pjb.52.1660705856852; Tue, 16 Aug 2022 20:10:56 -0700 (PDT) Received: from [10.85.119.42] ([139.177.225.251]) by smtp.gmail.com with ESMTPSA id e3-20020a17090301c300b0016d95380e8esm139114plh.140.2022.08.16.20.10.54 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 16 Aug 2022 20:10:56 -0700 (PDT) Message-ID: Date: Wed, 17 Aug 2022 11:10:52 +0800 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH v3] mm/compaction: fix set skip in fast_find_migrateblock From: Chuyi Zhou To: Mel Gorman Cc: Andrew Morton , linux-mm@kvack.org References: <20220713062009.597255-1-zhouchuyi@bytedance.com> <20220713082814.bed234e00d7f5ceb3858352a@linux-foundation.org> <20220714115020.GA3563@techsingularity.net> <3529e159-8c22-5d67-5e5d-c912784df9a0@bytedance.com> <20220719082854.GB3563@techsingularity.net> <7d2bbb38-a96c-8212-8f89-915cd2c8668f@bytedance.com> In-Reply-To: <7d2bbb38-a96c-8212-8f89-915cd2c8668f@bytedance.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1660705859; a=rsa-sha256; cv=none; b=TljFTXlJAsxfs6o9II4Gbc0U59Fc0ntJyid6iyHJIFkWEiacgitTI/QkjNCzfrGdrVhbIt /dv3jJC4htXvT9Y+WlNFRKZuyQqmlmCd1Xoj73Kt070XqsIsOUFEembFTw+as8L1Ia+o4g +xcWxS5ojF+mS1hLFYxgHmNRQKDHI0U= ARC-Authentication-Results: i=1; imf30.hostedemail.com; dkim=pass header.d=bytedance-com.20210112.gappssmtp.com header.s=20210112 header.b=Pmka1HMk; spf=pass (imf30.hostedemail.com: domain of zhouchuyi@bytedance.com designates 209.85.214.179 as permitted sender) smtp.mailfrom=zhouchuyi@bytedance.com; dmarc=pass (policy=none) header.from=bytedance.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1660705859; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=NP/Ah3LgdfnASmfV06u0hEiNxg+0eNNBbdR3/5igwBg=; b=GqMZEPObezmWL99LmM1t80+Bw95WOaHwV5Xuv1vzd8gRYzvAdOvGzBcpy281tZJrS9uhlp /3CbP0XWgLf94WlTG0YPSFAdxFZ16ZyXpYF3vqP+IF+XmUdJ9DXHrVgIgHWGYN8GrfSdaT 4lq5DQSr2Qatho2RRI87H2RqWRoK3p4= Authentication-Results: imf30.hostedemail.com; dkim=pass header.d=bytedance-com.20210112.gappssmtp.com header.s=20210112 header.b=Pmka1HMk; spf=pass (imf30.hostedemail.com: domain of zhouchuyi@bytedance.com designates 209.85.214.179 as permitted sender) smtp.mailfrom=zhouchuyi@bytedance.com; dmarc=pass (policy=none) header.from=bytedance.com X-Rspamd-Server: rspam05 X-Rspam-User: X-Stat-Signature: 6punbuap7c9boha8a8o7aneipi95apb8 X-Rspamd-Queue-Id: 46D7680063 X-HE-Tag: 1660705858-281237 X-Bogosity: Ham, tests=bogofilter, spamicity=0.066942, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: 在 2022/8/17 上午11:10, Chuyi Zhou 写道: > > > 在 2022/7/19 下午4:28, Mel Gorman 写道: >> On Fri, Jul 15, 2022 at 11:26:01PM +0800, Chuyi Zhou wrote: >>> >>> >>> ??? 2022/7/14 ??????7:50, Mel Gorman ??????: >>>> On Wed, Jul 13, 2022 at 08:28:14AM -0700, Andrew Morton wrote: >>>>> (cc Mel) >>>>> >>>>> On Wed, 13 Jul 2022 14:20:09 +0800 Chuyi Zhou >>>>> wrote: >>>>> >>>>>> From: zhouchuyi >>>>>> >>>>>> When we successfully find a pageblock in fast_find_migrateblock(), >>>>>> the block will be set skip-flag through set_pageblock_skip(). >>>>>> However, >>>>>> when entering isolate_migratepages_block(), the whole pageblock will >>>>>> be skipped due to the branch >>>>>> 'if (!valid_page && IS_ALIGNED(low_pfn, pageblock_nr_pages))'. >>>>>> Eventually we will goto isolate_abort and isolate nothing. That cause >>>>>> fast_find_migrateblock useless. >>>>>> >>>> >>>> It's not very clear *why* this failed from the changelog because >>>> superficially !valid_page will be true for the first pageblock and >>>> there >>>> is a reasonable expectation it will be aligned. Is the following >>>> accurate >>>> based on your analysis? >>>> >>>>     However, when entering isolate_migratepages_block(), the first >>>>     pageblock will be skipped in the branch 'if (!valid_page && >>>>     IS_ALIGNED(low_pfn, pageblock_nr_pages))' as isolation_suitable >>>>     returns true due to the skip bit set by fast_find_migrateblock(). >>>> >>>> If so, please update the changelog as a reviewer handling backports may >>>> wonder what exactly is wrong with that branch. >>>> >>> Hi Mel, thanks for your review. >>> >>> If fast scanning failed, the return block may not be aligned, because >>> we get >>> pfn from *free_pfn*. When fast-find success, the return value *pfn* >>> is get >>> from pageblock_start_pfn, and it will be passed to >>> isolate_migratepages_block as low_pfn. I think normally the value get >>> from >>> pageblock_start_pfn should be aligned with pageblock_nr_pages. I have >>> used >>> printk test it. Maby I miss something important? >>>      pfn = pageblock_start_pfn(free_pfn); >>>      ... >>>      found_block = true; >>>      set_pageblock_skip(freepage); >>>      break; >> >> The return block indeed may not be aligned. It could simply be a >> restart. The changelog could still do with a little clarification but >> your patch is still fine. >> >>>> Second, what guarantees a block returned by fast_find that is not >>>> aligned gets marked skipped after it is scanned? The set_pageblock_skip >>>> is only called when there is a valid page and it may not be set if >>>> !IS_ALIGNED(low_pfn). Is something like this untested hunk also >>>> necessary? >>>> >>> You are right, we do need some machenism to ensure mark skipped after >>> scanned a !IS_ALIGNED block from fast_find. However, I think the >>> following >>> code may not working. Because *skip_updated* has been reset: >>>      if (!skip_updated) { >>>          skip_updated = true; >>>          if (test_and_set_skip(cc, page, low_pfn)) >>>              goto isolate_abort; >>>      } >>> Why not ignore skip_updated after scanned a block, just like this: >>> >>> diff --git a/mm/compaction.c b/mm/compaction.c >>> index 962d05d1e187..1c388c45f127 100644 >>> --- a/mm/compaction.c >>> +++ b/mm/compaction.c >>> @@ -1180,7 +1180,7 @@ isolate_migratepages_block(struct >>> compact_control *cc, >>> unsigned long low_pfn, >>>           * rescanned twice in a row. >>>           */ >>>          if (low_pfn == end_pfn && (!nr_isolated || cc->rescan)) { >>> -               if (valid_page && !skip_updated) >>> +               if (valid_page) >>>                          set_pageblock_skip(valid_page); >>>                  update_cached_migrate(cc, low_pfn); >>>          } >>> >> >> Because valid_page still needs to be set but ensuring valid_page is set >> is sufficient. >> >> The pageblock will be marked for skip on the first locking of a lruvec >> >>                          /* Try get exclusive access under lock */ >>                          if (!skip_updated) { >>                                  skip_updated = true; >>                                  if (test_and_set_skip(cc, page, >> low_pfn)) >>                                          goto isolate_abort; >>                          } >> > In test_and_set_skip(cc, page, low_pfn), if > !IS_ALIGNED(low_pfn, pageblock_nr_pages) is true, the block would not > get marked skip, furthermore *skip_updated* will be set true which result > second check useless even valid_page has been set. >> That will happen regardless of alignment. The intent here was to avoid >> multiple scanners trying to isolate the same pageblock. The second >> check for update happens at the end of the block >> >>          if (low_pfn == end_pfn && (!nr_isolated || cc->rescan)) { >>                  if (valid_page && !skip_updated) >>                          set_pageblock_skip(valid_page); >>                  update_cached_migrate(cc, low_pfn); >>          } >> > One way to guarantee a not aligned pageblock get marked is ensuring > valid_page is set and ignoring skip_updated in the second check. > Another way is ignoring IS_ALIGNED(pfn, pageblock_nr_pages) in > test_and_set_skip(). > However, both of them may increase the chance of duplicate mark > a pageblock skip. > Tanks. >> And it's the second one that requires a valid_page. If valid_page is >> available then the pageblock will get marked skip in most cases that >> matter. If anything, it can get set prematurely due to "Try get >> exclusive access" if the pageblock is not fully scanned but checking if >> pageblock should be *cleared* if there was a partial scan would be a >> different patch and not even clear that it worth the complexity. >>