Re: [PATCH] memcg: make cache index determination more robust

[Michal Hocko <mhocko@suse.cz>]

On Wed 12-06-13 16:43:28, Glauber Costa wrote:
> 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 the tests inside memcg_cache_id
> and make sure it always return a meaningful value.

This is quite a mess, to be honest. Some callers test/require
memcg_can_account_kmem others !p->is_root_cache. Can we have that
unified, please?

Also the return value of this function is used mostly as an index to
memcg_params->memcg_caches array so returning -1 sounds like a bad idea.
Few other cases use it as a real id. Maybe we need to split this up.

Pulling the check inside the function is OK but can we settle with a
common pattern here, pretty please?

> 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..749f7a4 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_kmem_is_active(memcg))
> +		return -1;
> +	return memcg->kmemcg_id;
>  }
>  
>  /*
> -- 
> 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>