Re: [PATCH v2 3/7] memcg: provide online test for memcg

[Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>]

On 01/18/2013 07:37 AM, Michal Hocko wrote:
> On Fri 11-01-13 13:45:23, Glauber Costa wrote:
>> Since we are now splitting the memcg creation in two parts, following
>> the cgroup standard, it would be helpful to be able to determine if a
>> created memcg is already online.
>>
>> We can do this by initially forcing the refcnt to 0, and waiting until
>> the last minute to flip it to 1.
> 
> Is this useful, though? What does it tell you? mem_cgroup_online can say
> false even though half of the attributes have been already copied for
> example. I think it should be vice versa. It should mark the point when
> we _start_ copying values. mem_cgroup_online is not the best name then
> of course. It depends what it is going to be used for...
> 

I think you are right in the sense that setting it before copying any
fields is the correct behavior - thanks.

In this sense, this works as a commitment that we will have a complete
child, rather than a statement that we have a complete child.

>> During memcg's lifetime, this value
>> will vary. But if it ever reaches 0 again, memcg will be destructed. We
>> can therefore be sure that any value different than 0 will mean that
>> our group is online.
>>
>> Signed-off-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>> ---
>>  mm/memcontrol.c | 15 ++++++++++++---
>>  1 file changed, 12 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 2229945..2ac2808 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -475,6 +475,11 @@ enum res_type {
>>  static void mem_cgroup_get(struct mem_cgroup *memcg);
>>  static void mem_cgroup_put(struct mem_cgroup *memcg);
>>  
>> +static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
>> +{
>> +	return atomic_read(&memcg->refcnt) > 0;
>> +}
>> +
>>  static inline
>>  struct mem_cgroup *mem_cgroup_from_css(struct cgroup_subsys_state *s)
>>  {
>> @@ -6098,7 +6103,7 @@ mem_cgroup_css_alloc(struct cgroup *cont)
>>  
>>  	memcg->last_scanned_node = MAX_NUMNODES;
>>  	INIT_LIST_HEAD(&memcg->oom_notify);
>> -	atomic_set(&memcg->refcnt, 1);
>> +	atomic_set(&memcg->refcnt, 0);
> 
> I would prefer a comment rather than an explicit atomic_set. The value
> is zero already.
> 
Yes, Sir!

>>  	memcg->move_charge_at_immigrate = 0;
>>  	mutex_init(&memcg->thresholds_lock);
>>  	spin_lock_init(&memcg->move_lock);
>> @@ -6116,10 +6121,13 @@ mem_cgroup_css_online(struct cgroup *cont)
>>  	struct mem_cgroup *memcg, *parent;
>>  	int error = 0;
>>
> 	
> as I said above atomic_set(&memc->refcnt, 1) should be set here before
> we start copying anything.
> 
> But maybe I have missed your intention and later patches in the series
> will convince me...
> 

It went the other way around...