[PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Li Zefan]

As cgroup supports rename, it's unsafe to dereference dentry->d_name
without proper vfs locks. Fix this by using cgroup_name().

Signed-off-by: Li Zefan <lizefan@huawei.com>
---

This patch depends on "cgroup: fix cgroup_path() vs rename() race",
which has been queued for 3.10.

---
 mm/memcontrol.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 53b8201..72be5c9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
 {
 	char *name;
-	struct dentry *dentry;
+
+	name = (char *)__get_free_page(GFP_TEMPORARY);
+	if (!name)
+		return NULL;
 
 	rcu_read_lock();
-	dentry = rcu_dereference(memcg->css.cgroup->dentry);
+	snprintf(name, PAGE_SIZE, "%s(%d:%s)", s->name, memcg_cache_id(memcg),
+		 cgroup_name(memcg->css.cgroup));
 	rcu_read_unlock();
 
-	BUG_ON(dentry == NULL);
-
-	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), dentry->d_name.name);
-
 	return name;
 }
 
@@ -3247,7 +3246,7 @@ static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
 	if (new)
 		new->allocflags |= __GFP_KMEMCG;
 
-	kfree(name);
+	free_page((unsigned long)name);
 	return new;
 }
 
-- 
1.8.0.2

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Thu 21-03-13 09:22:21, Li Zefan wrote:
> As cgroup supports rename, it's unsafe to dereference dentry->d_name
> without proper vfs locks. Fix this by using cgroup_name().
> 
> Signed-off-by: Li Zefan <lizefan@huawei.com>
> ---
> 
> This patch depends on "cgroup: fix cgroup_path() vs rename() race",
> which has been queued for 3.10.
> 
> ---
>  mm/memcontrol.c | 15 +++++++--------
>  1 file changed, 7 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 53b8201..72be5c9 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
>  static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
>  {
>  	char *name;
> -	struct dentry *dentry;
> +
> +	name = (char *)__get_free_page(GFP_TEMPORARY);

Ouch. Can we use a static temporary buffer instead? This is called from
workqueue context so we do not have to be afraid of the deep call chain.
It is also not a hot path AFAICS.

Even GFP_ATOMIC for kasprintf would be an improvement IMO.

> +	if (!name)
> +		return NULL;
>  
>  	rcu_read_lock();
> -	dentry = rcu_dereference(memcg->css.cgroup->dentry);
> +	snprintf(name, PAGE_SIZE, "%s(%d:%s)", s->name, memcg_cache_id(memcg),
> +		 cgroup_name(memcg->css.cgroup));
>  	rcu_read_unlock();
>  
> -	BUG_ON(dentry == NULL);
> -
> -	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
> -			 memcg_cache_id(memcg), dentry->d_name.name);
> -
>  	return name;
>  }
>  
> @@ -3247,7 +3246,7 @@ static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
>  	if (new)
>  		new->allocflags |= __GFP_KMEMCG;
>  
> -	kfree(name);
> +	free_page((unsigned long)name);
>  	return new;
>  }
>  
> -- 
> 1.8.0.2
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Thu 21-03-13 10:08:49, Michal Hocko wrote:
> On Thu 21-03-13 09:22:21, Li Zefan wrote:
> > As cgroup supports rename, it's unsafe to dereference dentry->d_name
> > without proper vfs locks. Fix this by using cgroup_name().
> > 
> > Signed-off-by: Li Zefan <lizefan@huawei.com>
> > ---
> > 
> > This patch depends on "cgroup: fix cgroup_path() vs rename() race",
> > which has been queued for 3.10.
> > 
> > ---
> >  mm/memcontrol.c | 15 +++++++--------
> >  1 file changed, 7 insertions(+), 8 deletions(-)
> > 
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 53b8201..72be5c9 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
> >  static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
> >  {
> >  	char *name;
> > -	struct dentry *dentry;
> > +
> > +	name = (char *)__get_free_page(GFP_TEMPORARY);
> 
> Ouch. Can we use a static temporary buffer instead?

> This is called from workqueue context so we do not have to be afraid
> of the deep call chain.

Bahh, I was thinking about two things at the same time and that is how
it ends... I meant a temporary buffer on the stack. But a separate
allocation sounds even easier.

> It is also not a hot path AFAICS.
> 
> Even GFP_ATOMIC for kasprintf would be an improvement IMO.

What about the following (not even compile tested because I do not have
cgroup_name in my tree yet):
---
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f608546..ede0382 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3370,13 +3370,18 @@ static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
 	struct dentry *dentry;
 
 	rcu_read_lock();
-	dentry = rcu_dereference(memcg->css.cgroup->dentry);
+	name = kasprintf(GFP_ATOMIC, "%s(%d:%s)", s->name,
+			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
 	rcu_read_unlock();
 
-	BUG_ON(dentry == NULL);
-
-	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), dentry->d_name.name);
+	if (!name) {
+		name = kmalloc(PAGE_SIZE, GFP_KERNEL);
+		rcu_read_lock();
+		name = snprintf(name, PAGE_SIZE, "%s(%d:%s)", s->name,
+				memcg_cache_id(memcg),
+				cgroup_name(memcg->css.cgroup));
+		rcu_read_unlock();
+	}
 
 	return name;
 }
-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Li Zefan]

On 2013/3/21 18:22, Michal Hocko wrote:
> On Thu 21-03-13 10:08:49, Michal Hocko wrote:
>> On Thu 21-03-13 09:22:21, Li Zefan wrote:
>>> As cgroup supports rename, it's unsafe to dereference dentry->d_name
>>> without proper vfs locks. Fix this by using cgroup_name().
>>>
>>> Signed-off-by: Li Zefan <lizefan@huawei.com>
>>> ---
>>>
>>> This patch depends on "cgroup: fix cgroup_path() vs rename() race",
>>> which has been queued for 3.10.
>>>
>>> ---
>>>  mm/memcontrol.c | 15 +++++++--------
>>>  1 file changed, 7 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index 53b8201..72be5c9 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
>>>  static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
>>>  {
>>>  	char *name;
>>> -	struct dentry *dentry;
>>> +
>>> +	name = (char *)__get_free_page(GFP_TEMPORARY);
>>
>> Ouch. Can we use a static temporary buffer instead?
> 
>> This is called from workqueue context so we do not have to be afraid
>> of the deep call chain.
> 
> Bahh, I was thinking about two things at the same time and that is how
> it ends... I meant a temporary buffer on the stack. But a separate
> allocation sounds even easier.
> 

Actually I don't care much about which way to take. Use on-stack buffer (if stack
usage is not a concern) or local static buffer (caller already held memcg_cache_mutex)
is simplest.

But why it's bad to allocate a page for temp use?

>> It is also not a hot path AFAICS.
>>
>> Even GFP_ATOMIC for kasprintf would be an improvement IMO.
> 
> What about the following (not even compile tested because I do not have
> cgroup_name in my tree yet):

No, it won't compile. ;)

