Re: [PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

2014-02-03 Thread Vladimir Davydov 
On 02/04/2014 02:08 AM, Andrew Morton wrote:
> On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov  
> 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(>memcg_params->destroy);
>>  }
>>  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

2014-02-03 Thread Andrew Morton 
On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov  
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.

> + 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(>memcg_params->destroy);
>  }
>  

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

2014-02-03 Thread Vladimir Davydov 
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.

Signed-off-by: Vladimir Davydov 
---
 include/linux/memcontrol.h |9 +
 mm/memcontrol.c|   94 ++--
 mm/slab_common.c   |5 ++-
 3 files changed, 59 insertions(+), 49 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index abd0113b6620..84e4801fc36c 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -497,6 +497,9 @@ void __memcg_kmem_commit_charge(struct page *page,
 void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
+
+char *memcg_create_cache_name(struct mem_cgroup *memcg,
+ struct kmem_cache *root_cache);
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 struct kmem_cache *root_cache);
 void memcg_free_cache_params(struct kmem_cache *s);
@@ -641,6 +644,12 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
return -1;
 }
 
+static inline char *memcg_create_cache_name(struct mem_cgroup *memcg,
+   struct kmem_cache *root_cache)
+{
+   return NULL;
+}
+
 static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
struct kmem_cache *s, struct kmem_cache *root_cache)
 {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 53385cd4e6f0..9e321650efb2 100644
--- 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);
+
+   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(>memcg_params->destroy);
 }
 
-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 */
-
-   BUG_ON(!memcg_can_account_kmem(memcg));
-
-   mutex_lock();
-   /*
-* 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)
-   goto out;
-   }
-
-   rcu_read_lock();
-   snprintf(tmp_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,
- (s->flags & ~SLAB_PANIC), s->ctor, s);
-   if (new)
-   new->allocflags |= __GFP_KMEMCG;
-   else
-   new = s;
-out:
-   mutex_unlock();
-   return new;
-}
-
 void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
struct kmem_cache *c;
@@ -3481,12 +3474,6 @@ void kmem_cache_destroy_memcg_children(struct kmem_cache 
*s)
mutex_unlock(_kmem_mutex);
 }
 
-struct create_work {
-   struct mem_cgroup *memcg;
-   struct kmem_cache *cachep;
-   struct work_struct work;
-};
-
 static void mem_cgroup_destroy_all_caches(struct mem_cgroup

[PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

2014-02-03 Thread Vladimir Davydov 
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.

Signed-off-by: Vladimir Davydov vdavy...@parallels.com
---
 include/linux/memcontrol.h |9 +
 mm/memcontrol.c|   94 ++--
 mm/slab_common.c   |5 ++-
 3 files changed, 59 insertions(+), 49 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index abd0113b6620..84e4801fc36c 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -497,6 +497,9 @@ void __memcg_kmem_commit_charge(struct page *page,
 void __memcg_kmem_uncharge_pages(struct page *page, int order);
 
 int memcg_cache_id(struct mem_cgroup *memcg);
+
+char *memcg_create_cache_name(struct mem_cgroup *memcg,
+ struct kmem_cache *root_cache);
 int memcg_alloc_cache_params(struct mem_cgroup *memcg, struct kmem_cache *s,
 struct kmem_cache *root_cache);
 void memcg_free_cache_params(struct kmem_cache *s);
@@ -641,6 +644,12 @@ static inline int memcg_cache_id(struct mem_cgroup *memcg)
return -1;
 }
 
+static inline char *memcg_create_cache_name(struct mem_cgroup *memcg,
+   struct kmem_cache *root_cache)
+{
+   return NULL;
+}
+
 static inline int memcg_alloc_cache_params(struct mem_cgroup *memcg,
struct kmem_cache *s, struct kmem_cache *root_cache)
 {
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 53385cd4e6f0..9e321650efb2 100644
--- 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);
+
+   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);
 }
 
-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 */
-
-   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)
-   goto out;
-   }
-
-   rcu_read_lock();
-   snprintf(tmp_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,
- (s-flags  ~SLAB_PANIC), s-ctor, s);
-   if (new)
-   new-allocflags |= __GFP_KMEMCG;
-   else
-   new = s;
-out:
-   mutex_unlock(mutex);
-   return new;
-}
-
 void kmem_cache_destroy_memcg_children(struct kmem_cache *s)
 {
struct kmem_cache *c;
@@ -3481,12 +3474,6 @@ void kmem_cache_destroy_memcg_children(struct kmem_cache 
*s)
mutex_unlock(activate_kmem_mutex);
 }
 
-struct create_work {
-   struct mem_cgroup *memcg;
-   struct kmem_cache *cachep;
-   struct work_struct work;
-};
-
 static void

Re: [PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

2014-02-03 Thread Andrew Morton 
On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov vdavy...@parallels.com 
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.

 + 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 from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v2 2/7] memcg, slab: cleanup memcg cache name creation

2014-02-03 Thread Vladimir Davydov 
On 02/04/2014 02:08 AM, Andrew Morton wrote:
 On Mon, 3 Feb 2014 19:54:37 +0400 Vladimir Davydov vdavy...@parallels.com 
 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 from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/