Re: [PATCH v2 2/5] memcg: provide root figures from system totals

[Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

(2013/03/06 17:30), Glauber Costa wrote:
> On 03/06/2013 04:27 AM, Kamezawa Hiroyuki wrote:
>> (2013/03/05 22:10), Glauber Costa wrote:
>>> For the root memcg, there is no need to rely on the res_counters if hierarchy
>>> is enabled The sum of all mem cgroups plus the tasks in root itself, is
>>> necessarily the amount of memory used for the whole system. Since those figures
>>> are already kept somewhere anyway, we can just return them here, without too
>>> much hassle.
>>>
>>> Limit and soft limit can't be set for the root cgroup, so they are left at
>>> RESOURCE_MAX. Failcnt is left at 0, because its actual meaning is how many
>>> times we failed allocations due to the limit being hit. We will fail
>>> allocations in the root cgroup, but the limit will never the reason.
>>>
>>> Signed-off-by: Glauber Costa <glommer@parallels.com>
>>> CC: Michal Hocko <mhocko@suse.cz>
>>> CC: Kamezawa Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
>>> CC: Johannes Weiner <hannes@cmpxchg.org>
>>> CC: Mel Gorman <mgorman@suse.de>
>>> CC: Andrew Morton <akpm@linux-foundation.org>
>>
>> I think this patch's calculation is wrong.
>>
> where exactly ?
> 
>>> ---
>>>    mm/memcontrol.c | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>    1 file changed, 64 insertions(+)
>>>
>>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>>> index b8b363f..bfbf1c2 100644
>>> --- a/mm/memcontrol.c
>>> +++ b/mm/memcontrol.c
>>> @@ -4996,6 +4996,56 @@ static inline u64 mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
>>>    	return val << PAGE_SHIFT;
>>>    }
>>>    
>>> +static u64 memcg_read_root_rss(void)
>>> +{
>>> +	struct task_struct *p;
>>> +
>>> +	u64 rss = 0;
>>> +	read_lock(&tasklist_lock);
>>> +	for_each_process(p) {
>>> +		if (!p->mm)
>>> +			continue;
>>> +		task_lock(p);
>>> +		rss += get_mm_rss(p->mm);
>>> +		task_unlock(p);
>>> +	}
>>> +	read_unlock(&tasklist_lock);
>>> +	return rss;
>>> +}
>>
>> I think you can use rcu_read_lock() instead of tasklist_lock.
>> Isn't it enough to use NR_ANON_LRU rather than this ?
> 
> Is it really just ANON_LRU ? get_mm_rss also include filepages, which
> are not in this list.

And mlocked ones counted as Unevictable
> 
> Maybe if we sum up *all* LRUs we would get the right result ?
> 
_MEM...i.e. ...usage_in_bytes is the sum of all LRUs.

> About the tasklist lock, if I get values from the LRUs, maybe. Otherwise
> it is still necessary, no ?

tasklist is RCU list and we don't need locking at reading values, I think.

Thanks,
-Kame




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