> ---
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index f608546..ede0382 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3370,13 +3370,18 @@ static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
>  	struct dentry *dentry;
>  
>  	rcu_read_lock();
> -	dentry = rcu_dereference(memcg->css.cgroup->dentry);
> +	name = kasprintf(GFP_ATOMIC, "%s(%d:%s)", s->name,
> +			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
>  	rcu_read_unlock();
>  
> -	BUG_ON(dentry == NULL);
> -
> -	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
> -			 memcg_cache_id(memcg), dentry->d_name.name);
> +	if (!name) {
> +		name = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +		rcu_read_lock();
> +		name = snprintf(name, PAGE_SIZE, "%s(%d:%s)", s->name,
> +				memcg_cache_id(memcg),
> +				cgroup_name(memcg->css.cgroup));
> +		rcu_read_unlock();
> +	}
>  
>  	return name;
>  }
> 

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Fri 22-03-13 09:22:06, Li Zefan wrote:
> On 2013/3/21 18:22, Michal Hocko wrote:
> > On Thu 21-03-13 10:08:49, Michal Hocko wrote:
> >> On Thu 21-03-13 09:22:21, Li Zefan wrote:
> >>> As cgroup supports rename, it's unsafe to dereference dentry->d_name
> >>> without proper vfs locks. Fix this by using cgroup_name().
> >>>
> >>> Signed-off-by: Li Zefan <lizefan@huawei.com>
> >>> ---
> >>>
> >>> This patch depends on "cgroup: fix cgroup_path() vs rename() race",
> >>> which has been queued for 3.10.
> >>>
> >>> ---
> >>>  mm/memcontrol.c | 15 +++++++--------
> >>>  1 file changed, 7 insertions(+), 8 deletions(-)
> >>>
> >>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >>> index 53b8201..72be5c9 100644
> >>> --- a/mm/memcontrol.c
> >>> +++ b/mm/memcontrol.c
> >>> @@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
> >>>  static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
> >>>  {
> >>>  	char *name;
> >>> -	struct dentry *dentry;
> >>> +
> >>> +	name = (char *)__get_free_page(GFP_TEMPORARY);
> >>
> >> Ouch. Can we use a static temporary buffer instead?
> > 
> >> This is called from workqueue context so we do not have to be afraid
> >> of the deep call chain.
> > 
> > Bahh, I was thinking about two things at the same time and that is how
> > it ends... I meant a temporary buffer on the stack. But a separate
> > allocation sounds even easier.
> > 
> 
> Actually I don't care much about which way to take. Use on-stack buffer (if stack
> usage is not a concern) or local static buffer (caller already held memcg_cache_mutex)
> is simplest.
> 
> But why it's bad to allocate a page for temp use?

GFP_TEMPORARY groups short lived allocations but the mem cache is not
an ideal candidate of this type of allocations..

> >> It is also not a hot path AFAICS.
> >>
> >> Even GFP_ATOMIC for kasprintf would be an improvement IMO.
> > 
> > What about the following (not even compile tested because I do not have
> > cgroup_name in my tree yet):
> 
> No, it won't compile. ;)

Somehow expected so as this was just a quick hack to show what I meant.
The full patch is bellow (compile time tested on top of for-3.10 branch
this time :P)
---

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Li Zefan]

>>>>> @@ -3217,17 +3217,16 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
>>>>>  static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
>>>>>  {
>>>>>  	char *name;
>>>>> -	struct dentry *dentry;
>>>>> +
>>>>> +	name = (char *)__get_free_page(GFP_TEMPORARY);
>>>>
>>>> Ouch. Can we use a static temporary buffer instead?
>>>
>>>> This is called from workqueue context so we do not have to be afraid
>>>> of the deep call chain.
>>>
>>> Bahh, I was thinking about two things at the same time and that is how
>>> it ends... I meant a temporary buffer on the stack. But a separate
>>> allocation sounds even easier.
>>>
>>
>> Actually I don't care much about which way to take. Use on-stack buffer (if stack
>> usage is not a concern) or local static buffer (caller already held memcg_cache_mutex)
>> is simplest.
>>
>> But why it's bad to allocate a page for temp use?
> 
> GFP_TEMPORARY groups short lived allocations but the mem cache is not
> an ideal candidate of this type of allocations..
> 

I'm not sure I'm following you...

char *memcg_cache_name()
{
	char *name = alloc();
	return name;
}

kmem_cache_dup()
{
	name = memcg_cache_name();
	kmem_cache_create_memcg(name);
	free(name);
}

Isn't this a short lived allocation?

>>>> It is also not a hot path AFAICS.
>>>>
>>>> Even GFP_ATOMIC for kasprintf would be an improvement IMO.
>>>
>>> What about the following (not even compile tested because I do not have
>>> cgroup_name in my tree yet):
>>
>> No, it won't compile. ;)
> 
> Somehow expected so as this was just a quick hack to show what I meant.
> The full patch is bellow (compile time tested on top of for-3.10 branch
> this time :P)
> ---
>>From 7e1f6f0e266a230ced238a9bf2398b4069a6a764 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Fri, 22 Mar 2013 09:04:58 +0100
> Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()
> 
> As cgroup supports rename, it's unsafe to dereference dentry->d_name
> without proper vfs locks. Fix this by using cgroup_name().
> 
> Signed-off-by: Li Zefan <lizefan@huawei.com>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> ---
>  mm/memcontrol.c |   13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 53b8201..5741bf5 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3220,13 +3220,18 @@ static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
>  	struct dentry *dentry;
>  
>  	rcu_read_lock();
> -	dentry = rcu_dereference(memcg->css.cgroup->dentry);
> +	name = kasprintf(GFP_ATOMIC, "%s(%d:%s)", s->name,
> +			 memcg_cache_id(memcg), dcgroup_name(memcg->css.cgroup));
>  	rcu_read_unlock();
>  
> -	BUG_ON(dentry == NULL);
> +	if (!name) {
> +		name = kmalloc(PAGE_SIZE, GFP_KERNEL);
> +		rcu_read_lock();
> +		name = snprintf(name, PAGE_SIZE, "%s(%d:%s)", s->name,
> +				 memcg_cache_id(memcg), dcgroup_name(memcg->css.cgroup));
> +		rcu_read_unlock();
>  
> -	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
> -			 memcg_cache_id(memcg), dentry->d_name.name);
> +	}
>  
>  	return name;
>  }
> 

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Glauber Costa]

On 03/22/2013 12:17 PM, Li Zefan wrote:
>> GFP_TEMPORARY groups short lived allocations but the mem cache is not
>> > an ideal candidate of this type of allocations..
>> > 
> I'm not sure I'm following you...
> 
> char *memcg_cache_name()
> {
> 	char *name = alloc();
> 	return name;
> }
> 
> kmem_cache_dup()
> {
> 	name = memcg_cache_name();
> 	kmem_cache_create_memcg(name);
> 	free(name);
> }
> 
> Isn't this a short lived allocation?
> 

Hi,

Thanks for identifying and fixing this.

Li is right. The cache name will live long, but this is because the
slab/slub caches will strdup it internally. So the actual memcg
allocation is short lived.

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Fri 22-03-13 12:22:23, Glauber Costa wrote:
> On 03/22/2013 12:17 PM, Li Zefan wrote:
> >> GFP_TEMPORARY groups short lived allocations but the mem cache is not
> >> > an ideal candidate of this type of allocations..
> >> > 
> > I'm not sure I'm following you...
> > 
> > char *memcg_cache_name()
> > {
> > 	char *name = alloc();
> > 	return name;
> > }
> > 
> > kmem_cache_dup()
> > {
> > 	name = memcg_cache_name();
> > 	kmem_cache_create_memcg(name);
> > 	free(name);
> > }
> > 
> > Isn't this a short lived allocation?
> > 
> 
> Hi,
> 
> Thanks for identifying and fixing this.
> 
> Li is right. The cache name will live long, but this is because the
> slab/slub caches will strdup it internally. So the actual memcg
> allocation is short lived.

OK, I have totally missed that. Sorry about the confusion. Then all the
churn around the allocation is pointless, no?
What about:
---

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Glauber Costa]

