linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [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).