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=-10.2 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,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 4EF0BC433F5 for ; Tue, 21 Sep 2021 11:12:40 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E682061166 for ; Tue, 21 Sep 2021 11:12:39 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org E682061166 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=techsingularity.net Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 5FE6C94000D; Tue, 21 Sep 2021 07:12:39 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5AE0C940009; Tue, 21 Sep 2021 07:12:39 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4C40694000D; Tue, 21 Sep 2021 07:12:39 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0208.hostedemail.com [216.40.44.208]) by kanga.kvack.org (Postfix) with ESMTP id 3DA0F940009 for ; Tue, 21 Sep 2021 07:12:39 -0400 (EDT) Received: from smtpin23.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id EF25B18079ED5 for ; Tue, 21 Sep 2021 11:12:38 +0000 (UTC) X-FDA: 78611317596.23.AE75C84 Received: from outbound-smtp57.blacknight.com (outbound-smtp57.blacknight.com [46.22.136.241]) by imf24.hostedemail.com (Postfix) with ESMTP id 615FFB0000A1 for ; Tue, 21 Sep 2021 11:12:38 +0000 (UTC) Received: from mail.blacknight.com (pemlinmail06.blacknight.ie [81.17.255.152]) by outbound-smtp57.blacknight.com (Postfix) with ESMTPS id D672EFAFCD for ; Tue, 21 Sep 2021 12:12:36 +0100 (IST) Received: (qmail 18735 invoked from network); 21 Sep 2021 11:12:36 -0000 Received: from unknown (HELO techsingularity.net) (mgorman@techsingularity.net@[84.203.17.29]) by 81.17.254.9 with ESMTPSA (AES256-SHA encrypted, authenticated); 21 Sep 2021 11:12:36 -0000 Date: Tue, 21 Sep 2021 12:12:34 +0100 From: Mel Gorman To: NeilBrown Cc: Linux-MM , Theodore Ts'o , Andreas Dilger , "Darrick J . Wong" , Matthew Wilcox , Michal Hocko , Dave Chinner , Rik van Riel , Vlastimil Babka , Johannes Weiner , Jonathan Corbet , Linux-fsdevel , LKML Subject: Re: [PATCH 1/5] mm/vmscan: Throttle reclaim until some writeback completes if congested Message-ID: <20210921111234.GQ3959@techsingularity.net> References: <20210920085436.20939-1-mgorman@techsingularity.net> <20210920085436.20939-2-mgorman@techsingularity.net> <163217994752.3992.5443677201798473600@noble.neil.brown.name> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-15 Content-Disposition: inline In-Reply-To: <163217994752.3992.5443677201798473600@noble.neil.brown.name> User-Agent: Mutt/1.10.1 (2018-07-13) X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 615FFB0000A1 X-Stat-Signature: kah4bwyhz8sai861n1he745ttbrk7ph7 Authentication-Results: imf24.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf24.hostedemail.com: domain of mgorman@techsingularity.net designates 46.22.136.241 as permitted sender) smtp.mailfrom=mgorman@techsingularity.net X-HE-Tag: 1632222758-227587 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 Tue, Sep 21, 2021 at 09:19:07AM +1000, NeilBrown wrote: > On Mon, 20 Sep 2021, Mel Gorman wrote: > > > > +void __acct_reclaim_writeback(pg_data_t *pgdat, struct page *page); > > +static inline void acct_reclaim_writeback(struct page *page) > > +{ > > + pg_data_t *pgdat = page_pgdat(page); > > + > > + if (atomic_read(&pgdat->nr_reclaim_throttled)) > > + __acct_reclaim_writeback(pgdat, page); > > The first thing __acct_reclaim_writeback() does is repeat that > atomic_read(). > Should we read it once and pass the value in to > __acct_reclaim_writeback(), or is that an unnecessary > micro-optimisation? > I think it's a micro-optimisation but I can still do it. > > > +/* > > + * Account for pages written if tasks are throttled waiting on dirty > > + * pages to clean. If enough pages have been cleaned since throttling > > + * started then wakeup the throttled tasks. > > + */ > > +void __acct_reclaim_writeback(pg_data_t *pgdat, struct page *page) > > +{ > > + unsigned long nr_written; > > + int nr_throttled = atomic_read(&pgdat->nr_reclaim_throttled); > > + > > + __inc_node_page_state(page, NR_THROTTLED_WRITTEN); > > + nr_written = node_page_state(pgdat, NR_THROTTLED_WRITTEN) - > > + READ_ONCE(pgdat->nr_reclaim_start); > > + > > + if (nr_written > SWAP_CLUSTER_MAX * nr_throttled) > > + wake_up_interruptible_all(&pgdat->reclaim_wait); > > A simple wake_up() could be used here. "interruptible" is only needed > if non-interruptible waiters should be left alone. "_all" is only needed > if there are some exclusive waiters. Neither of these apply, so I think > the simpler interface is best. > You're right. > > > +} > > + > > /* possible outcome of pageout() */ > > typedef enum { > > /* failed to write page out, page is locked */ > > @@ -1412,9 +1453,8 @@ static unsigned int shrink_page_list(struct list_head *page_list, > > > > /* > > * The number of dirty pages determines if a node is marked > > - * reclaim_congested which affects wait_iff_congested. kswapd > > - * will stall and start writing pages if the tail of the LRU > > - * is all dirty unqueued pages. > > + * reclaim_congested. kswapd will stall and start writing > > + * pages if the tail of the LRU is all dirty unqueued pages. > > */ > > page_check_dirty_writeback(page, &dirty, &writeback); > > if (dirty || writeback) > > @@ -3180,19 +3220,20 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) > > * If kswapd scans pages marked for immediate > > * reclaim and under writeback (nr_immediate), it > > * implies that pages are cycling through the LRU > > - * faster than they are written so also forcibly stall. > > + * faster than they are written so forcibly stall > > + * until some pages complete writeback. > > */ > > if (sc->nr.immediate) > > - congestion_wait(BLK_RW_ASYNC, HZ/10); > > + reclaim_throttle(pgdat, VMSCAN_THROTTLE_WRITEBACK, HZ/10); > > } > > > > /* > > * Tag a node/memcg as congested if all the dirty pages > > * scanned were backed by a congested BDI and > > "congested BDI" doesn't mean anything any more. Is this a good time to > correct that comment. > This comment seems to refer to the test > > sc->nr.dirty && sc->nr.dirty == sc->nr.congested) > > a few lines down. But nr.congested is set from nr_congested which > counts when inode_write_congested() is true - almost never - and when > "writeback and PageReclaim()". > > Is that last test the sign that we are cycling through the LRU to fast? > So the comment could become: > > Tag a node/memcg as congested if all the dirty page were > already marked for writeback and immediate reclaim (counted in > nr.congested). > > ?? > > Patch seems to make sense to me, but I'm not expert in this area. > Comments updated. Diff on top looks like diff --git a/mm/internal.h b/mm/internal.h index e25b3686bfab..90764d646e02 100644 --- a/mm/internal.h +++ b/mm/internal.h @@ -34,13 +34,15 @@ void page_writeback_init(void); -void __acct_reclaim_writeback(pg_data_t *pgdat, struct page *page); +void __acct_reclaim_writeback(pg_data_t *pgdat, struct page *page, + int nr_throttled); static inline void acct_reclaim_writeback(struct page *page) { pg_data_t *pgdat = page_pgdat(page); + int nr_throttled = atomic_read(&pgdat->nr_reclaim_throttled); - if (atomic_read(&pgdat->nr_reclaim_throttled)) - __acct_reclaim_writeback(pgdat, page); + if (nr_throttled) + __acct_reclaim_writeback(pgdat, page, nr_throttled); } vm_fault_t do_swap_page(struct vm_fault *vmf); diff --git a/mm/vmscan.c b/mm/vmscan.c index b58ea0b13286..2dc17de91d32 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1034,10 +1034,10 @@ reclaim_throttle(pg_data_t *pgdat, enum vmscan_throttle_state reason, * pages to clean. If enough pages have been cleaned since throttling * started then wakeup the throttled tasks. */ -void __acct_reclaim_writeback(pg_data_t *pgdat, struct page *page) +void __acct_reclaim_writeback(pg_data_t *pgdat, struct page *page, + int nr_throttled) { unsigned long nr_written; - int nr_throttled = atomic_read(&pgdat->nr_reclaim_throttled); __inc_node_page_state(page, NR_THROTTLED_WRITTEN); nr_written = node_page_state(pgdat, NR_THROTTLED_WRITTEN) - @@ -3228,9 +3228,8 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) } /* - * Tag a node/memcg as congested if all the dirty pages - * scanned were backed by a congested BDI and - * non-kswapd tasks will stall on reclaim_throttle. + * Tag a node/memcg as congested if all the dirty pages were marked + * for writeback and immediate reclaim (counted in nr.congested). * * Legacy memcg will stall in page writeback so avoid forcibly * stalling in reclaim_throttle(). @@ -3241,8 +3240,8 @@ static void shrink_node(pg_data_t *pgdat, struct scan_control *sc) set_bit(LRUVEC_CONGESTED, &target_lruvec->flags); /* - * Stall direct reclaim for IO completions if underlying BDIs - * and node is congested. Allow kswapd to continue until it + * Stall direct reclaim for IO completions if the lruvec is + * node is congested. Allow kswapd to continue until it * starts encountering unqueued dirty pages or cycling through * the LRU too quickly. */ @@ -4427,7 +4426,7 @@ void wakeup_kswapd(struct zone *zone, gfp_t gfp_flags, int order, trace_mm_vmscan_wakeup_kswapd(pgdat->node_id, highest_zoneidx, order, gfp_flags); - wake_up_interruptible(&pgdat->kswapd_wait); + wake_up_all(&pgdat->kswapd_wait); } #ifdef CONFIG_HIBERNATION