On 03/22/2013 01:31 PM, Michal Hocko wrote:
> On Fri 22-03-13 12:22:23, Glauber Costa wrote:
>> On 03/22/2013 12:17 PM, Li Zefan wrote:
>>>> GFP_TEMPORARY groups short lived allocations but the mem cache is not
>>>>> an ideal candidate of this type of allocations..
>>>>>
>>> I'm not sure I'm following you...
>>>
>>> char *memcg_cache_name()
>>> {
>>> 	char *name = alloc();
>>> 	return name;
>>> }
>>>
>>> kmem_cache_dup()
>>> {
>>> 	name = memcg_cache_name();
>>> 	kmem_cache_create_memcg(name);
>>> 	free(name);
>>> }
>>>
>>> Isn't this a short lived allocation?
>>>
>>
>> Hi,
>>
>> Thanks for identifying and fixing this.
>>
>> Li is right. The cache name will live long, but this is because the
>> slab/slub caches will strdup it internally. So the actual memcg
>> allocation is short lived.
> 
> OK, I have totally missed that. Sorry about the confusion. Then all the
> churn around the allocation is pointless, no?
> What about:

If we're really not concerned about stack, then yes. Even if always
running from workqueues, a PAGE_SIZEd stack variable seems risky to me.

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Fri 22-03-13 13:41:40, Glauber Costa wrote:
> On 03/22/2013 01:31 PM, Michal Hocko wrote:
> > On Fri 22-03-13 12:22:23, Glauber Costa wrote:
> >> On 03/22/2013 12:17 PM, Li Zefan wrote:
> >>>> GFP_TEMPORARY groups short lived allocations but the mem cache is not
> >>>>> an ideal candidate of this type of allocations..
> >>>>>
> >>> I'm not sure I'm following you...
> >>>
> >>> char *memcg_cache_name()
> >>> {
> >>> 	char *name = alloc();
> >>> 	return name;
> >>> }
> >>>
> >>> kmem_cache_dup()
> >>> {
> >>> 	name = memcg_cache_name();
> >>> 	kmem_cache_create_memcg(name);
> >>> 	free(name);
> >>> }
> >>>
> >>> Isn't this a short lived allocation?
> >>>
> >>
> >> Hi,
> >>
> >> Thanks for identifying and fixing this.
> >>
> >> Li is right. The cache name will live long, but this is because the
> >> slab/slub caches will strdup it internally. So the actual memcg
> >> allocation is short lived.
> > 
> > OK, I have totally missed that. Sorry about the confusion. Then all the
> > churn around the allocation is pointless, no?
> > What about:
> 
> If we're really not concerned about stack, then yes. Even if always
> running from workqueues, a PAGE_SIZEd stack variable seems risky to me.

This is not on stack. It is static

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Glauber Costa]

On 03/22/2013 01:48 PM, Michal Hocko wrote:
> On Fri 22-03-13 13:41:40, Glauber Costa wrote:
>> On 03/22/2013 01:31 PM, Michal Hocko wrote:
>>> On Fri 22-03-13 12:22:23, Glauber Costa wrote:
>>>> On 03/22/2013 12:17 PM, Li Zefan wrote:
>>>>>> GFP_TEMPORARY groups short lived allocations but the mem cache is not
>>>>>>> an ideal candidate of this type of allocations..
>>>>>>>
>>>>> I'm not sure I'm following you...
>>>>>
>>>>> char *memcg_cache_name()
>>>>> {
>>>>> 	char *name = alloc();
>>>>> 	return name;
>>>>> }
>>>>>
>>>>> kmem_cache_dup()
>>>>> {
>>>>> 	name = memcg_cache_name();
>>>>> 	kmem_cache_create_memcg(name);
>>>>> 	free(name);
>>>>> }
>>>>>
>>>>> Isn't this a short lived allocation?
>>>>>
>>>>
>>>> Hi,
>>>>
>>>> Thanks for identifying and fixing this.
>>>>
>>>> Li is right. The cache name will live long, but this is because the
>>>> slab/slub caches will strdup it internally. So the actual memcg
>>>> allocation is short lived.
>>>
>>> OK, I have totally missed that. Sorry about the confusion. Then all the
>>> churn around the allocation is pointless, no?
>>> What about:
>>
>> If we're really not concerned about stack, then yes. Even if always
>> running from workqueues, a PAGE_SIZEd stack variable seems risky to me.
> 
> This is not on stack. It is static
> 
Ah, right, I totally missed that. And then you're taking the mutex.

But actually, you don't need to take the mutex. All calls to
kmem_cache_dup are protected by the memcg_cache_mutex. So you should be
able to just use the buffer without further problems.

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Fri 22-03-13 14:03:30, Glauber Costa wrote:
> On 03/22/2013 01:48 PM, Michal Hocko wrote:
> > On Fri 22-03-13 13:41:40, Glauber Costa wrote:
> >> On 03/22/2013 01:31 PM, Michal Hocko wrote:
> >>> On Fri 22-03-13 12:22:23, Glauber Costa wrote:
> >>>> On 03/22/2013 12:17 PM, Li Zefan wrote:
> >>>>>> GFP_TEMPORARY groups short lived allocations but the mem cache is not
> >>>>>>> an ideal candidate of this type of allocations..
> >>>>>>>
> >>>>> I'm not sure I'm following you...
> >>>>>
> >>>>> char *memcg_cache_name()
> >>>>> {
> >>>>> 	char *name = alloc();
> >>>>> 	return name;
> >>>>> }
> >>>>>
> >>>>> kmem_cache_dup()
> >>>>> {
> >>>>> 	name = memcg_cache_name();
> >>>>> 	kmem_cache_create_memcg(name);
> >>>>> 	free(name);
> >>>>> }
> >>>>>
> >>>>> Isn't this a short lived allocation?
> >>>>>
> >>>>
> >>>> Hi,
> >>>>
> >>>> Thanks for identifying and fixing this.
> >>>>
> >>>> Li is right. The cache name will live long, but this is because the
> >>>> slab/slub caches will strdup it internally. So the actual memcg
> >>>> allocation is short lived.
> >>>
> >>> OK, I have totally missed that. Sorry about the confusion. Then all the
> >>> churn around the allocation is pointless, no?
> >>> What about:
> >>
> >> If we're really not concerned about stack, then yes. Even if always
> >> running from workqueues, a PAGE_SIZEd stack variable seems risky to me.
> > 
> > This is not on stack. It is static
> > 
> Ah, right, I totally missed that. And then you're taking the mutex.
> 
> But actually, you don't need to take the mutex. All calls to
> kmem_cache_dup are protected by the memcg_cache_mutex.

Yes and I am not taking that mutex. I've just added lockdep assert to
make sure that this still holds true.

> So you should be able to just use the buffer without further problems.

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Glauber Costa]

On 03/22/2013 02:06 PM, Michal Hocko wrote:
> On Fri 22-03-13 14:03:30, Glauber Costa wrote:
>> On 03/22/2013 01:48 PM, Michal Hocko wrote:
>>> On Fri 22-03-13 13:41:40, Glauber Costa wrote:
>>>> On 03/22/2013 01:31 PM, Michal Hocko wrote:
>>>>> On Fri 22-03-13 12:22:23, Glauber Costa wrote:
>>>>>> On 03/22/2013 12:17 PM, Li Zefan wrote:
>>>>>>>> GFP_TEMPORARY groups short lived allocations but the mem cache is not
>>>>>>>>> an ideal candidate of this type of allocations..
>>>>>>>>>
>>>>>>> I'm not sure I'm following you...
>>>>>>>
>>>>>>> char *memcg_cache_name()
>>>>>>> {
>>>>>>> 	char *name = alloc();
>>>>>>> 	return name;
>>>>>>> }
>>>>>>>
>>>>>>> kmem_cache_dup()
>>>>>>> {
>>>>>>> 	name = memcg_cache_name();
>>>>>>> 	kmem_cache_create_memcg(name);
>>>>>>> 	free(name);
>>>>>>> }
>>>>>>>
>>>>>>> Isn't this a short lived allocation?
>>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Thanks for identifying and fixing this.
>>>>>>
>>>>>> Li is right. The cache name will live long, but this is because the
>>>>>> slab/slub caches will strdup it internally. So the actual memcg
>>>>>> allocation is short lived.
>>>>>
>>>>> OK, I have totally missed that. Sorry about the confusion. Then all the
>>>>> churn around the allocation is pointless, no?
>>>>> What about:
>>>>
>>>> If we're really not concerned about stack, then yes. Even if always
>>>> running from workqueues, a PAGE_SIZEd stack variable seems risky to me.
>>>
>>> This is not on stack. It is static
>>>
>> Ah, right, I totally missed that. And then you're taking the mutex.
>>
>> But actually, you don't need to take the mutex. All calls to
>> kmem_cache_dup are protected by the memcg_cache_mutex.
> 
> Yes and I am not taking that mutex. I've just added lockdep assert to
> make sure that this still holds true.
> 
It is impressive what a busy week does to our brains...

