On Thu, Jul 26, 2018 at 04:25:12PM -0700, Andrew Morton wrote: > On Fri, 13 Apr 2018 14:51:01 +0200 Michal Hocko <mhocko@xxxxxxxxxx> wrote: > > > On Fri 13-04-18 14:14:33, Michal Hocko wrote: > > [...] > > > Well, this is probably a matter of taste. I will not argue. I will not > > > object if Johannes is OK with your patch. But the whole thing confused > > > hell out of me so I would rather un-clutter it... > > > > In other words, this > > > > This discussion has rather petered out. afaict we're waiting for > hannes to offer an opinion? > > > From: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> > Subject: memcg: remove memcg_cgroup::id from IDR on mem_cgroup_css_alloc() failure > > In case of memcg_online_kmem() failure, memcg_cgroup::id remains hashed in > mem_cgroup_idr even after memcg memory is freed. This leads to leak of ID > in mem_cgroup_idr. > > This patch adds removal into mem_cgroup_css_alloc(), which fixes the > problem. For better readability, it adds a generic helper which is used > in mem_cgroup_alloc() and mem_cgroup_id_put_many() as well. > > Link: http://lkml.kernel.org/r/152354470916.22460.14397070748001974638.stgit@localhost.localdomain > Fixes 73f576c04b94 ("mm: memcontrol: fix cgroup creation failure after many small jobs") > Signed-off-by: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx> > Cc: Johannes Weiner <hannes@xxxxxxxxxxx> > Cc: Vladimir Davydov <vdavydov.dev@xxxxxxxxx> > Cc: Michal Hocko <mhocko@xxxxxxxxxx> > Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> I also do wonder if we can do it cleaner, but since it's a fix I don't want that discussion to hold things up: Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx> That said, the lifetime of the root reference on the ID is the online state, we put that in css_offline. Is there a reason we need to have the ID ready and the memcg in the IDR before onlining it? Can we do something like this and not mess with the alloc/free sequence at all? Michal, Vladimir, am I missing something? diff --git a/mm/memcontrol.c b/mm/memcontrol.c index c59519d600ea..865e6d41d3d1 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -4144,12 +4144,6 @@ static struct mem_cgroup *mem_cgroup_alloc(void) if (!memcg) return NULL; - memcg->id.id = idr_alloc(&mem_cgroup_idr, NULL, - 1, MEM_CGROUP_ID_MAX, - GFP_KERNEL); - if (memcg->id.id < 0) - goto fail; - memcg->stat_cpu = alloc_percpu(struct mem_cgroup_stat_cpu); if (!memcg->stat_cpu) goto fail; @@ -4176,11 +4170,8 @@ static struct mem_cgroup *mem_cgroup_alloc(void) #ifdef CONFIG_CGROUP_WRITEBACK INIT_LIST_HEAD(&memcg->cgwb_list); #endif - idr_replace(&mem_cgroup_idr, memcg, memcg->id.id); return memcg; fail: - if (memcg->id.id > 0) - idr_remove(&mem_cgroup_idr, memcg->id.id); __mem_cgroup_free(memcg); return NULL; } @@ -4246,10 +4237,17 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) static int mem_cgroup_css_online(struct cgroup_subsys_state *css) { struct mem_cgroup *memcg = mem_cgroup_from_css(css); + int i; + + i = idr_alloc(&mem_cgroup_idr, memcg, 1, MEM_CGROUP_ID_MAX, GFP_KERNEL); + if (i < 0) + return i; /* Online state pins memcg ID, memcg ID pins CSS */ + memcg->id.id = i; atomic_set(&memcg->id.ref, 1); css_get(css); + return 0; }
