| Message ID | 20210323145653.25684-1-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: memcontrol: fix memsw uncharge for root_mem_cgroup | expand |
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
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 >
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?
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
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.
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); } /*
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(-)