Re: [PATCH 3/3] mm: Fix missing mem cgroup soft limit tree updates
From: Michal Hocko <mhocko@suse.com>
Date: 2021-02-10 10:23:24
Also in:
cgroups, lkml
On Tue 09-02-21 12:29:47, Tim Chen wrote:
On a per node basis, the mem cgroup soft limit tree on each node tracks how much a cgroup has exceeded its soft limit memory limit and sorts the cgroup by its excess usage. On page release, the trees are not updated right away, until we have gathered a batch of pages belonging to the same cgroup. This reduces the frequency of updating the soft limit tree and locking of the tree and associated cgroup. However, the batch of pages could contain pages from multiple nodes but only the soft limit tree from one node would get updated. Change the logic so that we update the tree in batch of pages, with each batch of pages all in the same mem cgroup and memory node. An update is issued for the batch of pages of a node collected till now whenever we encounter a page belonging to a different node.
I do agree with Johannes here. This shouldn't be done unconditionally
for all memcgs. Wouldn't it be much better to do the fix up in the
mem_cgroup_soft_reclaim path instead. Simply check the excess before
doing any reclaim?
Btw. have you seen this triggering a noticeable misbehaving? I would
expect this to have a rather small effect considering how many sources
of memcg_check_events we have.
Unless I have missed something this has been introduced by 747db954cab6
("mm: memcontrol: use page lists for uncharge batching"). Please add
Fixes tag as well if this is really worth fixing.
quoted hunk ↗ jump to hunk
Reviewed-by: Ying Huang <redacted> Signed-off-by: Tim Chen <redacted> --- mm/memcontrol.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)diff --git a/mm/memcontrol.c b/mm/memcontrol.c index d72449eeb85a..f5a4a0e4e2ec 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c@@ -6804,6 +6804,7 @@ struct uncharge_gather { unsigned long pgpgout; unsigned long nr_kmem; struct page *dummy_page; + int nid; }; static inline void uncharge_gather_clear(struct uncharge_gather *ug)@@ -6849,7 +6850,9 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) * exclusive access to the page. */ - if (ug->memcg != page_memcg(page)) { + if (ug->memcg != page_memcg(page) || + /* uncharge batch update soft limit tree on a node basis */ + (ug->dummy_page && ug->nid != page_to_nid(page))) { if (ug->memcg) { uncharge_batch(ug); uncharge_gather_clear(ug);@@ -6869,6 +6872,7 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) ug->pgpgout++; ug->dummy_page = page; + ug->nid = page_to_nid(page); page->memcg_data = 0; css_put(&ug->memcg->css); }-- 2.20.1
-- Michal Hocko SUSE Labs