On 02/04/2014 02:08 AM, Andrew Morton wrote:
> On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov <vdavydov@xxxxxxxxxxxxx> wrote:
>
>> The way memcg_create_kmem_cache() creates the name for a memcg cache
>> looks rather strange: it first formats the name in the static buffer
>> tmp_name protected by a mutex, then passes the pointer to the buffer to
>> kmem_cache_create_memcg(), which finally duplicates it to the cache
>> name.
>>
>> Let's clean this up by moving memcg cache name creation to a separate
>> function to be called by kmem_cache_create_memcg(), and estimating the
>> length of the name string before copying anything to it so that we won't
>> need a temporary buffer.
>>
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -3193,6 +3193,37 @@ int memcg_update_cache_size(struct kmem_cache *s, int num_groups)
>> return 0;
>> }
>>
>> +static int memcg_print_cache_name(char *buf, size_t size,
>> + struct mem_cgroup *memcg, struct kmem_cache *root_cache)
>> +{
>> + int ret;
>> +
>> + rcu_read_lock();
>> + ret = snprintf(buf, size, "%s(%d:%s)", root_cache->name,
>> + memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
>> + rcu_read_unlock();
>> + return ret;
>> +}
>> +
>> +char *memcg_create_cache_name(struct mem_cgroup *memcg,
>> + struct kmem_cache *root_cache)
>> +{
>> + int len;
>> + char *name;
>> +
>> + /*
>> + * We cannot use kasprintf() here, because cgroup_name() must be called
>> + * under RCU protection.
>> + */
>> + len = memcg_print_cache_name(NULL, 0, memcg, root_cache);
>> +
>> + name = kmalloc(len + 1, GFP_KERNEL);
>> + if (name)
>> + memcg_print_cache_name(name, len + 1, memcg, root_cache);
> but but but this assumes that cgroup_name(memcg->css.cgroup) did not
> change between the two calls to memcg_print_cache_name(). If that is
> the case then the locking was unneeded anyway.
Oops, I missed that. Thank you for pointing me out. It seems the usage
of the temporary buffer is inevitable. However, a dedicated mutex
protecting it can be removed, because we already hold the slab_mutex
while calling this function. Will rework.
Thanks.
>
>> + return name;
>> +}
>> +
>> int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
>> struct kmem_cache *root_cache)
>> {
>> @@ -3397,44 +3428,6 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
>> schedule_work(&cachep->memcg_params->destroy);
>> }
>>
--
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>