I read the code as lockdep_assert(memcg_cache_mutex), and then later on
mutex_lock(&memcg_mutex). But reading again, that was a just an
rcu_read_lock(). Good thing it is Friday

You guys can add my Acked-by, and thanks again

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Fri 22-03-13 14:25:23, Glauber Costa wrote:
> On 03/22/2013 02:06 PM, Michal Hocko wrote:
> > On Fri 22-03-13 14:03:30, Glauber Costa wrote:
> >> On 03/22/2013 01:48 PM, Michal Hocko wrote:
> >>> On Fri 22-03-13 13:41:40, Glauber Costa wrote:
> >>>> On 03/22/2013 01:31 PM, Michal Hocko wrote:
> >>>>> On Fri 22-03-13 12:22:23, Glauber Costa wrote:
> >>>>>> On 03/22/2013 12:17 PM, Li Zefan wrote:
> >>>>>>>> GFP_TEMPORARY groups short lived allocations but the mem cache is not
> >>>>>>>>> an ideal candidate of this type of allocations..
> >>>>>>>>>
> >>>>>>> I'm not sure I'm following you...
> >>>>>>>
> >>>>>>> char *memcg_cache_name()
> >>>>>>> {
> >>>>>>> 	char *name = alloc();
> >>>>>>> 	return name;
> >>>>>>> }
> >>>>>>>
> >>>>>>> kmem_cache_dup()
> >>>>>>> {
> >>>>>>> 	name = memcg_cache_name();
> >>>>>>> 	kmem_cache_create_memcg(name);
> >>>>>>> 	free(name);
> >>>>>>> }
> >>>>>>>
> >>>>>>> Isn't this a short lived allocation?
> >>>>>>>
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Thanks for identifying and fixing this.
> >>>>>>
> >>>>>> Li is right. The cache name will live long, but this is because the
> >>>>>> slab/slub caches will strdup it internally. So the actual memcg
> >>>>>> allocation is short lived.
> >>>>>
> >>>>> OK, I have totally missed that. Sorry about the confusion. Then all the
> >>>>> churn around the allocation is pointless, no?
> >>>>> What about:
> >>>>
> >>>> If we're really not concerned about stack, then yes. Even if always
> >>>> running from workqueues, a PAGE_SIZEd stack variable seems risky to me.
> >>>
> >>> This is not on stack. It is static
> >>>
> >> Ah, right, I totally missed that. And then you're taking the mutex.
> >>
> >> But actually, you don't need to take the mutex. All calls to
> >> kmem_cache_dup are protected by the memcg_cache_mutex.
> > 
> > Yes and I am not taking that mutex. I've just added lockdep assert to
> > make sure that this still holds true.
> > 
> It is impressive what a busy week does to our brains...

Tell me something about that.

> I read the code as lockdep_assert(memcg_cache_mutex), and then later on
> mutex_lock(&memcg_mutex). But reading again, that was a just an
> rcu_read_lock(). Good thing it is Friday
> 
> You guys can add my Acked-by, and thanks again

Li, are you ok to take the page via your tree?

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Li Zefan]

>> Thanks for identifying and fixing this.
>>
>> Li is right. The cache name will live long, but this is because the
>> slab/slub caches will strdup it internally. So the actual memcg
>> allocation is short lived.
> 
> OK, I have totally missed that. Sorry about the confusion. Then all the
> churn around the allocation is pointless, no?
> What about:
> ---
>>From 7ed7f53bb597e8cb40d9ac91ce16142fb60f1e93 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Fri, 22 Mar 2013 10:22:54 +0100
> Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()
> 
> As cgroup supports rename, it's unsafe to dereference dentry->d_name
> without proper vfs locks. Fix this by using cgroup_name() rather than
> dentry directly.
> 
> Also open code memcg_cache_name because it is called only from
> kmem_cache_dup which frees the returned name right after
> kmem_cache_create_memcg makes a copy of it. Such a short-lived
> allocation doesn't make too much sense. So replace it by a static
> buffer as kmem_cache_dup is called with memcg_cache_mutex.
> 

I doubt it's a win to add 4K to kernel text size instead of adding
a few extra lines of code... but it's up to you.

> Signed-off-by: Li Zefan <lizefan@huawei.com>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> ---
>  mm/memcontrol.c |   33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
...
>  static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
>  					 struct kmem_cache *s)
>  {
>  	char *name;
>  	struct kmem_cache *new;
> +	static char tmp_name[PAGE_SIZE];
>  
> -	name = memcg_cache_name(memcg, s);
> -	if (!name)
> -		return NULL;
> +	lockdep_assert_held(&memcg_cache_mutex);
> +
> +	rcu_read_lock();
> +	tmp_name = snprintf(tmp_name, sizeof(tmp_name), "%s(%d:%s)", s->name,
> +			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));

I guess you didn't turn on CONFIG_MEMCG_KMEM?

snprintf() returns a int value.

> +	rcu_read_unlock();
>  
> -	new = kmem_cache_create_memcg(memcg, name, s->object_size, s->align,
> +	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;
>  
> -	kfree(name);
>  	return new;
>  }
>  
> 

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Li Zefan]

>> I read the code as lockdep_assert(memcg_cache_mutex), and then later on
>> mutex_lock(&memcg_mutex). But reading again, that was a just an
>> rcu_read_lock(). Good thing it is Friday
>>
>> You guys can add my Acked-by, and thanks again
> 
> Li, are you ok to take the page via your tree?
> 

I don't have a git tree in kernel.org. It's Tejun that picks up
cgroup patches.

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Sun 24-03-13 15:34:51, Li Zefan wrote:
> >> I read the code as lockdep_assert(memcg_cache_mutex), and then later on
> >> mutex_lock(&memcg_mutex). But reading again, that was a just an
> >> rcu_read_lock(). Good thing it is Friday
> >>
> >> You guys can add my Acked-by, and thanks again
> > 
> > Li, are you ok to take the page via your tree?
> > 
> 
> I don't have a git tree in kernel.org. It's Tejun that picks up
> cgroup patches.

Oh, I thought both of you push to that tree. Anyway, I will rework the
patch and send it again.
-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Sun 24-03-13 15:33:21, Li Zefan wrote:
> >> Thanks for identifying and fixing this.
> >>
> >> Li is right. The cache name will live long, but this is because the
> >> slab/slub caches will strdup it internally. So the actual memcg
> >> allocation is short lived.
> > 
> > OK, I have totally missed that. Sorry about the confusion. Then all the
> > churn around the allocation is pointless, no?
> > What about:
> > ---
> >>From 7ed7f53bb597e8cb40d9ac91ce16142fb60f1e93 Mon Sep 17 00:00:00 2001
> > From: Michal Hocko <mhocko@suse.cz>
> > Date: Fri, 22 Mar 2013 10:22:54 +0100
> > Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()
> > 
> > As cgroup supports rename, it's unsafe to dereference dentry->d_name
> > without proper vfs locks. Fix this by using cgroup_name() rather than
> > dentry directly.
> > 
> > Also open code memcg_cache_name because it is called only from
> > kmem_cache_dup which frees the returned name right after
> > kmem_cache_create_memcg makes a copy of it. Such a short-lived
> > allocation doesn't make too much sense. So replace it by a static
> > buffer as kmem_cache_dup is called with memcg_cache_mutex.
> > 
> 
> I doubt it's a win to add 4K to kernel text size instead of adding
> a few extra lines of code... but it's up to you.

