[PATCH v2 0/2] slightly rework memcg cache id determination

[PATCH v2 0/2] slightly rework memcg cache id determination

[Glauber Costa]

Michal,

Let me know if this is more acceptable to you. I didn't take your suggestion of
having an id and idx functions, because I think this could potentially be even
more confusing: in the sense that people would need to wonder a bit what is the
difference between them.

Note please that we never use the id as an array index outside of memcg core.
So for memcg core, I have changed, in Patch 2, each direct use of idx as an
index to include a VM_BUG_ON in case we would get an invalid index.

For the other cases, I have consolidated a bit the usage pattern around
memcg_cache_id.  Now the tests are all pretty standardized.

Glauber Costa (2):
  memcg: make cache index determination more robust
  memcg: consolidate callers of memcg_cache_id

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

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

[PATCH v2 1/2] memcg: make cache index determination more robust

[Glauber Costa]

I caught myself doing something like the following outside memcg core:

	memcg_id = -1;
	if (memcg && memcg_kmem_is_active(memcg))
		memcg_id = memcg_cache_id(memcg);

to be able to handle all possible memcgs in a sane manner. In particular, the
root cache will have kmemcg_id = -1 (just because we don't call memcg_kmem_init
to the root cache since it is not limitable). We have always coped with that by
making sure we sanitize which cache is passed to memcg_cache_id. Although this
example is given for root, what we really need to know is whether or not a
cache is kmem active.

But outside the memcg core testing for root, for instance, is not trivial since
we don't export mem_cgroup_is_root. I ended up realizing that this tests really
belong inside memcg_cache_id. This patch moves a similar but stronger test
inside memcg_cache_id and make sure it always return a meaningful value.

Signed-off-by: Glauber Costa <glommer@openvz.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 mm/memcontrol.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2e851f4..359a53b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3081,7 +3081,9 @@ void memcg_cache_list_add(struct mem_cgroup *memcg, struct kmem_cache *cachep)
  */
 int memcg_cache_id(struct mem_cgroup *memcg)
 {
-	return memcg ? memcg->kmemcg_id : -1;
+	if (!memcg || !memcg_can_account_kmem(memcg))
+		return -1;
+	return memcg->kmemcg_id;
 }
 
 /*
-- 
1.8.1.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>

[PATCH v2 2/2] memcg: consolidate callers of memcg_cache_id

[Glauber Costa]

Each caller of memcg_cache_id ends up sanitizing its parameters in its own way.
Now that the memcg_cache_id itself is more robust, we can consolidate this.

There are callers that really cannot handle anything other than a valid memcg
being passed to the function, otherwise something is seriously wrong.  In those
cases, we at least VM_BUG_ON explicitly before using the value any further

Signed-off-by: Glauber Costa <glommer@openvz.org>
Cc: Johannes Weiner <hannes@cmpxchg.org>
Cc: Michal Hocko <mhocko@suse.cz>
Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 mm/memcontrol.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 359a53b..adbc09b 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2975,10 +2975,14 @@ static inline bool memcg_can_account_kmem(struct mem_cgroup *memcg)
 static struct kmem_cache *memcg_params_to_cache(struct memcg_cache_params *p)
 {
 	struct kmem_cache *cachep;
+	int idx;
 
 	VM_BUG_ON(p->is_root_cache);
 	cachep = p->root_cache;
-	return cachep->memcg_params->memcg_caches[memcg_cache_id(p->memcg)];
+
+	idx = memcg_cache_id(p->memcg);
+	VM_BUG_ON(idx < 0);
+	return cachep->memcg_params->memcg_caches[idx];
 }
 
 #ifdef CONFIG_SLABINFO
@@ -3246,6 +3250,7 @@ void memcg_release_cache(struct kmem_cache *s)
 
 	memcg = s->memcg_params->memcg;
 	id  = memcg_cache_id(memcg);
+	VM_BUG_ON(id < 0);
 
 	root = s->memcg_params->root_cache;
 	root->memcg_params->memcg_caches[id] = NULL;
@@ -3408,9 +3413,8 @@ static struct kmem_cache *memcg_create_kmem_cache(struct mem_cgroup *memcg,
 	struct kmem_cache *new_cachep;
 	int idx;
 
-	BUG_ON(!memcg_can_account_kmem(memcg));
-
 	idx = memcg_cache_id(memcg);
+	BUG_ON(!idx < 0);
 
 	mutex_lock(&memcg_cache_mutex);
 	new_cachep = cachep->memcg_params->memcg_caches[idx];
@@ -3583,10 +3587,9 @@ struct kmem_cache *__memcg_kmem_get_cache(struct kmem_cache *cachep,
 	rcu_read_lock();
 	memcg = mem_cgroup_from_task(rcu_dereference(current->mm->owner));
 
-	if (!memcg_can_account_kmem(memcg))
-		goto out;
-
 	idx = memcg_cache_id(memcg);
+	if (idx < 0)
+		return cachep;
 
 	/*
 	 * barrier to mare sure we're always seeing the up to date value.  The
-- 
1.8.1.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 v2 0/2] slightly rework memcg cache id determination

[Michal Hocko]

On Fri 14-06-13 14:04:34, Glauber Costa wrote:
> Michal,
> 
> Let me know if this is more acceptable to you. I didn't take your suggestion of
> having an id and idx functions, because I think this could potentially be even
> more confusing: in the sense that people would need to wonder a bit what is the
> difference between them.

Any clean up is better than nothing. I still think that split up and
making the 2 functions explicit would be better but I do not think this
is really that important. 

> Note please that we never use the id as an array index outside of memcg core.

Now but that doesn't prevent future abuse.

> So for memcg core, I have changed, in Patch 2, each direct use of idx as an
> index to include a VM_BUG_ON in case we would get an invalid index.

OK. If you had an _idx variant then you wouldn't need to add that
VM_BUG_ON at every single place where you use it as an index and do not
risk that future calls would forget about VM_BUG_ON.

> For the other cases, I have consolidated a bit the usage pattern around
> memcg_cache_id.  Now the tests are all pretty standardized.

OK, Great!
 
> Glauber Costa (2):
>   memcg: make cache index determination more robust
>   memcg: consolidate callers of memcg_cache_id
> 
>  mm/memcontrol.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> -- 
> 1.8.1.4
> 

-- 
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 v2 0/2] slightly rework memcg cache id determination

[Glauber Costa]

On Mon, Jun 17, 2013 at 03:30:49PM +0200, Michal Hocko wrote:
> On Fri 14-06-13 14:04:34, Glauber Costa wrote:
> > Michal,
> > 
> > Let me know if this is more acceptable to you. I didn't take your suggestion of
> > having an id and idx functions, because I think this could potentially be even
> > more confusing: in the sense that people would need to wonder a bit what is the
> > difference between them.
> 
> Any clean up is better than nothing. I still think that split up and
> making the 2 functions explicit would be better but I do not think this
> is really that important. 
> 
Being all the same to you, I prefer like this. At least while the users are self
contained and live inside memcg core. This is because I believe having two functions
can be a bit confusing, and while not *totally* confusing, the array-like users
are relatively few.

 
> OK. If you had an _idx variant then you wouldn't need to add that
> VM_BUG_ON at every single place where you use it as an index and do not
> risk that future calls would forget about VM_BUG_ON.
> 
> > For the other cases, I have consolidated a bit the usage pattern around
> > memcg_cache_id.  Now the tests are all pretty standardized.
> 
> OK, Great!
>  
Thanks Michal! Please take a look at the individual patches if you can.

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