Re: [PATCH v2 3/5] memcg: make it suck faster

[Kamezawa Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>]

(2013/03/06 17:38), Glauber Costa wrote:
> 
>>
>>> Signed-off-by: Glauber Costa <glommer-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>>> CC: Michal Hocko <mhocko-AlSwsSmVLrQ@public.gmane.org>
>>> CC: Kamezawa Hiroyuki <kamezawa.hiroyu-+CUm20s59erQFUHtdCDX3A@public.gmane.org>
>>> CC: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
>>> CC: Mel Gorman <mgorman-l3A5Bk7waGM@public.gmane.org>
>>> CC: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+b@public.gmane.org>
>>
>> After quick look, it seems most parts are good. But I have a concern.
>>
>> At memcg enablement, you move the numbers from vm_stat[] to res_counters.
>>
> Not only to res_counters. Mostly to mem_cgroup_stat_cpu, but I do move
> to res_counters as well.
> 
>> Why you need it ? It's not explained.
> 
> Because at this point, the bypass will no longer be in effect and we
> need accurate figures in root cgroup about what happened so far.
> 
> If we always have root-level hierarchy, then the bypass could go on
> forever. But if we have not, we'll need to rely on whatever was in there.
> 
>> And if it's necessary, uncharge will leak because page_cgroup is not marked
>> as PCG_USED, pc->mem_cgroup == NULL. So, res.usage will not be decreased.
>>
> 
> The same problem happen when deriving an mz from a page. Since
> pc->mem_cgroup will be NULL. I am interpreting that as "root mem cgroup".
> 
yes.

> Maybe even better would be to scan page cgroup writing a magic. Then if
> we see that magic we are sure it is an uninitialized pc.
> 
>> Could you fix it if you need to move numbers to res_counter ?
>>
> 
> At least for the pages in LRUs, I can scan them all, and update their
> page information. I am just wondering if this isn't a *very* expensive
> operation. Fine that we do it once, but still, is potentially scanning
> *all* pages in the system.
> 
> So I've basically decided it is better to interpret pc->mem_cgroup =
> NULL as this uninitialized state. (and can change to a magic)
> 

I think it can work. 

Thanks,
-Kame