Re: [PATCH 2/2] memcg: debugging facility to access dangling memcgs.

[Michal Hocko <mhocko@suse.cz>]

On Fri 23-11-12 14:37:05, Glauber Costa wrote:
> On 11/23/2012 02:33 PM, Michal Hocko wrote:
> > On Fri 23-11-12 13:33:36, Glauber Costa wrote:
> >> On 11/23/2012 01:20 PM, Michal Hocko wrote:
> >>> On Thu 22-11-12 14:29:50, Glauber Costa wrote:
> >>> [...]
> >>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> >>>> index 05b87aa..46f7cfb 100644
> >>>> --- a/mm/memcontrol.c
> >>>> +++ b/mm/memcontrol.c
> >>> [...]
> >>>> @@ -349,6 +366,33 @@ struct mem_cgroup {
> >>>>  #endif
> >>>>  };
> >>>>  
> >>>> +#if defined(CONFIG_MEMCG_KMEM) || defined(CONFIG_MEMCG_SWAP)
> >>>
> >>> Can we have a common config for this something like CONFIG_MEMCG_ASYNC_DESTROY
> >>> which would be selected if either of the two (or potentially others)
> >>> would be selected.
> >>> Also you are saying that the feature is only for debugging purposes so
> >>> it shouldn't be on by default probably.
> >>>
> >>
> >> I personally wouldn't mind. But the big value I see from it is basically
> >> being able to turn it off. For all the rest, we would have to wrap it
> >> under one of those config options anyway...
> > 
> > Sure you would need to habe mem_cgroup_dangling_FOO wrapped by the
> > correct one anyway but that still need to live inside a bigger ifdef and
> > naming all the FOO is awkward. Besides that one
> > CONFIG_MEMCG_ASYNC_DESTROY_DEBUG could have a Kconfig entry and so be
> > enabled separately.
> > 
> 
> How about a more general memcg debug option like CONFIG_MEMCG_DEBUG?
> Do you foresee more facilities we could enable under this?

This sounds to generic and it doesn't say what kind of debugging you
get. CONFIG_MEMCG_ASYNC_DESTROY_DEBUG on the other hand can be pretty
explicit that it is aimed at debugging memory leaks caused by async
memcg destruction.

> > Ohh, you are right you are using kmem_cache name for those. Sorry for
> > the confusion
> >  
> >>> And finally it would be really nice if you described what is the
> >>> exported information good for. Can I somehow change the current state
> >>> (e.g. force freeing those objects so that the memcg can finally pass out
> >>> in piece)?
> >>>
> >> I am open, but I would personally like to have this as a view-only
> >> interface,
> > 
> > And I was not proposing to make it RW. I am just missing a description
> > that would explain: "Ohh well, the file says there are some dangling
> > memcgs. Should I report a bug or sue somebody or just have a coffee and
> > wait some more?"
> > 
> 
> People should pay me beer if the number of pending caches is odd, and
> pay you beer if the number is even. If the number is 0, we both get it.

Deal

> >> just so we suspect a leak occurs, we can easily see what is
> >> the dead memcg contribution to it. It shows you where the data come
> >> from, and if you want to free it, you go search for subsystem-specific
> >> ways to force a free should you want.
> > 
> > Yes, I can imagine its usefulness for developers but I do not see much
> > of an use for admins yet. So I am a bit hesitant for this being on by
> > default.
> > 
> Fully agreed. I am implementing this because Kame suggested. I promptly
> agreed because I remembered how many times I asked myself "Who is
> holding this?" and had to go put some printks all over...

So please make it configurable, off by default and be explicit about its
usefulness.

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>