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.
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>