I will leave the decision to Glauber. The updated version which uses
kmalloc for the static buffer is bellow.

> > Signed-off-by: Li Zefan <lizefan@huawei.com>
> > Signed-off-by: Michal Hocko <mhocko@suse.cz>
> > ---
> >  mm/memcontrol.c |   33 +++++++++++----------------------
> >  1 file changed, 11 insertions(+), 22 deletions(-)
> ...
> >  static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
> >  					 struct kmem_cache *s)
> >  {
> >  	char *name;
> >  	struct kmem_cache *new;
> > +	static char tmp_name[PAGE_SIZE];
> >  
> > -	name = memcg_cache_name(memcg, s);
> > -	if (!name)
> > -		return NULL;
> > +	lockdep_assert_held(&memcg_cache_mutex);
> > +
> > +	rcu_read_lock();
> > +	tmp_name = snprintf(tmp_name, sizeof(tmp_name), "%s(%d:%s)", s->name,
> > +			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
> 
> I guess you didn't turn on CONFIG_MEMCG_KMEM?

dohh. Friday effect...

> snprintf() returns a int value.
[...]
---

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Li Zefan]

>>> >From 7ed7f53bb597e8cb40d9ac91ce16142fb60f1e93 Mon Sep 17 00:00:00 2001
>>> From: Michal Hocko <mhocko@suse.cz>
>>> Date: Fri, 22 Mar 2013 10:22:54 +0100
>>> Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()
>>>
>>> As cgroup supports rename, it's unsafe to dereference dentry->d_name
>>> without proper vfs locks. Fix this by using cgroup_name() rather than
>>> dentry directly.
>>>
>>> Also open code memcg_cache_name because it is called only from
>>> kmem_cache_dup which frees the returned name right after
>>> kmem_cache_create_memcg makes a copy of it. Such a short-lived
>>> allocation doesn't make too much sense. So replace it by a static
>>> buffer as kmem_cache_dup is called with memcg_cache_mutex.
>>>
>>
>> I doubt it's a win to add 4K to kernel text size instead of adding
>> a few extra lines of code... but it's up to you.
> 
> I will leave the decision to Glauber. The updated version which uses
> kmalloc for the static buffer is bellow.
> 

I don't have strong preference. Glauber, what's your opinion?

...
>  static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
>  					 struct kmem_cache *s)
>  {
> -	char *name;
>  	struct kmem_cache *new;
> +	static char *tmp_name = NULL;

(minor nitpick) why not preserve the name "name"

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Tue 26-03-13 15:52:32, Li Zefan wrote:
[...]
> ...
> >  static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
> >  					 struct kmem_cache *s)
> >  {
> > -	char *name;
> >  	struct kmem_cache *new;
> > +	static char *tmp_name = NULL;
> 
> (minor nitpick) why not preserve the name "name"

I wanted to make it clear that the name is just temporal

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Glauber Costa]

[-- Attachment #1: Type: text/plain, Size: 1412 bytes --]

>>
>> I doubt it's a win to add 4K to kernel text size instead of adding
>> a few extra lines of code... but it's up to you.
> 
> I will leave the decision to Glauber. The updated version which uses
> kmalloc for the static buffer is bellow.
> 
I prefer to allocate dynamically here. But although I understand why we
need to call cgroup_name, I don't understand what is wrong with
kasprintf if we're going to allocate anyway. It will allocate a string
just big enough. A PAGE_SIZE'd allocation is a lot more likely to fail.

Now, if we really want to be smart here, we can do something like what
I've done for the slub attribute buffers, that can actually have very
long values.

allocate a small buffer that will hold 80 % > of the allocations (256
bytes should be enough for most cache names), and if the string is
bigger than this, we allocate. Once we allocate, we save it in a static
pointer and leave it there. The hope here is that we may be able to
live without ever allocating in many systems.

> +
> +	/*
> +	 * 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.
> +	 */
The comment is also no longer true if you don't resort to a static buffer.

The following (untested) patch implements the idea I outlined above.

What do you guys think ?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-memcg-fix-memcg_cache_name-to-use-cgroup_name.patch --]
[-- Type: text/x-patch; name="0001-memcg-fix-memcg_cache_name-to-use-cgroup_name.patch", Size: 0 bytes --]

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Tue 26-03-13 12:35:58, Glauber Costa wrote:
> >>
> >> I doubt it's a win to add 4K to kernel text size instead of adding
> >> a few extra lines of code... but it's up to you.
> > 
> > I will leave the decision to Glauber. The updated version which uses
> > kmalloc for the static buffer is bellow.
> > 
> I prefer to allocate dynamically here. But although I understand why we
> need to call cgroup_name, I don't understand what is wrong with
> kasprintf if we're going to allocate anyway. It will allocate a string
> just big enough. A PAGE_SIZE'd allocation is a lot more likely to fail.
> 
> Now, if we really want to be smart here, we can do something like what
> I've done for the slub attribute buffers, that can actually have very
> long values.
> 
> allocate a small buffer that will hold 80 % > of the allocations (256
> bytes should be enough for most cache names), and if the string is
> bigger than this, we allocate. Once we allocate, we save it in a static
> pointer and leave it there. The hope here is that we may be able to
> live without ever allocating in many systems.
> 
> > +
> > +	/*
> > +	 * 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.
> > +	 */
> The comment is also no longer true if you don't resort to a static buffer.

The buffer _is_ static (read global variable hidden with the function
scope).

> The following (untested) patch implements the idea I outlined above.
> 
> What do you guys think ?

I really do not care which way to fix this.

[...]
> +static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
> +					 struct kmem_cache *s)
>  {
> -	char *name;
> -	struct dentry *dentry;
> +	const char *cgname; /* actual cache name */
> +	char *name = NULL; /* actual cache name */
> +	char buf[256]; /* stack buffer for small allocations */
> +	int buf_len;
> +	static char *buf_name; /* pointer to a page, if we ever need */
> +	struct kmem_cache *new;
> +
> +	lockdep_assert_held(&memcg_cache_mutex);
>  
>  	rcu_read_lock();
> -	dentry = rcu_dereference(memcg->css.cgroup->dentry);
> +	cgname = cgroup_name(memcg->css.cgroup);
>  	rcu_read_unlock();

cgname is valid only within RCU read lock AFAIU.

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Glauber Costa]

On 03/26/2013 12:43 PM, Michal Hocko wrote:
> On Tue 26-03-13 12:35:58, Glauber Costa wrote:
>>>>
>>>> I doubt it's a win to add 4K to kernel text size instead of adding
>>>> a few extra lines of code... but it's up to you.
>>>
>>> I will leave the decision to Glauber. The updated version which uses
>>> kmalloc for the static buffer is bellow.
>>>
>> I prefer to allocate dynamically here. But although I understand why we
>> need to call cgroup_name, I don't understand what is wrong with
>> kasprintf if we're going to allocate anyway. It will allocate a string
>> just big enough. A PAGE_SIZE'd allocation is a lot more likely to fail.
>>
>> Now, if we really want to be smart here, we can do something like what
>> I've done for the slub attribute buffers, that can actually have very
>> long values.
>>
>> allocate a small buffer that will hold 80 % > of the allocations (256
>> bytes should be enough for most cache names), and if the string is
>> bigger than this, we allocate. Once we allocate, we save it in a static
>> pointer and leave it there. The hope here is that we may be able to
>> live without ever allocating in many systems.
>>
>>> +
>>> +	/*
>>> +	 * 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.
>>> +	 */
>> The comment is also no longer true if you don't resort to a static buffer.
> 
> The buffer _is_ static (read global variable hidden with the function
> scope).
> 

