Received: by 2002:a25:683:0:0:0:0:0 with SMTP id 125csp357514ybg; Fri, 12 Jun 2020 03:29:05 -0700 (PDT) X-Google-Smtp-Source: ABdhPJxOJ++ES6liq2ER+khFGVcVgrBX5qV83z4nB1KK6i/ch+xU+KbL3tAEnlxRLgnlA8LIJiUg X-Received: by 2002:aa7:cdda:: with SMTP id h26mr11163591edw.327.1591957744849; Fri, 12 Jun 2020 03:29:04 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1591957744; cv=none; d=google.com; s=arc-20160816; b=SOElSN78g672957jIPNJ7aYH2+LHR5aPRcoK/ukTQ9P4sprDNq2i4aBvcE/VxSm/0r 6P9iolfGfqPOJThy3jhVBavAntn3XhsCuqlanLgQ+jbHRzsxNzE9m3VpzHoJlDlHe51f qtY+7kvvNfROm9SCdfJFlC/eLq6q3Ac6p38Gdm1pl1x2TMOZ/Pf4eja8Lsez0Ef94Wmr ITJEMtX9td3lV4oj0J0jMAF/JtPIA0pROPTXYjvmepZimu3Psi6sp64+5W1441LlkYxJ Pwz8XTCO6PnkF3tl2X8R6GzM6ttoa5y1u+l9ZCoBkP4z4DyoU8k5cSKnaB4GBojSn24p rqGg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:in-reply-to :mime-version:user-agent:date:message-id:references:to:from:subject; bh=eiUX9GexgRIbxKJ+VP4pvq2v4G09bQsWyugIaXIcCcs=; b=FmUtaM+35ONuhSjaECxCs62PlT1JZVjLRNQDZVIuIb7fLtmGWGQn8I2VfTWa4ZUs8H hZNWflj9WIzhZibuAVILqsQUQl6luwupUMMVjRF1xutq69KSWy/hWx1zHPc2k/MPpfcj jy1+mlJWdL530IaM3FEwPSPSlZaEs+SQ6NVqgkuauegGg5FHISsDLowhX3DAiHNvb/he X4VnomDvskio8XE+Droj3jCfkuvdqET1MSGgh3Qgc6jDwGTc4Va2GlDc2XsMgEbdawvg KUnXqfOkGhXz5Q2WSdApz2sdHSMgLOCPKcZe6+uTKKI7jllfR2LbV2TvTIgZfymfgpeR cKZA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alibaba.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id y21si3182507edt.501.2020.06.12.03.28.41; Fri, 12 Jun 2020 03:29:04 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=alibaba.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726306AbgFLK0N (ORCPT + 99 others); Fri, 12 Jun 2020 06:26:13 -0400 Received: from out30-132.freemail.mail.aliyun.com ([115.124.30.132]:51583 "EHLO out30-132.freemail.mail.aliyun.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726281AbgFLK0N (ORCPT ); Fri, 12 Jun 2020 06:26:13 -0400 X-Alimail-AntiSpam: AC=PASS;BC=-1|-1;BR=01201311R191e4;CH=green;DM=||false|;DS=||;FP=0|-1|-1|-1|0|-1|-1|-1;HT=e01f04427;MF=alex.shi@linux.alibaba.com;NM=1;PH=DS;RN=16;SR=0;TI=SMTPD_---0U.LngYm_1591957564; Received: from IT-FVFX43SYHV2H.local(mailfrom:alex.shi@linux.alibaba.com fp:SMTPD_---0U.LngYm_1591957564) by smtp.aliyun-inc.com(127.0.0.1); Fri, 12 Jun 2020 18:26:05 +0800 Subject: Re: [PATCH v12 10/16] mm/compaction: do page isolation first in compaction From: Alex Shi To: akpm@linux-foundation.org, mgorman@techsingularity.net, tj@kernel.org, hughd@google.com, khlebnikov@yandex-team.ru, daniel.m.jordan@oracle.com, yang.shi@linux.alibaba.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 References: <1591856209-166869-1-git-send-email-alex.shi@linux.alibaba.com> <1591856209-166869-11-git-send-email-alex.shi@linux.alibaba.com> Message-ID: <16b0041f-59ce-5685-f2c5-875af81f5221@linux.alibaba.com> Date: Fri, 12 Jun 2020 18:26:02 +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: <1591856209-166869-11-git-send-email-alex.shi@linux.alibaba.com> Content-Type: text/plain; charset=gbk Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Merged Hugh Dickins' fix, Hi Hugh, Do you mind to add a Signed-off for this patch? Thanks Alex From eca8de4afe04afd4ec1160ef593587d7b1a26129 Mon Sep 17 00:00:00 2001 From: Alex Shi Date: Tue, 26 May 2020 16:52:07 +0800 Subject: [PATCH v12 10/16] mm/compaction: do page isolation first in compaction Johannes Weiner has suggested: "So here is a crazy idea that may be worth exploring: Right now, pgdat->lru_lock protects both PageLRU *and* the lruvec's linked list. Can we make PageLRU atomic and use it to stabilize the lru_lock instead, and then use the lru_lock only serialize list operations? ..." Yes, this patch is doing so on __isolate_lru_page which is the core page isolation func in compaction and shrinking path. With this patch, the compaction will only deal the PageLRU set and now isolated pages to skip the just alloced page which no LRU bit. And the isolation could exclusive the other isolations in memcg move_account, page migrations and thp split_huge_page. As a side effect, PageLRU may be cleared during shrink_inactive_list path for isolation reason. If so, we can skip that page. 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 again 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: Alex Shi Cc: Andrew Morton Cc: Matthew Wilcox Cc: Hugh Dickins Cc: linux-kernel@vger.kernel.org Cc: linux-mm@kvack.org --- include/linux/swap.h | 2 +- mm/compaction.c | 42 +++++++++++++++++++++++++++++++++--------- mm/vmscan.c | 38 ++++++++++++++++++++++---------------- 3 files changed, 56 insertions(+), 26 deletions(-) diff --git a/include/linux/swap.h b/include/linux/swap.h index 29ba748b0d99..6811d0c34261 100644 --- a/include/linux/swap.h +++ b/include/linux/swap.h @@ -357,7 +357,7 @@ extern void lru_cache_add_active_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 5d59e77cd424..8d222036b8a0 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -869,6 +869,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 = end_pfn; + page = NULL; goto isolate_abort; } valid_page = page; @@ -950,6 +951,21 @@ static bool too_many_isolated(pg_data_t *pgdat) if (!(cc->gfp_mask & __GFP_FS) && page_mapping(page)) goto isolate_fail; + /* + * 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) != 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 = compact_lock_irqsave(&pgdat->lru_lock, @@ -962,10 +978,6 @@ static bool too_many_isolated(pg_data_t *pgdat) goto isolate_abort; } - /* 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 @@ -973,16 +985,13 @@ static bool too_many_isolated(pg_data_t *pgdat) */ if (unlikely(PageCompound(page) && !cc->alloc_contig)) { low_pfn += compound_nr(page) - 1; - goto isolate_fail; + SetPageLRU(page); + goto isolate_fail_put; } } lruvec = mem_cgroup_page_lruvec(page, pgdat); - /* Try isolate the page */ - if (__isolate_lru_page(page, isolate_mode) != 0) - goto isolate_fail; - /* The whole page is taken off the LRU; skip the tail pages. */ if (PageCompound(page)) low_pfn += compound_nr(page) - 1; @@ -1011,6 +1020,15 @@ static bool too_many_isolated(pg_data_t *pgdat) } continue; + +isolate_fail_put: + /* Avoid potential deadlock in freeing page under lru_lock */ + if (locked) { + spin_unlock_irqrestore(&pgdat->lru_lock, flags); + locked = false; + } + put_page(page); + isolate_fail: if (!skip_on_failure) continue; @@ -1047,9 +1065,15 @@ static bool too_many_isolated(pg_data_t *pgdat) if (unlikely(low_pfn > end_pfn)) low_pfn = end_pfn; + page = NULL; + isolate_abort: if (locked) spin_unlock_irqrestore(&pgdat->lru_lock, flags); + if (page) { + SetPageLRU(page); + put_page(page); + } /* * Updated the cached scanner pfn once the pageblock has been scanned diff --git a/mm/vmscan.c b/mm/vmscan.c index 08910ca9fa40..17b34e5b7ad5 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1543,7 +1543,7 @@ unsigned int reclaim_clean_pages_from_list(struct 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 = -EINVAL; @@ -1597,20 +1597,9 @@ int __isolate_lru_page(struct page *page, isolate_mode_t mode) if ((mode & ISOLATE_UNMAPPED) && page_mapped(page)) return ret; - 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 = 0; - } - - return ret; + return 0; } - /* * Update LRU sizes after isolating pages. The LRU size updates must * be complete before mem_cgroup_update_lru_size due to a sanity check. @@ -1690,17 +1679,34 @@ static unsigned long isolate_lru_pages(unsigned long nr_to_scan, * only when the page is being freed somewhere else. */ scan += 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 += nr_pages; nr_zone_taken[page_zonenum(page)] += nr_pages; list_move(&page->lru, dst); break; - +busy: case -EBUSY: /* else it is being freed elsewhere */ list_move(&page->lru, src); - continue; + break; default: BUG(); -- 1.8.3.1