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=-18.9 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_1,USER_IN_DEF_DKIM_WL 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 EFA6AC4727C for ; Mon, 21 Sep 2020 23:16:28 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 5C03523A61 for ; Mon, 21 Sep 2020 23:16:28 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="OwUkAegz" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5C03523A61 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id B271E900007; Mon, 21 Sep 2020 19:16:27 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id AB0DE900004; Mon, 21 Sep 2020 19:16:27 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 97755900007; Mon, 21 Sep 2020 19:16:27 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0105.hostedemail.com [216.40.44.105]) by kanga.kvack.org (Postfix) with ESMTP id 7CB15900004 for ; Mon, 21 Sep 2020 19:16:27 -0400 (EDT) Received: from smtpin13.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 44967180AD806 for ; Mon, 21 Sep 2020 23:16:27 +0000 (UTC) X-FDA: 77288629614.13.horn25_0211e1f27149 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin13.hostedemail.com (Postfix) with ESMTP id 21EBF18140B69 for ; Mon, 21 Sep 2020 23:16:27 +0000 (UTC) X-HE-Tag: horn25_0211e1f27149 X-Filterd-Recvd-Size: 14645 Received: from mail-oo1-f68.google.com (mail-oo1-f68.google.com [209.85.161.68]) by imf17.hostedemail.com (Postfix) with ESMTP for ; Mon, 21 Sep 2020 23:16:26 +0000 (UTC) Received: by mail-oo1-f68.google.com with SMTP id m25so3708635oou.0 for ; Mon, 21 Sep 2020 16:16:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=date:from:to:cc:subject:in-reply-to:message-id:references :user-agent:mime-version; bh=BcuKgc3jGxItkyWiUdeL+SY34z52t2Wt1/MsImKvlh8=; b=OwUkAegznMAyRepl3dJUGb6AvehwhPSLy4u20VbZg7b+iDXgW9jL56zxMha44sB13R b7CQj+PEtHbo/sPf6l9KPOdgDx/la5T3n/cKY2W4QElvKFp/TMCWvLVXJN/ciFtjjO5d Va3JBo4OiFysnblBrHIAXeOeESaLSYPEmu5M068HhHikVBrqb1OowVkOfw4xMWoLFr+Q zzLJjj36wE2eg3hiGR4dehp4k+9zAkBkwU7M69DTdJtkWY/FViEjHFgZ1bO0A6mTE+lb jMtXZLwDIw7LH1GX0vl/tXViaZYdZx3tgTlmsmIwpb1bfnEn9bLowtektJJksCMuf4G3 oC4A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:in-reply-to:message-id :references:user-agent:mime-version; bh=BcuKgc3jGxItkyWiUdeL+SY34z52t2Wt1/MsImKvlh8=; b=hSwcb3DZrmn78k/k2/WaYZ2HJfWfmXDzVlYLTzzBoKqE9pjw5Ka+k9xLzymhC8qImH zRag5Zvh0QiLmS12NXi9LgmTnVhxz3pbYZue869Kr3XOtoqRFPyfWK3SSa2V6ILF68mp E9Ugo9W4O88gRVUS9b7IdIWkTZDtyBsbU9B4P6TiSLw9x0QQ6wz1bCTRwYFXOhHRQQYl woTG+gkW3cUeQoDjScwHNjlR1ZTJ+zGZREEGqJjEfej0DSYF4yKMH8haByI0SjgZj0Fz Rkp+dt5Fls/wNtCh5e/HBfm9qXCPM7EbArv1ysBkBzkB2Pn3ndwbJVlJPnWonDlQABJ0 Q82g== X-Gm-Message-State: AOAM533AHmmS4YL39TsZKDCWVKfSu8Vq/eWkwmbbvRYVzTpaGXuPPu5h BJrnD/gwqB2DVrTNdN0RzFUgjA== X-Google-Smtp-Source: ABdhPJxtiuaJpoRjNXXE4WIRRGhdkEvNn9ovTsPd3sw5oRN/vPR8+OxRV9InWwcopYhHYtbW4+11nQ== X-Received: by 2002:a4a:a58f:: with SMTP id d15mr1160919oom.36.1600730185416; Mon, 21 Sep 2020 16:16:25 -0700 (PDT) Received: from eggly.attlocal.net (172-10-233-147.lightspeed.sntcca.sbcglobal.net. [172.10.233.147]) by smtp.gmail.com with ESMTPSA id j24sm6298704otn.64.2020.09.21.16.16.22 (version=TLS1 cipher=ECDHE-ECDSA-AES128-SHA bits=128/128); Mon, 21 Sep 2020 16:16:24 -0700 (PDT) Date: Mon, 21 Sep 2020 16:16:16 -0700 (PDT) From: Hugh Dickins X-X-Sender: hugh@eggly.anvils To: Alex Shi cc: akpm@linux-foundation.org, mgorman@techsingularity.net, tj@kernel.org, hughd@google.com, khlebnikov@yandex-team.ru, daniel.m.jordan@oracle.com, willy@infradead.org, hannes@cmpxchg.org, lkp@intel.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org, shakeelb@google.com, iamjoonsoo.kim@lge.com, richard.weiyang@gmail.com, kirill@shutemov.name, alexander.duyck@gmail.com, rong.a.chen@intel.com, mhocko@suse.com, vdavydov.dev@gmail.com, shy828301@gmail.com, Michal Hocko Subject: Re: [PATCH v18 16/32] mm/lru: introduce TestClearPageLRU In-Reply-To: <1598273705-69124-17-git-send-email-alex.shi@linux.alibaba.com> Message-ID: References: <1598273705-69124-1-git-send-email-alex.shi@linux.alibaba.com> <1598273705-69124-17-git-send-email-alex.shi@linux.alibaba.com> User-Agent: Alpine 2.11 (LSU 23 2013-08-11) MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII 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 Mon, 24 Aug 2020, Alex Shi wrote: > Currently lru_lock still guards both lru list and page's lru bit, that's > ok. but if we want to use specific lruvec lock on the page, we need to > pin down the page's lruvec/memcg during locking. Just taking lruvec > lock first may be undermined by the page's memcg charge/migration. To > fix this problem, we could clear the lru bit out of locking and use > it as pin down action to block the page isolation in memcg changing. > > So now a standard steps of page isolation is following: > 1, get_page(); #pin the page avoid to be free > 2, TestClearPageLRU(); #block other isolation like memcg change > 3, spin_lock on lru_lock; #serialize lru list access > 4, delete page from lru list; > The step 2 could be optimzed/replaced in scenarios which page is > unlikely be accessed or be moved between memcgs. > > This patch start with the first part: TestClearPageLRU, which combines > PageLRU check and ClearPageLRU into a macro func TestClearPageLRU. This > function will be used as page isolation precondition to prevent other > isolations some where else. Then there are may !PageLRU page on lru > list, need to remove BUG() checking accordingly. > > There 2 rules for lru bit now: > 1, the lru bit still indicate if a page on lru list, just in some > temporary moment(isolating), the page may have no lru bit when > it's on lru list. but the page still must be on lru list when the > lru bit set. > 2, have to remove lru bit before delete it from lru list. > > Hugh Dickins pointed that when a page is in free path and no one is > possible to take it, non atomic lru bit clearing is better, like in > __page_cache_release and release_pages. > And no need get_page() before lru bit clear in isolate_lru_page, > since it '(1) Must be called with an elevated refcount on the page'. Delete that paragraph: you're justifying changes made during the course of earlier review, but not needed here. If we start to comment on everything that is not done...! > > 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 Please use a lore link instead, lkml.org is nice but unreliable: https://lore.kernel.org/lkml/20200304090301.GB5972@shao2-debian/ > > Suggested-by: Johannes Weiner > Signed-off-by: Alex Shi Acked-by: Hugh Dickins when you make the changes suggested above and below. I still have long-standing reservations about this TestClearPageLRU technique (it's hard to reason about, and requires additional atomic ops in some places); but it's working, so I'd like it to go in, then later we can experiment with whether lock_page_memcg() does a better job, or rechecking memcg when getting the lru_lock (my original technique). > Cc: Hugh Dickins > Cc: Johannes Weiner > Cc: Michal Hocko > Cc: Vladimir Davydov > Cc: Andrew Morton > Cc: linux-kernel@vger.kernel.org > Cc: cgroups@vger.kernel.org > Cc: linux-mm@kvack.org > --- > include/linux/page-flags.h | 1 + > mm/mlock.c | 3 +-- > mm/swap.c | 5 ++--- > mm/vmscan.c | 18 +++++++----------- > 4 files changed, 11 insertions(+), 16 deletions(-) > > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h > index 6be1aa559b1e..9554ed1387dc 100644 > --- a/include/linux/page-flags.h > +++ b/include/linux/page-flags.h > @@ -326,6 +326,7 @@ static inline void page_init_poison(struct page *page, size_t size) > PAGEFLAG(Dirty, dirty, PF_HEAD) TESTSCFLAG(Dirty, dirty, PF_HEAD) > __CLEARPAGEFLAG(Dirty, dirty, PF_HEAD) > PAGEFLAG(LRU, lru, PF_HEAD) __CLEARPAGEFLAG(LRU, lru, PF_HEAD) > + TESTCLEARFLAG(LRU, lru, PF_HEAD) > PAGEFLAG(Active, active, PF_HEAD) __CLEARPAGEFLAG(Active, active, PF_HEAD) > TESTCLEARFLAG(Active, active, PF_HEAD) > PAGEFLAG(Workingset, workingset, PF_HEAD) > diff --git a/mm/mlock.c b/mm/mlock.c > index 93ca2bf30b4f..3762d9dd5b31 100644 > --- a/mm/mlock.c > +++ b/mm/mlock.c > @@ -107,13 +107,12 @@ void mlock_vma_page(struct page *page) > */ > static bool __munlock_isolate_lru_page(struct page *page, bool getpage) > { > - if (PageLRU(page)) { > + if (TestClearPageLRU(page)) { > struct lruvec *lruvec; > > lruvec = mem_cgroup_page_lruvec(page, page_pgdat(page)); > if (getpage) > get_page(page); > - ClearPageLRU(page); > del_page_from_lru_list(page, lruvec, page_lru(page)); > return true; > } > diff --git a/mm/swap.c b/mm/swap.c > index f80ccd6f3cb4..446ffe280809 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 = 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); > } > @@ -880,9 +879,9 @@ void release_pages(struct page **pages, int nr) > spin_lock_irqsave(&locked_pgdat->lru_lock, flags); > } > > - lruvec = mem_cgroup_page_lruvec(page, locked_pgdat); > VM_BUG_ON_PAGE(!PageLRU(page), page); > __ClearPageLRU(page); > + lruvec = mem_cgroup_page_lruvec(page, locked_pgdat); > del_page_from_lru_list(page, lruvec, page_off_lru(page)); > } > Please delete all those mods to mm/swap.c from this patch. This patch is about introducing TestClearPageLRU, but that is not involved here. Several versions ago, yes it was, then I pointed out that these are operations on refcount 0 pages, and we don't want to add unnecessary atomic operations on them. I expect you want to keep the rearrangements, but do them where you need them later (I expect that's in 20/32). And I notice that one VM_BUG_ON_PAGE was kept and the other deleted: though one can certainly argue that they're redundant (as all BUGs should be), I think most people will feel safer to keep them both. > diff --git a/mm/vmscan.c b/mm/vmscan.c > index 7b7b36bd1448..1b3e0eeaad64 100644 > --- a/mm/vmscan.c > +++ b/mm/vmscan.c > @@ -1665,8 +1665,6 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan, > page = lru_to_page(src); > prefetchw_prev_lru_page(page, src, flags); > > - VM_BUG_ON_PAGE(!PageLRU(page), page); > - > nr_pages = compound_nr(page); > total_scan += nr_pages; > It is not enough to remove just that one VM_BUG_ON_PAGE there. This is a patch series, and we don't need it to be perfect at every bisection point between patches, but we do need it to be reasonably robust, so as not to waste unrelated bughunters' time. It didn't take me very long to crash on the "default: BUG()" further down isolate_lru_pages(), because now PageLRU may get cleared at any instant, whatever locks are held. (But you're absolutely right to leave the compaction and pagevec mods to subsequent patches: it's fairly safe to separate those out, and much easier for reviewers that you did so.) This patch is much more robust with __isolate_lru_page() mods below on top. I agree there's other ways to do it, but given that nobody cares what the error return is from __isolate_lru_page(), except for the isolate_lru_pages() switch statement BUG() which has become invalid, I suggest just use -EBUSY throughout __isolate_lru_page(). Yes, we can and should change that switch statement to an "if {} else {}" without any BUG(), but I don't want to mess you around at this time, leave cleanup like that until later. Please fold in this patch on top: --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1540,7 +1540,7 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone, */ int __isolate_lru_page(struct page *page, isolate_mode_t mode) { - int ret = -EINVAL; + int ret = -EBUSY; /* Only take pages on the LRU. */ if (!PageLRU(page)) @@ -1550,8 +1550,6 @@ int __isolate_lru_page(struct page *page, isolate_mode_t mode) if (PageUnevictable(page) && !(mode & ISOLATE_UNEVICTABLE)) return ret; - ret = -EBUSY; - /* * To minimise LRU disruption, the caller can indicate that it only * wants to isolate pages it will be able to operate on without @@ -1598,8 +1596,10 @@ int __isolate_lru_page(struct page *page, isolate_mode_t mode) * sure the page is not being freed elsewhere -- the * page release code relies on it. */ - ClearPageLRU(page); - ret = 0; + if (TestClearPageLRU(page)) + ret = 0; + else + put_page(page); } return ret; > @@ -1763,21 +1761,19 @@ int isolate_lru_page(struct page *page) > VM_BUG_ON_PAGE(!page_count(page), page); > WARN_RATELIMIT(PageTail(page), "trying to isolate tail page"); > > - if (PageLRU(page)) { > + if (TestClearPageLRU(page)) { > pg_data_t *pgdat = page_pgdat(page); > struct lruvec *lruvec; > + int lru = page_lru(page); > > - spin_lock_irq(&pgdat->lru_lock); > + get_page(page); > lruvec = mem_cgroup_page_lruvec(page, pgdat); > - if (PageLRU(page)) { > - int lru = page_lru(page); > - get_page(page); > - ClearPageLRU(page); > - del_page_from_lru_list(page, lruvec, lru); > - ret = 0; > - } > + spin_lock_irq(&pgdat->lru_lock); > + del_page_from_lru_list(page, lruvec, lru); > spin_unlock_irq(&pgdat->lru_lock); > + ret = 0; > } > + > return ret; > } And a small mod to isolate_lru_page() to be folded in. I had never noticed this before, but here you are evaluating page_lru() after clearing PageLRU, but before getting lru_lock: that seems unsafe. I'm pretty sure it's unsafe at this stage of the series; I did once persuade myself that it becomes safe by the end of the series, but I've already forgotten the argument for that (I have already said TestClearPageLRU is difficult to reason about). Please don't force us to have to think about this! Just get page_lru after lru_lock. --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1764,12 +1764,11 @@ int isolate_lru_page(struct page *page) if (TestClearPageLRU(page)) { pg_data_t *pgdat = page_pgdat(page); struct lruvec *lruvec; - int lru = page_lru(page); get_page(page); lruvec = mem_cgroup_page_lruvec(page, pgdat); spin_lock_irq(&pgdat->lru_lock); - del_page_from_lru_list(page, lruvec, lru); + del_page_from_lru_list(page, lruvec, page_lru(page)); spin_unlock_irq(&pgdat->lru_lock); ret = 0; } And lastly, please do check_move_unevictable_pages()'s TestClearPageLRU mod here at the end of mm/vmscan.c in this patch: I noticed that your lruv19 branch is doing it in a later patch, but it fits better here.