Although correct, it is a bit misleading. It is static in the sense it
is held by a static variable. But it is acquired by kmalloc...

In any way, this is a tiny detail.

FWIW, I am fine with the patch you provided:

Acked-by: Glauber Costa <glommer@parallels.com>

We  could go seeing how big those allocations are in practice, but I
doubt it is worth the trouble.

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Li Zefan]

> Although correct, it is a bit misleading. It is static in the sense it
> is held by a static variable. But it is acquired by kmalloc...
> 
> In any way, this is a tiny detail.
> 
> FWIW, I am fine with the patch you provided:
> 
> Acked-by: Glauber Costa <glommer@parallels.com>
> 

Michal, could you resend your final patch to Tejun in a new mail thread?
There are quite a few different patches inlined in this thread.

--
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>

[PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

As cgroup supports rename, it's unsafe to dereference dentry->d_name
without proper vfs locks. Fix this by using cgroup_name() rather than
dentry directly.

Also open code memcg_cache_name because it is called only from
kmem_cache_dup which frees the returned name right after
kmem_cache_create_memcg makes a copy of it. Such a short-lived
allocation doesn't make too much sense. So replace it by a static
buffer as kmem_cache_dup is called with memcg_cache_mutex.

Signed-off-by: Li Zefan <lizefan@huawei.com>
Signed-off-by: Michal Hocko <mhocko@suse.cz>
Acked-by: Glauber Costa <glommer@parallels.com>
---
 mm/memcontrol.c |   64 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index f608546..b30547b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3364,52 +3364,54 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
 	schedule_work(&cachep->memcg_params->destroy);
 }
 
-static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
-{
-	char *name;
-	struct dentry *dentry;
-
-	rcu_read_lock();
-	dentry = rcu_dereference(memcg->css.cgroup->dentry);
-	rcu_read_unlock();
-
-	BUG_ON(dentry == NULL);
-
-	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
-			 memcg_cache_id(memcg), dentry->d_name.name);
-
-	return name;
-}
+/*
+ * This lock protects updaters, not readers. We want readers to be as fast as
+ * they can, and they will either see NULL or a valid cache value. Our model
+ * allow them to see NULL, in which case the root memcg will be selected.
+ *
+ * We need this lock because multiple allocations to the same cache from a non
+ * will span more than one worker. Only one of them can create the cache.
+ */
+static DEFINE_MUTEX(memcg_cache_mutex);
 
+/*
+ * Called with memcg_cache_mutex held
+ */
 static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
 					 struct kmem_cache *s)
 {
-	char *name;
 	struct kmem_cache *new;
+	static char *tmp_name = NULL;
 
-	name = memcg_cache_name(memcg, s);
-	if (!name)
-		return NULL;
+	lockdep_assert_held(&memcg_cache_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(PAGE_SIZE, GFP_KERNEL);
+		WARN_ON_ONCE(!tmp_name);
+		if (!tmp_name)
+			return NULL;
+	}
+
+	rcu_read_lock();
+	snprintf(tmp_name, PAGE_SIZE, "%s(%d:%s)", s->name,
+			 memcg_cache_id(memcg), cgroup_name(memcg->css.cgroup));
+	rcu_read_unlock();
 
-	new = kmem_cache_create_memcg(memcg, name, s->object_size, s->align,
+	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;
 
-	kfree(name);
 	return new;
 }
 
-/*
- * This lock protects updaters, not readers. We want readers to be as fast as
- * they can, and they will either see NULL or a valid cache value. Our model
- * allow them to see NULL, in which case the root memcg will be selected.
- *
- * We need this lock because multiple allocations to the same cache from a non
- * will span more than one worker. Only one of them can create the cache.
- */
-static DEFINE_MUTEX(memcg_cache_mutex);
 static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 						  struct kmem_cache *cachep)
 {
-- 
1.7.10.4

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Wed 27-03-13 09:15:53, Li Zefan wrote:
> > Although correct, it is a bit misleading. It is static in the sense it
> > is held by a static variable. But it is acquired by kmalloc...
> > 
> > In any way, this is a tiny detail.
> > 
> > FWIW, I am fine with the patch you provided:
> > 
> > Acked-by: Glauber Costa <glommer@parallels.com>
> > 
> 
> Michal, could you resend your final patch to Tejun in a new mail thread?
> There are quite a few different patches inlined in this thread.

Done.

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Johannes Weiner]

