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;
}
This will allow us to consolidate the tests around it a bit in my follow up patch.
> > Signed-off-by: Glauber Costa <glommer@xxxxxxxxxx>
> > Cc: Johannes Weiner <hannes@xxxxxxxxxxx>
> > Cc: Michal Hocko <mhocko@xxxxxxx>
> > Cc: Kamezawa Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>