Re: [PATCH] Add file based RSS accounting for memory resource controller (v2)

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

On Thu, 16 Apr 2009 11:02:46 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Thu, 16 Apr 2009 07:29:55 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
> > Thanks, I could have almost sworn I had it.. but I clearly don't
> > 
> > Here is the fixed version
> > 
> > Feature: Add file RSS tracking per memory cgroup
> > 
> > From: Balbir Singh <balbir@linux.vnet.ibm.com>
> > 
> > Changelog v3 -> v2
> > 1. Add corresponding put_cpu() for every get_cpu()
> > 
> > Changelog v2 -> v1
> > 
> > 1. Rename file_rss to mapped_file
> > 2. Add hooks into mem_cgroup_move_account for updating MAPPED_FILE statistics
> > 3. Use a better name for the statistics routine.
> > 
> > 
> > We currently don't track file RSS, the RSS we report is actually anon RSS.
> > All the file mapped pages, come in through the page cache and get accounted
> > there. This patch adds support for accounting file RSS pages. It should
> > 
> > 1. Help improve the metrics reported by the memory resource controller
> > 2. Will form the basis for a future shared memory accounting heuristic
> >    that has been proposed by Kamezawa.
> > 
> > Unfortunately, we cannot rename the existing "rss" keyword used in memory.stat
> > to "anon_rss". We however, add "mapped_file" data and hope to educate the end
> > user through documentation.
> > 
> > Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> 
> Nice feature :) Thanks.
> 
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> I'll test this today.
> 
Sorry, some troubles found. Ignore above Ack. 3points now.

1. get_cpu should be after (*)
==mem_cgroup_update_mapped_file_stat()
+	int cpu = get_cpu();
+
+	if (!page_is_file_cache(page))
+		return;
+
+	if (unlikely(!mm))
+		mm = &init_mm;
+
+	mem = try_get_mem_cgroup_from_mm(mm);
+	if (!mem)
+		return;
+ ----------------------------------------(*)
+	stat = &mem->stat;
+	cpustat = &stat->cpustat[cpu];
+
+	__mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, val);
+	put_cpu();
+}
==

2. In above, "mem" shouldn't be got from "mm"....please get "mem" from page_cgroup.
(Because it's file cache, pc->mem_cgroup is not NULL always.)

I saw this very easily.
==
Cache: 4096
mapped_file: 20480
==

3. at force_empty().
==
+
+	cpu = get_cpu();
+	/* Update mapped_file data for mem_cgroup "from" */
+	stat = &from->stat;
+	cpustat = &stat->cpustat[cpu];
+	__mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, -1);
+
+	/* Update mapped_file data for mem_cgroup "to" */
+	stat = &to->stat;
+	cpustat = &stat->cpustat[cpu];
+	__mem_cgroup_stat_add_safe(cpustat, MEM_CGROUP_STAT_MAPPED_FILE, 1);
+	put_cpu();

This just breaks counter when page is not mapped. please check page_mapped().

like this:
==
    if (page_is_file_cache(page) && page_mapped(page)) {
	modify counter.
    }
==

and call lock_page_cgroup() in  mem_cgroup_update_mapped_file_stat().

This will be slow, but optimization will be very tricky and need some amount of time.


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