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 7F7DEC63705 for ; Wed, 7 Dec 2022 22:15:04 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id E291E8E0003; Wed, 7 Dec 2022 17:15:03 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id DD7D28E0001; Wed, 7 Dec 2022 17:15:03 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id CA0038E0003; Wed, 7 Dec 2022 17:15:03 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id B9A338E0001 for ; Wed, 7 Dec 2022 17:15:03 -0500 (EST) Received: from smtpin08.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 80D661204EB for ; Wed, 7 Dec 2022 22:15:03 +0000 (UTC) X-FDA: 80216916486.08.FE900CE Received: from mail-vk1-f170.google.com (mail-vk1-f170.google.com [209.85.221.170]) by imf11.hostedemail.com (Postfix) with ESMTP id D2C1140015 for ; Wed, 7 Dec 2022 22:15:00 +0000 (UTC) Authentication-Results: imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b="QIM/aEKY"; spf=pass (imf11.hostedemail.com: domain of almasrymina@google.com designates 209.85.221.170 as permitted sender) smtp.mailfrom=almasrymina@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1670451302; a=rsa-sha256; cv=none; b=WrpxAexmaq3b3R3FTDHv4HK1qrTXZXx6JkJtO0pnh2IQRcp+bncVI0eP0Ql6dO/Q9SWZcv S1fw4YUSnHW1abpasFrZmOYYgMAtG6vQ4dfSvxo1yGSAdgF9fmFC1kvCs3aQUGntNsmSkR 9j9Z5LN8fUqEl3cnR+WZ2NYfsWYhTJs= ARC-Authentication-Results: i=1; imf11.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b="QIM/aEKY"; spf=pass (imf11.hostedemail.com: domain of almasrymina@google.com designates 209.85.221.170 as permitted sender) smtp.mailfrom=almasrymina@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1670451301; 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=PH5qkkhOdX31aPnHkE3V+O9BkOMtpCmn+q9akZ7n0EA=; b=W/Y61mS4PuSm7lQzBXXrFerM4y7yyAhhD8oXPysZPLzQB7qcOpyhHvon8WQFAtHxl4HUnA N/wFm8ouEG9LKFKDBFaUzq7s7fq6Y9QpPRrRodeHgx9MhnhES4vZ1wlPBKbbFT+Y1VLsBO VReoUmHk6QHOXiVU9RS3gejK0Cf50Io= Received: by mail-vk1-f170.google.com with SMTP id r3so4311491vkq.13 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=uPrTdWfYtwhDeNJ4MjVXdzdRIJp0EM/lDU0joRZ/Yc3fstRs4GSgSyW4to4HCmH9A9 0DZPfyDaCKPotMaW+FIYa3F0oq48wthdrIPZ98ymT/Ntc7vibssmY5r7MmLlykcYGtrO srYtPifg9//IS4EuzXBcq+hXaDyLwFakkSOoV4LlLAF2hClYnK8FZlybb8BxMJnFKiy8 xSw3lbY/Ij74glfZH6TBSNfy2Y9zBFsAr+NnR/ShCsbyXu8AXpeY8KyWnS9MnTZIZNbt OxcF3K/CwIAO2sKYF0BM3w++YZUkOz+9+a/7EKghoJAGqXE6j+INxNsBetYRkfYrXFTR d16A== X-Gm-Message-State: ANoB5plEdexMspPUu+ZWzAhGYw9Ka7fGq9bU3daSldx5r8/XJPHV+WMD NHwtMNeoLH9UUjwRyRv/BnpsymVPB5uhAh7UImAkXQ== X-Google-Smtp-Source: AA0mqf7BlqYV0+pSW8BXET5o66pRi5GBn8x6wyurAuCKkYJ71gzniB+ce4r1wB7lf9rGvcBc+8IxJ9Ot9ixd/0JXmQ8= 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-Rspam-User: X-Rspamd-Queue-Id: D2C1140015 X-Rspamd-Server: rspam01 X-Stat-Signature: rn1rqa96eajt59deijnm43spwjwy16et X-HE-Tag: 1670451300-864723 X-HE-Meta: U2FsdGVkX1+ROn+4E7RpgYza5uIllD13AE4QhVSKYmYmfPtN1sTPr4284HJZXJTD/ulmk2AfyQG3aIezjuOgTNFso1Q12U/eqnmsIKhSsNeoQJxlveEcoBHYSvZFgSNn/cvGHf6NY2WJ/o5Xnp4KJTUAt3uwPLrwVk0lq/qgud4cFj8Z0Tb+tpnRkf66oPR9GMp0b+GplxZg+i35T26NAUZ5rVe10OFzc983vkZvpWvJwsx81px6whohTuf/z1M6pa5DPjYklZpwK0kUopPpPjzFqiQiCNn0iarIkFvpB4k8UohrwXIOuV2zJfeHTGzpjwAXNTlQDGb5Vteo18zofPZSaDcCqTJyuCYV1hUnsKZesPqDbjeBf9LQydO7g9zEVFZZVelgSw6HsyTmzw8FGiwjeq/l2uQOz0E6Fb67BQhyPB+bS2Mq7aqr8tJ26hqwF+D73ej0qKXHpGZpS53ZdFE2RWMQ8Mz2dJuuxzH2u3STzMw7n2bhXfMuD4z9v2lBeEofQCLDGP7+dah8NoPLEcD78EMbbS1oNYkZKsRU6ghcLtCJz/mGjtNkfyMdLf2ShsoNEeWuDgXi8iPR0i30Nd69gcafYkcK7Ht7ql64k3PvWrMpwtkbIgkziGiTocB4Vf1O3MewIFN7buV1rPeQP6R4b7CvjnfzU1fnHwDScDG7BINlONjXhv83MgMPyNX38f/5DA420TXuHOY5OcKpea/2bjFBZz4F4sjVetLsqmfR12cW0nKLsldOrCAaJMZ5RmSmwCuBRgoHwFzaqLha2GhTxa9KZmaYh7+FcSEsXsSn7E1eE0xVtuq+PG8sy+Mv3PVAywYbsx1KZlzilEu3ScWIT1c7pADjyGEcdRR0qcRuclrPnE2a8nGC1TchPWQMUHxjzzETJ1e2YTeBVivirSQWXKyxTNGlaNs76ekGnOif7dk/N8xUJ0RKFr51VxHZy8Fpr5xjGlM98nG73is 12EOyiYD bRW7K3bLT6ew26Zs= 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 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