On Wed, Mar 27, 2013 at 09:36:39AM +0100, Michal Hocko wrote:
> As cgroup supports rename, it's unsafe to dereference dentry->d_name
> without proper vfs locks. Fix this by using cgroup_name() rather than
> dentry directly.
> 
> Also open code memcg_cache_name because it is called only from
> kmem_cache_dup which frees the returned name right after
> kmem_cache_create_memcg makes a copy of it. Such a short-lived
> allocation doesn't make too much sense. So replace it by a static
> buffer as kmem_cache_dup is called with memcg_cache_mutex.
> 
> Signed-off-by: Li Zefan <lizefan@huawei.com>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Acked-by: Glauber Costa <glommer@parallels.com>
> ---
>  mm/memcontrol.c |   64 ++++++++++++++++++++++++++++---------------------------
>  1 file changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index f608546..b30547b 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3364,52 +3364,54 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
>  	schedule_work(&cachep->memcg_params->destroy);
>  }
>  
> -static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
> -{
> -	char *name;
> -	struct dentry *dentry;
> -
> -	rcu_read_lock();
> -	dentry = rcu_dereference(memcg->css.cgroup->dentry);
> -	rcu_read_unlock();
> -
> -	BUG_ON(dentry == NULL);
> -
> -	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
> -			 memcg_cache_id(memcg), dentry->d_name.name);
> -
> -	return name;
> -}
> +/*
> + * This lock protects updaters, not readers. We want readers to be as fast as
> + * they can, and they will either see NULL or a valid cache value. Our model
> + * allow them to see NULL, in which case the root memcg will be selected.
> + *
> + * We need this lock because multiple allocations to the same cache from a non
> + * will span more than one worker. Only one of them can create the cache.
> + */
> +static DEFINE_MUTEX(memcg_cache_mutex);
>  
> +/*
> + * Called with memcg_cache_mutex held
> + */
>  static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
>  					 struct kmem_cache *s)
>  {
> -	char *name;
>  	struct kmem_cache *new;
> +	static char *tmp_name = NULL;
>  
> -	name = memcg_cache_name(memcg, s);
> -	if (!name)
> -		return NULL;
> +	lockdep_assert_held(&memcg_cache_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(PAGE_SIZE, GFP_KERNEL);
> +		WARN_ON_ONCE(!tmp_name);

Just use the page allocator directly and get a free allocation failure
warning.  Then again, order-0 pages are considered cheap enough that
they never even fail in our current implementation.

Which brings me to my other point: why not just a simple single-page
allocation?  This just seems a little overelaborate.  I think this
path would be taken predominantly after cgroup creation and fork where
we do a bunch of allocations anyway.  And it happens asynchroneously
from userspace, so it's not even really performance critical.

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Wed 27-03-13 10:58:25, Johannes Weiner wrote:
> On Wed, Mar 27, 2013 at 09:36:39AM +0100, Michal Hocko wrote:
[...]
> > +	/*
> > +	 * 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(PAGE_SIZE, GFP_KERNEL);
> > +		WARN_ON_ONCE(!tmp_name);
> 
> Just use the page allocator directly and get a free allocation failure
> warning. 

WARN_ON_ONCE is probably pointless.

> Then again, order-0 pages are considered cheap enough that they never
> even fail in our current implementation.
> 
> Which brings me to my other point: why not just a simple single-page
> allocation?

No objection from me. I was previously thinking about the "proper"
size for something that is a file name. So I originally wanted to use
PATH_MAX instead but ended up with PAGE_SIZE for reasons I do not
remember now.  Maybe we can use NAME_MAX instead. I just do not like to
use page allocator directly when allocatating something like strings
etc...

To be honest, I do not care much which way to go.

> This just seems a little overelaborate.  I think this path would be
> taken predominantly after cgroup creation and fork where we do a bunch
> of allocations anyway.  And it happens asynchroneously from userspace,
> so it's not even really performance critical.

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Glauber Costa]

On 03/27/2013 07:11 PM, Michal Hocko wrote:
> On Wed 27-03-13 10:58:25, Johannes Weiner wrote:
>> On Wed, Mar 27, 2013 at 09:36:39AM +0100, Michal Hocko wrote:
> [...]
>>> +	/*
>>> +	 * 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(PAGE_SIZE, GFP_KERNEL);
>>> +		WARN_ON_ONCE(!tmp_name);
>>
>> Just use the page allocator directly and get a free allocation failure
>> warning. 
> 
> WARN_ON_ONCE is probably pointless.
> 
>> Then again, order-0 pages are considered cheap enough that they never
>> even fail in our current implementation.
>>
>> Which brings me to my other point: why not just a simple single-page
>> allocation?
> 
> No objection from me. I was previously thinking about the "proper"
> size for something that is a file name. So I originally wanted to use
> PATH_MAX instead but ended up with PAGE_SIZE for reasons I do not
> remember now.

theoretically, this is PATH_MAX + max cache name.


--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Wed 27-03-13 19:19:58, Glauber Costa wrote:
> On 03/27/2013 07:11 PM, Michal Hocko wrote:
> > On Wed 27-03-13 10:58:25, Johannes Weiner wrote:
> >> On Wed, Mar 27, 2013 at 09:36:39AM +0100, Michal Hocko wrote:
> > [...]
> >>> +	/*
> >>> +	 * 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(PAGE_SIZE, GFP_KERNEL);
> >>> +		WARN_ON_ONCE(!tmp_name);
> >>
> >> Just use the page allocator directly and get a free allocation failure
> >> warning. 
> > 
> > WARN_ON_ONCE is probably pointless.
> > 
> >> Then again, order-0 pages are considered cheap enough that they never
> >> even fail in our current implementation.
> >>
> >> Which brings me to my other point: why not just a simple single-page
> >> allocation?
> > 
> > No objection from me. I was previously thinking about the "proper"
> > size for something that is a file name. So I originally wanted to use
> > PATH_MAX instead but ended up with PAGE_SIZE for reasons I do not
> > remember now.
> 
> theoretically, this is PATH_MAX + max cache name.

So do you prefer kmalloc(PATH_MAX) or the page allocator directly as
Johannes suggests? I agree tha kamlloc(PAGE_SIZE) looks weird.

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Johannes Weiner]

On Wed, Mar 27, 2013 at 04:11:04PM +0100, Michal Hocko wrote:
> On Wed 27-03-13 10:58:25, Johannes Weiner wrote:
> > On Wed, Mar 27, 2013 at 09:36:39AM +0100, Michal Hocko wrote:
> [...]
> > > +	/*
> > > +	 * 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(PAGE_SIZE, GFP_KERNEL);
> > > +		WARN_ON_ONCE(!tmp_name);
> > 
> > Just use the page allocator directly and get a free allocation failure
> > warning. 
> 
> WARN_ON_ONCE is probably pointless.
> 
> > Then again, order-0 pages are considered cheap enough that they never
> > even fail in our current implementation.
> > 
> > Which brings me to my other point: why not just a simple single-page
> > allocation?
> 
> No objection from me. I was previously thinking about the "proper"
> size for something that is a file name. So I originally wanted to use
> PATH_MAX instead but ended up with PAGE_SIZE for reasons I do not
> remember now.  Maybe we can use NAME_MAX instead. I just do not like to
> use page allocator directly when allocatating something like strings
> etc...

Don't grep for GFP_TEMPORARY then, we do it in a couple places :-)
NAME_MAX is just for single dentry names, not path names, no?  Might
be a little short.

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Wed 27-03-13 11:32:26, Johannes Weiner wrote:
> On Wed, Mar 27, 2013 at 04:11:04PM +0100, Michal Hocko wrote:
> > On Wed 27-03-13 10:58:25, Johannes Weiner wrote:
> > > On Wed, Mar 27, 2013 at 09:36:39AM +0100, Michal Hocko wrote:
> > [...]
> > > > +	/*
> > > > +	 * 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(PAGE_SIZE, GFP_KERNEL);
> > > > +		WARN_ON_ONCE(!tmp_name);
> > > 
> > > Just use the page allocator directly and get a free allocation failure
> > > warning. 
> > 
> > WARN_ON_ONCE is probably pointless.
> > 
> > > Then again, order-0 pages are considered cheap enough that they never
> > > even fail in our current implementation.
> > > 
> > > Which brings me to my other point: why not just a simple single-page
> > > allocation?
> > 
> > No objection from me. I was previously thinking about the "proper"
> > size for something that is a file name. So I originally wanted to use
> > PATH_MAX instead but ended up with PAGE_SIZE for reasons I do not
> > remember now.  Maybe we can use NAME_MAX instead. I just do not like to
> > use page allocator directly when allocatating something like strings
> > etc...
> 
> Don't grep for GFP_TEMPORARY then, we do it in a couple places :-)

This is what Li suggested in the beginning and right, I didn't like it.

> NAME_MAX is just for single dentry names, not path names, no?  Might
> be a little short.

Yes, Glauber confirmed that MAX_PATH is needed.

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Tejun Heo]

On Wed, Mar 27, 2013 at 09:36:39AM +0100, Michal Hocko wrote:
> +/*
> + * Called with memcg_cache_mutex held
> + */
>  static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
>  					 struct kmem_cache *s)

Maybe the name could signify it's part of memcg?

Thanks.

-- 
tejun

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Wed 27-03-13 09:15:27, Tejun Heo wrote:
> On Wed, Mar 27, 2013 at 09:36:39AM +0100, Michal Hocko wrote:
> > +/*
> > + * Called with memcg_cache_mutex held
> > + */
> >  static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
> >  					 struct kmem_cache *s)
> 
> Maybe the name could signify it's part of memcg?

kmem_ prefix is used for all CONFIG_MEMCG_KMEM functions. I understand
it clashes with sl?b naming but this is out of scope of this patch IMO.

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Tejun Heo]

On Wed, Mar 27, 2013 at 9:19 AM, Michal Hocko <mhocko@suse.cz> wrote:
>> Maybe the name could signify it's part of memcg?
>
> kmem_ prefix is used for all CONFIG_MEMCG_KMEM functions. I understand
> it clashes with sl?b naming but this is out of scope of this patch IMO.

Oh, it's not using kmemcg? I see. Maybe we can rename later.

Thanks.

-- 
tejun

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Wed 27-03-13 09:21:02, Tejun Heo wrote:
> On Wed, Mar 27, 2013 at 9:19 AM, Michal Hocko <mhocko@suse.cz> wrote:
> >> Maybe the name could signify it's part of memcg?
> >
> > kmem_ prefix is used for all CONFIG_MEMCG_KMEM functions. I understand
> > it clashes with sl?b naming but this is out of scope of this patch IMO.
> 
> Oh, it's not using kmemcg? I see. Maybe we can rename later.

