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

[Michal Hocko <mhocko@suse.cz>]

On Fri 14-06-13 15:24:00, Glauber Costa wrote:
> On Thu, Jun 13, 2013 at 06:38:49PM +0200, Michal Hocko wrote:
> > 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?
> > 
> BTW: Since the test for memcg_can_account_kmem is a bit stronger than
> memcg_kmem_is_active (the difference is that it tests the extra bit that we need
> to coordinate the static branches), I will test for that, instead. Like this:
> 
> int memcg_cache_id(struct mem_cgroup *memcg)
> {
>         if (!memcg_can_account_kmem(memcg))                       
>                 return -1;
>         return memcg->kmemcg_id;                                          
> }

Makes sense. You also need to test memcg == NULL, right?

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