On 2/24/21 11:47 AM, Kirill Tkhai wrote: > On 22.02.2021 21:40, Vasily Averin wrote: >> When mem_cgroup is removes mem_cgroup_css_offline() calls >> memcg_deactivate_kmem() which disables kmem accounting. >> If memcg still have some charged kmem final css_put is not called, >> and delayed till last kmem will be uncharged. >> >> Usually kmem is uncharged by using memcg_uncharge_kmem() which have >> according checks and if required calls final css_put(). >> >> Though patch added to vz7.162.14 kernel >> "mm/memcg: Use per-cpu stock charges for ->kmem and ->cache counters" >> enabled kmem charge/uncharge in refill_stock()/drain_stock() >> without using of memcg_uncharge_kmem(), as result nobody called >> final css_put() after last kmem uncharge. >> >> This patch adds css_get/put for safe access to memcg in drain_stock(), >> and calls an additional css_put() after last kmem uncharge. >> >> Fixes: "mm/memcg: Use per-cpu stock charges for ->kmem and ->cache counters" >> https://bugs.openvz.org/browse/OVZ-7250 >> Signed-off-by: Vasily Averin <[email protected]> >> --- >> mm/memcontrol.c | 20 ++++++++++++++++---- >> 1 file changed, 16 insertions(+), 4 deletions(-) >> >> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >> index e0a4309..24e3bd7 100644 >> --- a/mm/memcontrol.c >> +++ b/mm/memcontrol.c >> @@ -439,6 +439,8 @@ enum { >> KMEM_ACCOUNTED_DEAD, /* dead memcg with pending kmem charges */ >> }; >> >> +static void memcg_kmem_release_css(struct mem_cgroup *memcg); >> + >> static struct mem_cgroup_per_node * >> mem_cgroup_nodeinfo(struct mem_cgroup *memcg, int nid) >> { >> @@ -2928,11 +2930,15 @@ static void drain_stock(struct memcg_stock_pcp >> *stock) >> { >> struct mem_cgroup *old = stock->cached; >> unsigned long nr_pages = stock->nr_pages + stock->cache_nr_pages + >> stock->kmem_nr_pages; >> + u64 kmem = 1; >> + >> + if (!old) >> + return; >> >> if (stock->cache_nr_pages) >> page_counter_uncharge(&old->cache, stock->cache_nr_pages); >> if (stock->kmem_nr_pages) >> - page_counter_uncharge(&old->kmem, stock->kmem_nr_pages); >> + kmem = page_counter_uncharge(&old->kmem, stock->kmem_nr_pages); >> >> if (nr_pages) { >> page_counter_uncharge(&old->memory, nr_pages); >> @@ -2942,6 +2948,9 @@ static void drain_stock(struct memcg_stock_pcp *stock) >> stock->kmem_nr_pages = 0; >> stock->cache_nr_pages = 0; >> } >> + css_put(&old->css); > > css_put()/css_get() pair looks excess. What is your arguments, why do we need > them? > We have a problem with kmem only, while the rest of accounting looks safe: > they are safely uncharged from css_offline->mem_cgroup_reparent_charges(), > and they > never own css counter.
We know that kmem can be uncharged after mem_cgroup_css_offline() -> memcg_deactivate_kmem() because kmem was not 0 inside memcg_deactivate_kmem and according css_put() was not called. I did not investigated how is called drain_stock uncharged last kmem, and I do not see who keeps memcg refcount here. Please explain this if you know. If drain_stock() can be called after end of mem_cgroup_css_offline it is a problem. css_get/put present in refill_stock()/drain_stock() long time ago, though I did not investigated when and why exactly it was added. Anyway, I'm not sure that it is really required in vz7 kernel, we did not saw any related troubles. _______________________________________________ Devel mailing list [email protected] https://lists.openvz.org/mailman/listinfo/devel