Some parts use memcg_kmem_* other kmem_. A cleanup would be nice.
Glauber?

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Wed 27-03-13 16:32:20, Michal Hocko wrote:
> On Wed 27-03-13 19:19:58, Glauber Costa wrote:
> > On 03/27/2013 07:11 PM, Michal Hocko wrote:
> > > On Wed 27-03-13 10:58:25, Johannes Weiner wrote:
> > >> On Wed, Mar 27, 2013 at 09:36:39AM +0100, Michal Hocko wrote:
> > > [...]
> > >>> +	/*
> > >>> +	 * 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(PAGE_SIZE, GFP_KERNEL);
> > >>> +		WARN_ON_ONCE(!tmp_name);
> > >>
> > >> Just use the page allocator directly and get a free allocation failure
> > >> warning. 
> > > 
> > > WARN_ON_ONCE is probably pointless.
> > > 
> > >> Then again, order-0 pages are considered cheap enough that they never
> > >> even fail in our current implementation.
> > >>
> > >> Which brings me to my other point: why not just a simple single-page
> > >> allocation?
> > > 
> > > No objection from me. I was previously thinking about the "proper"
> > > size for something that is a file name. So I originally wanted to use
> > > PATH_MAX instead but ended up with PAGE_SIZE for reasons I do not
> > > remember now.
> > 
> > theoretically, this is PATH_MAX + max cache name.
> 
> So do you prefer kmalloc(PATH_MAX) or the page allocator directly as
> Johannes suggests? I agree tha kamlloc(PAGE_SIZE) looks weird.

Removed WARN_ON_ONCE as suggested by Johannes and kept kmalloc with
PATH_MAX used instead of PAGE_SIZE. I've kept Glauber's acked-by but I
can remove it.
---

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Glauber Costa]

On 03/27/2013 08:27 PM, Michal Hocko wrote:
> On Wed 27-03-13 09:21:02, Tejun Heo wrote:
>> On Wed, Mar 27, 2013 at 9:19 AM, Michal Hocko <mhocko@suse.cz> wrote:
>>>> Maybe the name could signify it's part of memcg?
>>>
>>> kmem_ prefix is used for all CONFIG_MEMCG_KMEM functions. I understand
>>> it clashes with sl?b naming but this is out of scope of this patch IMO.
>>
>> Oh, it's not using kmemcg? I see. Maybe we can rename later.
> 
> Some parts use memcg_kmem_* other kmem_. A cleanup would be nice.
> Glauber?
> 
I have been using kmem_ only in functions that will deal directly with
the slab caches and with the single purpose of operating them.

kmem_cache_destroy_work_func => worker interface to kmem_cache_destroy
kmem_cache_destroy_memcg_children => cache destructor iterator
kmem_cache_dup => interface to kmem_cache_create

All the other functions start with memcg_
Analogously, all slab-side functions that deal with memcg *ends* with
_memcg. except the functions that are only there to operate memcg data
structures:

memcg_update_all_caches.

In general, those functions could very well live in the other file (slab
or memcg), but they need to take locks or manipulate data structures
that are internal to slab/memcg.

I believe this is a sound convention.

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Wed 27-03-13 18:32:23, Michal Hocko wrote:
[...]
> Removed WARN_ON_ONCE as suggested by Johannes and kept kmalloc with
> PATH_MAX used instead of PAGE_SIZE. I've kept Glauber's acked-by but I
> can remove it.

And hopefully the last version. I forgot to s/PAGE_SIZE/MAX_PATH/ in
snprintf.
---

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

Tejun,
could you take this one please?

On Thu 28-03-13 08:48:14, Michal Hocko wrote:
> On Wed 27-03-13 18:32:23, Michal Hocko wrote:
> [...]
> > Removed WARN_ON_ONCE as suggested by Johannes and kept kmalloc with
> > PATH_MAX used instead of PAGE_SIZE. I've kept Glauber's acked-by but I
> > can remove it.
> 
> And hopefully the last version. I forgot to s/PAGE_SIZE/MAX_PATH/ in
> snprintf.
> ---
> From 551d7b5960904503da8f050faa533278a1d1bc6c Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.cz>
> Date: Thu, 28 Mar 2013 08:46:49 +0100
> Subject: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()
> 
> As cgroup supports rename, it's unsafe to dereference dentry->d_name
> without proper vfs locks. Fix this by using cgroup_name() rather than
> dentry directly.
> 
> Also open code memcg_cache_name because it is called only from
> kmem_cache_dup which frees the returned name right after
> kmem_cache_create_memcg makes a copy of it. Such a short-lived
> allocation doesn't make too much sense. So replace it by a static
> buffer as kmem_cache_dup is called with memcg_cache_mutex.
> 
> Signed-off-by: Li Zefan <lizefan@huawei.com>
> Signed-off-by: Michal Hocko <mhocko@suse.cz>
> Acked-by: Glauber Costa <glommer@parallels.com>
> ---
>  mm/memcontrol.c |   63 ++++++++++++++++++++++++++++---------------------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 53b8201..9715c0c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3214,52 +3214,53 @@ void mem_cgroup_destroy_cache(struct kmem_cache *cachep)
>  	schedule_work(&cachep->memcg_params->destroy);
>  }
>  
> -static char *memcg_cache_name(struct mem_cgroup *memcg, struct kmem_cache *s)
> -{
> -	char *name;
> -	struct dentry *dentry;
> -
> -	rcu_read_lock();
> -	dentry = rcu_dereference(memcg->css.cgroup->dentry);
> -	rcu_read_unlock();
> -
> -	BUG_ON(dentry == NULL);
> -
> -	name = kasprintf(GFP_KERNEL, "%s(%d:%s)", s->name,
> -			 memcg_cache_id(memcg), dentry->d_name.name);
> -
> -	return name;
> -}
> +/*
> + * This lock protects updaters, not readers. We want readers to be as fast as
> + * they can, and they will either see NULL or a valid cache value. Our model
> + * allow them to see NULL, in which case the root memcg will be selected.
> + *
> + * We need this lock because multiple allocations to the same cache from a non
> + * will span more than one worker. Only one of them can create the cache.
> + */
> +static DEFINE_MUTEX(memcg_cache_mutex);
>  
> +/*
> + * Called with memcg_cache_mutex held
> + */
>  static struct kmem_cache *kmem_cache_dup(struct mem_cgroup *memcg,
>  					 struct kmem_cache *s)
>  {
> -	char *name;
>  	struct kmem_cache *new;
> +	static char *tmp_name = NULL;
>  
> -	name = memcg_cache_name(memcg, s);
> -	if (!name)
> -		return NULL;
> +	lockdep_assert_held(&memcg_cache_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;
> +	}
> +
> +	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, name, s->object_size, s->align,
> +	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;
>  
> -	kfree(name);
>  	return new;
>  }
>  
> -/*
> - * This lock protects updaters, not readers. We want readers to be as fast as
> - * they can, and they will either see NULL or a valid cache value. Our model
> - * allow them to see NULL, in which case the root memcg will be selected.
> - *
> - * We need this lock because multiple allocations to the same cache from a non
> - * will span more than one worker. Only one of them can create the cache.
> - */
> -static DEFINE_MUTEX(memcg_cache_mutex);
>  static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
>  						  struct kmem_cache *cachep)
>  {
> -- 
> 1.7.10.4
> 
> -- 
> Michal Hocko
> SUSE Labs
> --
> To unsubscribe from this list: send the line "unsubscribe cgroups" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Tejun Heo]

On Tue, Apr 02, 2013 at 10:26:48AM +0200, Michal Hocko wrote:
> Tejun,
> could you take this one please?

Aye aye, applied to cgroup/for-3.10.

Thanks.

-- 
tejun

--
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>

Re: [PATCH] memcg: fix memcg_cache_name() to use cgroup_name()

[Michal Hocko]

On Wed 03-04-13 14:33:34, Tejun Heo wrote:
> On Tue, Apr 02, 2013 at 10:26:48AM +0200, Michal Hocko wrote:
> > Tejun,
> > could you take this one please?
> 
> Aye aye, applied to cgroup/for-3.10.

Thanks!
-- 
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>