Re: [PATCH v5 5/9] memcg: add dirty page accounting infrastructure

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

On Mon, 28 Feb 2011 01:47:30 +0900
Minchan Kim <minchan.kim@gmail.com> wrote:

> On Fri, Feb 25, 2011 at 01:35:56PM -0800, Greg Thelen wrote:
> > Add memcg routines to track dirty, writeback, and unstable_NFS pages.
> > These routines are not yet used by the kernel to count such pages.
> > A later change adds kernel calls to these new routines.
> > 
> > Signed-off-by: Greg Thelen <gthelen@google.com>
> > Signed-off-by: Andrea Righi <arighi@develer.com>
> > Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > ---
> > Changelog since v1:
> > - Renamed "nfs"/"total_nfs" to "nfs_unstable"/"total_nfs_unstable" in per cgroup
> >   memory.stat to match /proc/meminfo.
> > - Rename (for clarity):
> >   - mem_cgroup_write_page_stat_item -> mem_cgroup_page_stat_item
> >   - mem_cgroup_read_page_stat_item -> mem_cgroup_nr_pages_item
> > - Remove redundant comments.
> > - Made mem_cgroup_move_account_page_stat() inline.
> > 
> >  include/linux/memcontrol.h |    5 ++-
> >  mm/memcontrol.c            |   87 ++++++++++++++++++++++++++++++++++++++++----
> >  2 files changed, 83 insertions(+), 9 deletions(-)
> > 
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index 3da48ae..e1f70a9 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -25,9 +25,12 @@ struct page_cgroup;
> >  struct page;
> >  struct mm_struct;
> >  
> > -/* Stats that can be updated by kernel. */
> > +/* mem_cgroup page counts accessed by kernel. */
> 
> I confused by 'kernel', 'access'?
> So, What's the page counts accessed by user?
> I don't like such words.
> 
> Please, clarify the comment.
> 'Stats of page that can be tracking by memcg' or whatever.
> 
Ah, yes. that's better.




> >  enum mem_cgroup_page_stat_item {
> >  	MEMCG_NR_FILE_MAPPED, /* # of pages charged as file rss */
> > +	MEMCG_NR_FILE_DIRTY, /* # of dirty pages in page cache */
> > +	MEMCG_NR_FILE_WRITEBACK, /* # of pages under writeback */
> > +	MEMCG_NR_FILE_UNSTABLE_NFS, /* # of NFS unstable pages */
> >  };
> >  
> >  extern unsigned long mem_cgroup_isolate_pages(unsigned long nr_to_scan,
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 1c2704a..38f786b 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -92,8 +92,11 @@ enum mem_cgroup_stat_index {
> >  	 */
> >  	MEM_CGROUP_STAT_CACHE, 	   /* # of pages charged as cache */
> >  	MEM_CGROUP_STAT_RSS,	   /* # of pages charged as anon rss */
> > -	MEM_CGROUP_STAT_FILE_MAPPED,  /* # of pages charged as file rss */
> >  	MEM_CGROUP_STAT_SWAPOUT, /* # of pages, swapped out */
> > +	MEM_CGROUP_STAT_FILE_MAPPED,  /* # of pages charged as file rss */
> > +	MEM_CGROUP_STAT_FILE_DIRTY,	/* # of dirty pages in page cache */
> > +	MEM_CGROUP_STAT_FILE_WRITEBACK,		/* # of pages under writeback */
> > +	MEM_CGROUP_STAT_FILE_UNSTABLE_NFS,	/* # of NFS unstable pages */
> >  	MEM_CGROUP_STAT_DATA, /* end of data requires synchronization */
> >  	MEM_CGROUP_ON_MOVE,	/* someone is moving account between groups */
> >  	MEM_CGROUP_STAT_NSTATS,
> > @@ -1622,6 +1625,44 @@ void mem_cgroup_update_page_stat(struct page *page,
> >  			ClearPageCgroupFileMapped(pc);
> >  		idx = MEM_CGROUP_STAT_FILE_MAPPED;
> >  		break;
> > +
> > +	case MEMCG_NR_FILE_DIRTY:
> > +		/* Use Test{Set,Clear} to only un/charge the memcg once. */
> > +		if (val > 0) {
> > +			if (TestSetPageCgroupFileDirty(pc))
> > +				val = 0;
> > +		} else {
> > +			if (!TestClearPageCgroupFileDirty(pc))
> > +				val = 0;
> > +		}
> > +		idx = MEM_CGROUP_STAT_FILE_DIRTY;
> > +		break;
> > +
> > +	case MEMCG_NR_FILE_WRITEBACK:
> > +		/*
> > +		 * This counter is adjusted while holding the mapping's
> > +		 * tree_lock.  Therefore there is no race between settings and
> > +		 * clearing of this flag.
> > +		 */
> > +		if (val > 0)
> > +			SetPageCgroupFileWriteback(pc);
> > +		else
> > +			ClearPageCgroupFileWriteback(pc);
> > +		idx = MEM_CGROUP_STAT_FILE_WRITEBACK;
> > +		break;
> > +
> > +	case MEMCG_NR_FILE_UNSTABLE_NFS:
> > +		/* Use Test{Set,Clear} to only un/charge the memcg once. */
> > +		if (val > 0) {
> > +			if (TestSetPageCgroupFileUnstableNFS(pc))
> > +				val = 0;
> > +		} else {
> > +			if (!TestClearPageCgroupFileUnstableNFS(pc))
> > +				val = 0;
> > +		}
> > +		idx = MEM_CGROUP_STAT_FILE_UNSTABLE_NFS;
> > +		break;
> 
> This part can be simplified by some macro work.
> But it's another issue.
> 
Agreed, doing in another patch is better.

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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>