From: Michal Hocko <mhocko@kernel.org> To: Johannes Weiner <hannes@cmpxchg.org> Cc: Stable tree <stable@vger.kernel.org>, Vladimir Davydov <vdavydov@parallels.com>, Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>, Nikolay Borisov <kernel@kyup.com>, Linus Torvalds <torvalds@linux-foundation.org> Subject: Re: [PATCH stable-4.4 1/3] mm: memcontrol: fix cgroup creation failure after many small jobs Date: Mon, 15 Aug 2016 16:04:39 +0200 [thread overview] Message-ID: <20160815140439.GF3360@dhcp22.suse.cz> (raw) In-Reply-To: <20160815133748.GA3775@cmpxchg.org> On Mon 15-08-16 09:37:48, Johannes Weiner wrote: > On Mon, Aug 15, 2016 at 02:46:19PM +0200, Michal Hocko wrote: > > On Mon 15-08-16 08:34:07, Johannes Weiner wrote: > > > Hi Michal, thanks for doing this. There is only one issue I can see: > > > > > > On Fri, Aug 12, 2016 at 11:56:17AM +0200, Michal Hocko wrote: > > > > @@ -4171,17 +4211,27 @@ 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 out_free; > > > > + > > > > memcg->stat = alloc_percpu(struct mem_cgroup_stat_cpu); > > > > if (!memcg->stat) > > > > - goto out_free; > > > > + goto out_idr; > > > > > > > > if (memcg_wb_domain_init(memcg, GFP_KERNEL)) > > > > goto out_free_stat; > > > > > > > > + idr_replace(&mem_cgroup_idr, memcg, memcg->id.id); > > > > > > This publishes the memcg object too early. Before 4.5, the memcg is > > > not fully initialized in mem_cgroup_alloc(). You have to move the > > > idr_replace() down to that function (and idr_remove() on free_out). > > > > You are right. I am just wondering whether it matters. Nobody should see > > the id so nobody will be looking it up, no? > > Page cache shadow entries refer to these IDs weakly. It's possible to > refault with a recently recycled memcg ID and crash. That's why we do > the whole alloc(NULL) -> replace(memcg) dance in the first place. Ahh, OK, you are right. So I have moved the idr_replace into mem_cgroup_css_alloc. Does the following incremental diff looks better? --- diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 41fb6a0d2d03..7d6ac40efa81 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -4239,12 +4239,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 out_free; - memcg->stat = alloc_percpu(struct mem_cgroup_stat_cpu); if (!memcg->stat) goto out_idr; @@ -4252,13 +4246,16 @@ static struct mem_cgroup *mem_cgroup_alloc(void) if (memcg_wb_domain_init(memcg, GFP_KERNEL)) goto out_free_stat; - idr_replace(&mem_cgroup_idr, memcg, memcg->id.id); + memcg->id.id = idr_alloc(&mem_cgroup_idr, NULL, + 1, MEM_CGROUP_ID_MAX, + GFP_KERNEL); + if (memcg->id.id < 0) + goto out_free_stat; + return memcg; out_free_stat: free_percpu(memcg->stat); -out_idr: - idr_remove(&mem_cgroup_idr, memcg->id.id); out_free: kfree(memcg); return NULL; @@ -4340,9 +4337,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) #ifdef CONFIG_CGROUP_WRITEBACK INIT_LIST_HEAD(&memcg->cgwb_list); #endif + idr_replace(&mem_cgroup_idr, memcg, memcg->id.id); return &memcg->css; free_out: + idr_remove(&mem_cgroup_idr, memcg->id.id); __mem_cgroup_free(memcg); return ERR_PTR(error); } -- Michal Hocko SUSE Labs
WARNING: multiple messages have this Message-ID (diff)
From: Michal Hocko <mhocko@kernel.org> To: Johannes Weiner <hannes@cmpxchg.org> Cc: Stable tree <stable@vger.kernel.org>, Vladimir Davydov <vdavydov@parallels.com>, Andrew Morton <akpm@linux-foundation.org>, linux-mm@kvack.org, LKML <linux-kernel@vger.kernel.org>, Nikolay Borisov <kernel@kyup.com>, Linus Torvalds <torvalds@linux-foundation.org> Subject: Re: [PATCH stable-4.4 1/3] mm: memcontrol: fix cgroup creation failure after many small jobs Date: Mon, 15 Aug 2016 16:04:39 +0200 [thread overview] Message-ID: <20160815140439.GF3360@dhcp22.suse.cz> (raw) In-Reply-To: <20160815133748.GA3775@cmpxchg.org> On Mon 15-08-16 09:37:48, Johannes Weiner wrote: > On Mon, Aug 15, 2016 at 02:46:19PM +0200, Michal Hocko wrote: > > On Mon 15-08-16 08:34:07, Johannes Weiner wrote: > > > Hi Michal, thanks for doing this. There is only one issue I can see: > > > > > > On Fri, Aug 12, 2016 at 11:56:17AM +0200, Michal Hocko wrote: > > > > @@ -4171,17 +4211,27 @@ 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 out_free; > > > > + > > > > memcg->stat = alloc_percpu(struct mem_cgroup_stat_cpu); > > > > if (!memcg->stat) > > > > - goto out_free; > > > > + goto out_idr; > > > > > > > > if (memcg_wb_domain_init(memcg, GFP_KERNEL)) > > > > goto out_free_stat; > > > > > > > > + idr_replace(&mem_cgroup_idr, memcg, memcg->id.id); > > > > > > This publishes the memcg object too early. Before 4.5, the memcg is > > > not fully initialized in mem_cgroup_alloc(). You have to move the > > > idr_replace() down to that function (and idr_remove() on free_out). > > > > You are right. I am just wondering whether it matters. Nobody should see > > the id so nobody will be looking it up, no? > > Page cache shadow entries refer to these IDs weakly. It's possible to > refault with a recently recycled memcg ID and crash. That's why we do > the whole alloc(NULL) -> replace(memcg) dance in the first place. Ahh, OK, you are right. So I have moved the idr_replace into mem_cgroup_css_alloc. Does the following incremental diff looks better? --- diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 41fb6a0d2d03..7d6ac40efa81 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -4239,12 +4239,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 out_free; - memcg->stat = alloc_percpu(struct mem_cgroup_stat_cpu); if (!memcg->stat) goto out_idr; @@ -4252,13 +4246,16 @@ static struct mem_cgroup *mem_cgroup_alloc(void) if (memcg_wb_domain_init(memcg, GFP_KERNEL)) goto out_free_stat; - idr_replace(&mem_cgroup_idr, memcg, memcg->id.id); + memcg->id.id = idr_alloc(&mem_cgroup_idr, NULL, + 1, MEM_CGROUP_ID_MAX, + GFP_KERNEL); + if (memcg->id.id < 0) + goto out_free_stat; + return memcg; out_free_stat: free_percpu(memcg->stat); -out_idr: - idr_remove(&mem_cgroup_idr, memcg->id.id); out_free: kfree(memcg); return NULL; @@ -4340,9 +4337,11 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css) #ifdef CONFIG_CGROUP_WRITEBACK INIT_LIST_HEAD(&memcg->cgwb_list); #endif + idr_replace(&mem_cgroup_idr, memcg, memcg->id.id); return &memcg->css; free_out: + idr_remove(&mem_cgroup_idr, memcg->id.id); __mem_cgroup_free(memcg); return ERR_PTR(error); } -- Michal Hocko SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
next prev parent reply other threads:[~2016-08-15 14:06 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-08-12 9:56 [PATCH stable-4.4 0/3] backport memcg id patches Michal Hocko 2016-08-12 9:56 ` Michal Hocko 2016-08-12 9:56 ` Michal Hocko 2016-08-12 9:56 ` [PATCH stable-4.4 1/3] mm: memcontrol: fix cgroup creation failure after many small jobs Michal Hocko 2016-08-12 9:56 ` Michal Hocko 2016-08-12 9:56 ` Michal Hocko 2016-08-15 12:34 ` Johannes Weiner 2016-08-15 12:34 ` Johannes Weiner 2016-08-15 12:46 ` Michal Hocko 2016-08-15 12:46 ` Michal Hocko 2016-08-15 13:37 ` Johannes Weiner 2016-08-15 13:37 ` Johannes Weiner 2016-08-15 14:04 ` Michal Hocko [this message] 2016-08-15 14:04 ` Michal Hocko 2016-08-15 14:34 ` Johannes Weiner 2016-08-15 14:34 ` Johannes Weiner 2016-08-12 9:56 ` [PATCH stable-4.4 2/3] mm: memcontrol: fix swap counter leak on swapout from offline cgroup Michal Hocko 2016-08-12 9:56 ` Michal Hocko 2016-08-12 9:56 ` Michal Hocko 2016-08-12 9:56 ` [PATCH stable-4.4 3/3] mm: memcontrol: fix memcg id ref counter on swap charge move Michal Hocko 2016-08-12 9:56 ` Michal Hocko 2016-08-12 9:56 ` Michal Hocko 2016-08-14 16:08 ` [PATCH stable-4.4 0/3] backport memcg id patches Greg KH 2016-08-14 16:08 ` Greg KH 2016-08-15 15:06 [PATCH stable-4.4 0/3 v2] " Michal Hocko 2016-08-15 15:06 ` [PATCH stable-4.4 1/3] mm: memcontrol: fix cgroup creation failure after many small jobs Michal Hocko 2016-08-15 15:06 ` Michal Hocko 2016-08-15 15:06 ` Michal Hocko 2016-08-15 15:16 ` Johannes Weiner 2016-08-15 15:16 ` Johannes Weiner 2016-08-15 15:29 ` Michal Hocko 2016-08-15 15:29 ` Michal Hocko 2016-08-15 15:22 ` Greg KH 2016-08-15 15:22 ` Greg KH 2016-08-15 15:33 ` Michal Hocko 2016-08-15 15:33 ` Michal Hocko 2016-08-15 15:35 ` Michal Hocko 2016-08-15 15:35 ` Michal Hocko 2016-08-15 15:35 ` Michal Hocko 2016-08-15 19:09 ` Greg KH 2016-08-15 19:09 ` Greg KH
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160815140439.GF3360@dhcp22.suse.cz \ --to=mhocko@kernel.org \ --cc=akpm@linux-foundation.org \ --cc=hannes@cmpxchg.org \ --cc=kernel@kyup.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=stable@vger.kernel.org \ --cc=torvalds@linux-foundation.org \ --cc=vdavydov@parallels.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.