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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 850B0C352A1 for ; Tue, 6 Dec 2022 05:23:17 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D19A38E0002; Tue, 6 Dec 2022 00:23:16 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id CC90F8E0001; Tue, 6 Dec 2022 00:23:16 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B90EA8E0002; Tue, 6 Dec 2022 00:23:16 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id A93BD8E0001 for ; Tue, 6 Dec 2022 00:23:16 -0500 (EST) Received: from smtpin26.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay04.hostedemail.com (Postfix) with ESMTP id 4C22C1A07B4 for ; Tue, 6 Dec 2022 05:23:16 +0000 (UTC) X-FDA: 80210737992.26.F31F2E5 Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by imf24.hostedemail.com (Postfix) with ESMTP id 99A9B180012 for ; Tue, 6 Dec 2022 05:23:14 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=bolrK7lv; spf=pass (imf24.hostedemail.com: domain of ying.huang@intel.com designates 192.55.52.136 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1670304195; a=rsa-sha256; cv=none; b=JHREwdHfdN7QkwysGIJt7KUYgi7VK/g05I1q4QnI56hUGxXvBOCZVIdNiRscIQPChn88W+ 5EB9DehnowyRDasH7bbF1jRchBfVAU7c2RpdV832Kv56o/mT99k1LsjT2mbmirxOljUJxV 2asYa4E7KIGpGVyTFyQ1LF6GfTzhtVs= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=intel.com header.s=Intel header.b=bolrK7lv; spf=pass (imf24.hostedemail.com: domain of ying.huang@intel.com designates 192.55.52.136 as permitted sender) smtp.mailfrom=ying.huang@intel.com; dmarc=pass (policy=none) header.from=intel.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1670304195; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=+z6dhlXZF20r0EBoEqHRy+lsMAAhOU7dy0A2RnCxGcA=; b=nZPn7grdv9Fm035JUwvrPDBPIWqqodFnzrc8gx68HuPMfyyMtaKDtyPL4WY8KR6b1oEN0c KLOzv28l5tWKrnuuKwPxrh+SsQ7+lsxymVHruanUmygKmjcbUHVgXFRQWiu6dOtJwGGwvG zjBYX5A5seFI4TX8zwEs+OhNL0Z2BeQ= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1670304194; x=1701840194; h=from:to:cc:subject:references:date:in-reply-to: message-id:mime-version; bh=zSTVWtJ6FaGtwqMMWWIBrtnA7IZJUKkeBk1eG1Jw/c0=; b=bolrK7lvXkFYbRW/HibodqUt2tHRJCZdT6xCqksTMf7BMWVQVobE2a5k JFREag/jrOd0vLPPPX7zeeWJLlU2HePdNTltqxXupp1JXwIn5iEy+ktY5 J+af1nmwmzBC73kWoLTa55IeEQPIKfYtnITOC1Zh2ClS86vvgLO6/HowH i9+N3SI0Emrof0r5NkPFYRH8jD9Us0uwT8w7Xb09lo19xz9BvtgKlg41S eltIZ7OMQrqoULtl+/rqDxFJ5G5wgMIAiPE1fKxc4CXhDeTiWjDhoYblO R0m/dsI94VmK9Igjk0TJ2rsWehjyy2HYAD/LfW+1s8e/4YmV5UROORz5D Q==; X-IronPort-AV: E=McAfee;i="6500,9779,10552"; a="296229117" X-IronPort-AV: E=Sophos;i="5.96,220,1665471600"; d="scan'208";a="296229117" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Dec 2022 21:23:12 -0800 X-IronPort-AV: E=McAfee;i="6500,9779,10552"; a="623769909" X-IronPort-AV: E=Sophos;i="5.96,220,1665471600"; d="scan'208";a="623769909" Received: from yhuang6-desk2.sh.intel.com (HELO yhuang6-desk2.ccr.corp.intel.com) ([10.238.208.55]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 05 Dec 2022 21:23:08 -0800 From: "Huang, Ying" To: Mina Almasry Cc: Andrew Morton , Johannes Weiner , Michal Hocko , Roman Gushchin , Shakeel Butt , Muchun Song , Yang Shi , Yosry Ahmed , weixugc@google.com, fvdl@google.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org, David Rientjes Subject: Re: [PATCH v3] [mm-unstable] mm: Fix memcg reclaim on memory tiered systems References: <20221206023406.3182800-1-almasrymina@google.com> <87wn75dy1q.fsf@yhuang6-desk2.ccr.corp.intel.com> Date: Tue, 06 Dec 2022 13:22:04 +0800 In-Reply-To: (Mina Almasry's message of "Mon, 5 Dec 2022 20:15:28 -0800") Message-ID: <87bkohds3n.fsf@yhuang6-desk2.ccr.corp.intel.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=ascii X-Spamd-Result: default: False [-6.90 / 9.00]; BAYES_HAM(-6.00)[100.00%]; DMARC_POLICY_ALLOW(-0.50)[intel.com,none]; R_SPF_ALLOW(-0.20)[+ip4:192.55.52.136/32]; R_DKIM_ALLOW(-0.20)[intel.com:s=Intel]; MIME_GOOD(-0.10)[text/plain]; RCVD_NO_TLS_LAST(0.10)[]; FROM_EQ_ENVFROM(0.00)[]; RCPT_COUNT_TWELVE(0.00)[14]; DKIM_TRACE(0.00)[intel.com:+]; MID_RHS_MATCH_FROMTLD(0.00)[]; TO_MATCH_ENVRCPT_SOME(0.00)[]; RCVD_COUNT_THREE(0.00)[3]; FROM_HAS_DN(0.00)[]; MIME_TRACE(0.00)[0:+]; TO_DN_SOME(0.00)[]; ARC_SIGNED(0.00)[hostedemail.com:s=arc-20220608:i=1]; ARC_NA(0.00)[] X-Rspam-User: X-Rspamd-Server: rspam03 X-Rspamd-Queue-Id: 99A9B180012 X-Stat-Signature: mj3zydgms3wx76j4za5ch7uzxoj7yw7r X-HE-Tag: 1670304194-677502 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: Mina Almasry writes: > On Mon, Dec 5, 2022 at 7:14 PM Huang, Ying wrote: >> >> Mina Almasry writes: >> >> > commit 3f1509c57b1b ("Revert "mm/vmscan: never demote for memcg >> > reclaim"") enabled demotion in memcg reclaim, which is the right thing >> > to do, however, it introduced a regression in the behavior of >> > try_to_free_mem_cgroup_pages(). >> > >> > The callers of try_to_free_mem_cgroup_pages() expect it to attempt to >> > reclaim - not demote - nr_pages from the cgroup. I.e. the memory usage >> > of the cgroup should reduce by nr_pages. The callers expect >> > try_to_free_mem_cgroup_pages() to also return the number of pages >> > reclaimed, not demoted. >> > >> > However, what try_to_free_mem_cgroup_pages() actually does is it >> > unconditionally counts demoted pages as reclaimed pages. So in practnice >> > when it is called it will often demote nr_pages and return the number of >> > demoted pages to the caller. Demoted pages don't lower the memcg usage, >> > and so try_to_free_mem_cgroup_pages() is not actually doing what the >> > callers want it to do. >> > >> > Various things work suboptimally on memory tiered systems or don't work >> > at all due to this: >> > >> > - memory.high enforcement likely doesn't work (it just demotes nr_pages >> > instead of lowering the memcg usage by nr_pages). >> > - try_charge_memcg() will keep retrying the charge while >> > try_to_free_mem_cgroup_pages() is just demoting pages and not actually >> > making any room for the charge. >> > - memory.reclaim has a wonky interface. It advertises to the user it >> > reclaims the provided amount but it will actually often demote that >> > amount. >> > >> > There may be more effects to this issue. >> > >> > To fix these issues I propose shrink_folio_list() to only count pages >> > demoted from inside of sc->nodemask to outside of sc->nodemask as >> > 'reclaimed'. >> > >> > For callers such as reclaim_high() or try_charge_memcg() that set >> > sc->nodemask to NULL, try_to_free_mem_cgroup_pages() will try to >> > actually reclaim nr_pages and return the number of pages reclaimed. No >> > demoted pages would count towards the nr_pages requirement. >> > >> > For callers such as memory_reclaim() that set sc->nodemask, >> > try_to_free_mem_cgroup_pages() will free nr_pages from that nodemask >> > with either reclaim or demotion. >> > >> > Tested this change using memory.reclaim interface. I set up a test case where >> > I allocate 500m in a cgroup, and then do: >> > >> > echo "50m" > memory.reclaim >> > >> > Without this fix, my kernel demotes 70mb and reclaims 4 mb >> > (memory.current is reduced by about 4mb). >> > >> > With this fix, my kernel demotes all memory possible and reclaims 60mb >> > (memory.current is reduced by about 60mb). >> > >> > Signed-off-by: Mina Almasry >> > >> > --- >> > >> > Changes in v3: >> > - Reverted back to v1 implementation: "try to demote but don't count >> > demoted pages unless they are demoted to outside the nodemask" as Ying >> > suggested. >> > - Made sure demotions that fall back to non next_demotion_target() are >> > not counted as Wei suggested. >> > - Updated comment in shrink_folio_list() as Ying suggested. >> > - Added before/after for the test case in commit message since Ying >> > asked. >> > - Fixed call sites that don't provide sc->nodemask but expect demotion >> > from a specific node as Ying pointed out. >> > >> > Cc: weixugc@google.com >> > Cc: ying.huang@intel.com >> > >> > This is developed on top of mm-unstable largely because I want the >> > solution to be compatible with the recently added nodes= arg on >> > mm-unstable. >> > --- >> > mm/vmscan.c | 91 +++++++++++++++++++++++++++++++++++++++++++++-------- >> > 1 file changed, 78 insertions(+), 13 deletions(-) >> > >> > diff --git a/mm/vmscan.c b/mm/vmscan.c >> > index 2b42ac9ad755..f324e80395c3 100644 >> > --- a/mm/vmscan.c >> > +++ b/mm/vmscan.c >> > @@ -1555,13 +1555,18 @@ static void folio_check_dirty_writeback(struct folio *folio, >> > mapping->a_ops->is_dirty_writeback(folio, dirty, writeback); >> > } >> > >> > +struct demotion_control { >> > + struct migration_target_control *mtc; >> > + nodemask_t *demote_from_nodemask; >> > + unsigned long nr_demoted_outside_nodemask; >> > +}; >> > + >> > static struct page *alloc_demote_page(struct page *page, unsigned long private) >> > { >> > struct page *target_page; >> > nodemask_t *allowed_mask; >> > - struct migration_target_control *mtc; >> > - >> > - mtc = (struct migration_target_control *)private; >> > + struct demotion_control *dc = (struct demotion_control *)private; >> > + struct migration_target_control *mtc = dc->mtc; >> > >> > allowed_mask = mtc->nmask; >> > /* >> > @@ -1576,13 +1581,31 @@ static struct page *alloc_demote_page(struct page *page, unsigned long private) >> > mtc->nmask = NULL; >> > mtc->gfp_mask |= __GFP_THISNODE; >> > target_page = alloc_migration_target(page, (unsigned long)mtc); >> > - if (target_page) >> > + if (!target_page) { >> > + mtc->gfp_mask &= ~__GFP_THISNODE; >> > + mtc->nmask = allowed_mask; >> > + target_page = alloc_migration_target(page, (unsigned long)mtc); >> > + } >> > + >> > + if (!target_page) >> > return target_page; >> > >> > - mtc->gfp_mask &= ~__GFP_THISNODE; >> > - mtc->nmask = allowed_mask; >> > + if (dc->demote_from_nodemask && >> > + !node_isset(page_to_nid(target_page), *dc->demote_from_nodemask)) >> >> Use mtc->nid directly? >> > > mtc->nid is the next_demotion_node(). Wei's earlier comment is that > the page may be allocated anywhere on the get_allowed_targets(), not > necessarily the next_demotion_node(), so I don't think I can use > mtc->nid. I think I have to check on which node the page was allocated > as I'm doing here. Let me know if I missed something. Yes. You are right. >> > + dc->nr_demoted_outside_nodemask++; >> > >> > - return alloc_migration_target(page, (unsigned long)mtc); >> > + return target_page; >> > +} >> > + >> > +void free_demote_page(struct page *page, unsigned long private) >> > +{ >> > + struct demotion_control *dc = (struct demotion_control *)private; >> > + >> > + if (dc->demote_from_nodemask && >> > + !node_isset(page_to_nid(page), *dc->demote_from_nodemask)) >> >> ditto >> >> > + dc->nr_demoted_outside_nodemask--; >> > + >> > + folio_put(page_folio(page)); >> > } >> > >> > /* >> > @@ -1590,7 +1613,8 @@ static struct page *alloc_demote_page(struct page *page, unsigned long private) >> > * Folios which are not demoted are left on @demote_folios. >> > */ >> > static unsigned int demote_folio_list(struct list_head *demote_folios, >> > - struct pglist_data *pgdat) >> > + struct pglist_data *pgdat, >> > + nodemask_t *nodemask) >> > { >> > int target_nid = next_demotion_node(pgdat->node_id); >> > unsigned int nr_succeeded; >> > @@ -1608,6 +1632,12 @@ static unsigned int demote_folio_list(struct list_head *demote_folios, >> > .nmask = &allowed_mask >> > }; >> > >> > + struct demotion_control dc = { >> > + .mtc = &mtc, >> > + .demote_from_nodemask = nodemask, >> > + .nr_demoted_outside_nodemask = 0, >> > + }; >> > + >> > if (list_empty(demote_folios)) >> > return 0; >> > >> > @@ -1617,13 +1647,13 @@ static unsigned int demote_folio_list(struct list_head *demote_folios, >> > node_get_allowed_targets(pgdat, &allowed_mask); >> > >> > /* Demotion ignores all cpuset and mempolicy settings */ >> > - migrate_pages(demote_folios, alloc_demote_page, NULL, >> > - (unsigned long)&mtc, MIGRATE_ASYNC, MR_DEMOTION, >> > + migrate_pages(demote_folios, alloc_demote_page, free_demote_page, >> > + (unsigned long)&dc, MIGRATE_ASYNC, MR_DEMOTION, >> > &nr_succeeded); >> > >> > __count_vm_events(PGDEMOTE_KSWAPD + reclaimer_offset(), nr_succeeded); >> > >> > - return nr_succeeded; >> > + return dc.nr_demoted_outside_nodemask; >> > } >> > >> > static bool may_enter_fs(struct folio *folio, gfp_t gfp_mask) >> > @@ -1643,7 +1673,12 @@ static bool may_enter_fs(struct folio *folio, gfp_t gfp_mask) >> > } >> > >> > /* >> > - * shrink_folio_list() returns the number of reclaimed pages >> > + * shrink_folio_list() returns the number of reclaimed pages. >> > + * >> > + * Demoted pages are counted as reclaimed iff: >> > + * (a) sc->nodemask arg is provided. >> > + * (b) page has been demoted from a node inside sc->nodemask to a node >> > + * outside sc->nodemask. >> > */ >> > static unsigned int shrink_folio_list(struct list_head *folio_list, >> > struct pglist_data *pgdat, struct scan_control *sc, >> > @@ -1653,6 +1688,7 @@ static unsigned int shrink_folio_list(struct list_head *folio_list, >> > LIST_HEAD(free_folios); >> > LIST_HEAD(demote_folios); >> > unsigned int nr_reclaimed = 0; >> > + unsigned int nr_demoted_outside_nodemask = 0; >> > unsigned int pgactivate = 0; >> > bool do_demote_pass; >> > struct swap_iocb *plug = NULL; >> > @@ -2085,7 +2121,12 @@ static unsigned int shrink_folio_list(struct list_head *folio_list, >> > /* 'folio_list' is always empty here */ >> > >> > /* Migrate folios selected for demotion */ >> > - nr_reclaimed += demote_folio_list(&demote_folios, pgdat); >> > + nr_demoted_outside_nodemask = >> > + demote_folio_list(&demote_folios, pgdat, sc->nodemask); >> > + >> > + if (sc->nodemask) >> > + nr_reclaimed += nr_demoted_outside_nodemask; >> > + >> > /* Folios that could not be demoted are still in @demote_folios */ >> > if (!list_empty(&demote_folios)) { >> > /* Folios which weren't demoted go back on @folio_list */ >> > @@ -2130,9 +2171,11 @@ static unsigned int shrink_folio_list(struct list_head *folio_list, >> > unsigned int reclaim_clean_pages_from_list(struct zone *zone, >> > struct list_head *folio_list) >> > { >> > + nodemask_t nodemask = NODE_MASK_NONE; >> >> Is it necessary for us to use NODEMASK_ALLOC/NODEMASK_FREE to save stack space? >> > > I can certainly use NODEMASK_ALLOC/NODEMASK_FREE if you'd like. I > think there are a few places that stack allocate nodemask_t already, > including one place I recently added in memory_reclaim(), so it > doesn't seem _necessary_ per say. > > If you're asking my opinion, AFAICT it's not an issue. I think you > need > 32 numa nodes before nodemask_t becomes an array of size 2 > longs on a 32-bit machine, and even then I think it's not a huge deal. > Up to you; I have no issue with converting to > NODEMASK_ALLOC/NODEMASK_FREE in v4. Both are OK for me. Want to know David Rientjes's idea here. Best Regards, Huang, Ying >> >> > struct scan_control sc = { >> > .gfp_mask = GFP_KERNEL, >> > .may_unmap = 1, >> > + .nodemask = &nodemask >> > }; >> > struct reclaim_stat stat; >> > unsigned int nr_reclaimed; >> > @@ -2140,6 +2183,12 @@ unsigned int reclaim_clean_pages_from_list(struct zone *zone, >> > LIST_HEAD(clean_folios); >> > unsigned int noreclaim_flag; >> > >> > + /* >> > + * Set the nodemask in sc to indicate to shrink_folio_list() that we're >> > + * looking for reclaim from this node. >> > + */ >> > + node_set(zone->zone_pgdat->node_id, nodemask); >> > + >> > list_for_each_entry_safe(folio, next, folio_list, lru) { >> > if (!folio_test_hugetlb(folio) && folio_is_file_lru(folio) && >> > !folio_test_dirty(folio) && !__folio_test_movable(folio) && >> > @@ -7031,12 +7080,20 @@ static int balance_pgdat(pg_data_t *pgdat, int order, int highest_zoneidx) >> > unsigned long zone_boosts[MAX_NR_ZONES] = { 0, }; >> > bool boosted; >> > struct zone *zone; >> > + nodemask_t nodemask = NODE_MASK_NONE; >> > struct scan_control sc = { >> > .gfp_mask = GFP_KERNEL, >> > .order = order, >> > .may_unmap = 1, >> > + .nodemask = &nodemask, >> > }; >> > >> > + /* >> > + * Set the nodemask in sc to indicate to kswapd_shrink_node() that we're >> > + * looking for reclaim from this node. >> > + */ >> > + node_set(pgdat->node_id, nodemask); >> > + >> > set_task_reclaim_state(current, &sc.reclaim_state); >> > psi_memstall_enter(&pflags); >> > __fs_reclaim_acquire(_THIS_IP_); >> > @@ -7642,6 +7699,7 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in >> > const unsigned long nr_pages = 1 << order; >> > struct task_struct *p = current; >> > unsigned int noreclaim_flag; >> > + nodemask_t nodemask = NODE_MASK_NONE; >> > struct scan_control sc = { >> > .nr_to_reclaim = max(nr_pages, SWAP_CLUSTER_MAX), >> > .gfp_mask = current_gfp_context(gfp_mask), >> > @@ -7651,9 +7709,16 @@ static int __node_reclaim(struct pglist_data *pgdat, gfp_t gfp_mask, unsigned in >> > .may_unmap = !!(node_reclaim_mode & RECLAIM_UNMAP), >> > .may_swap = 1, >> > .reclaim_idx = gfp_zone(gfp_mask), >> > + .nodemask = &nodemask, >> > }; >> > unsigned long pflags; >> > >> > + /* >> > + * Set the nodemask in sc to indicate to shrink_node() that we're >> > + * looking for reclaim from this node. >> > + */ >> > + node_set(pgdat->node_id, nodemask); >> > + >> > trace_mm_vmscan_node_reclaim_begin(pgdat->node_id, order, >> > sc.gfp_mask); >> > >> > -- >> > 2.39.0.rc0.267.gcb52ba06e7-goog >>