* [PATCH] mm: memcontrol: fix memsw uncharge for root_mem_cgroup
@ 2021-03-23 14:56 Muchun Song
2021-03-23 16:11 ` Michal Hocko
2021-03-24 4:11 ` Muchun Song
0 siblings, 2 replies; 6+ messages in thread
From: Muchun Song @ 2021-03-23 14:56 UTC (permalink / raw)
To: guro, hannes, mhocko, akpm, shakeelb, vdavydov.dev
Cc: linux-kernel, linux-mm, duanxiongchun, Muchun Song
The pages aren't accounted at the root level, so we cannot uncharge the
page to the memsw counter for the root memcg. Fix this.
Fixes: 1f47b61fb407 ("mm: memcontrol: fix swap counter leak on swapout from offline cgroup")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
mm/memcontrol.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 533b4b31b464..7d765a106684 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -7155,7 +7155,8 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
if (!cgroup_memory_noswap && memcg != swap_memcg) {
if (!mem_cgroup_is_root(swap_memcg))
page_counter_charge(&swap_memcg->memsw, nr_entries);
- page_counter_uncharge(&memcg->memsw, nr_entries);
+ if (!mem_cgroup_is_root(memcg))
+ page_counter_uncharge(&memcg->memsw, nr_entries);
}
/*
--
2.11.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] mm: memcontrol: fix memsw uncharge for root_mem_cgroup
2021-03-23 14:56 [PATCH] mm: memcontrol: fix memsw uncharge for root_mem_cgroup Muchun Song
@ 2021-03-23 16:11 ` Michal Hocko
2021-03-24 4:11 ` Muchun Song
1 sibling, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2021-03-23 16:11 UTC (permalink / raw)
To: Muchun Song
Cc: guro, hannes, akpm, shakeelb, vdavydov.dev, linux-kernel,
linux-mm, duanxiongchun
On Tue 23-03-21 22:56:53, Muchun Song wrote:
> The pages aren't accounted at the root level, so we cannot uncharge the
> page to the memsw counter for the root memcg. Fix this.
The patch is correct but I do wonder whether this matters much in the
end. We shouldn't really rely on a correct page counter for the root
memcg AFAICS in the kernel. We do not display the value
(mem_cgroup_usage) so there shouldn't be any actual problem. Unless I am
missing something make sure to spell that out in the changelog.
> Fixes: 1f47b61fb407 ("mm: memcontrol: fix swap counter leak on swapout from offline cgroup")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
Acked-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/memcontrol.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 533b4b31b464..7d765a106684 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7155,7 +7155,8 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
> if (!cgroup_memory_noswap && memcg != swap_memcg) {
> if (!mem_cgroup_is_root(swap_memcg))
> page_counter_charge(&swap_memcg->memsw, nr_entries);
> - page_counter_uncharge(&memcg->memsw, nr_entries);
> + if (!mem_cgroup_is_root(memcg))
> + page_counter_uncharge(&memcg->memsw, nr_entries);
> }
>
> /*
> --
> 2.11.0
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm: memcontrol: fix memsw uncharge for root_mem_cgroup
2021-03-23 14:56 [PATCH] mm: memcontrol: fix memsw uncharge for root_mem_cgroup Muchun Song
2021-03-23 16:11 ` Michal Hocko
@ 2021-03-24 4:11 ` Muchun Song
2021-03-24 8:33 ` Michal Hocko
1 sibling, 1 reply; 6+ messages in thread
From: Muchun Song @ 2021-03-24 4:11 UTC (permalink / raw)
To: Roman Gushchin, Johannes Weiner, Michal Hocko, Andrew Morton,
Shakeel Butt, Vladimir Davydov
Cc: LKML, Linux Memory Management List
On Tue, Mar 23, 2021 at 11:04 PM Muchun Song <songmuchun@bytedance.com> wrote:
>
> The pages aren't accounted at the root level, so we cannot uncharge the
> page to the memsw counter for the root memcg. Fix this.
>
> Fixes: 1f47b61fb407 ("mm: memcontrol: fix swap counter leak on swapout from offline cgroup")
> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
I am very sorry. I should repent. I suddenly realise the fix is totally
wrong. Because the @memcg cannot be root memcg when
@memcg != @swap_memcg. Please ignore this patch. I am very
sorry for the noise. And sorry to Michal.
> ---
> mm/memcontrol.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 533b4b31b464..7d765a106684 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -7155,7 +7155,8 @@ void mem_cgroup_swapout(struct page *page, swp_entry_t entry)
> if (!cgroup_memory_noswap && memcg != swap_memcg) {
> if (!mem_cgroup_is_root(swap_memcg))
> page_counter_charge(&swap_memcg->memsw, nr_entries);
> - page_counter_uncharge(&memcg->memsw, nr_entries);
> + if (!mem_cgroup_is_root(memcg))
> + page_counter_uncharge(&memcg->memsw, nr_entries);
> }
>
> /*
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] mm: memcontrol: fix memsw uncharge for root_mem_cgroup
2021-03-24 4:11 ` Muchun Song
@ 2021-03-24 8:33 ` Michal Hocko
2021-03-24 8:50 ` [External] " Muchun Song
0 siblings, 1 reply; 6+ messages in thread
From: Michal Hocko @ 2021-03-24 8:33 UTC (permalink / raw)
To: Muchun Song
Cc: Roman Gushchin, Johannes Weiner, Andrew Morton, Shakeel Butt,
Vladimir Davydov, LKML, Linux Memory Management List
On Wed 24-03-21 12:11:35, Muchun Song wrote:
> On Tue, Mar 23, 2021 at 11:04 PM Muchun Song <songmuchun@bytedance.com> wrote:
> >
> > The pages aren't accounted at the root level, so we cannot uncharge the
> > page to the memsw counter for the root memcg. Fix this.
> >
> > Fixes: 1f47b61fb407 ("mm: memcontrol: fix swap counter leak on swapout from offline cgroup")
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
>
> I am very sorry. I should repent. I suddenly realise the fix is totally
> wrong. Because the @memcg cannot be root memcg when
> @memcg != @swap_memcg.
I am probably blind but I do not see why this would be the case.
We have memcg != swap_memcg in this branch but we do not know the
neither of the two is root_mem_cgroup, no? If we did knot that we
wouldn't have to check for swap_memcg != root_mem_cgroup. Or do I miss
something?
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [External] Re: [PATCH] mm: memcontrol: fix memsw uncharge for root_mem_cgroup
2021-03-24 8:33 ` Michal Hocko
@ 2021-03-24 8:50 ` Muchun Song
2021-03-24 9:20 ` Michal Hocko
0 siblings, 1 reply; 6+ messages in thread
From: Muchun Song @ 2021-03-24 8:50 UTC (permalink / raw)
To: Michal Hocko
Cc: Roman Gushchin, Johannes Weiner, Andrew Morton, Shakeel Butt,
Vladimir Davydov, LKML, Linux Memory Management List
On Wed, Mar 24, 2021 at 4:33 PM Michal Hocko <mhocko@suse.com> wrote:
>
> On Wed 24-03-21 12:11:35, Muchun Song wrote:
> > On Tue, Mar 23, 2021 at 11:04 PM Muchun Song <songmuchun@bytedance.com> wrote:
> > >
> > > The pages aren't accounted at the root level, so we cannot uncharge the
> > > page to the memsw counter for the root memcg. Fix this.
> > >
> > > Fixes: 1f47b61fb407 ("mm: memcontrol: fix swap counter leak on swapout from offline cgroup")
> > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> >
> > I am very sorry. I should repent. I suddenly realise the fix is totally
> > wrong. Because the @memcg cannot be root memcg when
> > @memcg != @swap_memcg.
>
> I am probably blind but I do not see why this would be the case.
> We have memcg != swap_memcg in this branch but we do not know the
> neither of the two is root_mem_cgroup, no? If we did knot that we
> wouldn't have to check for swap_memcg != root_mem_cgroup. Or do I miss
> something?
I look at the mem_cgroup_id_get_online() closely. If memcg is root, this
function always returns root memcg. So memcg will equal swap_memcg.
I apologize for not carefully reviewing the code myself.
Thanks.
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [External] Re: [PATCH] mm: memcontrol: fix memsw uncharge for root_mem_cgroup
2021-03-24 8:50 ` [External] " Muchun Song
@ 2021-03-24 9:20 ` Michal Hocko
0 siblings, 0 replies; 6+ messages in thread
From: Michal Hocko @ 2021-03-24 9:20 UTC (permalink / raw)
To: Muchun Song
Cc: Roman Gushchin, Johannes Weiner, Andrew Morton, Shakeel Butt,
Vladimir Davydov, LKML, Linux Memory Management List
On Wed 24-03-21 16:50:41, Muchun Song wrote:
> On Wed, Mar 24, 2021 at 4:33 PM Michal Hocko <mhocko@suse.com> wrote:
> >
> > On Wed 24-03-21 12:11:35, Muchun Song wrote:
> > > On Tue, Mar 23, 2021 at 11:04 PM Muchun Song <songmuchun@bytedance.com> wrote:
> > > >
> > > > The pages aren't accounted at the root level, so we cannot uncharge the
> > > > page to the memsw counter for the root memcg. Fix this.
> > > >
> > > > Fixes: 1f47b61fb407 ("mm: memcontrol: fix swap counter leak on swapout from offline cgroup")
> > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > >
> > > I am very sorry. I should repent. I suddenly realise the fix is totally
> > > wrong. Because the @memcg cannot be root memcg when
> > > @memcg != @swap_memcg.
> >
> > I am probably blind but I do not see why this would be the case.
> > We have memcg != swap_memcg in this branch but we do not know the
> > neither of the two is root_mem_cgroup, no? If we did knot that we
> > wouldn't have to check for swap_memcg != root_mem_cgroup. Or do I miss
> > something?
>
> I look at the mem_cgroup_id_get_online() closely. If memcg is root, this
> function always returns root memcg. So memcg will equal swap_memcg.
Ahh, I can see it now. I have completely missed that the swap_memcg is
a parent of an offline memcg. I should have looked more closely.
--
Michal Hocko
SUSE Labs
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-03-24 9:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-23 14:56 [PATCH] mm: memcontrol: fix memsw uncharge for root_mem_cgroup Muchun Song
2021-03-23 16:11 ` Michal Hocko
2021-03-24 4:11 ` Muchun Song
2021-03-24 8:33 ` Michal Hocko
2021-03-24 8:50 ` [External] " Muchun Song
2021-03-24 9:20 ` Michal Hocko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).