* [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path @ 2014-01-30 16:01 Vladimir Davydov 2014-01-30 21:01 ` Andrew Morton 2014-01-30 21:09 ` David Rientjes 0 siblings, 2 replies; 16+ messages in thread From: Vladimir Davydov @ 2014-01-30 16:01 UTC (permalink / raw) To: akpm; +Cc: mhocko, linux-mm, linux-kernel Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> --- mm/memcontrol.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 19d5d4274e22..53385cd4e6f0 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3400,7 +3400,7 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep) static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, struct kmem_cache *s) { - struct kmem_cache *new; + struct kmem_cache *new = NULL; static char *tmp_name = NULL; static DEFINE_MUTEX(mutex); /* protects tmp_name */ @@ -3416,7 +3416,7 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, if (!tmp_name) { tmp_name = kmalloc(PATH_MAX, GFP_KERNEL); if (!tmp_name) - return NULL; + goto out; } rcu_read_lock(); @@ -3426,12 +3426,11 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align, (s->flags & ~SLAB_PANIC), s->ctor, s); - if (new) new->allocflags |= __GFP_KMEMCG; else new = s; - +out: mutex_unlock(&mutex); return new; } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 16:01 [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path Vladimir Davydov @ 2014-01-30 21:01 ` Andrew Morton 2014-01-30 21:14 ` David Rientjes 2014-01-30 21:09 ` David Rientjes 1 sibling, 1 reply; 16+ messages in thread From: Andrew Morton @ 2014-01-30 21:01 UTC (permalink / raw) To: Vladimir Davydov; +Cc: mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014 20:01:33 +0400 Vladimir Davydov <vdavydov@parallels.com> wrote: > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -3400,7 +3400,7 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep) > static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > struct kmem_cache *s) > { > - struct kmem_cache *new; > + struct kmem_cache *new = NULL; > static char *tmp_name = NULL; > static DEFINE_MUTEX(mutex); /* protects tmp_name */ > > @@ -3416,7 +3416,7 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > if (!tmp_name) { > tmp_name = kmalloc(PATH_MAX, GFP_KERNEL); > if (!tmp_name) > - return NULL; > + goto out; > } > > rcu_read_lock(); > @@ -3426,12 +3426,11 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > > new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align, > (s->flags & ~SLAB_PANIC), s->ctor, s); > - > if (new) > new->allocflags |= __GFP_KMEMCG; > else > new = s; > - > +out: > mutex_unlock(&mutex); > return new; > } Well gee, how did that one get through? What was the point in permanently allocating tmp_name, btw? "This static temporary buffer is used to prevent from pointless shortliving allocation"? That's daft - memcg_create_kmem_cache() is not a fastpath and there are a million places in the kernel where we could permanently leak memory because it is "pointless" to allocate on demand. The allocation of PATH_MAX bytes is unfortunate - kasprintf() wouild work well here, but cgroup_name()'s need for rcu_read_lock() screws us up. So how about doing this? From: Andrew Morton <akpm@linux-foundation.org> Subject: mm/memcontrol.c: memcg_create_kmem_cache() tweaks Allocate tmp_name on demand rather than permanently consuming PATH_MAX bytes of memory. This permits a small reduction in the mutex hold time as well. Cc: Glauber Costa <glommer@parallels.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Michal Hocko <mhocko@suse.cz> Cc: Vladimir Davydov <vdavydov@parallels.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- mm/memcontrol.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff -puN mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks mm/memcontrol.c --- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks +++ a/mm/memcontrol.c @@ -3401,17 +3401,14 @@ static struct kmem_cache *memcg_create_k struct kmem_cache *s) { struct kmem_cache *new = NULL; - static char *tmp_name = NULL; + static char *tmp_name; static DEFINE_MUTEX(mutex); /* protects tmp_name */ BUG_ON(!memcg_can_account_kmem(memcg)); - mutex_lock(&mutex); /* - * kmem_cache_create_memcg duplicates the given name and - * cgroup_name for this name requires RCU context. - * This static temporary buffer is used to prevent from - * pointless shortliving allocation. + * kmem_cache_create_memcg duplicates the given name and cgroup_name() + * for this name requires rcu_read_lock(). */ if (!tmp_name) { tmp_name = kmalloc(PATH_MAX, GFP_KERNEL); @@ -3419,6 +3416,7 @@ static struct kmem_cache *memcg_create_k goto out; } + mutex_lock(&mutex); rcu_read_lock(); snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name, memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup)); @@ -3432,6 +3430,7 @@ static struct kmem_cache *memcg_create_k new = s; out: mutex_unlock(&mutex); + kfree(tmp_name); return new; } _ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 21:01 ` Andrew Morton @ 2014-01-30 21:14 ` David Rientjes 2014-01-30 21:29 ` Andrew Morton 2014-01-30 21:36 ` David Rientjes 0 siblings, 2 replies; 16+ messages in thread From: David Rientjes @ 2014-01-30 21:14 UTC (permalink / raw) To: Andrew Morton; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014, Andrew Morton wrote: > Well gee, how did that one get through? > > What was the point in permanently allocating tmp_name, btw? "This > static temporary buffer is used to prevent from pointless shortliving > allocation"? That's daft - memcg_create_kmem_cache() is not a fastpath > and there are a million places in the kernel where we could permanently > leak memory because it is "pointless" to allocate on demand. > > The allocation of PATH_MAX bytes is unfortunate - kasprintf() wouild > work well here, but cgroup_name()'s need for rcu_read_lock() screws us > up. > What's funnier is that tmp_name isn't required at all since kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so you could easily just pass in the pointer to memory that has been allocated for s->name rather than allocating memory twice. > So how about doing this? > > From: Andrew Morton <akpm@linux-foundation.org> > Subject: mm/memcontrol.c: memcg_create_kmem_cache() tweaks > > Allocate tmp_name on demand rather than permanently consuming PATH_MAX > bytes of memory. This permits a small reduction in the mutex hold time as > well. > > Cc: Glauber Costa <glommer@parallels.com> > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: Michal Hocko <mhocko@suse.cz> > Cc: Vladimir Davydov <vdavydov@parallels.com> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > --- > > mm/memcontrol.c | 11 +++++------ > 1 file changed, 5 insertions(+), 6 deletions(-) > > diff -puN mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks mm/memcontrol.c > --- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks > +++ a/mm/memcontrol.c > @@ -3401,17 +3401,14 @@ static struct kmem_cache *memcg_create_k > struct kmem_cache *s) > { > struct kmem_cache *new = NULL; > - static char *tmp_name = NULL; > + static char *tmp_name; You're keeping it static and the mutex so you're still keeping it global, ok... > static DEFINE_MUTEX(mutex); /* protects tmp_name */ > > BUG_ON(!memcg_can_account_kmem(memcg)); > > - mutex_lock(&mutex); > /* > - * kmem_cache_create_memcg duplicates the given name and > - * cgroup_name for this name requires RCU context. > - * This static temporary buffer is used to prevent from > - * pointless shortliving allocation. > + * kmem_cache_create_memcg duplicates the given name and cgroup_name() > + * for this name requires rcu_read_lock(). > */ > if (!tmp_name) { > tmp_name = kmalloc(PATH_MAX, GFP_KERNEL); Eek, memory leak. Two concurrent calls to memcg_create_kmem_cache() find !tmp_name and do the kmalloc() concurrently. > @@ -3419,6 +3416,7 @@ static struct kmem_cache *memcg_create_k > goto out; > } > > + mutex_lock(&mutex); > rcu_read_lock(); > snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name, > memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup)); > @@ -3432,6 +3430,7 @@ static struct kmem_cache *memcg_create_k > new = s; > out: > mutex_unlock(&mutex); > + kfree(tmp_name); Why would we free the global buffer? > return new; > } > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 21:14 ` David Rientjes @ 2014-01-30 21:29 ` Andrew Morton 2014-01-30 21:38 ` David Rientjes 2014-01-30 21:36 ` David Rientjes 1 sibling, 1 reply; 16+ messages in thread From: Andrew Morton @ 2014-01-30 21:29 UTC (permalink / raw) To: David Rientjes; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014 13:14:46 -0800 (PST) David Rientjes <rientjes@google.com> wrote: > On Thu, 30 Jan 2014, Andrew Morton wrote: > > > Well gee, how did that one get through? > > > > What was the point in permanently allocating tmp_name, btw? "This > > static temporary buffer is used to prevent from pointless shortliving > > allocation"? That's daft - memcg_create_kmem_cache() is not a fastpath > > and there are a million places in the kernel where we could permanently > > leak memory because it is "pointless" to allocate on demand. > > > > The allocation of PATH_MAX bytes is unfortunate - kasprintf() wouild > > work well here, but cgroup_name()'s need for rcu_read_lock() screws us > > up. > > > > What's funnier is that tmp_name isn't required at all since > kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so > you could easily just pass in the pointer to memory that has been > allocated for s->name rather than allocating memory twice. We need a buffer to sprintf() into. > > --- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks > > +++ a/mm/memcontrol.c > > @@ -3401,17 +3401,14 @@ static struct kmem_cache *memcg_create_k > > struct kmem_cache *s) > > { > > struct kmem_cache *new = NULL; > > - static char *tmp_name = NULL; > > + static char *tmp_name; > > You're keeping it static and the mutex so you're still keeping it global, > ok... oop, I forgot to remove the `static'. And I suppose the mutex now doesn't do anything, so... From: Andrew Morton <akpm@linux-foundation.org> Subject: mm/memcontrol.c: memcg_create_kmem_cache() tweaks Allocate tmp_name on demand rather than permanently consuming PATH_MAX bytes of memory. Remove the mutex which protected the static tmp_name. Cc: Glauber Costa <glommer@parallels.com> Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Michal Hocko <mhocko@suse.cz> Cc: Vladimir Davydov <vdavydov@parallels.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> --- mm/memcontrol.c | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff -puN mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks mm/memcontrol.c --- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks +++ a/mm/memcontrol.c @@ -3400,24 +3400,18 @@ void mem_cgroup_destroy_cache(struct kme static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, struct kmem_cache *s) { - struct kmem_cache *new = NULL; - static char *tmp_name = NULL; - static DEFINE_MUTEX(mutex); /* protects tmp_name */ + struct kmem_cache *new; + char *tmp_name; BUG_ON(!memcg_can_account_kmem(memcg)); - mutex_lock(&mutex); /* - * kmem_cache_create_memcg duplicates the given name and - * cgroup_name for this name requires RCU context. - * This static temporary buffer is used to prevent from - * pointless shortliving allocation. + * kmem_cache_create_memcg duplicates the given name and cgroup_name() + * for this name requires rcu_read_lock(). */ - if (!tmp_name) { - tmp_name = kmalloc(PATH_MAX, GFP_KERNEL); - if (!tmp_name) - goto out; - } + tmp_name = kmalloc(PATH_MAX, GFP_KERNEL); + if (!tmp_name) + return NULL; rcu_read_lock(); snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name, @@ -3430,8 +3424,7 @@ static struct kmem_cache *memcg_create_k new->allocflags |= __GFP_KMEMCG; else new = s; -out: - mutex_unlock(&mutex); + kfree(tmp_name); return new; } _ ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 21:29 ` Andrew Morton @ 2014-01-30 21:38 ` David Rientjes 2014-01-30 21:50 ` Andrew Morton 0 siblings, 1 reply; 16+ messages in thread From: David Rientjes @ 2014-01-30 21:38 UTC (permalink / raw) To: Andrew Morton; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014, Andrew Morton wrote: > > What's funnier is that tmp_name isn't required at all since > > kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so > > you could easily just pass in the pointer to memory that has been > > allocated for s->name rather than allocating memory twice. > > We need a buffer to sprintf() into. > Yeah, it shouldn't be temporary it should be the one and only allocation. We should construct the name in memcg_create_kmem_cache() and be done with it. > diff -puN mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks mm/memcontrol.c > --- a/mm/memcontrol.c~mm-memcontrolc-memcg_create_kmem_cache-tweaks > +++ a/mm/memcontrol.c > @@ -3400,24 +3400,18 @@ void mem_cgroup_destroy_cache(struct kme > static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > struct kmem_cache *s) > { > - struct kmem_cache *new = NULL; > - static char *tmp_name = NULL; > - static DEFINE_MUTEX(mutex); /* protects tmp_name */ > + struct kmem_cache *new; > + char *tmp_name; > > BUG_ON(!memcg_can_account_kmem(memcg)); > > - mutex_lock(&mutex); > /* > - * kmem_cache_create_memcg duplicates the given name and > - * cgroup_name for this name requires RCU context. > - * This static temporary buffer is used to prevent from > - * pointless shortliving allocation. > + * kmem_cache_create_memcg duplicates the given name and cgroup_name() > + * for this name requires rcu_read_lock(). > */ > - if (!tmp_name) { > - tmp_name = kmalloc(PATH_MAX, GFP_KERNEL); > - if (!tmp_name) > - goto out; > - } > + tmp_name = kmalloc(PATH_MAX, GFP_KERNEL); > + if (!tmp_name) > + return NULL; > > rcu_read_lock(); > snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name, > @@ -3430,8 +3424,7 @@ static struct kmem_cache *memcg_create_k > new->allocflags |= __GFP_KMEMCG; > else > new = s; > -out: > - mutex_unlock(&mutex); > + kfree(tmp_name); > return new; > } > This is fine, but kmem_cache_create_memcg() is still just going to do a pointless kstrdup() on it which isn't necessary. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 21:38 ` David Rientjes @ 2014-01-30 21:50 ` Andrew Morton 2014-01-30 22:04 ` David Rientjes 0 siblings, 1 reply; 16+ messages in thread From: Andrew Morton @ 2014-01-30 21:50 UTC (permalink / raw) To: David Rientjes; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014 13:38:56 -0800 (PST) David Rientjes <rientjes@google.com> wrote: > > > What's funnier is that tmp_name isn't required at all since > > > kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so > > > you could easily just pass in the pointer to memory that has been > > > allocated for s->name rather than allocating memory twice. > > > > We need a buffer to sprintf() into. > > > > Yeah, it shouldn't be temporary it should be the one and only allocation. > We should construct the name in memcg_create_kmem_cache() and be done with > it. Could. That would require converting memcg_create_kmem_cache() to take a va_list and call kasprintf() on it. The problem is that pesky rcu_read_lock() which is required around cgroup_name() - we'd have to call memcg_create_kmem_cache() under rcu_read_lock() so the usual GFP_foo limitations apply. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 21:50 ` Andrew Morton @ 2014-01-30 22:04 ` David Rientjes 2014-01-30 22:09 ` Andrew Morton 0 siblings, 1 reply; 16+ messages in thread From: David Rientjes @ 2014-01-30 22:04 UTC (permalink / raw) To: Andrew Morton; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014, Andrew Morton wrote: > > Yeah, it shouldn't be temporary it should be the one and only allocation. > > We should construct the name in memcg_create_kmem_cache() and be done with > > it. > > Could. That would require converting memcg_create_kmem_cache() to take > a va_list and call kasprintf() on it. > Why? We already construct the name in memcg_create_kmem_cache() appropriately, we just want to avoid the kstrdup() in kmem_cache_create_memcg() since it's pointless like my patch does. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 22:04 ` David Rientjes @ 2014-01-30 22:09 ` Andrew Morton 2014-01-30 22:13 ` David Rientjes 0 siblings, 1 reply; 16+ messages in thread From: Andrew Morton @ 2014-01-30 22:09 UTC (permalink / raw) To: David Rientjes; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014 14:04:12 -0800 (PST) David Rientjes <rientjes@google.com> wrote: > On Thu, 30 Jan 2014, Andrew Morton wrote: > > > > Yeah, it shouldn't be temporary it should be the one and only allocation. > > > We should construct the name in memcg_create_kmem_cache() and be done with > > > it. > > > > Could. That would require converting memcg_create_kmem_cache() to take > > a va_list and call kasprintf() on it. > > > > Why? We already construct the name in memcg_create_kmem_cache() > appropriately, we just want to avoid the kstrdup() in > kmem_cache_create_memcg() since it's pointless like my patch does. oh, OK, missed that. The problem now is that the string at kmem_cache.name is PATH_MAX bytes, and PATH_MAX is huuuuuuuge. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 22:09 ` Andrew Morton @ 2014-01-30 22:13 ` David Rientjes 2014-01-30 22:15 ` Andrew Morton 0 siblings, 1 reply; 16+ messages in thread From: David Rientjes @ 2014-01-30 22:13 UTC (permalink / raw) To: Andrew Morton; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014, Andrew Morton wrote: > > Why? We already construct the name in memcg_create_kmem_cache() > > appropriately, we just want to avoid the kstrdup() in > > kmem_cache_create_memcg() since it's pointless like my patch does. > > oh, OK, missed that. > > The problem now is that the string at kmem_cache.name is PATH_MAX > bytes, and PATH_MAX is huuuuuuuge. > It always was. Google uses pretty long memcg names (although admittedly not as long as PATH_MAX!) and it hasn't caused any problem with /proc/slabinfo or slabtop(1). ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 22:13 ` David Rientjes @ 2014-01-30 22:15 ` Andrew Morton 2014-01-30 22:39 ` David Rientjes 0 siblings, 1 reply; 16+ messages in thread From: Andrew Morton @ 2014-01-30 22:15 UTC (permalink / raw) To: David Rientjes; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014 14:13:18 -0800 (PST) David Rientjes <rientjes@google.com> wrote: > On Thu, 30 Jan 2014, Andrew Morton wrote: > > > > Why? We already construct the name in memcg_create_kmem_cache() > > > appropriately, we just want to avoid the kstrdup() in > > > kmem_cache_create_memcg() since it's pointless like my patch does. > > > > oh, OK, missed that. > > > > The problem now is that the string at kmem_cache.name is PATH_MAX > > bytes, and PATH_MAX is huuuuuuuge. > > > > It always was. eh? kmem_cache_create_memcg()'s kstrdup() will allocate the minimum needed amount of memory. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 22:15 ` Andrew Morton @ 2014-01-30 22:39 ` David Rientjes 2014-01-31 6:53 ` Vladimir Davydov 0 siblings, 1 reply; 16+ messages in thread From: David Rientjes @ 2014-01-30 22:39 UTC (permalink / raw) To: Andrew Morton; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014, Andrew Morton wrote: > > It always was. > > eh? kmem_cache_create_memcg()'s kstrdup() will allocate the minimum > needed amount of memory. > Ah, good point. We could this incrementally on my patch: diff --git a/mm/memcontrol.c b/mm/memcontrol.c --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -637,6 +637,9 @@ int memcg_limited_groups_array_size; * better kept as an internal representation in cgroup.c. In any case, the * cgrp_id space is not getting any smaller, and we don't have to necessarily * increase ours as well if it increases. + * + * Updates to MAX_SIZE should update the space for the memcg name in + * memcg_create_kmem_cache(). */ #define MEMCG_CACHES_MIN_SIZE 4 #define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX @@ -3400,8 +3403,10 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep) static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, struct kmem_cache *s) { - char *name = NULL; struct kmem_cache *new; + const char *cgrp_name; + char *name = NULL; + size_t len; BUG_ON(!memcg_can_account_kmem(memcg)); @@ -3409,9 +3414,22 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, if (unlikely(!name)) return NULL; + /* + * Format of a memcg's kmem cache name: + * <cache-name>(<memcg-id>:<cgroup-name>) + */ + len = strlen(s->name); + /* Space for parentheses, colon, terminator */ + len += 4; + /* MEMCG_CACHES_MAX_SIZE is USHRT_MAX */ + len += 5; + BUILD_BUG_ON(MEMCG_CACHES_MAX_SIZE > USHRT_MAX); + rcu_read_lock(); - snprintf(name, PATH_MAX, "%s(%d:%s)", s->name, memcg_cache_id(memcg), - cgroup_name(memcg->css.cgroup)); + cgrp_name = cgroup_name(memcg->css.cgroup); + len += strlen(cgrp_name); + snprintf(name, len, "%s(%d:%s)", s->name, memcg_cache_id(memcg), + cgrp_name); rcu_read_unlock(); new = kmem_cache_create_memcg(memcg, name, s->object_size, s->align, ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 22:39 ` David Rientjes @ 2014-01-31 6:53 ` Vladimir Davydov 2014-01-31 10:42 ` David Rientjes 0 siblings, 1 reply; 16+ messages in thread From: Vladimir Davydov @ 2014-01-31 6:53 UTC (permalink / raw) To: David Rientjes; +Cc: Andrew Morton, mhocko, linux-mm, linux-kernel Hi, David Thank you for taking look at this and adding the missing patch description. WRT your patch, please see the comment inline. On 01/31/2014 02:39 AM, David Rientjes wrote: > On Thu, 30 Jan 2014, Andrew Morton wrote: > >>> It always was. >> eh? kmem_cache_create_memcg()'s kstrdup() will allocate the minimum >> needed amount of memory. >> > Ah, good point. We could this incrementally on my patch: > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -637,6 +637,9 @@ int memcg_limited_groups_array_size; > * better kept as an internal representation in cgroup.c. In any case, the > * cgrp_id space is not getting any smaller, and we don't have to necessarily > * increase ours as well if it increases. > + * > + * Updates to MAX_SIZE should update the space for the memcg name in > + * memcg_create_kmem_cache(). > */ > #define MEMCG_CACHES_MIN_SIZE 4 > #define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX > @@ -3400,8 +3403,10 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep) > static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > struct kmem_cache *s) > { > - char *name = NULL; > struct kmem_cache *new; > + const char *cgrp_name; > + char *name = NULL; > + size_t len; > > BUG_ON(!memcg_can_account_kmem(memcg)); > > @@ -3409,9 +3414,22 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > if (unlikely(!name)) > return NULL; > > + /* > + * Format of a memcg's kmem cache name: > + * <cache-name>(<memcg-id>:<cgroup-name>) > + */ > + len = strlen(s->name); > + /* Space for parentheses, colon, terminator */ > + len += 4; > + /* MEMCG_CACHES_MAX_SIZE is USHRT_MAX */ > + len += 5; > + BUILD_BUG_ON(MEMCG_CACHES_MAX_SIZE > USHRT_MAX); > + This looks cumbersome, IMO. Let's leave it as is for now. AFAIK, cgroup_name() will be reworked soon so that it won't require RCU-context (https://lkml.org/lkml/2014/1/28/530). Therefore, it will be possible to get rid of this pointless tmp_name allocation by making kmem_cache_create_memcg() take not just name, but printf-like format + vargs. Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-31 6:53 ` Vladimir Davydov @ 2014-01-31 10:42 ` David Rientjes 2014-01-31 11:29 ` Vladimir Davydov 0 siblings, 1 reply; 16+ messages in thread From: David Rientjes @ 2014-01-31 10:42 UTC (permalink / raw) To: Vladimir Davydov; +Cc: Andrew Morton, mhocko, linux-mm, linux-kernel On Fri, 31 Jan 2014, Vladimir Davydov wrote: > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > > --- a/mm/memcontrol.c > > +++ b/mm/memcontrol.c > > @@ -637,6 +637,9 @@ int memcg_limited_groups_array_size; > > * better kept as an internal representation in cgroup.c. In any case, the > > * cgrp_id space is not getting any smaller, and we don't have to necessarily > > * increase ours as well if it increases. > > + * > > + * Updates to MAX_SIZE should update the space for the memcg name in > > + * memcg_create_kmem_cache(). > > */ > > #define MEMCG_CACHES_MIN_SIZE 4 > > #define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX > > @@ -3400,8 +3403,10 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep) > > static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > > struct kmem_cache *s) > > { > > - char *name = NULL; > > struct kmem_cache *new; > > + const char *cgrp_name; > > + char *name = NULL; > > + size_t len; > > > > BUG_ON(!memcg_can_account_kmem(memcg)); > > > > @@ -3409,9 +3414,22 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, > > if (unlikely(!name)) > > return NULL; > > > > + /* > > + * Format of a memcg's kmem cache name: > > + * <cache-name>(<memcg-id>:<cgroup-name>) > > + */ > > + len = strlen(s->name); > > + /* Space for parentheses, colon, terminator */ > > + len += 4; > > + /* MEMCG_CACHES_MAX_SIZE is USHRT_MAX */ > > + len += 5; > > + BUILD_BUG_ON(MEMCG_CACHES_MAX_SIZE > USHRT_MAX); > > + > > This looks cumbersome, IMO. Let's leave it as is for now. AFAIK, > cgroup_name() will be reworked soon so that it won't require RCU-context > (https://lkml.org/lkml/2014/1/28/530). Therefore, it will be possible to > get rid of this pointless tmp_name allocation by making > kmem_cache_create_memcg() take not just name, but printf-like format + > vargs. > You believe it's less cumbersome to do two memory allocations to figure out how much memory you really need to allocate rather than just calculating the necessary size? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-31 10:42 ` David Rientjes @ 2014-01-31 11:29 ` Vladimir Davydov 0 siblings, 0 replies; 16+ messages in thread From: Vladimir Davydov @ 2014-01-31 11:29 UTC (permalink / raw) To: David Rientjes; +Cc: Andrew Morton, mhocko, linux-mm, linux-kernel On 01/31/2014 02:42 PM, David Rientjes wrote: > On Fri, 31 Jan 2014, Vladimir Davydov wrote: > >>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c >>> --- a/mm/memcontrol.c >>> +++ b/mm/memcontrol.c >>> @@ -637,6 +637,9 @@ int memcg_limited_groups_array_size; >>> * better kept as an internal representation in cgroup.c. In any case, the >>> * cgrp_id space is not getting any smaller, and we don't have to necessarily >>> * increase ours as well if it increases. >>> + * >>> + * Updates to MAX_SIZE should update the space for the memcg name in >>> + * memcg_create_kmem_cache(). >>> */ >>> #define MEMCG_CACHES_MIN_SIZE 4 >>> #define MEMCG_CACHES_MAX_SIZE MEM_CGROUP_ID_MAX >>> @@ -3400,8 +3403,10 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep) >>> static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, >>> struct kmem_cache *s) >>> { >>> - char *name = NULL; >>> struct kmem_cache *new; >>> + const char *cgrp_name; >>> + char *name = NULL; >>> + size_t len; >>> >>> BUG_ON(!memcg_can_account_kmem(memcg)); >>> >>> @@ -3409,9 +3414,22 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, >>> if (unlikely(!name)) >>> return NULL; >>> >>> + /* >>> + * Format of a memcg's kmem cache name: >>> + * <cache-name>(<memcg-id>:<cgroup-name>) >>> + */ >>> + len = strlen(s->name); >>> + /* Space for parentheses, colon, terminator */ >>> + len += 4; >>> + /* MEMCG_CACHES_MAX_SIZE is USHRT_MAX */ >>> + len += 5; >>> + BUILD_BUG_ON(MEMCG_CACHES_MAX_SIZE > USHRT_MAX); >>> + >> This looks cumbersome, IMO. Let's leave it as is for now. AFAIK, >> cgroup_name() will be reworked soon so that it won't require RCU-context >> (https://lkml.org/lkml/2014/1/28/530). Therefore, it will be possible to >> get rid of this pointless tmp_name allocation by making >> kmem_cache_create_memcg() take not just name, but printf-like format + >> vargs. >> > You believe it's less cumbersome to do two memory allocations to figure > out how much memory you really need to allocate rather than just > calculating the necessary size? Well, I mean not the approach - here everything is right - but how it looks. This len += 4 len += 5 looks scary even with comments, IMHO. Note, I do not stand for this temporary buffer - it was introduced long before I started tweaking this code. I just want to say that substituting it now with something (OK, less, but IMHO still) cumbersome is not a good idea provided soon it will be possible to remove tmp_name while still having the code looking nice. If you insist, I don't mind, but... why? Thanks. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 21:14 ` David Rientjes 2014-01-30 21:29 ` Andrew Morton @ 2014-01-30 21:36 ` David Rientjes 1 sibling, 0 replies; 16+ messages in thread From: David Rientjes @ 2014-01-30 21:36 UTC (permalink / raw) To: Andrew Morton; +Cc: Vladimir Davydov, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014, David Rientjes wrote: > What's funnier is that tmp_name isn't required at all since > kmem_cache_create_memcg() is just going to do a kstrdup() on it anyway, so > you could easily just pass in the pointer to memory that has been > allocated for s->name rather than allocating memory twice. > Something like this untested patch? mm, memcg: only allocate memory for kmem slab cache name once We must allocate memory to store a slab cache name when using kmem accounting since it involves the name of the memcg itself. This should be the one and only memory allocation for that name, though. Currently, we keep around a global buffer to construct the "memcg slab cache name" since it requires rcu protection and then pass it into kmem_cache_create_memcg() which does its own kstrdup(). This patch only allocates and creates the slab cache name once and then passes a pointer into kmem_cache_create_memcg() as the name. Signed-off-by: David Rientjes <rientjes@google.com> --- mm/memcontrol.c | 25 +++++++------------------ mm/slab_common.c | 11 +++++------ 2 files changed, 12 insertions(+), 24 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -3400,31 +3400,21 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep) static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, struct kmem_cache *s) { + char *name = NULL; struct kmem_cache *new; - static char *tmp_name = NULL; - static DEFINE_MUTEX(mutex); /* protects tmp_name */ BUG_ON(!memcg_can_account_kmem(memcg)); - mutex_lock(&mutex); - /* - * kmem_cache_create_memcg duplicates the given name and - * cgroup_name for this name requires RCU context. - * This static temporary buffer is used to prevent from - * pointless shortliving allocation. - */ - if (!tmp_name) { - tmp_name = kmalloc(PATH_MAX, GFP_KERNEL); - if (!tmp_name) - return NULL; - } + name = kmalloc(PATH_MAX, GFP_KERNEL); + if (unlikely(!name)) + return NULL; rcu_read_lock(); - snprintf(tmp_name, PATH_MAX, "%s(%d:%s)", s->name, - memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup)); + snprintf(name, PATH_MAX, "%s(%d:%s)", s->name, memcg_cache_id(memcg), + cgroup_name(memcg->css.cgroup)); rcu_read_unlock(); - new = kmem_cache_create_memcg(memcg, tmp_name, s->object_size, s->align, + new = kmem_cache_create_memcg(memcg, name, s->object_size, s->align, (s->flags & ~SLAB_PANIC), s->ctor, s); if (new) @@ -3432,7 +3422,6 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg, else new = s; - mutex_unlock(&mutex); return new; } diff --git a/mm/slab_common.c b/mm/slab_common.c --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -142,7 +142,7 @@ unsigned long calculate_alignment(unsigned long flags, /* * kmem_cache_create - Create a cache. - * @name: A string which is used in /proc/slabinfo to identify this cache. + * @name: A string allocated for this cache used in /proc/slabinfo. * @size: The size of objects to be created in this cache. * @align: The required alignment for the objects. * @flags: SLAB flags @@ -212,10 +212,7 @@ kmem_cache_create_memcg(struct mem_cgroup *memcg, const char *name, size_t size, s->object_size = s->size = size; s->align = calculate_alignment(flags, align, size); s->ctor = ctor; - - s->name = kstrdup(name, GFP_KERNEL); - if (!s->name) - goto out_free_cache; + s->name = name; err = memcg_alloc_cache_params(memcg, s, parent_cache); if (err) @@ -258,7 +255,6 @@ out_unlock: out_free_cache: memcg_free_cache_params(s); - kfree(s->name); kmem_cache_free(kmem_cache, s); goto out_unlock; } @@ -267,6 +263,9 @@ struct kmem_cache * kmem_cache_create(const char *name, size_t size, size_t align, unsigned long flags, void (*ctor)(void *)) { + const char *cache_name = kstrdup(name, GFP_KERNEL); + if (unlikely(!cache_name)) + return NULL; return kmem_cache_create_memcg(NULL, name, size, align, flags, ctor, NULL); } EXPORT_SYMBOL(kmem_cache_create); ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path 2014-01-30 16:01 [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path Vladimir Davydov 2014-01-30 21:01 ` Andrew Morton @ 2014-01-30 21:09 ` David Rientjes 1 sibling, 0 replies; 16+ messages in thread From: David Rientjes @ 2014-01-30 21:09 UTC (permalink / raw) To: Vladimir Davydov; +Cc: akpm, mhocko, linux-mm, linux-kernel On Thu, 30 Jan 2014, Vladimir Davydov wrote: > Signed-off-by: Vladimir Davydov <vdavydov@parallels.com> Some changelog would be helpful since this fixes an issue already in Linus's tree. Commit 842e2873697e ("memcg: get rid of kmem_cache_dup()") introduced a mutex for memcg_create_kmem_cache() to protect the tmp_name buffer that holds the memcg name. It failed to unlock the mutex if this buffer could not be allocated. This patch fixes the issue by appropriately unlocking the mutex if the allocation fails. Acked-by: David Rientjes <rientjes@google.com> That said, this tmp_name stuff seems totally unnecessary. kmem_cache_create_memcg() already does the kstrdup() so why not just pass in a pointer to already allocated memory for s->name rather than having this mutex or global buffer at all? ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-01-31 11:29 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-01-30 16:01 [PATCH] memcg: fix mutex not unlocked on memcg_create_kmem_cache fail path Vladimir Davydov 2014-01-30 21:01 ` Andrew Morton 2014-01-30 21:14 ` David Rientjes 2014-01-30 21:29 ` Andrew Morton 2014-01-30 21:38 ` David Rientjes 2014-01-30 21:50 ` Andrew Morton 2014-01-30 22:04 ` David Rientjes 2014-01-30 22:09 ` Andrew Morton 2014-01-30 22:13 ` David Rientjes 2014-01-30 22:15 ` Andrew Morton 2014-01-30 22:39 ` David Rientjes 2014-01-31 6:53 ` Vladimir Davydov 2014-01-31 10:42 ` David Rientjes 2014-01-31 11:29 ` Vladimir Davydov 2014-01-30 21:36 ` David Rientjes 2014-01-30 21:09 ` David Rientjes
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).