Received: by 2002:a05:6358:d09b:b0:dc:cd0c:909e with SMTP id jc27csp70987rwb; Wed, 7 Dec 2022 14:37:17 -0800 (PST) X-Google-Smtp-Source: AA0mqf4uhAPqFpaAtv66LEzrJiVjdB9fgeDaG4y45zG4CLh9Rhn9d6H+o+ka8YQHmBuDKJW7B6+P X-Received: by 2002:a17:906:809:b0:7c0:e4b8:7587 with SMTP id e9-20020a170906080900b007c0e4b87587mr14301940ejd.593.1670452636820; Wed, 07 Dec 2022 14:37:16 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1670452636; cv=none; d=google.com; s=arc-20160816; b=FyJSE9d7P3LeMJT/lEhwT2Ls8+/DRFEhbupiY6pug5Jn/N8suXw6kZ61qbKfqYaDJf hfGJZsjNSuQ8e3bY0OHn1eK9muYl91w3Dor3WcyrbwmkcMsYQ/Cmfy1YQ7EXvaJ2epus LGKPBFsvMoHZcZPc1n7AqDxvJW9pnq8lcF33RGV+GZKIujhktOJKhUf/TxgcWILGpbE6 ysaPt3LBLNB2cR2GStp0JyU9W9eBSZedvBFbpC7hnoqFcZzVHu8z9jilcx/Klk5F/Xhx NDTJKAPNYxvqx4rIOvofYpoTl5mGpWgyhZ91B2mji6vahe7YuZzhLyytWuKY8HjN19JF volg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:subject:message-id:date:from:in-reply-to :references:mime-version:dkim-signature; bh=PH5qkkhOdX31aPnHkE3V+O9BkOMtpCmn+q9akZ7n0EA=; b=m2i5uYfCi9U/6C1Dpnpjm2i8ToHPxmo+f4y2vWINjTfLtKSKQgLefcoxR4GE/Z/ncR Q1CJQsV1brnTG6KM40KHVpU3/t4lZtRmZhilqD21hM7ntuG/T5nv8Zt0S7CHI6YkVHkl emPNLcouY4GkRdEcCebk36a87lDcvP0wjBRtObGvZkZIPdUcy4zHwBIWi7RFzHqEfVIJ aHGw4gFVX7eyCzqBimHoQ4ypZ8LX8qGPRYlIilk9eQvYfmTOZ2Vy6jVS5DiAlJGNPQEL 4mLTLGdxXEYgCwj0rGMZONEC7/wfX8kToAg9FND1iDwSLu8TxTLg+haVb3yC1fRqD2+d tFyw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b="QIM/aEKY"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t24-20020a170906269800b007ae1e635ea3si14243002ejc.754.2022.12.07.14.36.57; Wed, 07 Dec 2022 14:37:16 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20210112 header.b="QIM/aEKY"; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229621AbiLGWPF (ORCPT + 75 others); Wed, 7 Dec 2022 17:15:05 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:54894 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229479AbiLGWPD (ORCPT ); Wed, 7 Dec 2022 17:15:03 -0500 Received: from mail-vk1-xa2d.google.com (mail-vk1-xa2d.google.com [IPv6:2607:f8b0:4864:20::a2d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E4D905E3C1 for ; Wed, 7 Dec 2022 14:15:01 -0800 (PST) Received: by mail-vk1-xa2d.google.com with SMTP id 6so6869332vkz.0 for ; Wed, 07 Dec 2022 14:15:01 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=PH5qkkhOdX31aPnHkE3V+O9BkOMtpCmn+q9akZ7n0EA=; b=QIM/aEKYPS5i0a6wGmSCuQVinwVODi25J4CePg8iqVxPgyHOudOqe7w8qSplB6in7l B3GF1ZLp4cP5LfWUwT+TyAx+zmeOplPMcaZlpslrRQYmRX+dtGKpyxOq68piI4AJa4t8 V6WVSLW5A19Snbf5ubtt7lpkbrKHzs+kgRlLEg5jZO5IXxi7bLCSFjLQqo8f5X+pCVXS ZYGw2gVkCWSkgxqpUOD5FuVeLdkOfjifJxbldHHdeMDG9pzD2EDYOrySMS2ULBKd8/zh DQsl+KjSqCoxcLUX0z0+w6iUkhSoB38LuHgMRA/t38QmGpnO4wOoCBmRxoFBjPnxy5rz OvSg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=PH5qkkhOdX31aPnHkE3V+O9BkOMtpCmn+q9akZ7n0EA=; b=kEujE3T/M5TB/aEBRY8FeoVvtBIVauHtYS60zQ2G+90n1MjpIDkhDX7mdCLeN4VciE p8tlHW64TJlRR9FuFiWCByu6tdBT1uGPsTKEjFbQLD6LXJbhiWStV1hBbZmD3f5uTVhn ZwssiaOOY6IqxwaQXJpGL9Dj2Ndn6Ou+pgUMIuD8YG4YiOcf3FyBMAq3b8nc7kxsroER daOTQAU+PVD0+zr38q3X7cN1cJy8DKSao1oCC8e7qLdIOIih/j0r+oV+x9RloXxma5Ot lYKYneLCEq5aY/oK8Wn6kd4EURJF80ouW/th8bdK6npa+CPQSpoHJRdBz11q+KhsTA5u P3kw== X-Gm-Message-State: ANoB5pniJkucYuRyATrq9XO3iJK+ljxeGDxprDs+4Wd/FH119mRXPAov 1FKLmFt7uC1Vvh0CRrjdz6IK+9CaGzFy1GcklbHzhg== X-Received: by 2002:a05:6122:41e:b0:3bd:ad7c:b3ec with SMTP id e30-20020a056122041e00b003bdad7cb3ecmr8016418vkd.0.1670451300897; Wed, 07 Dec 2022 14:15:00 -0800 (PST) MIME-Version: 1.0 References: <20221204093008.2620459-1-almasrymina@google.com> <87k033eiwj.fsf@linux.ibm.com> In-Reply-To: <87k033eiwj.fsf@linux.ibm.com> From: Mina Almasry Date: Wed, 7 Dec 2022 14:14:49 -0800 Message-ID: Subject: Re: [PATCH v2] [mm-unstable] mm: Fix memcg reclaim on memory tiered systems To: "Aneesh Kumar K.V" Cc: Andrew Morton , Johannes Weiner , Michal Hocko , Roman Gushchin , Shakeel Butt , Muchun Song , Huang Ying , Yang Shi , Yosry Ahmed , weixugc@google.com, fvdl@google.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org Content-Type: text/plain; charset="UTF-8" X-Spam-Status: No, score=-17.6 required=5.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF, ENV_AND_HDR_SPF_MATCH,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL,USER_IN_DEF_SPF_WL autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Dec 7, 2022 at 12:07 AM Aneesh Kumar K.V 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, but 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, try_to_free_mem_cgroup_pages() actually unconditionally counts > > demoted pages as reclaimed pages. So in practice 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 as the caller requested. > > > > I suspect various things work suboptimally on memory 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 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 demotion or reclaim. > > > > Tested this change using memory.reclaim interface. With this change, > > > > echo "1m" > memory.reclaim > > > > Will cause freeing of 1m of memory from the cgroup regardless of the > > demotions happening inside. > > > > echo "1m nodes=0" > memory.reclaim > > > > Will cause freeing of 1m of node 0 by demotion if a demotion target is > > available, and by reclaim if no demotion target is available. > > > > Signed-off-by: Mina Almasry > > > > --- > > > > This is developed on top of mm-unstable largely to test with memory.reclaim > > nodes= arg and ensure the fix is compatible with that. > > > > v2: > > - Shortened the commit message a bit. > > - Fixed issue when demotion falls back to other allowed target nodes returned by > > node_get_allowed_targets() as Wei suggested. > > > > Cc: weixugc@google.com > > --- > > include/linux/memory-tiers.h | 7 +++++-- > > mm/memory-tiers.c | 10 +++++++++- > > mm/vmscan.c | 20 +++++++++++++++++--- > > 3 files changed, 31 insertions(+), 6 deletions(-) > > > > diff --git a/include/linux/memory-tiers.h b/include/linux/memory-tiers.h > > index fc9647b1b4f9..f3f359760fd0 100644 > > --- a/include/linux/memory-tiers.h > > +++ b/include/linux/memory-tiers.h > > @@ -38,7 +38,8 @@ void init_node_memory_type(int node, struct memory_dev_type *default_type); > > void clear_node_memory_type(int node, struct memory_dev_type *memtype); > > #ifdef CONFIG_MIGRATION > > int next_demotion_node(int node); > > -void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets); > > +void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets, > > + nodemask_t *demote_from_targets); > > bool node_is_toptier(int node); > > #else > > static inline int next_demotion_node(int node) > > @@ -46,7 +47,9 @@ static inline int next_demotion_node(int node) > > return NUMA_NO_NODE; > > } > > > > -static inline void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets) > > +static inline void node_get_allowed_targets(pg_data_t *pgdat, > > + nodemask_t *targets, > > + nodemask_t *demote_from_targets) > > { > > *targets = NODE_MASK_NONE; > > } > > diff --git a/mm/memory-tiers.c b/mm/memory-tiers.c > > index c734658c6242..7f8f0b5de2b3 100644 > > --- a/mm/memory-tiers.c > > +++ b/mm/memory-tiers.c > > @@ -264,7 +264,8 @@ bool node_is_toptier(int node) > > return toptier; > > } > > > > -void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets) > > +void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets, > > + nodemask_t *demote_from_targets) > > { > > struct memory_tier *memtier; > > > > @@ -280,6 +281,13 @@ void node_get_allowed_targets(pg_data_t *pgdat, nodemask_t *targets) > > else > > *targets = NODE_MASK_NONE; > > rcu_read_unlock(); > > + > > + /* > > + * Exclude the demote_from_targets from the allowed targets if we're > > + * trying to demote from a specific set of nodes. > > + */ > > + if (demote_from_targets) > > + nodes_andnot(*targets, *targets, *demote_from_targets); > > } > > Will this cause demotion to not work when we have memory policy like > MPOL_BIND with nodemask including demotion targets? > Hi Aneesh, You may want to review v3 of this patch that removed this bit: https://lore.kernel.org/linux-mm/202212070124.VxwbfKCK-lkp@intel.com/T/#t To answer your question though, it will disable demotion between the MPOL_BIND nodes I think, yes. That may be another reason not to do this (it's already removed in v3). > > > > > /** > > diff --git a/mm/vmscan.c b/mm/vmscan.c > > index 2b42ac9ad755..97ca0445b5dc 100644 > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -1590,7 +1590,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 *demote_from_nodemask) > > { > > int target_nid = next_demotion_node(pgdat->node_id); > > unsigned int nr_succeeded; > > @@ -1614,7 +1615,7 @@ static unsigned int demote_folio_list(struct list_head *demote_folios, > > if (target_nid == NUMA_NO_NODE) > > return 0; > > > > - node_get_allowed_targets(pgdat, &allowed_mask); > > + node_get_allowed_targets(pgdat, &allowed_mask, demote_from_nodemask); > > > > /* Demotion ignores all cpuset and mempolicy settings */ > > migrate_pages(demote_folios, alloc_demote_page, NULL, > > @@ -1653,6 +1654,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 = 0; > > unsigned int pgactivate = 0; > > bool do_demote_pass; > > struct swap_iocb *plug = NULL; > > @@ -2085,7 +2087,19 @@ 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 = demote_folio_list(&demote_folios, pgdat, sc->nodemask); > > + > > + /* > > + * Only count demoted folios as reclaimed if the caller has requested > > + * demotion from a specific nodemask. In this case pages inside the > > + * noedmask have been demoted to outside the nodemask and we can count > > + * these pages as reclaimed. If no nodemask is passed, then the caller > > + * is requesting reclaim from all memory, which should not count > > + * demoted pages. > > + */ > > + if (sc->nodemask) > > + nr_reclaimed += nr_demoted; > > + > > /* 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 */ > > -- > > 2.39.0.rc0.267.gcb52ba06e7-goog