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=-8.5 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,USER_AGENT_SANE_1 autolearn=unavailable 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 3DB50C433E6 for ; Fri, 17 Jul 2020 07:51:40 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id EE9E120704 for ; Fri, 17 Jul 2020 07:51:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EE9E120704 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.alibaba.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 9D6248D0011; Fri, 17 Jul 2020 03:51:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 987E88D0001; Fri, 17 Jul 2020 03:51:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 8C4E08D0011; Fri, 17 Jul 2020 03:51:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0136.hostedemail.com [216.40.44.136]) by kanga.kvack.org (Postfix) with ESMTP id 791968D0001 for ; Fri, 17 Jul 2020 03:51:39 -0400 (EDT) Received: from smtpin16.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 3BB48349B for ; Fri, 17 Jul 2020 07:51:39 +0000 (UTC) X-FDA: 77046798318.16.van00_311568a26f09 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin16.hostedemail.com (Postfix) with ESMTP id 65B6C100E4B0A for ; Fri, 17 Jul 2020 07:50:07 +0000 (UTC) X-HE-Tag: van00_311568a26f09 X-Filterd-Recvd-Size: 7749 Received: from out30-44.freemail.mail.aliyun.com (out30-44.freemail.mail.aliyun.com [115.124.30.44]) by imf27.hostedemail.com (Postfix) with ESMTP for ; Fri, 17 Jul 2020 07:47:19 +0000 (UTC) X-Alimail-AntiSpam:AC=PASS;BC=-1|-1;BR=01201311R181e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01e07425;MF=alex.shi@linux.alibaba.com;NM=1;PH=DS;RN=20;SR=0;TI=SMTPD_---0U2ysnp5_1594971981; Received: from IT-FVFX43SYHV2H.local(mailfrom:alex.shi@linux.alibaba.com fp:SMTPD_---0U2ysnp5_1594971981) by smtp.aliyun-inc.com(127.0.0.1); Fri, 17 Jul 2020 15:46:23 +0800 Subject: Re: [PATCH v16 13/22] mm/lru: introduce TestClearPageLRU To: Alexander Duyck Cc: Andrew Morton , Mel Gorman , Tejun Heo , Hugh Dickins , Konstantin Khlebnikov , Daniel Jordan , Yang Shi , Matthew Wilcox , Johannes Weiner , kbuild test robot , linux-mm , LKML , cgroups@vger.kernel.org, Shakeel Butt , Joonsoo Kim , Wei Yang , "Kirill A. Shutemov" , Michal Hocko , Vladimir Davydov References: <1594429136-20002-1-git-send-email-alex.shi@linux.alibaba.com> <1594429136-20002-14-git-send-email-alex.shi@linux.alibaba.com> From: Alex Shi Message-ID: <072b39ac-b95a-94f1-67a2-3293d4550ff8@linux.alibaba.com> Date: Fri, 17 Jul 2020 15:45:28 +0800 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:68.0) Gecko/20100101 Thunderbird/68.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 X-Rspamd-Queue-Id: 65B6C100E4B0A X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam04 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: =E5=9C=A8 2020/7/17 =E4=B8=8A=E5=8D=885:12, Alexander Duyck =E5=86=99=E9=81= =93: > On Fri, Jul 10, 2020 at 5:59 PM Alex Shi w= rote: >> >> Combine PageLRU check and ClearPageLRU into a function by new >> introduced func TestClearPageLRU. This function will be used as page >> isolation precondition to prevent other isolations some where else. >> Then there are may non PageLRU page on lru list, need to remove BUG >> checking accordingly. >> >> Hugh Dickins pointed that __page_cache_release and release_pages >> has no need to do atomic clear bit since no user on the page at that >> moment. and no need get_page() before lru bit clear in isolate_lru_pag= e, >> since it '(1) Must be called with an elevated refcount on the page'. >> >> As Andrew Morton mentioned this change would dirty cacheline for page >> isn't on LRU. But the lost would be acceptable with Rong Chen >> report: >> https://lkml.org/lkml/2020/3/4/173 >> ... >> diff --git a/mm/swap.c b/mm/swap.c >> index f645965fde0e..5092fe9c8c47 100644 >> --- a/mm/swap.c >> +++ b/mm/swap.c >> @@ -83,10 +83,9 @@ static void __page_cache_release(struct page *page) >> struct lruvec *lruvec; >> unsigned long flags; >> >> + __ClearPageLRU(page); >> spin_lock_irqsave(&pgdat->lru_lock, flags); >> lruvec =3D mem_cgroup_page_lruvec(page, pgdat); >> - VM_BUG_ON_PAGE(!PageLRU(page), page); >> - __ClearPageLRU(page); >> del_page_from_lru_list(page, lruvec, page_off_lru(page= )); >> spin_unlock_irqrestore(&pgdat->lru_lock, flags); >> } >=20 > So this piece doesn't make much sense to me. Why not use > TestClearPageLRU(page) here? Just a few lines above you are testing > for PageLRU(page) and it seems like if you are going to go for an > atomic test/clear and then remove the page from the LRU list you > should be using it here as well otherwise it seems like you could run > into a potential collision since you are testing here without clearing > the bit. >=20 Hi Alex, Thanks a lot for comments!=20 In this func's call path __page_cache_release, the page is unlikely be ClearPageLRU, since this page isn't used by anyone, and going to be freed= . just __ClearPageLRU would be safe, and could save a non lru page flags di= sturb. >> @@ -878,9 +877,8 @@ void release_pages(struct page **pages, int nr) >> spin_lock_irqsave(&locked_pgdat->lru_l= ock, flags); >> } >> >> - lruvec =3D mem_cgroup_page_lruvec(page, locked= _pgdat); >> - VM_BUG_ON_PAGE(!PageLRU(page), page); >> __ClearPageLRU(page); >> + lruvec =3D mem_cgroup_page_lruvec(page, locked= _pgdat); >> del_page_from_lru_list(page, lruvec, page_off_= lru(page)); >> } >> >=20 > Same here. You are just moving the flag clearing, but you didn't > combine it with the test. It seems like if you are expecting this to > be treated as an atomic operation. It should be a relatively low cost > to do since you already should own the cacheline as a result of > calling put_page_testzero so I am not sure why you are not combining > the two. before the ClearPageLRU, there is a put_page_testzero(), that means no on= e using this page, and isolate_lru_page can not run on this page the in func chec= king.=20 VM_BUG_ON_PAGE(!page_count(page), page); So it would be safe here. >=20 >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index c1c4259b4de5..18986fefd49b 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -1548,16 +1548,16 @@ int __isolate_lru_page(struct page *page, isol= ate_mode_t mode) >> { >> int ret =3D -EINVAL; >> >> - /* Only take pages on the LRU. */ >> - if (!PageLRU(page)) >> - return ret; >> - >> /* Compaction should not handle unevictable pages but CMA can = do so */ >> if (PageUnevictable(page) && !(mode & ISOLATE_UNEVICTABLE)) >> return ret; >> >> ret =3D -EBUSY; >> >> + /* Only take pages on the LRU. */ >> + if (!PageLRU(page)) >> + return ret; >> + >> /* >> * To minimise LRU disruption, the caller can indicate that it= only >> * wants to isolate pages it will be able to operate on withou= t >> @@ -1671,8 +1671,6 @@ static unsigned long isolate_lru_pages(unsigned = long nr_to_scan, >> page =3D lru_to_page(src); >> prefetchw_prev_lru_page(page, src, flags); >> >> - VM_BUG_ON_PAGE(!PageLRU(page), page); >> - >> nr_pages =3D compound_nr(page); >> total_scan +=3D nr_pages; >> >=20 > So effectively the changes here are making it so that a !PageLRU page > will cycle to the start of the LRU list. Now if I understand correctly > we are guaranteed that if the flag is not set it cannot be set while > we are holding the lru_lock, however it can be cleared while we are > holding the lock, correct? Thus that is why isolate_lru_pages has to > call TestClearPageLRU after the earlier check in __isolate_lru_page. Right.=20 >=20 > It might make it more readable to pull in the later patch that > modifies isolate_lru_pages that has it using TestClearPageLRU. As to this change, It has to do in this patch, since any TestClearPageLRU= may cause lru bit miss in the lru list, so the precondication check has to removed here. Thank Alex