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=-11.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,UNPARSEABLE_RELAY,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 DCB7FC4363D for ; Tue, 22 Sep 2020 04:59:16 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 4C71723A6C for ; Tue, 22 Sep 2020 04:59:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4C71723A6C 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 6DDA76B00A1; Tue, 22 Sep 2020 00:59:15 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 68E5E6B00A2; Tue, 22 Sep 2020 00:59:15 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5A58E6B00A3; Tue, 22 Sep 2020 00:59:15 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0252.hostedemail.com [216.40.44.252]) by kanga.kvack.org (Postfix) with ESMTP id 44D2E6B00A1 for ; Tue, 22 Sep 2020 00:59:15 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 0C838180AD806 for ; Tue, 22 Sep 2020 04:59:15 +0000 (UTC) X-FDA: 77289493470.04.laugh89_5d145bc2714b Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin04.hostedemail.com (Postfix) with ESMTP id CF6B58007635 for ; Tue, 22 Sep 2020 04:59:14 +0000 (UTC) X-HE-Tag: laugh89_5d145bc2714b X-Filterd-Recvd-Size: 12463 Received: from out30-42.freemail.mail.aliyun.com (out30-42.freemail.mail.aliyun.com [115.124.30.42]) by imf25.hostedemail.com (Postfix) with ESMTP for ; Tue, 22 Sep 2020 04:59:13 +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=e01e04423;MF=alex.shi@linux.alibaba.com;NM=1;PH=DS;RN=21;SR=0;TI=SMTPD_---0U9k1AHg_1600750746; Received: from IT-FVFX43SYHV2H.local(mailfrom:alex.shi@linux.alibaba.com fp:SMTPD_---0U9k1AHg_1600750746) by smtp.aliyun-inc.com(127.0.0.1); Tue, 22 Sep 2020 12:59:08 +0800 Subject: Re: [PATCH v18 17/32] mm/compaction: do page isolation first in compaction To: Hugh Dickins Cc: akpm@linux-foundation.org, mgorman@techsingularity.net, tj@kernel.org, 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 References: <1598273705-69124-1-git-send-email-alex.shi@linux.alibaba.com> <1598273705-69124-18-git-send-email-alex.shi@linux.alibaba.com> From: Alex Shi Message-ID: Date: Tue, 22 Sep 2020 12:57:00 +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=gbk 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: =D4=DA 2020/9/22 =C9=CF=CE=E77:49, Hugh Dickins =D0=B4=B5=C0: > On Mon, 24 Aug 2020, Alex Shi wrote: >=20 >> Currently, compaction would get the lru_lock and then do page isolatio= n >> which works fine with pgdat->lru_lock, since any page isoltion would >> compete for the lru_lock. If we want to change to memcg lru_lock, we >> have to isolate the page before getting lru_lock, thus isoltion would >> block page's memcg change which relay on page isoltion too. Then we >> could safely use per memcg lru_lock later. >> >> The new page isolation use previous introduced TestClearPageLRU() + >> pgdat lru locking which will be changed to memcg lru lock later. >> >> Hugh Dickins fixed following bugs in this patch's >> early version: >> >> Fix lots of crashes under compaction load: isolate_migratepages_block(= ) >> must clean up appropriately when rejecting a page, setting PageLRU aga= in >> if it had been cleared; and a put_page() after get_page_unless_zero() >> cannot safely be done while holding locked_lruvec - it may turn out to >> be the final put_page(), which will take an lruvec lock when PageLRU. >> And move __isolate_lru_page_prepare back after get_page_unless_zero to >> make trylock_page() safe: >> trylock_page() is not safe to use at this time: its setting PG_locked >> can race with the page being freed or allocated ("Bad page"), and can >> also erase flags being set by one of those "sole owners" of a freshly >> allocated page who use non-atomic __SetPageFlag(). >> >> Suggested-by: Johannes Weiner >> Signed-off-by: Hugh Dickins >> Signed-off-by: Alex Shi >=20 > Okay, whatever. I was about to say > Acked-by: Hugh Dickins Thanks! > With my signed-off-by there, someone will ask if it should say > "From: Hugh ..." at the top: no, it should not, this is Alex's patch, > but I proposed some fixes to it, as you already acknowledged. I guess you prefer to remove your signed off here, don't you? >=20 > A couple of comments below on the mm/vmscan.c part of it. >=20 >> Cc: Andrew Morton >> Cc: Matthew Wilcox >> Cc: linux-kernel@vger.kernel.org >> Cc: linux-mm@kvack.org >> --- >> include/linux/swap.h | 2 +- >> mm/compaction.c | 42 +++++++++++++++++++++++++++++++++--------- >> mm/vmscan.c | 46 ++++++++++++++++++++++++++-----------------= --- >> 3 files changed, 60 insertions(+), 30 deletions(-) >> >> diff --git a/include/linux/swap.h b/include/linux/swap.h >> index 43e6b3458f58..550fdfdc3506 100644 >> --- a/include/linux/swap.h >> +++ b/include/linux/swap.h >> @@ -357,7 +357,7 @@ extern void lru_cache_add_inactive_or_unevictable(= struct page *page, >> extern unsigned long zone_reclaimable_pages(struct zone *zone); >> extern unsigned long try_to_free_pages(struct zonelist *zonelist, int= order, >> gfp_t gfp_mask, nodemask_t *mask); >> -extern int __isolate_lru_page(struct page *page, isolate_mode_t mode)= ; >> +extern int __isolate_lru_page_prepare(struct page *page, isolate_mode= _t mode); >> extern unsigned long try_to_free_mem_cgroup_pages(struct mem_cgroup *= memcg, >> unsigned long nr_pages, >> gfp_t gfp_mask, >> diff --git a/mm/compaction.c b/mm/compaction.c >> index 4e2c66869041..253382d99969 100644 >> --- a/mm/compaction.c >> +++ b/mm/compaction.c >> @@ -887,6 +887,7 @@ static bool too_many_isolated(pg_data_t *pgdat) >> if (!valid_page && IS_ALIGNED(low_pfn, pageblock_nr_pages)) { >> if (!cc->ignore_skip_hint && get_pageblock_skip(page)) { >> low_pfn =3D end_pfn; >> + page =3D NULL; >> goto isolate_abort; >> } >> valid_page =3D page; >> @@ -968,6 +969,21 @@ static bool too_many_isolated(pg_data_t *pgdat) >> if (!(cc->gfp_mask & __GFP_FS) && page_mapping(page)) >> goto isolate_fail; >> =20 >> + /* >> + * Be careful not to clear PageLRU until after we're >> + * sure the page is not being freed elsewhere -- the >> + * page release code relies on it. >> + */ >> + if (unlikely(!get_page_unless_zero(page))) >> + goto isolate_fail; >> + >> + if (__isolate_lru_page_prepare(page, isolate_mode) !=3D 0) >> + goto isolate_fail_put; >> + >> + /* Try isolate the page */ >> + if (!TestClearPageLRU(page)) >> + goto isolate_fail_put; >> + >> /* If we already hold the lock, we can skip some rechecking */ >> if (!locked) { >> locked =3D compact_lock_irqsave(&pgdat->lru_lock, >> @@ -980,10 +996,6 @@ static bool too_many_isolated(pg_data_t *pgdat) >> goto isolate_abort; >> } >> =20 >> - /* Recheck PageLRU and PageCompound under lock */ >> - if (!PageLRU(page)) >> - goto isolate_fail; >> - >> /* >> * Page become compound since the non-locked check, >> * and it's on LRU. It can only be a THP so the order >> @@ -991,16 +1003,13 @@ static bool too_many_isolated(pg_data_t *pgdat) >> */ >> if (unlikely(PageCompound(page) && !cc->alloc_contig)) { >> low_pfn +=3D compound_nr(page) - 1; >> - goto isolate_fail; >> + SetPageLRU(page); >> + goto isolate_fail_put; >> } >> } >> =20 >> lruvec =3D mem_cgroup_page_lruvec(page, pgdat); >> =20 >> - /* Try isolate the page */ >> - if (__isolate_lru_page(page, isolate_mode) !=3D 0) >> - goto isolate_fail; >> - >> /* The whole page is taken off the LRU; skip the tail pages. */ >> if (PageCompound(page)) >> low_pfn +=3D compound_nr(page) - 1; >> @@ -1029,6 +1038,15 @@ static bool too_many_isolated(pg_data_t *pgdat) >> } >> =20 >> continue; >> + >> +isolate_fail_put: >> + /* Avoid potential deadlock in freeing page under lru_lock */ >> + if (locked) { >> + spin_unlock_irqrestore(&pgdat->lru_lock, flags); >> + locked =3D false; >> + } >> + put_page(page); >> + >> isolate_fail: >> if (!skip_on_failure) >> continue; >> @@ -1065,9 +1083,15 @@ static bool too_many_isolated(pg_data_t *pgdat) >> if (unlikely(low_pfn > end_pfn)) >> low_pfn =3D end_pfn; >> =20 >> + page =3D NULL; >> + >> isolate_abort: >> if (locked) >> spin_unlock_irqrestore(&pgdat->lru_lock, flags); >> + if (page) { >> + SetPageLRU(page); >> + put_page(page); >> + } >> =20 >> /* >> * Updated the cached scanner pfn once the pageblock has been scanne= d >> diff --git a/mm/vmscan.c b/mm/vmscan.c >> index 1b3e0eeaad64..48b50695f883 100644 >> --- a/mm/vmscan.c >> +++ b/mm/vmscan.c >> @@ -1538,20 +1538,20 @@ unsigned int reclaim_clean_pages_from_list(str= uct zone *zone, >> * >> * returns 0 on success, -ve errno on failure. >> */ >> -int __isolate_lru_page(struct page *page, isolate_mode_t mode) >> +int __isolate_lru_page_prepare(struct page *page, isolate_mode_t mode= ) >> { >> int ret =3D -EINVAL; >> =20 >> - /* 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; >> =20 >> ret =3D -EBUSY; >> =20 >> + /* Only take pages on the LRU. */ >> + if (!PageLRU(page)) >> + return ret; >> + >=20 > So here you do deal with that BUG() issue. But I'd prefer you to leave > it as I suggested in 16/32, just start with "int ret =3D -EBUSY;" and > don't rearrange the checks here at all. I say that partly because > the !PageLRU check is very important (when called for compaction), and > the easier it is to find (at the very start), the less anxious I get! yes, have done as your suggestion. >=20 >> /* >> * To minimise LRU disruption, the caller can indicate that it only >> * wants to isolate pages it will be able to operate on without >> @@ -1592,20 +1592,9 @@ int __isolate_lru_page(struct page *page, isola= te_mode_t mode) >> if ((mode & ISOLATE_UNMAPPED) && page_mapped(page)) >> return ret; >> =20 >> - if (likely(get_page_unless_zero(page))) { >> - /* >> - * Be careful not to clear PageLRU until after we're >> - * sure the page is not being freed elsewhere -- the >> - * page release code relies on it. >> - */ >> - ClearPageLRU(page); >> - ret =3D 0; >> - } >> - >> - return ret; >> + return 0; >> } >> =20 >> - >> /* >> * Update LRU sizes after isolating pages. The LRU size updates must >> * be complete before mem_cgroup_update_lru_size due to a sanity chec= k. >> @@ -1685,17 +1674,34 @@ static unsigned long isolate_lru_pages(unsigne= d long nr_to_scan, >> * only when the page is being freed somewhere else. >> */ >> scan +=3D nr_pages; >> - switch (__isolate_lru_page(page, mode)) { >> + switch (__isolate_lru_page_prepare(page, mode)) { >> case 0: >> + /* >> + * Be careful not to clear PageLRU until after we're >> + * sure the page is not being freed elsewhere -- the >> + * page release code relies on it. >> + */ >> + if (unlikely(!get_page_unless_zero(page))) >> + goto busy; >> + >> + if (!TestClearPageLRU(page)) { >> + /* >> + * This page may in other isolation path, >> + * but we still hold lru_lock. >> + */ >> + put_page(page); >> + goto busy; >> + } >> + >> nr_taken +=3D nr_pages; >> nr_zone_taken[page_zonenum(page)] +=3D nr_pages; >> list_move(&page->lru, dst); >> break; >> - >> +busy: >> case -EBUSY: >=20 > It's a long time since I read a C manual. I had to try that out in a > little test program: and it does seem to do the right thing. Maybe > I'm just very ignorant, and everybody else finds that natural: but I'd > feel more comfortable with the busy label on the line after the > "case -EBUSY:" - wouldn't you? will move down. Thanks! >=20 > You could, of course, change that "case -EBUSY" to "default", > and delete the "default: BUG();" that follows: whatever you prefer. >=20 yes, the default is enough after last patch's change. >> /* else it is being freed elsewhere */ >> list_move(&page->lru, src); >> - continue; >> + break; >=20 > Aha. Yes, I like that change, I'm not going to throw a tantrum, > accusing you of sneaking in unrelated changes etc. You made me look > back at the history: it was "continue" from back in the days of > lumpy reclaim, when there was stuff after the switch statement > which needed to be skipped in the -EBUSY case. "break" looks > more natural to me now. Thanks! with above 'default' change, the break could be saved finally. :) Thanks! >=20 >> =20 >> default: >> BUG(); >> --=20 >> 1.8.3.1