[PATCH] memcg: remove PCG_CACHE page_cgroup flag

[PATCH] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

This patch is onto memcg-devel, can be applied to linux-next, too.

==

[PATCH] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

This patch is onto memcg-devel, can be applied to linux-next, too.

==
>From 529653c266b0682894d64e4797fcaf6a3c35db25 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date: Thu, 19 Jan 2012 17:09:41 +0900
Subject: [PATCH] memcg: remove PCG_CACHE

We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
Here, "CACHE" means anonymous user pages (and SwapCache). This
doesn't include shmem.

Consdering callers, at charge/uncharge, the caller should know
what  the page is and we don't need to record it by using 1bit
per page.

This patch removes PCG_CACHE bit and make callers of
mem_cgroup_charge_statistics() to specify what the page is.

Changelog since RFC.
 - rebased onto memcg-devel
 - rename 'file' to 'not_rss'
 - some cleanup and added comment.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 include/linux/page_cgroup.h |    8 +-----
 mm/memcontrol.c             |   55 ++++++++++++++++++++++++++----------------
 2 files changed, 35 insertions(+), 28 deletions(-)

diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
index a2d1177..1060292 100644
--- a/include/linux/page_cgroup.h
+++ b/include/linux/page_cgroup.h
@@ -4,7 +4,6 @@
 enum {
 	/* flags for mem_cgroup */
 	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
-	PCG_CACHE, /* charged as cache */
 	PCG_USED, /* this object is in use. */
 	PCG_MIGRATION, /* under page migration */
 	/* flags for mem_cgroup and file and I/O status */
@@ -64,11 +63,6 @@ static inline void ClearPageCgroup##uname(struct page_cgroup *pc)	\
 static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
 	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
 
-/* Cache flag is set only once (at allocation) */
-TESTPCGFLAG(Cache, CACHE)
-CLEARPCGFLAG(Cache, CACHE)
-SETPCGFLAG(Cache, CACHE)
-
 TESTPCGFLAG(Used, USED)
 CLEARPCGFLAG(Used, USED)
 SETPCGFLAG(Used, USED)
@@ -85,7 +79,7 @@ static inline void lock_page_cgroup(struct page_cgroup *pc)
 {
 	/*
 	 * Don't take this lock in IRQ context.
-	 * This lock is for pc->mem_cgroup, USED, CACHE, MIGRATION
+	 * This lock is for pc->mem_cgroup, USED, MIGRATION
 	 */
 	bit_spin_lock(PCG_LOCK, &pc->flags);
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index fb2dfc3..de7721d 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -606,11 +606,16 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
 }
 
 static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
-					 bool file, int nr_pages)
+					 bool not_rss, int nr_pages)
 {
 	preempt_disable();
 
-	if (file)
+	/*
+	 * Here, RSS means 'mapped anon' and anon's SwapCache. Unlike LRU,
+	 * Shmem is not included to Anon. It' counted as 'file cache'
+	 * which tends to be shared between memcgs.
+	 */
+	if (not_rss)
 		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
 				nr_pages);
 	else
@@ -2343,6 +2348,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 				       struct page_cgroup *pc,
 				       enum charge_type ctype)
 {
+	bool not_rss;
+
 	lock_page_cgroup(pc);
 	if (unlikely(PageCgroupUsed(pc))) {
 		unlock_page_cgroup(pc);
@@ -2362,21 +2369,15 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 	 * See mem_cgroup_add_lru_list(), etc.
  	 */
 	smp_wmb();
-	switch (ctype) {
-	case MEM_CGROUP_CHARGE_TYPE_CACHE:
-	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
-		SetPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
-		ClearPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	default:
-		break;
-	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
+	SetPageCgroupUsed(pc);
+	if ((ctype == MEM_CGROUP_CHARGE_TYPE_CACHE) ||
+	    (ctype == MEM_CGROUP_CHARGE_TYPE_SHMEM))
+		not_rss = true;
+	else
+		not_rss = false;
+
+	mem_cgroup_charge_statistics(memcg, not_rss, nr_pages);
 	unlock_page_cgroup(pc);
 	WARN_ON_ONCE(PageLRU(page));
 	/*
@@ -2441,6 +2442,7 @@ static int mem_cgroup_move_account(struct page *page,
 {
 	unsigned long flags;
 	int ret;
+	bool not_rss = !PageAnon(page);
 
 	VM_BUG_ON(from == to);
 	VM_BUG_ON(PageLRU(page));
@@ -2469,14 +2471,14 @@ static int mem_cgroup_move_account(struct page *page,
 		__this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
 		preempt_enable();
 	}
-	mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(from, not_rss, -nr_pages);
 	if (uncharge)
 		/* This is not "cancel", but cancel_charge does all we need. */
 		__mem_cgroup_cancel_charge(from, nr_pages);
 
 	/* caller should have done css_get */
 	pc->mem_cgroup = to;
-	mem_cgroup_charge_statistics(to, PageCgroupCache(pc), nr_pages);
+	mem_cgroup_charge_statistics(to, not_rss, nr_pages);
 	/*
 	 * We charges against "to" which may not have any tasks. Then, "to"
 	 * can be under rmdir(). But in current implementation, caller of
@@ -2824,6 +2826,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 	struct mem_cgroup *memcg = NULL;
 	unsigned int nr_pages = 1;
 	struct page_cgroup *pc;
+	bool not_rss = false;
 
 	if (mem_cgroup_disabled())
 		return NULL;
@@ -2850,6 +2853,10 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 		goto unlock_out;
 
 	switch (ctype) {
+	case MEM_CGROUP_CHARGE_TYPE_CACHE:
+	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
+		not_rss = true;
+		break;
 	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
 	case MEM_CGROUP_CHARGE_TYPE_DROP:
 		/* See mem_cgroup_prepare_migration() */
@@ -2867,7 +2874,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 		break;
 	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(memcg, not_rss, -nr_pages);
 
 	ClearPageCgroupUsed(pc);
 	/*
@@ -2908,9 +2915,15 @@ void mem_cgroup_uncharge_page(struct page *page)
 
 void mem_cgroup_uncharge_cache_page(struct page *page)
 {
+	int ctype;
+
 	VM_BUG_ON(page_mapped(page));
 	VM_BUG_ON(page->mapping);
-	__mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE);
+	if (page_is_file_cache(page))
+		ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
+	else
+		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
+	__mem_cgroup_uncharge_common(page, ctype);
 }
 
 /*
@@ -3253,7 +3266,7 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,
 	/* fix accounting on old pages */
 	lock_page_cgroup(pc);
 	memcg = pc->mem_cgroup;
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -1);
+	mem_cgroup_charge_statistics(memcg, true, -1);
 	ClearPageCgroupUsed(pc);
 	unlock_page_cgroup(pc);
 
-- 
1.7.4.1

Re: [PATCH] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Thu, Jan 19, 2012 at 06:17:11PM +0900, KAMEZAWA Hiroyuki wrote:
> @@ -4,7 +4,6 @@
>  enum {
>  	/* flags for mem_cgroup */
>  	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
> -	PCG_CACHE, /* charged as cache */
>  	PCG_USED, /* this object is in use. */
>  	PCG_MIGRATION, /* under page migration */
>  	/* flags for mem_cgroup and file and I/O status */

Me gusta.

> @@ -606,11 +606,16 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
>  }
>  
>  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> -					 bool file, int nr_pages)
> +					 bool not_rss, int nr_pages)
>  {
>  	preempt_disable();
>  
> -	if (file)
> +	/*
> +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Unlike LRU,
> +	 * Shmem is not included to Anon. It' counted as 'file cache'
> +	 * which tends to be shared between memcgs.
> +	 */
> +	if (not_rss)

Could you invert that boolean and call it "anon"?

> @@ -2343,6 +2348,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  				       struct page_cgroup *pc,
>  				       enum charge_type ctype)
>  {
> +	bool not_rss;
> +
>  	lock_page_cgroup(pc);
>  	if (unlikely(PageCgroupUsed(pc))) {
>  		unlock_page_cgroup(pc);
> @@ -2362,21 +2369,15 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  	 * See mem_cgroup_add_lru_list(), etc.
>   	 */
>  	smp_wmb();
> -	switch (ctype) {
> -	case MEM_CGROUP_CHARGE_TYPE_CACHE:
> -	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
> -		SetPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
> -		ClearPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	default:
> -		break;
> -	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
> +	SetPageCgroupUsed(pc);
> +	if ((ctype == MEM_CGROUP_CHARGE_TYPE_CACHE) ||
> +	    (ctype == MEM_CGROUP_CHARGE_TYPE_SHMEM))
> +		not_rss = true;
> +	else
> +		not_rss = false;
> +
> +	mem_cgroup_charge_statistics(memcg, not_rss, nr_pages);

	mem_cgroup_charge_statistics(memcg,
				     ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED,
				     nr_pages);

and save even more lines, without sacrificing clarity! :)

> @@ -2908,9 +2915,15 @@ void mem_cgroup_uncharge_page(struct page *page)
>  
>  void mem_cgroup_uncharge_cache_page(struct page *page)
>  {
> +	int ctype;
> +
>  	VM_BUG_ON(page_mapped(page));
>  	VM_BUG_ON(page->mapping);
> -	__mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE);
> +	if (page_is_file_cache(page))
> +		ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> +	else
> +		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> +	__mem_cgroup_uncharge_common(page, ctype);

Looks like an unrelated bugfix on one hand, but on the other hand we
do not differentiate cache from shmem anywhere, afaik, and you do not
introduce anything that does.  Could you just leave this out?

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

Re: [PATCH] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Thu, Jan 19, 2012 at 06:17:11PM +0900, KAMEZAWA Hiroyuki wrote:
> @@ -4,7 +4,6 @@
>  enum {
>  	/* flags for mem_cgroup */
>  	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
> -	PCG_CACHE, /* charged as cache */
>  	PCG_USED, /* this object is in use. */
>  	PCG_MIGRATION, /* under page migration */
>  	/* flags for mem_cgroup and file and I/O status */

Me gusta.

> @@ -606,11 +606,16 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
>  }
>  
>  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> -					 bool file, int nr_pages)
> +					 bool not_rss, int nr_pages)
>  {
>  	preempt_disable();
>  
> -	if (file)
> +	/*
> +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Unlike LRU,
> +	 * Shmem is not included to Anon. It' counted as 'file cache'
> +	 * which tends to be shared between memcgs.
> +	 */
> +	if (not_rss)

Could you invert that boolean and call it "anon"?

> @@ -2343,6 +2348,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  				       struct page_cgroup *pc,
>  				       enum charge_type ctype)
>  {
> +	bool not_rss;
> +
>  	lock_page_cgroup(pc);
>  	if (unlikely(PageCgroupUsed(pc))) {
>  		unlock_page_cgroup(pc);
> @@ -2362,21 +2369,15 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  	 * See mem_cgroup_add_lru_list(), etc.
>   	 */
>  	smp_wmb();
> -	switch (ctype) {
> -	case MEM_CGROUP_CHARGE_TYPE_CACHE:
> -	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
> -		SetPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
> -		ClearPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	default:
> -		break;
> -	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
> +	SetPageCgroupUsed(pc);
> +	if ((ctype == MEM_CGROUP_CHARGE_TYPE_CACHE) ||
> +	    (ctype == MEM_CGROUP_CHARGE_TYPE_SHMEM))
> +		not_rss = true;
> +	else
> +		not_rss = false;
> +
> +	mem_cgroup_charge_statistics(memcg, not_rss, nr_pages);

	mem_cgroup_charge_statistics(memcg,
				     ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED,
				     nr_pages);

and save even more lines, without sacrificing clarity! :)

> @@ -2908,9 +2915,15 @@ void mem_cgroup_uncharge_page(struct page *page)
>  
>  void mem_cgroup_uncharge_cache_page(struct page *page)
>  {
> +	int ctype;
> +
>  	VM_BUG_ON(page_mapped(page));
>  	VM_BUG_ON(page->mapping);
> -	__mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE);
> +	if (page_is_file_cache(page))
> +		ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> +	else
> +		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> +	__mem_cgroup_uncharge_common(page, ctype);

Looks like an unrelated bugfix on one hand, but on the other hand we
do not differentiate cache from shmem anywhere, afaik, and you do not
introduce anything that does.  Could you just leave this out?

Re: [PATCH] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

On Thu, 19 Jan 2012 14:30:35 +0100
Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Thu, Jan 19, 2012 at 06:17:11PM +0900, KAMEZAWA Hiroyuki wrote:
> > @@ -4,7 +4,6 @@
> >  enum {
> >  	/* flags for mem_cgroup */
> >  	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
> > -	PCG_CACHE, /* charged as cache */
> >  	PCG_USED, /* this object is in use. */
> >  	PCG_MIGRATION, /* under page migration */
> >  	/* flags for mem_cgroup and file and I/O status */
> 
> Me gusta.
> 
> > @@ -606,11 +606,16 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
> >  }
> >  
> >  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> > -					 bool file, int nr_pages)
> > +					 bool not_rss, int nr_pages)
> >  {
> >  	preempt_disable();
> >  
> > -	if (file)
> > +	/*
> > +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Unlike LRU,
> > +	 * Shmem is not included to Anon. It' counted as 'file cache'
> > +	 * which tends to be shared between memcgs.
> > +	 */
> > +	if (not_rss)
> 
> Could you invert that boolean and call it "anon"?
> 

Sure. I wondered whta name is good other than 'file'.


> > @@ -2343,6 +2348,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
> >  				       struct page_cgroup *pc,
> >  				       enum charge_type ctype)
> >  {
> > +	bool not_rss;
> > +
> >  	lock_page_cgroup(pc);
> >  	if (unlikely(PageCgroupUsed(pc))) {
> >  		unlock_page_cgroup(pc);
> > @@ -2362,21 +2369,15 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
> >  	 * See mem_cgroup_add_lru_list(), etc.
> >   	 */
> >  	smp_wmb();
> > -	switch (ctype) {
> > -	case MEM_CGROUP_CHARGE_TYPE_CACHE:
> > -	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
> > -		SetPageCgroupCache(pc);
> > -		SetPageCgroupUsed(pc);
> > -		break;
> > -	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
> > -		ClearPageCgroupCache(pc);
> > -		SetPageCgroupUsed(pc);
> > -		break;
> > -	default:
> > -		break;
> > -	}
> >  
> > -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
> > +	SetPageCgroupUsed(pc);
> > +	if ((ctype == MEM_CGROUP_CHARGE_TYPE_CACHE) ||
> > +	    (ctype == MEM_CGROUP_CHARGE_TYPE_SHMEM))
> > +		not_rss = true;
> > +	else
> > +		not_rss = false;
> > +
> > +	mem_cgroup_charge_statistics(memcg, not_rss, nr_pages);
> 
> 	mem_cgroup_charge_statistics(memcg,
> 				     ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED,
> 				     nr_pages);
> 
> and save even more lines, without sacrificing clarity! :)
> 
> > @@ -2908,9 +2915,15 @@ void mem_cgroup_uncharge_page(struct page *page)
> >  
> >  void mem_cgroup_uncharge_cache_page(struct page *page)
> >  {
> > +	int ctype;
> > +
> >  	VM_BUG_ON(page_mapped(page));
> >  	VM_BUG_ON(page->mapping);
> > -	__mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE);
> > +	if (page_is_file_cache(page))
> > +		ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> > +	else
> > +		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> > +	__mem_cgroup_uncharge_common(page, ctype);
> 
> Looks like an unrelated bugfix on one hand, but on the other hand we
> do not differentiate cache from shmem anywhere, afaik, and you do not
> introduce anything that does.  Could you just leave this out?
> 

Ok. I'll remove.
I inserted this just for clarifying what we do.
will post v3.

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>

Re: [PATCH] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

On Thu, 19 Jan 2012 14:30:35 +0100
Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Thu, Jan 19, 2012 at 06:17:11PM +0900, KAMEZAWA Hiroyuki wrote:
> > @@ -4,7 +4,6 @@
> >  enum {
> >  	/* flags for mem_cgroup */
> >  	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
> > -	PCG_CACHE, /* charged as cache */
> >  	PCG_USED, /* this object is in use. */
> >  	PCG_MIGRATION, /* under page migration */
> >  	/* flags for mem_cgroup and file and I/O status */
> 
> Me gusta.
> 
> > @@ -606,11 +606,16 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
> >  }
> >  
> >  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> > -					 bool file, int nr_pages)
> > +					 bool not_rss, int nr_pages)
> >  {
> >  	preempt_disable();
> >  
> > -	if (file)
> > +	/*
> > +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Unlike LRU,
> > +	 * Shmem is not included to Anon. It' counted as 'file cache'
> > +	 * which tends to be shared between memcgs.
> > +	 */
> > +	if (not_rss)
> 
> Could you invert that boolean and call it "anon"?
> 

Sure. I wondered whta name is good other than 'file'.


> > @@ -2343,6 +2348,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
> >  				       struct page_cgroup *pc,
> >  				       enum charge_type ctype)
> >  {
> > +	bool not_rss;
> > +
> >  	lock_page_cgroup(pc);
> >  	if (unlikely(PageCgroupUsed(pc))) {
> >  		unlock_page_cgroup(pc);
> > @@ -2362,21 +2369,15 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
> >  	 * See mem_cgroup_add_lru_list(), etc.
> >   	 */
> >  	smp_wmb();
> > -	switch (ctype) {
> > -	case MEM_CGROUP_CHARGE_TYPE_CACHE:
> > -	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
> > -		SetPageCgroupCache(pc);
> > -		SetPageCgroupUsed(pc);
> > -		break;
> > -	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
> > -		ClearPageCgroupCache(pc);
> > -		SetPageCgroupUsed(pc);
> > -		break;
> > -	default:
> > -		break;
> > -	}
> >  
> > -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
> > +	SetPageCgroupUsed(pc);
> > +	if ((ctype == MEM_CGROUP_CHARGE_TYPE_CACHE) ||
> > +	    (ctype == MEM_CGROUP_CHARGE_TYPE_SHMEM))
> > +		not_rss = true;
> > +	else
> > +		not_rss = false;
> > +
> > +	mem_cgroup_charge_statistics(memcg, not_rss, nr_pages);
> 
> 	mem_cgroup_charge_statistics(memcg,
> 				     ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED,
> 				     nr_pages);
> 
> and save even more lines, without sacrificing clarity! :)
> 
> > @@ -2908,9 +2915,15 @@ void mem_cgroup_uncharge_page(struct page *page)
> >  
> >  void mem_cgroup_uncharge_cache_page(struct page *page)
> >  {
> > +	int ctype;
> > +
> >  	VM_BUG_ON(page_mapped(page));
> >  	VM_BUG_ON(page->mapping);
> > -	__mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE);
> > +	if (page_is_file_cache(page))
> > +		ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> > +	else
> > +		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> > +	__mem_cgroup_uncharge_common(page, ctype);
> 
> Looks like an unrelated bugfix on one hand, but on the other hand we
> do not differentiate cache from shmem anywhere, afaik, and you do not
> introduce anything that does.  Could you just leave this out?
> 

Ok. I'll remove.
I inserted this just for clarifying what we do.
will post v3.

Thanks,
-Kame

Re: [PATCH] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Thu 19-01-12 18:17:11, KAMEZAWA Hiroyuki wrote:
> This patch is onto memcg-devel, can be applied to linux-next, too.

Just for record memcg-devel tree should _always_ be compatible with
linux-next. It just contains patches which are memcg related to be more
stable for memcg specific development.

> 
> ==
> From 529653c266b0682894d64e4797fcaf6a3c35db25 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.

> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'

The name is confusing.

Other than that the patch looks reasonable. Minor comment bellow:

>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  include/linux/page_cgroup.h |    8 +-----
>  mm/memcontrol.c             |   55 ++++++++++++++++++++++++++----------------
>  2 files changed, 35 insertions(+), 28 deletions(-)
> 
[...]
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index fb2dfc3..de7721d 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
[...]
> @@ -2908,9 +2915,15 @@ void mem_cgroup_uncharge_page(struct page *page)
>  
>  void mem_cgroup_uncharge_cache_page(struct page *page)
>  {
> +	int ctype;
> +
>  	VM_BUG_ON(page_mapped(page));
>  	VM_BUG_ON(page->mapping);
> -	__mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE);
> +	if (page_is_file_cache(page))
> +		ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> +	else
> +		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> +	__mem_cgroup_uncharge_common(page, ctype);

OK, this makes sense but doesn't make any real difference now, so it is
more a clean up, right?

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

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

Re: [PATCH] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Thu 19-01-12 18:17:11, KAMEZAWA Hiroyuki wrote:
> This patch is onto memcg-devel, can be applied to linux-next, too.

Just for record memcg-devel tree should _always_ be compatible with
linux-next. It just contains patches which are memcg related to be more
stable for memcg specific development.

> 
> ==
> From 529653c266b0682894d64e4797fcaf6a3c35db25 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.

> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'

The name is confusing.

Other than that the patch looks reasonable. Minor comment bellow:

>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  include/linux/page_cgroup.h |    8 +-----
>  mm/memcontrol.c             |   55 ++++++++++++++++++++++++++----------------
>  2 files changed, 35 insertions(+), 28 deletions(-)
> 
[...]
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index fb2dfc3..de7721d 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
[...]
> @@ -2908,9 +2915,15 @@ void mem_cgroup_uncharge_page(struct page *page)
>  
>  void mem_cgroup_uncharge_cache_page(struct page *page)
>  {
> +	int ctype;
> +
>  	VM_BUG_ON(page_mapped(page));
>  	VM_BUG_ON(page->mapping);
> -	__mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE);
> +	if (page_is_file_cache(page))
> +		ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> +	else
> +		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> +	__mem_cgroup_uncharge_common(page, ctype);

OK, this makes sense but doesn't make any real difference now, so it is
more a clean up, right?

Thanks
-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

On Thu, 19 Jan 2012 14:43:58 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Thu 19-01-12 18:17:11, KAMEZAWA Hiroyuki wrote:
> > This patch is onto memcg-devel, can be applied to linux-next, too.
> 
> Just for record memcg-devel tree should _always_ be compatible with
> linux-next. It just contains patches which are memcg related to be more
> stable for memcg specific development.
> 
> > 
> > ==
> > From 529653c266b0682894d64e4797fcaf6a3c35db25 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Date: Thu, 19 Jan 2012 17:09:41 +0900
> > Subject: [PATCH] memcg: remove PCG_CACHE
> > 
> > We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> > Here, "CACHE" means anonymous user pages (and SwapCache). This
> > doesn't include shmem.
> 
> > Consdering callers, at charge/uncharge, the caller should know
> > what  the page is and we don't need to record it by using 1bit
> > per page.
> > 
> > This patch removes PCG_CACHE bit and make callers of
> > mem_cgroup_charge_statistics() to specify what the page is.
> > 
> > Changelog since RFC.
> >  - rebased onto memcg-devel
> >  - rename 'file' to 'not_rss'
> 
> The name is confusing.
> 
> Other than that the patch looks reasonable. Minor comment bellow:
> 

will use 'anon'.


> >  - some cleanup and added comment.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  include/linux/page_cgroup.h |    8 +-----
> >  mm/memcontrol.c             |   55 ++++++++++++++++++++++++++----------------
> >  2 files changed, 35 insertions(+), 28 deletions(-)
> > 
> [...]
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index fb2dfc3..de7721d 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> [...]
> > @@ -2908,9 +2915,15 @@ void mem_cgroup_uncharge_page(struct page *page)
> >  
> >  void mem_cgroup_uncharge_cache_page(struct page *page)
> >  {
> > +	int ctype;
> > +
> >  	VM_BUG_ON(page_mapped(page));
> >  	VM_BUG_ON(page->mapping);
> > -	__mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE);
> > +	if (page_is_file_cache(page))
> > +		ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> > +	else
> > +		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> > +	__mem_cgroup_uncharge_common(page, ctype);
> 
> OK, this makes sense but doesn't make any real difference now, so it is
> more a clean up, right?
> 

Yes. Johannes reuqested to remove this. I'll remove this in v3.

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>

Re: [PATCH] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

On Thu, 19 Jan 2012 14:43:58 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Thu 19-01-12 18:17:11, KAMEZAWA Hiroyuki wrote:
> > This patch is onto memcg-devel, can be applied to linux-next, too.
> 
> Just for record memcg-devel tree should _always_ be compatible with
> linux-next. It just contains patches which are memcg related to be more
> stable for memcg specific development.
> 
> > 
> > ==
> > From 529653c266b0682894d64e4797fcaf6a3c35db25 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Date: Thu, 19 Jan 2012 17:09:41 +0900
> > Subject: [PATCH] memcg: remove PCG_CACHE
> > 
> > We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> > Here, "CACHE" means anonymous user pages (and SwapCache). This
> > doesn't include shmem.
> 
> > Consdering callers, at charge/uncharge, the caller should know
> > what  the page is and we don't need to record it by using 1bit
> > per page.
> > 
> > This patch removes PCG_CACHE bit and make callers of
> > mem_cgroup_charge_statistics() to specify what the page is.
> > 
> > Changelog since RFC.
> >  - rebased onto memcg-devel
> >  - rename 'file' to 'not_rss'
> 
> The name is confusing.
> 
> Other than that the patch looks reasonable. Minor comment bellow:
> 

will use 'anon'.


> >  - some cleanup and added comment.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > ---
> >  include/linux/page_cgroup.h |    8 +-----
> >  mm/memcontrol.c             |   55 ++++++++++++++++++++++++++----------------
> >  2 files changed, 35 insertions(+), 28 deletions(-)
> > 
> [...]
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index fb2dfc3..de7721d 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> [...]
> > @@ -2908,9 +2915,15 @@ void mem_cgroup_uncharge_page(struct page *page)
> >  
> >  void mem_cgroup_uncharge_cache_page(struct page *page)
> >  {
> > +	int ctype;
> > +
> >  	VM_BUG_ON(page_mapped(page));
> >  	VM_BUG_ON(page->mapping);
> > -	__mem_cgroup_uncharge_common(page, MEM_CGROUP_CHARGE_TYPE_CACHE);
> > +	if (page_is_file_cache(page))
> > +		ctype = MEM_CGROUP_CHARGE_TYPE_CACHE;
> > +	else
> > +		ctype = MEM_CGROUP_CHARGE_TYPE_SHMEM;
> > +	__mem_cgroup_uncharge_common(page, ctype);
> 
> OK, this makes sense but doesn't make any real difference now, so it is
> more a clean up, right?
> 

Yes. Johannes reuqested to remove this. I'll remove this in v3.

Thanks,
-Kame

[PATCH v3] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

I think this version is much simplified.

==

[PATCH v3] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

I think this version is much simplified.

==
>From 5700a4fe9c581e1ebaa021ba6119dc8d921b024f Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date: Thu, 19 Jan 2012 17:09:41 +0900
Subject: [PATCH v3] memcg: remove PCG_CACHE

We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
Here, "CACHE" means anonymous user pages (and SwapCache). This
doesn't include shmem.

Consdering callers, at charge/uncharge, the caller should know
what  the page is and we don't need to record it by using 1bit
per page.

This patch removes PCG_CACHE bit and make callers of
mem_cgroup_charge_statistics() to specify what the page is.

Changelog since v2
 - removed 'not_rss', added 'anon'
 - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
 - removed a patch to mem_cgroup_uncharge_cache
 - simplified comment.

Changelog since RFC.
 - rebased onto memcg-devel
 - rename 'file' to 'not_rss'
 - some cleanup and added comment.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 include/linux/page_cgroup.h |    8 +------
 mm/memcontrol.c             |   48 +++++++++++++++++++++++-------------------
 2 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
index a2d1177..1060292 100644
--- a/include/linux/page_cgroup.h
+++ b/include/linux/page_cgroup.h
@@ -4,7 +4,6 @@
 enum {
 	/* flags for mem_cgroup */
 	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
-	PCG_CACHE, /* charged as cache */
 	PCG_USED, /* this object is in use. */
 	PCG_MIGRATION, /* under page migration */
 	/* flags for mem_cgroup and file and I/O status */
@@ -64,11 +63,6 @@ static inline void ClearPageCgroup##uname(struct page_cgroup *pc)	\
 static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
 	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
 
-/* Cache flag is set only once (at allocation) */
-TESTPCGFLAG(Cache, CACHE)
-CLEARPCGFLAG(Cache, CACHE)
-SETPCGFLAG(Cache, CACHE)
-
 TESTPCGFLAG(Used, USED)
 CLEARPCGFLAG(Used, USED)
 SETPCGFLAG(Used, USED)
@@ -85,7 +79,7 @@ static inline void lock_page_cgroup(struct page_cgroup *pc)
 {
 	/*
 	 * Don't take this lock in IRQ context.
-	 * This lock is for pc->mem_cgroup, USED, CACHE, MIGRATION
+	 * This lock is for pc->mem_cgroup, USED, MIGRATION
 	 */
 	bit_spin_lock(PCG_LOCK, &pc->flags);
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index ff24520..f000c82 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -672,15 +672,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
 }
 
 static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
-					 bool file, int nr_pages)
+					 bool rss, int nr_pages)
 {
 	preempt_disable();
 
-	if (file)
-		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
+	/*
+	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
+	 * counted as CACHE even if it's on ANON LRU.
+	 */
+	if (rss)
+		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
 				nr_pages);
 	else
-		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
+		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
 				nr_pages);
 
 	/* pagein of a big page is an event. So, ignore page size */
@@ -2409,6 +2413,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 				       struct page_cgroup *pc,
 				       enum charge_type ctype)
 {
+	bool anon;
+
 	lock_page_cgroup(pc);
 	if (unlikely(PageCgroupUsed(pc))) {
 		unlock_page_cgroup(pc);
@@ -2428,21 +2434,14 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 	 * See mem_cgroup_add_lru_list(), etc.
  	 */
 	smp_wmb();
-	switch (ctype) {
-	case MEM_CGROUP_CHARGE_TYPE_CACHE:
-	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
-		SetPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
-		ClearPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	default:
-		break;
-	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
+	SetPageCgroupUsed(pc);
+	if (ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
+		anon = true;
+	else
+		anon = false;
+
+	mem_cgroup_charge_statistics(memcg, anon, nr_pages);
 	unlock_page_cgroup(pc);
 	WARN_ON_ONCE(PageLRU(page));
 	/*
@@ -2507,6 +2506,7 @@ static int mem_cgroup_move_account(struct page *page,
 {
 	unsigned long flags;
 	int ret;
+	bool anon = PageAnon(page);
 
 	VM_BUG_ON(from == to);
 	VM_BUG_ON(PageLRU(page));
@@ -2535,14 +2535,14 @@ static int mem_cgroup_move_account(struct page *page,
 		__this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
 		preempt_enable();
 	}
-	mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(from, anon, -nr_pages);
 	if (uncharge)
 		/* This is not "cancel", but cancel_charge does all we need. */
 		__mem_cgroup_cancel_charge(from, nr_pages);
 
 	/* caller should have done css_get */
 	pc->mem_cgroup = to;
-	mem_cgroup_charge_statistics(to, PageCgroupCache(pc), nr_pages);
+	mem_cgroup_charge_statistics(to, anon, nr_pages);
 	/*
 	 * We charges against "to" which may not have any tasks. Then, "to"
 	 * can be under rmdir(). But in current implementation, caller of
@@ -2890,6 +2890,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 	struct mem_cgroup *memcg = NULL;
 	unsigned int nr_pages = 1;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (mem_cgroup_disabled())
 		return NULL;
@@ -2921,6 +2922,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 		/* See mem_cgroup_prepare_migration() */
 		if (page_mapped(page) || PageCgroupMigration(pc))
 			goto unlock_out;
+		anon = true;
 		break;
 	case MEM_CGROUP_CHARGE_TYPE_SWAPOUT:
 		if (!PageAnon(page)) {	/* Shared memory */
@@ -2928,12 +2930,14 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 				goto unlock_out;
 		} else if (page_mapped(page)) /* Anon */
 				goto unlock_out;
+		anon = true;
 		break;
 	default:
+		anon = false;
 		break;
 	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(memcg, anon, -nr_pages);
 
 	ClearPageCgroupUsed(pc);
 	/*
@@ -3319,7 +3323,7 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,
 	/* fix accounting on old pages */
 	lock_page_cgroup(pc);
 	memcg = pc->mem_cgroup;
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -1);
+	mem_cgroup_charge_statistics(memcg, false, -1);
 	ClearPageCgroupUsed(pc);
 	unlock_page_cgroup(pc);
 
-- 
1.7.4.1

Re: [PATCH v3] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Fri 20-01-12 12:26:58, KAMEZAWA Hiroyuki wrote:
> I think this version is much simplified.
> 
> ==
> From 5700a4fe9c581e1ebaa021ba6119dc8d921b024f Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH v3] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  include/linux/page_cgroup.h |    8 +------
>  mm/memcontrol.c             |   48 +++++++++++++++++++++++-------------------
>  2 files changed, 27 insertions(+), 29 deletions(-)
> 
[...]
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ff24520..f000c82 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -672,15 +672,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
>  }
>  
>  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> -					 bool file, int nr_pages)
> +					 bool rss, int nr_pages)

Can we make this anon as well?
>  {
>  	preempt_disable();
>  
> -	if (file)
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
> +	/*
> +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
> +	 * counted as CACHE even if it's on ANON LRU.
> +	 */
> +	if (rss)
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
>  				nr_pages);
>  	else
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
>  				nr_pages);
>  
>  	/* pagein of a big page is an event. So, ignore page size */
[...]

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

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

Re: [PATCH v3] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Fri 20-01-12 12:26:58, KAMEZAWA Hiroyuki wrote:
> I think this version is much simplified.
> 
> ==
> From 5700a4fe9c581e1ebaa021ba6119dc8d921b024f Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH v3] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
>  include/linux/page_cgroup.h |    8 +------
>  mm/memcontrol.c             |   48 +++++++++++++++++++++++-------------------
>  2 files changed, 27 insertions(+), 29 deletions(-)
> 
[...]
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index ff24520..f000c82 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -672,15 +672,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
>  }
>  
>  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> -					 bool file, int nr_pages)
> +					 bool rss, int nr_pages)

Can we make this anon as well?
>  {
>  	preempt_disable();
>  
> -	if (file)
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
> +	/*
> +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
> +	 * counted as CACHE even if it's on ANON LRU.
> +	 */
> +	if (rss)
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
>  				nr_pages);
>  	else
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
>  				nr_pages);
>  
>  	/* pagein of a big page is an event. So, ignore page size */
[...]

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

[PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

> Can we make this anon as well?

I'm sorry for long RTT. version 4 here.
==

[PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

> Can we make this anon as well?

I'm sorry for long RTT. version 4 here.
==
>From c40256561d6cdaee62be7ec34147e6079dc426f4 Mon Sep 17 00:00:00 2001
From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
Date: Thu, 19 Jan 2012 17:09:41 +0900
Subject: [PATCH] memcg: remove PCG_CACHE

We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
Here, "CACHE" means anonymous user pages (and SwapCache). This
doesn't include shmem.

Consdering callers, at charge/uncharge, the caller should know
what  the page is and we don't need to record it by using 1bit
per page.

This patch removes PCG_CACHE bit and make callers of
mem_cgroup_charge_statistics() to specify what the page is.

Changelog since v3
 - renamed a variable 'rss' to 'anon'

Changelog since v2
 - removed 'not_rss', added 'anon'
 - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
 - removed a patch to mem_cgroup_uncharge_cache
 - simplified comment.

Changelog since RFC.
 - rebased onto memcg-devel
 - rename 'file' to 'not_rss'
 - some cleanup and added comment.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 include/linux/page_cgroup.h |    8 +------
 mm/memcontrol.c             |   48 +++++++++++++++++++++++-------------------
 2 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
index a2d1177..1060292 100644
--- a/include/linux/page_cgroup.h
+++ b/include/linux/page_cgroup.h
@@ -4,7 +4,6 @@
 enum {
 	/* flags for mem_cgroup */
 	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
-	PCG_CACHE, /* charged as cache */
 	PCG_USED, /* this object is in use. */
 	PCG_MIGRATION, /* under page migration */
 	/* flags for mem_cgroup and file and I/O status */
@@ -64,11 +63,6 @@ static inline void ClearPageCgroup##uname(struct page_cgroup *pc)	\
 static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
 	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
 
-/* Cache flag is set only once (at allocation) */
-TESTPCGFLAG(Cache, CACHE)
-CLEARPCGFLAG(Cache, CACHE)
-SETPCGFLAG(Cache, CACHE)
-
 TESTPCGFLAG(Used, USED)
 CLEARPCGFLAG(Used, USED)
 SETPCGFLAG(Used, USED)
@@ -85,7 +79,7 @@ static inline void lock_page_cgroup(struct page_cgroup *pc)
 {
 	/*
 	 * Don't take this lock in IRQ context.
-	 * This lock is for pc->mem_cgroup, USED, CACHE, MIGRATION
+	 * This lock is for pc->mem_cgroup, USED, MIGRATION
 	 */
 	bit_spin_lock(PCG_LOCK, &pc->flags);
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 1c56c5f..bc2541c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -670,15 +670,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
 }
 
 static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
-					 bool file, int nr_pages)
+					 bool anon, int nr_pages)
 {
 	preempt_disable();
 
-	if (file)
-		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
+	/*
+	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
+	 * counted as CACHE even if it's on ANON LRU.
+	 */
+	if (anon)
+		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
 				nr_pages);
 	else
-		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
+		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
 				nr_pages);
 
 	/* pagein of a big page is an event. So, ignore page size */
@@ -2405,6 +2409,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 				       struct page_cgroup *pc,
 				       enum charge_type ctype)
 {
+	bool anon;
+
 	lock_page_cgroup(pc);
 	if (unlikely(PageCgroupUsed(pc))) {
 		unlock_page_cgroup(pc);
@@ -2424,21 +2430,14 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 	 * See mem_cgroup_add_lru_list(), etc.
  	 */
 	smp_wmb();
-	switch (ctype) {
-	case MEM_CGROUP_CHARGE_TYPE_CACHE:
-	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
-		SetPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
-		ClearPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	default:
-		break;
-	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
+	SetPageCgroupUsed(pc);
+	if (ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
+		anon = true;
+	else
+		anon = false;
+
+	mem_cgroup_charge_statistics(memcg, anon, nr_pages);
 	unlock_page_cgroup(pc);
 	WARN_ON_ONCE(PageLRU(page));
 	/*
@@ -2503,6 +2502,7 @@ static int mem_cgroup_move_account(struct page *page,
 {
 	unsigned long flags;
 	int ret;
+	bool anon = PageAnon(page);
 
 	VM_BUG_ON(from == to);
 	VM_BUG_ON(PageLRU(page));
@@ -2531,14 +2531,14 @@ static int mem_cgroup_move_account(struct page *page,
 		__this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
 		preempt_enable();
 	}
-	mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(from, anon, -nr_pages);
 	if (uncharge)
 		/* This is not "cancel", but cancel_charge does all we need. */
 		__mem_cgroup_cancel_charge(from, nr_pages);
 
 	/* caller should have done css_get */
 	pc->mem_cgroup = to;
-	mem_cgroup_charge_statistics(to, PageCgroupCache(pc), nr_pages);
+	mem_cgroup_charge_statistics(to, anon, nr_pages);
 	/*
 	 * We charges against "to" which may not have any tasks. Then, "to"
 	 * can be under rmdir(). But in current implementation, caller of
@@ -2884,6 +2884,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 	struct mem_cgroup *memcg = NULL;
 	unsigned int nr_pages = 1;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (mem_cgroup_disabled())
 		return NULL;
@@ -2915,6 +2916,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 		/* See mem_cgroup_prepare_migration() */
 		if (page_mapped(page) || PageCgroupMigration(pc))
 			goto unlock_out;
+		anon = true;
 		break;
 	case MEM_CGROUP_CHARGE_TYPE_SWAPOUT:
 		if (!PageAnon(page)) {	/* Shared memory */
@@ -2922,12 +2924,14 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 				goto unlock_out;
 		} else if (page_mapped(page)) /* Anon */
 				goto unlock_out;
+		anon = true;
 		break;
 	default:
+		anon = false;
 		break;
 	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(memcg, anon, -nr_pages);
 
 	ClearPageCgroupUsed(pc);
 	/*
@@ -3313,7 +3317,7 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,
 	/* fix accounting on old pages */
 	lock_page_cgroup(pc);
 	memcg = pc->mem_cgroup;
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -1);
+	mem_cgroup_charge_statistics(memcg, false, -1);
 	ClearPageCgroupUsed(pc);
 	unlock_page_cgroup(pc);
 
-- 
1.7.4.1

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Tue 24-01-12 12:16:36, KAMEZAWA Hiroyuki wrote:
> 
> > Can we make this anon as well?
> 
> I'm sorry for long RTT. version 4 here.
> ==
> From c40256561d6cdaee62be7ec34147e6079dc426f4 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since v3
>  - renamed a variable 'rss' to 'anon'
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks!

> ---
>  include/linux/page_cgroup.h |    8 +------
>  mm/memcontrol.c             |   48 +++++++++++++++++++++++-------------------
>  2 files changed, 27 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
> index a2d1177..1060292 100644
> --- a/include/linux/page_cgroup.h
> +++ b/include/linux/page_cgroup.h
> @@ -4,7 +4,6 @@
>  enum {
>  	/* flags for mem_cgroup */
>  	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
> -	PCG_CACHE, /* charged as cache */
>  	PCG_USED, /* this object is in use. */
>  	PCG_MIGRATION, /* under page migration */
>  	/* flags for mem_cgroup and file and I/O status */
> @@ -64,11 +63,6 @@ static inline void ClearPageCgroup##uname(struct page_cgroup *pc)	\
>  static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
>  	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
>  
> -/* Cache flag is set only once (at allocation) */
> -TESTPCGFLAG(Cache, CACHE)
> -CLEARPCGFLAG(Cache, CACHE)
> -SETPCGFLAG(Cache, CACHE)
> -
>  TESTPCGFLAG(Used, USED)
>  CLEARPCGFLAG(Used, USED)
>  SETPCGFLAG(Used, USED)
> @@ -85,7 +79,7 @@ static inline void lock_page_cgroup(struct page_cgroup *pc)
>  {
>  	/*
>  	 * Don't take this lock in IRQ context.
> -	 * This lock is for pc->mem_cgroup, USED, CACHE, MIGRATION
> +	 * This lock is for pc->mem_cgroup, USED, MIGRATION
>  	 */
>  	bit_spin_lock(PCG_LOCK, &pc->flags);
>  }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1c56c5f..bc2541c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -670,15 +670,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
>  }
>  
>  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> -					 bool file, int nr_pages)
> +					 bool anon, int nr_pages)
>  {
>  	preempt_disable();
>  
> -	if (file)
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
> +	/*
> +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
> +	 * counted as CACHE even if it's on ANON LRU.
> +	 */
> +	if (anon)
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
>  				nr_pages);
>  	else
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
>  				nr_pages);
>  
>  	/* pagein of a big page is an event. So, ignore page size */
> @@ -2405,6 +2409,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  				       struct page_cgroup *pc,
>  				       enum charge_type ctype)
>  {
> +	bool anon;
> +
>  	lock_page_cgroup(pc);
>  	if (unlikely(PageCgroupUsed(pc))) {
>  		unlock_page_cgroup(pc);
> @@ -2424,21 +2430,14 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  	 * See mem_cgroup_add_lru_list(), etc.
>   	 */
>  	smp_wmb();
> -	switch (ctype) {
> -	case MEM_CGROUP_CHARGE_TYPE_CACHE:
> -	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
> -		SetPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
> -		ClearPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	default:
> -		break;
> -	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
> +	SetPageCgroupUsed(pc);
> +	if (ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
> +		anon = true;
> +	else
> +		anon = false;
> +
> +	mem_cgroup_charge_statistics(memcg, anon, nr_pages);
>  	unlock_page_cgroup(pc);
>  	WARN_ON_ONCE(PageLRU(page));
>  	/*
> @@ -2503,6 +2502,7 @@ static int mem_cgroup_move_account(struct page *page,
>  {
>  	unsigned long flags;
>  	int ret;
> +	bool anon = PageAnon(page);
>  
>  	VM_BUG_ON(from == to);
>  	VM_BUG_ON(PageLRU(page));
> @@ -2531,14 +2531,14 @@ static int mem_cgroup_move_account(struct page *page,
>  		__this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
>  		preempt_enable();
>  	}
> -	mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
> +	mem_cgroup_charge_statistics(from, anon, -nr_pages);
>  	if (uncharge)
>  		/* This is not "cancel", but cancel_charge does all we need. */
>  		__mem_cgroup_cancel_charge(from, nr_pages);
>  
>  	/* caller should have done css_get */
>  	pc->mem_cgroup = to;
> -	mem_cgroup_charge_statistics(to, PageCgroupCache(pc), nr_pages);
> +	mem_cgroup_charge_statistics(to, anon, nr_pages);
>  	/*
>  	 * We charges against "to" which may not have any tasks. Then, "to"
>  	 * can be under rmdir(). But in current implementation, caller of
> @@ -2884,6 +2884,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  	struct mem_cgroup *memcg = NULL;
>  	unsigned int nr_pages = 1;
>  	struct page_cgroup *pc;
> +	bool anon;
>  
>  	if (mem_cgroup_disabled())
>  		return NULL;
> @@ -2915,6 +2916,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  		/* See mem_cgroup_prepare_migration() */
>  		if (page_mapped(page) || PageCgroupMigration(pc))
>  			goto unlock_out;
> +		anon = true;
>  		break;
>  	case MEM_CGROUP_CHARGE_TYPE_SWAPOUT:
>  		if (!PageAnon(page)) {	/* Shared memory */
> @@ -2922,12 +2924,14 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  				goto unlock_out;
>  		} else if (page_mapped(page)) /* Anon */
>  				goto unlock_out;
> +		anon = true;
>  		break;
>  	default:
> +		anon = false;
>  		break;
>  	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -nr_pages);
> +	mem_cgroup_charge_statistics(memcg, anon, -nr_pages);
>  
>  	ClearPageCgroupUsed(pc);
>  	/*
> @@ -3313,7 +3317,7 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,
>  	/* fix accounting on old pages */
>  	lock_page_cgroup(pc);
>  	memcg = pc->mem_cgroup;
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -1);
> +	mem_cgroup_charge_statistics(memcg, false, -1);
>  	ClearPageCgroupUsed(pc);
>  	unlock_page_cgroup(pc);
>  
> -- 
> 1.7.4.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

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

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Tue 24-01-12 12:16:36, KAMEZAWA Hiroyuki wrote:
> 
> > Can we make this anon as well?
> 
> I'm sorry for long RTT. version 4 here.
> ==
> From c40256561d6cdaee62be7ec34147e6079dc426f4 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since v3
>  - renamed a variable 'rss' to 'anon'
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Michal Hocko <mhocko@suse.cz>

Thanks!

> ---
>  include/linux/page_cgroup.h |    8 +------
>  mm/memcontrol.c             |   48 +++++++++++++++++++++++-------------------
>  2 files changed, 27 insertions(+), 29 deletions(-)
> 
> diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
> index a2d1177..1060292 100644
> --- a/include/linux/page_cgroup.h
> +++ b/include/linux/page_cgroup.h
> @@ -4,7 +4,6 @@
>  enum {
>  	/* flags for mem_cgroup */
>  	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
> -	PCG_CACHE, /* charged as cache */
>  	PCG_USED, /* this object is in use. */
>  	PCG_MIGRATION, /* under page migration */
>  	/* flags for mem_cgroup and file and I/O status */
> @@ -64,11 +63,6 @@ static inline void ClearPageCgroup##uname(struct page_cgroup *pc)	\
>  static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
>  	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
>  
> -/* Cache flag is set only once (at allocation) */
> -TESTPCGFLAG(Cache, CACHE)
> -CLEARPCGFLAG(Cache, CACHE)
> -SETPCGFLAG(Cache, CACHE)
> -
>  TESTPCGFLAG(Used, USED)
>  CLEARPCGFLAG(Used, USED)
>  SETPCGFLAG(Used, USED)
> @@ -85,7 +79,7 @@ static inline void lock_page_cgroup(struct page_cgroup *pc)
>  {
>  	/*
>  	 * Don't take this lock in IRQ context.
> -	 * This lock is for pc->mem_cgroup, USED, CACHE, MIGRATION
> +	 * This lock is for pc->mem_cgroup, USED, MIGRATION
>  	 */
>  	bit_spin_lock(PCG_LOCK, &pc->flags);
>  }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1c56c5f..bc2541c 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -670,15 +670,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
>  }
>  
>  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> -					 bool file, int nr_pages)
> +					 bool anon, int nr_pages)
>  {
>  	preempt_disable();
>  
> -	if (file)
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
> +	/*
> +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
> +	 * counted as CACHE even if it's on ANON LRU.
> +	 */
> +	if (anon)
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
>  				nr_pages);
>  	else
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
>  				nr_pages);
>  
>  	/* pagein of a big page is an event. So, ignore page size */
> @@ -2405,6 +2409,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  				       struct page_cgroup *pc,
>  				       enum charge_type ctype)
>  {
> +	bool anon;
> +
>  	lock_page_cgroup(pc);
>  	if (unlikely(PageCgroupUsed(pc))) {
>  		unlock_page_cgroup(pc);
> @@ -2424,21 +2430,14 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  	 * See mem_cgroup_add_lru_list(), etc.
>   	 */
>  	smp_wmb();
> -	switch (ctype) {
> -	case MEM_CGROUP_CHARGE_TYPE_CACHE:
> -	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
> -		SetPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
> -		ClearPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	default:
> -		break;
> -	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
> +	SetPageCgroupUsed(pc);
> +	if (ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
> +		anon = true;
> +	else
> +		anon = false;
> +
> +	mem_cgroup_charge_statistics(memcg, anon, nr_pages);
>  	unlock_page_cgroup(pc);
>  	WARN_ON_ONCE(PageLRU(page));
>  	/*
> @@ -2503,6 +2502,7 @@ static int mem_cgroup_move_account(struct page *page,
>  {
>  	unsigned long flags;
>  	int ret;
> +	bool anon = PageAnon(page);
>  
>  	VM_BUG_ON(from == to);
>  	VM_BUG_ON(PageLRU(page));
> @@ -2531,14 +2531,14 @@ static int mem_cgroup_move_account(struct page *page,
>  		__this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
>  		preempt_enable();
>  	}
> -	mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
> +	mem_cgroup_charge_statistics(from, anon, -nr_pages);
>  	if (uncharge)
>  		/* This is not "cancel", but cancel_charge does all we need. */
>  		__mem_cgroup_cancel_charge(from, nr_pages);
>  
>  	/* caller should have done css_get */
>  	pc->mem_cgroup = to;
> -	mem_cgroup_charge_statistics(to, PageCgroupCache(pc), nr_pages);
> +	mem_cgroup_charge_statistics(to, anon, nr_pages);
>  	/*
>  	 * We charges against "to" which may not have any tasks. Then, "to"
>  	 * can be under rmdir(). But in current implementation, caller of
> @@ -2884,6 +2884,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  	struct mem_cgroup *memcg = NULL;
>  	unsigned int nr_pages = 1;
>  	struct page_cgroup *pc;
> +	bool anon;
>  
>  	if (mem_cgroup_disabled())
>  		return NULL;
> @@ -2915,6 +2916,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  		/* See mem_cgroup_prepare_migration() */
>  		if (page_mapped(page) || PageCgroupMigration(pc))
>  			goto unlock_out;
> +		anon = true;
>  		break;
>  	case MEM_CGROUP_CHARGE_TYPE_SWAPOUT:
>  		if (!PageAnon(page)) {	/* Shared memory */
> @@ -2922,12 +2924,14 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  				goto unlock_out;
>  		} else if (page_mapped(page)) /* Anon */
>  				goto unlock_out;
> +		anon = true;
>  		break;
>  	default:
> +		anon = false;
>  		break;
>  	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -nr_pages);
> +	mem_cgroup_charge_statistics(memcg, anon, -nr_pages);
>  
>  	ClearPageCgroupUsed(pc);
>  	/*
> @@ -3313,7 +3317,7 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,
>  	/* fix accounting on old pages */
>  	lock_page_cgroup(pc);
>  	memcg = pc->mem_cgroup;
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -1);
> +	mem_cgroup_charge_statistics(memcg, false, -1);
>  	ClearPageCgroupUsed(pc);
>  	unlock_page_cgroup(pc);
>  
> -- 
> 1.7.4.1
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Tue, Jan 24, 2012 at 12:16:36PM +0900, KAMEZAWA Hiroyuki wrote:
> 
> > Can we make this anon as well?
> 
> I'm sorry for long RTT. version 4 here.
> ==
> >From c40256561d6cdaee62be7ec34147e6079dc426f4 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.

!CACHE means anonymous/swapcache

> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since v3
>  - renamed a variable 'rss' to 'anon'
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

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

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Tue, Jan 24, 2012 at 12:16:36PM +0900, KAMEZAWA Hiroyuki wrote:
> 
> > Can we make this anon as well?
> 
> I'm sorry for long RTT. version 4 here.
> ==
> >From c40256561d6cdaee62be7ec34147e6079dc426f4 Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.

!CACHE means anonymous/swapcache

> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since v3
>  - renamed a variable 'rss' to 'anon'
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Tue, Jan 24, 2012 at 12:16:44PM +0100, Johannes Weiner wrote:
> On Tue, Jan 24, 2012 at 12:16:36PM +0900, KAMEZAWA Hiroyuki wrote:
> > 
> > > Can we make this anon as well?
> > 
> > I'm sorry for long RTT. version 4 here.
> > ==
> > >From c40256561d6cdaee62be7ec34147e6079dc426f4 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Date: Thu, 19 Jan 2012 17:09:41 +0900
> > Subject: [PATCH] memcg: remove PCG_CACHE
> > 
> > We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> > Here, "CACHE" means anonymous user pages (and SwapCache). This
> > doesn't include shmem.
> 
> !CACHE means anonymous/swapcache
> 
> > Consdering callers, at charge/uncharge, the caller should know
> > what  the page is and we don't need to record it by using 1bit
> > per page.
> > 
> > This patch removes PCG_CACHE bit and make callers of
> > mem_cgroup_charge_statistics() to specify what the page is.
> > 
> > Changelog since v3
> >  - renamed a variable 'rss' to 'anon'
> > 
> > Changelog since v2
> >  - removed 'not_rss', added 'anon'
> >  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
> >  - removed a patch to mem_cgroup_uncharge_cache
> >  - simplified comment.
> > 
> > Changelog since RFC.
> >  - rebased onto memcg-devel
> >  - rename 'file' to 'not_rss'
> >  - some cleanup and added comment.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Hold on, I think this patch is still not complete: end_migration()
directly uses __mem_cgroup_uncharge_common() with the FORCE charge
type.  This will uncharge all migrated anon pages as cache, when it
should decide based on PageAnon(used), which is the page where
->mapping is intact after migration.

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

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Tue, Jan 24, 2012 at 12:16:44PM +0100, Johannes Weiner wrote:
> On Tue, Jan 24, 2012 at 12:16:36PM +0900, KAMEZAWA Hiroyuki wrote:
> > 
> > > Can we make this anon as well?
> > 
> > I'm sorry for long RTT. version 4 here.
> > ==
> > >From c40256561d6cdaee62be7ec34147e6079dc426f4 Mon Sep 17 00:00:00 2001
> > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > Date: Thu, 19 Jan 2012 17:09:41 +0900
> > Subject: [PATCH] memcg: remove PCG_CACHE
> > 
> > We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> > Here, "CACHE" means anonymous user pages (and SwapCache). This
> > doesn't include shmem.
> 
> !CACHE means anonymous/swapcache
> 
> > Consdering callers, at charge/uncharge, the caller should know
> > what  the page is and we don't need to record it by using 1bit
> > per page.
> > 
> > This patch removes PCG_CACHE bit and make callers of
> > mem_cgroup_charge_statistics() to specify what the page is.
> > 
> > Changelog since v3
> >  - renamed a variable 'rss' to 'anon'
> > 
> > Changelog since v2
> >  - removed 'not_rss', added 'anon'
> >  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
> >  - removed a patch to mem_cgroup_uncharge_cache
> >  - simplified comment.
> > 
> > Changelog since RFC.
> >  - rebased onto memcg-devel
> >  - rename 'file' to 'not_rss'
> >  - some cleanup and added comment.
> > 
> > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Hold on, I think this patch is still not complete: end_migration()
directly uses __mem_cgroup_uncharge_common() with the FORCE charge
type.  This will uncharge all migrated anon pages as cache, when it
should decide based on PageAnon(used), which is the page where
->mapping is intact after migration.

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> On Tue, Jan 24, 2012 at 12:16:44PM +0100, Johannes Weiner wrote:
> > On Tue, Jan 24, 2012 at 12:16:36PM +0900, KAMEZAWA Hiroyuki wrote:
> > > 
> > > > Can we make this anon as well?
> > > 
> > > I'm sorry for long RTT. version 4 here.
> > > ==
> > > >From c40256561d6cdaee62be7ec34147e6079dc426f4 Mon Sep 17 00:00:00 2001
> > > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > > Date: Thu, 19 Jan 2012 17:09:41 +0900
> > > Subject: [PATCH] memcg: remove PCG_CACHE
> > > 
> > > We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> > > Here, "CACHE" means anonymous user pages (and SwapCache). This
> > > doesn't include shmem.
> > 
> > !CACHE means anonymous/swapcache
> > 
> > > Consdering callers, at charge/uncharge, the caller should know
> > > what  the page is and we don't need to record it by using 1bit
> > > per page.
> > > 
> > > This patch removes PCG_CACHE bit and make callers of
> > > mem_cgroup_charge_statistics() to specify what the page is.
> > > 
> > > Changelog since v3
> > >  - renamed a variable 'rss' to 'anon'
> > > 
> > > Changelog since v2
> > >  - removed 'not_rss', added 'anon'
> > >  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
> > >  - removed a patch to mem_cgroup_uncharge_cache
> > >  - simplified comment.
> > > 
> > > Changelog since RFC.
> > >  - rebased onto memcg-devel
> > >  - rename 'file' to 'not_rss'
> > >  - some cleanup and added comment.
> > > 
> > > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> Hold on, I think this patch is still not complete: end_migration()
> directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> type.  This will uncharge all migrated anon pages as cache, when it
> should decide based on PageAnon(used), which is the page where
> ->mapping is intact after migration.

You are right, I've missed that one as well. Anyway
MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
these days and it got out of sync with its documentation (used by
force_empty) quite some time ago (f817ed48). What about something like
the following on top of the previous patch?
--- 
Should be foldet into the previous patch with the updated changelog:

Mapping of the unused page is not touched during migration (see
page_remove_rmap) so we can rely on it and push the correct charge type
down to __mem_cgroup_uncharge_common from end_migration. The force flag
was misleading anyway.

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4d655ee..c541551 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3217,7 +3217,9 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	ClearPageCgroupMigration(pc);
 	unlock_page_cgroup(pc);
 
-	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
+	__mem_cgroup_uncharge_common(unused,
+			PageAnon(unused) ? MEM_CGROUP_CHARGE_TYPE_MAPPED
+			: MEM_CGROUP_CHARGE_TYPE_CACHE);
 
 	/*
 	 * If a page is a file cache, radix-tree replacement is very atomic

And then we can get rid of the FORCE as well.
---

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> On Tue, Jan 24, 2012 at 12:16:44PM +0100, Johannes Weiner wrote:
> > On Tue, Jan 24, 2012 at 12:16:36PM +0900, KAMEZAWA Hiroyuki wrote:
> > > 
> > > > Can we make this anon as well?
> > > 
> > > I'm sorry for long RTT. version 4 here.
> > > ==
> > > >From c40256561d6cdaee62be7ec34147e6079dc426f4 Mon Sep 17 00:00:00 2001
> > > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > > Date: Thu, 19 Jan 2012 17:09:41 +0900
> > > Subject: [PATCH] memcg: remove PCG_CACHE
> > > 
> > > We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> > > Here, "CACHE" means anonymous user pages (and SwapCache). This
> > > doesn't include shmem.
> > 
> > !CACHE means anonymous/swapcache
> > 
> > > Consdering callers, at charge/uncharge, the caller should know
> > > what  the page is and we don't need to record it by using 1bit
> > > per page.
> > > 
> > > This patch removes PCG_CACHE bit and make callers of
> > > mem_cgroup_charge_statistics() to specify what the page is.
> > > 
> > > Changelog since v3
> > >  - renamed a variable 'rss' to 'anon'
> > > 
> > > Changelog since v2
> > >  - removed 'not_rss', added 'anon'
> > >  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
> > >  - removed a patch to mem_cgroup_uncharge_cache
> > >  - simplified comment.
> > > 
> > > Changelog since RFC.
> > >  - rebased onto memcg-devel
> > >  - rename 'file' to 'not_rss'
> > >  - some cleanup and added comment.
> > > 
> > > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> > 
> > Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> 
> Hold on, I think this patch is still not complete: end_migration()
> directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> type.  This will uncharge all migrated anon pages as cache, when it
> should decide based on PageAnon(used), which is the page where
> ->mapping is intact after migration.

You are right, I've missed that one as well. Anyway
MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
these days and it got out of sync with its documentation (used by
force_empty) quite some time ago (f817ed48). What about something like
the following on top of the previous patch?
--- 
Should be foldet into the previous patch with the updated changelog:

Mapping of the unused page is not touched during migration (see
page_remove_rmap) so we can rely on it and push the correct charge type
down to __mem_cgroup_uncharge_common from end_migration. The force flag
was misleading anyway.

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4d655ee..c541551 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3217,7 +3217,9 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	ClearPageCgroupMigration(pc);
 	unlock_page_cgroup(pc);
 
-	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
+	__mem_cgroup_uncharge_common(unused,
+			PageAnon(unused) ? MEM_CGROUP_CHARGE_TYPE_MAPPED
+			: MEM_CGROUP_CHARGE_TYPE_CACHE);
 
 	/*
 	 * If a page is a file cache, radix-tree replacement is very atomic

And then we can get rid of the FORCE as well.
---
>From 1a3bc42e7608f33e99b1cc4a689e37ec608a4ee8 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.cz>
Date: Tue, 24 Jan 2012 16:56:37 +0100
Subject: [PATCH] memcg: Get rid of MEM_CGROUP_CHARGE_TYPE_FORCE charge flag

The flag was originaly intended for force empty code path but we are not
using it from there since f817ed48: memcg: move all acccounting to
parent at rmdir().

Later we have abused the flag for end_migration code path by ac39cf8c:
memcg: fix mis-accounting of file mapped racy with migration. This is
not the case anymore so let's get rid of it finally.

Signed-off-by: Michal Hocko <mhocko@suse.cz>
---
 mm/memcontrol.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index c541551..3585ed9 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -338,7 +338,6 @@ enum charge_type {
 	MEM_CGROUP_CHARGE_TYPE_CACHE = 0,
 	MEM_CGROUP_CHARGE_TYPE_MAPPED,
 	MEM_CGROUP_CHARGE_TYPE_SHMEM,	/* used by page migration of shmem */
-	MEM_CGROUP_CHARGE_TYPE_FORCE,	/* used by force_empty */
 	MEM_CGROUP_CHARGE_TYPE_SWAPOUT,	/* for accounting swapcache */
 	MEM_CGROUP_CHARGE_TYPE_DROP,	/* a page was unused swap cache */
 	NR_CHARGE_TYPE,
-- 
1.7.8.3



-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > Hold on, I think this patch is still not complete: end_migration()
> > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > type.  This will uncharge all migrated anon pages as cache, when it
> > should decide based on PageAnon(used), which is the page where
> > ->mapping is intact after migration.
> 
> You are right, I've missed that one as well. Anyway
> MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> these days and it got out of sync with its documentation (used by
> force_empty) quite some time ago (f817ed48). What about something like
> the following on top of the previous patch?
> --- 
> Should be foldet into the previous patch with the updated changelog:
> 
> Mapping of the unused page is not touched during migration (see

used one, not unused.  unused->mapping is globbered during migration.

> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration. The force flag
> was misleading anyway.

Kinda.  It had the effect of skipping the needless page_mapped() /
PageCgroupMigration() check, we know the unused page is no longer
mapped and cleared the migration flag just a few lines up.  But doing
the checks is no biggie and it's not worth adding another flag just to
skip them.  But I guess this should be mentioned in the changelog.

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

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > Hold on, I think this patch is still not complete: end_migration()
> > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > type.  This will uncharge all migrated anon pages as cache, when it
> > should decide based on PageAnon(used), which is the page where
> > ->mapping is intact after migration.
> 
> You are right, I've missed that one as well. Anyway
> MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> these days and it got out of sync with its documentation (used by
> force_empty) quite some time ago (f817ed48). What about something like
> the following on top of the previous patch?
> --- 
> Should be foldet into the previous patch with the updated changelog:
> 
> Mapping of the unused page is not touched during migration (see

used one, not unused.  unused->mapping is globbered during migration.

> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration. The force flag
> was misleading anyway.

Kinda.  It had the effect of skipping the needless page_mapped() /
PageCgroupMigration() check, we know the unused page is no longer
mapped and cleared the migration flag just a few lines up.  But doing
the checks is no biggie and it's not worth adding another flag just to
skip them.  But I guess this should be mentioned in the changelog.

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Tue 24-01-12 17:44:49, Johannes Weiner wrote:
> On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> > On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > > Hold on, I think this patch is still not complete: end_migration()
> > > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > > type.  This will uncharge all migrated anon pages as cache, when it
> > > should decide based on PageAnon(used), which is the page where
> > > ->mapping is intact after migration.
> > 
> > You are right, I've missed that one as well. Anyway
> > MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> > these days and it got out of sync with its documentation (used by
> > force_empty) quite some time ago (f817ed48). What about something like
> > the following on top of the previous patch?
> > --- 
> > Should be foldet into the previous patch with the updated changelog:
> > 
> > Mapping of the unused page is not touched during migration (see
> 
> used one, not unused.  unused->mapping is globbered during migration.

Yes, you are right:
---
Should be foldet into the previous patch with the updated changelog:

Mapping of the unused page is not touched during migration (see
page_remove_rmap) so we can rely on it and push the correct charge type
down to __mem_cgroup_uncharge_common from end_migration.
The force flag was misleading was abused for skipping the needless
page_mapped() / PageCgroupMigration() check, as we know the unused page
is no longer mapped and cleared the migration flag just a few lines
up.  But doing the checks is no biggie and it's not worth adding another
flag just to skip them.  But I guess this should be mentioned in the
changelog.

[hannes@cmpxchg.org: fix for end_migration with clarification]
---
 mm/memcontrol.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4d655ee..6a8cc56 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3195,6 +3195,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (!memcg)
 		return;
@@ -3207,6 +3208,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 		used = newpage;
 		unused = oldpage;
 	}
+
 	/*
 	 * We disallowed uncharge of pages under migration because mapcount
 	 * of the page goes down to zero, temporarly.
@@ -3217,7 +3219,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	ClearPageCgroupMigration(pc);
 	unlock_page_cgroup(pc);
 
-	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
+	anon = PageAnon(used);
+	__mem_cgroup_uncharge_common(unused,
+			anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
+			: MEM_CGROUP_CHARGE_TYPE_CACHE);
 
 	/*
 	 * If a page is a file cache, radix-tree replacement is very atomic
-- 
1.7.8.3


-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

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

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Tue 24-01-12 17:44:49, Johannes Weiner wrote:
> On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> > On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > > Hold on, I think this patch is still not complete: end_migration()
> > > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > > type.  This will uncharge all migrated anon pages as cache, when it
> > > should decide based on PageAnon(used), which is the page where
> > > ->mapping is intact after migration.
> > 
> > You are right, I've missed that one as well. Anyway
> > MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> > these days and it got out of sync with its documentation (used by
> > force_empty) quite some time ago (f817ed48). What about something like
> > the following on top of the previous patch?
> > --- 
> > Should be foldet into the previous patch with the updated changelog:
> > 
> > Mapping of the unused page is not touched during migration (see
> 
> used one, not unused.  unused->mapping is globbered during migration.

Yes, you are right:
---
Should be foldet into the previous patch with the updated changelog:

Mapping of the unused page is not touched during migration (see
page_remove_rmap) so we can rely on it and push the correct charge type
down to __mem_cgroup_uncharge_common from end_migration.
The force flag was misleading was abused for skipping the needless
page_mapped() / PageCgroupMigration() check, as we know the unused page
is no longer mapped and cleared the migration flag just a few lines
up.  But doing the checks is no biggie and it's not worth adding another
flag just to skip them.  But I guess this should be mentioned in the
changelog.

[hannes@cmpxchg.org: fix for end_migration with clarification]
---
 mm/memcontrol.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4d655ee..6a8cc56 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3195,6 +3195,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (!memcg)
 		return;
@@ -3207,6 +3208,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 		used = newpage;
 		unused = oldpage;
 	}
+
 	/*
 	 * We disallowed uncharge of pages under migration because mapcount
 	 * of the page goes down to zero, temporarly.
@@ -3217,7 +3219,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	ClearPageCgroupMigration(pc);
 	unlock_page_cgroup(pc);
 
-	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
+	anon = PageAnon(used);
+	__mem_cgroup_uncharge_common(unused,
+			anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
+			: MEM_CGROUP_CHARGE_TYPE_CACHE);
 
 	/*
 	 * If a page is a file cache, radix-tree replacement is very atomic
-- 
1.7.8.3


-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Tue 24-01-12 18:23:08, Michal Hocko wrote:
> On Tue 24-01-12 17:44:49, Johannes Weiner wrote:
> > On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> > > On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > > > Hold on, I think this patch is still not complete: end_migration()
> > > > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > > > type.  This will uncharge all migrated anon pages as cache, when it
> > > > should decide based on PageAnon(used), which is the page where
> > > > ->mapping is intact after migration.
> > > 
> > > You are right, I've missed that one as well. Anyway
> > > MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> > > these days and it got out of sync with its documentation (used by
> > > force_empty) quite some time ago (f817ed48). What about something like
> > > the following on top of the previous patch?
> > > --- 
> > > Should be foldet into the previous patch with the updated changelog:
> > > 
> > > Mapping of the unused page is not touched during migration (see
> > 
> > used one, not unused.  unused->mapping is globbered during migration.
> 
> Yes, you are right:

Sorry I haven't sent the most recent update. Here we go:
---
Should be folded into the previous patch with the updated changelog:

Mapping of the used page is not touched during migration (see
page_remove_rmap) so we can rely on it and push the correct charge type
down to __mem_cgroup_uncharge_common from end_migration for unused page.
The force flag was misleading was abused for skipping the needless
page_mapped() / PageCgroupMigration() check, as we know the unused page
is no longer mapped and cleared the migration flag just a few lines
up.  But doing the checks is no biggie and it's not worth adding another
flag just to skip them.  But I guess this should be mentioned in the
changelog.
---
 mm/memcontrol.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4d655ee..869744a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3195,6 +3195,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (!memcg)
 		return;
@@ -3207,6 +3208,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 		used = newpage;
 		unused = oldpage;
 	}
+
 	/*
 	 * We disallowed uncharge of pages under migration because mapcount
 	 * of the page goes down to zero, temporarly.
@@ -3217,7 +3219,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	ClearPageCgroupMigration(pc);
 	unlock_page_cgroup(pc);
 
-	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
+	anon = PageAnon(used);
+	__mem_cgroup_uncharge_common(unused,
+			anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
+			: MEM_CGROUP_CHARGE_TYPE_CACHE);
 
 	/*
 	 * If a page is a file cache, radix-tree replacement is very atomic
@@ -3227,7 +3232,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	 * and USED bit check in mem_cgroup_uncharge_page() will do enough
 	 * check. (see prepare_charge() also)
 	 */
-	if (PageAnon(used))
+	if (anon)
 		mem_cgroup_uncharge_page(used);
 	/*
 	 * At migration, we may charge account against cgroup which has no
-- 
1.7.8.3

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

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

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Tue 24-01-12 18:23:08, Michal Hocko wrote:
> On Tue 24-01-12 17:44:49, Johannes Weiner wrote:
> > On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> > > On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > > > Hold on, I think this patch is still not complete: end_migration()
> > > > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > > > type.  This will uncharge all migrated anon pages as cache, when it
> > > > should decide based on PageAnon(used), which is the page where
> > > > ->mapping is intact after migration.
> > > 
> > > You are right, I've missed that one as well. Anyway
> > > MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> > > these days and it got out of sync with its documentation (used by
> > > force_empty) quite some time ago (f817ed48). What about something like
> > > the following on top of the previous patch?
> > > --- 
> > > Should be foldet into the previous patch with the updated changelog:
> > > 
> > > Mapping of the unused page is not touched during migration (see
> > 
> > used one, not unused.  unused->mapping is globbered during migration.
> 
> Yes, you are right:

Sorry I haven't sent the most recent update. Here we go:
---
Should be folded into the previous patch with the updated changelog:

Mapping of the used page is not touched during migration (see
page_remove_rmap) so we can rely on it and push the correct charge type
down to __mem_cgroup_uncharge_common from end_migration for unused page.
The force flag was misleading was abused for skipping the needless
page_mapped() / PageCgroupMigration() check, as we know the unused page
is no longer mapped and cleared the migration flag just a few lines
up.  But doing the checks is no biggie and it's not worth adding another
flag just to skip them.  But I guess this should be mentioned in the
changelog.
---
 mm/memcontrol.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4d655ee..869744a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3195,6 +3195,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (!memcg)
 		return;
@@ -3207,6 +3208,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 		used = newpage;
 		unused = oldpage;
 	}
+
 	/*
 	 * We disallowed uncharge of pages under migration because mapcount
 	 * of the page goes down to zero, temporarly.
@@ -3217,7 +3219,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	ClearPageCgroupMigration(pc);
 	unlock_page_cgroup(pc);
 
-	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
+	anon = PageAnon(used);
+	__mem_cgroup_uncharge_common(unused,
+			anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
+			: MEM_CGROUP_CHARGE_TYPE_CACHE);
 
 	/*
 	 * If a page is a file cache, radix-tree replacement is very atomic
@@ -3227,7 +3232,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	 * and USED bit check in mem_cgroup_uncharge_page() will do enough
 	 * check. (see prepare_charge() also)
 	 */
-	if (PageAnon(used))
+	if (anon)
 		mem_cgroup_uncharge_page(used);
 	/*
 	 * At migration, we may charge account against cgroup which has no
-- 
1.7.8.3

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

On Tue, 24 Jan 2012 19:09:47 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Tue 24-01-12 18:23:08, Michal Hocko wrote:
> > On Tue 24-01-12 17:44:49, Johannes Weiner wrote:
> > > On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> > > > On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > > > > Hold on, I think this patch is still not complete: end_migration()
> > > > > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > > > > type.  This will uncharge all migrated anon pages as cache, when it
> > > > > should decide based on PageAnon(used), which is the page where
> > > > > ->mapping is intact after migration.
> > > > 
> > > > You are right, I've missed that one as well. Anyway
> > > > MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> > > > these days and it got out of sync with its documentation (used by
> > > > force_empty) quite some time ago (f817ed48). What about something like
> > > > the following on top of the previous patch?
> > > > --- 
> > > > Should be foldet into the previous patch with the updated changelog:
> > > > 
> > > > Mapping of the unused page is not touched during migration (see
> > > 
> > > used one, not unused.  unused->mapping is globbered during migration.
> > 
> > Yes, you are right:
> 
> Sorry I haven't sent the most recent update. Here we go:
> ---
> Should be folded into the previous patch with the updated changelog:
> 
Thanks, I'll fold this into v5.
-Kame


> Mapping of the used page is not touched during migration (see
> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration for unused page.
> The force flag was misleading was abused for skipping the needless
> page_mapped() / PageCgroupMigration() check, as we know the unused page
> is no longer mapped and cleared the migration flag just a few lines
> up.  But doing the checks is no biggie and it's not worth adding another
> flag just to skip them.  But I guess this should be mentioned in the
> changelog.
> ---
>  mm/memcontrol.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4d655ee..869744a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3195,6 +3195,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  {
>  	struct page *used, *unused;
>  	struct page_cgroup *pc;
> +	bool anon;
>  
>  	if (!memcg)
>  		return;
> @@ -3207,6 +3208,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  		used = newpage;
>  		unused = oldpage;
>  	}
> +
>  	/*
>  	 * We disallowed uncharge of pages under migration because mapcount
>  	 * of the page goes down to zero, temporarly.
> @@ -3217,7 +3219,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  	ClearPageCgroupMigration(pc);
>  	unlock_page_cgroup(pc);
>  
> -	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
> +	anon = PageAnon(used);
> +	__mem_cgroup_uncharge_common(unused,
> +			anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
> +			: MEM_CGROUP_CHARGE_TYPE_CACHE);
>  
>  	/*
>  	 * If a page is a file cache, radix-tree replacement is very atomic
> @@ -3227,7 +3232,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  	 * and USED bit check in mem_cgroup_uncharge_page() will do enough
>  	 * check. (see prepare_charge() also)
>  	 */
> -	if (PageAnon(used))
> +	if (anon)
>  		mem_cgroup_uncharge_page(used);
>  	/*
>  	 * At migration, we may charge account against cgroup which has no
> -- 
> 1.7.8.3
> 
> -- 
> Michal Hocko
> SUSE Labs
> SUSE LINUX s.r.o.
> Lihovarska 1060/12
> 190 00 Praha 9    
> Czech Republic
> 

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

Re: [PATCH v4] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

On Tue, 24 Jan 2012 19:09:47 +0100
Michal Hocko <mhocko@suse.cz> wrote:

> On Tue 24-01-12 18:23:08, Michal Hocko wrote:
> > On Tue 24-01-12 17:44:49, Johannes Weiner wrote:
> > > On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> > > > On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > > > > Hold on, I think this patch is still not complete: end_migration()
> > > > > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > > > > type.  This will uncharge all migrated anon pages as cache, when it
> > > > > should decide based on PageAnon(used), which is the page where
> > > > > ->mapping is intact after migration.
> > > > 
> > > > You are right, I've missed that one as well. Anyway
> > > > MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> > > > these days and it got out of sync with its documentation (used by
> > > > force_empty) quite some time ago (f817ed48). What about something like
> > > > the following on top of the previous patch?
> > > > --- 
> > > > Should be foldet into the previous patch with the updated changelog:
> > > > 
> > > > Mapping of the unused page is not touched during migration (see
> > > 
> > > used one, not unused.  unused->mapping is globbered during migration.
> > 
> > Yes, you are right:
> 
> Sorry I haven't sent the most recent update. Here we go:
> ---
> Should be folded into the previous patch with the updated changelog:
> 
Thanks, I'll fold this into v5.
-Kame


> Mapping of the used page is not touched during migration (see
> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration for unused page.
> The force flag was misleading was abused for skipping the needless
> page_mapped() / PageCgroupMigration() check, as we know the unused page
> is no longer mapped and cleared the migration flag just a few lines
> up.  But doing the checks is no biggie and it's not worth adding another
> flag just to skip them.  But I guess this should be mentioned in the
> changelog.
> ---
>  mm/memcontrol.c |    9 +++++++--
>  1 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4d655ee..869744a 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3195,6 +3195,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  {
>  	struct page *used, *unused;
>  	struct page_cgroup *pc;
> +	bool anon;
>  
>  	if (!memcg)
>  		return;
> @@ -3207,6 +3208,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  		used = newpage;
>  		unused = oldpage;
>  	}
> +
>  	/*
>  	 * We disallowed uncharge of pages under migration because mapcount
>  	 * of the page goes down to zero, temporarly.
> @@ -3217,7 +3219,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  	ClearPageCgroupMigration(pc);
>  	unlock_page_cgroup(pc);
>  
> -	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
> +	anon = PageAnon(used);
> +	__mem_cgroup_uncharge_common(unused,
> +			anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
> +			: MEM_CGROUP_CHARGE_TYPE_CACHE);
>  
>  	/*
>  	 * If a page is a file cache, radix-tree replacement is very atomic
> @@ -3227,7 +3232,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  	 * and USED bit check in mem_cgroup_uncharge_page() will do enough
>  	 * check. (see prepare_charge() also)
>  	 */
> -	if (PageAnon(used))
> +	if (anon)
>  		mem_cgroup_uncharge_page(used);
>  	/*
>  	 * At migration, we may charge account against cgroup which has no
> -- 
> 1.7.8.3
> 
> -- 
> Michal Hocko
> SUSE Labs
> SUSE LINUX s.r.o.
> Lihovarska 1060/12
> 190 00 Praha 9    
> Czech Republic
> 

[PATCH v5] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

On Wed, 25 Jan 2012 09:00:25 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Tue, 24 Jan 2012 19:09:47 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > On Tue 24-01-12 18:23:08, Michal Hocko wrote:
> > > On Tue 24-01-12 17:44:49, Johannes Weiner wrote:
> > > > On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> > > > > On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > > > > > Hold on, I think this patch is still not complete: end_migration()
> > > > > > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > > > > > type.  This will uncharge all migrated anon pages as cache, when it
> > > > > > should decide based on PageAnon(used), which is the page where
> > > > > > ->mapping is intact after migration.
> > > > > 
> > > > > You are right, I've missed that one as well. Anyway
> > > > > MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> > > > > these days and it got out of sync with its documentation (used by
> > > > > force_empty) quite some time ago (f817ed48). What about something like
> > > > > the following on top of the previous patch?
> > > > > --- 
> > > > > Should be foldet into the previous patch with the updated changelog:
> > > > > 
> > > > > Mapping of the unused page is not touched during migration (see
> > > > 
> > > > used one, not unused.  unused->mapping is globbered during migration.
> > > 
> > > Yes, you are right:
> > 
> > Sorry I haven't sent the most recent update. Here we go:
> > ---
> > Should be folded into the previous patch with the updated changelog:
> > 
> Thanks, I'll fold this into v5.
> -Kame
> 

v5 here
==
Subject: [PATCH v5] memcg: remove PCG_CACHE

We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
Here, "CACHE" means anonymous user pages (and SwapCache). This
doesn't include shmem.

Consdering callers, at charge/uncharge, the caller should know
what  the page is and we don't need to record it by using 1bit
per page.

This patch removes PCG_CACHE bit and make callers of
mem_cgroup_charge_statistics() to specify what the page is.

About page migration:
Mapping of the used page is not touched during migration (see
page_remove_rmap) so we can rely on it and push the correct charge type
down to __mem_cgroup_uncharge_common from end_migration for unused page.
The force flag was misleading was abused for skipping the needless
page_mapped() / PageCgroupMigration() check, as we know the unused page
is no longer mapped and cleared the migration flag just a few lines
up.  But doing the checks is no biggie and it's not worth adding another
flag just to skip them.

Changelog since v4
 - fixed a bug at page migration by Michal Hokko.

Changelog since v3
 - renamed a variable 'rss' to 'anon'

Changelog since v2
 - removed 'not_rss', added 'anon'
 - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
 - removed a patch to mem_cgroup_uncharge_cache
 - simplified comment.

Changelog since RFC.
 - rebased onto memcg-devel
 - rename 'file' to 'not_rss'
 - some cleanup and added comment.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 include/linux/page_cgroup.h |    8 +-----
 mm/memcontrol.c             |   57 ++++++++++++++++++++++++-------------------
 2 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
index a2d1177..1060292 100644
--- a/include/linux/page_cgroup.h
+++ b/include/linux/page_cgroup.h
@@ -4,7 +4,6 @@
 enum {
 	/* flags for mem_cgroup */
 	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
-	PCG_CACHE, /* charged as cache */
 	PCG_USED, /* this object is in use. */
 	PCG_MIGRATION, /* under page migration */
 	/* flags for mem_cgroup and file and I/O status */
@@ -64,11 +63,6 @@ static inline void ClearPageCgroup##uname(struct page_cgroup *pc)	\
 static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
 	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
 
-/* Cache flag is set only once (at allocation) */
-TESTPCGFLAG(Cache, CACHE)
-CLEARPCGFLAG(Cache, CACHE)
-SETPCGFLAG(Cache, CACHE)
-
 TESTPCGFLAG(Used, USED)
 CLEARPCGFLAG(Used, USED)
 SETPCGFLAG(Used, USED)
@@ -85,7 +79,7 @@ static inline void lock_page_cgroup(struct page_cgroup *pc)
 {
 	/*
 	 * Don't take this lock in IRQ context.
-	 * This lock is for pc->mem_cgroup, USED, CACHE, MIGRATION
+	 * This lock is for pc->mem_cgroup, USED, MIGRATION
 	 */
 	bit_spin_lock(PCG_LOCK, &pc->flags);
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 1c56c5f..45dab06 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -670,15 +670,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
 }
 
 static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
-					 bool file, int nr_pages)
+					 bool anon, int nr_pages)
 {
 	preempt_disable();
 
-	if (file)
-		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
+	/*
+	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
+	 * counted as CACHE even if it's on ANON LRU.
+	 */
+	if (anon)
+		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
 				nr_pages);
 	else
-		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
+		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
 				nr_pages);
 
 	/* pagein of a big page is an event. So, ignore page size */
@@ -2405,6 +2409,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 				       struct page_cgroup *pc,
 				       enum charge_type ctype)
 {
+	bool anon;
+
 	lock_page_cgroup(pc);
 	if (unlikely(PageCgroupUsed(pc))) {
 		unlock_page_cgroup(pc);
@@ -2424,21 +2430,14 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 	 * See mem_cgroup_add_lru_list(), etc.
  	 */
 	smp_wmb();
-	switch (ctype) {
-	case MEM_CGROUP_CHARGE_TYPE_CACHE:
-	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
-		SetPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
-		ClearPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	default:
-		break;
-	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
+	SetPageCgroupUsed(pc);
+	if (ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
+		anon = true;
+	else
+		anon = false;
+
+	mem_cgroup_charge_statistics(memcg, anon, nr_pages);
 	unlock_page_cgroup(pc);
 	WARN_ON_ONCE(PageLRU(page));
 	/*
@@ -2503,6 +2502,7 @@ static int mem_cgroup_move_account(struct page *page,
 {
 	unsigned long flags;
 	int ret;
+	bool anon = PageAnon(page);
 
 	VM_BUG_ON(from == to);
 	VM_BUG_ON(PageLRU(page));
@@ -2531,14 +2531,14 @@ static int mem_cgroup_move_account(struct page *page,
 		__this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
 		preempt_enable();
 	}
-	mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(from, anon, -nr_pages);
 	if (uncharge)
 		/* This is not "cancel", but cancel_charge does all we need. */
 		__mem_cgroup_cancel_charge(from, nr_pages);
 
 	/* caller should have done css_get */
 	pc->mem_cgroup = to;
-	mem_cgroup_charge_statistics(to, PageCgroupCache(pc), nr_pages);
+	mem_cgroup_charge_statistics(to, anon, nr_pages);
 	/*
 	 * We charges against "to" which may not have any tasks. Then, "to"
 	 * can be under rmdir(). But in current implementation, caller of
@@ -2884,6 +2884,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 	struct mem_cgroup *memcg = NULL;
 	unsigned int nr_pages = 1;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (mem_cgroup_disabled())
 		return NULL;
@@ -2915,6 +2916,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 		/* See mem_cgroup_prepare_migration() */
 		if (page_mapped(page) || PageCgroupMigration(pc))
 			goto unlock_out;
+		anon = true;
 		break;
 	case MEM_CGROUP_CHARGE_TYPE_SWAPOUT:
 		if (!PageAnon(page)) {	/* Shared memory */
@@ -2922,12 +2924,14 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 				goto unlock_out;
 		} else if (page_mapped(page)) /* Anon */
 				goto unlock_out;
+		anon = true;
 		break;
 	default:
+		anon = false;
 		break;
 	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(memcg, anon, -nr_pages);
 
 	ClearPageCgroupUsed(pc);
 	/*
@@ -3251,6 +3255,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (!memcg)
 		return;
@@ -3272,8 +3277,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	lock_page_cgroup(pc);
 	ClearPageCgroupMigration(pc);
 	unlock_page_cgroup(pc);
-
-	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
+	anon = PageAnon(used);
+	__mem_cgroup_uncharge_common(unused,
+		anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
+                     : MEM_CGROUP_CHARGE_TYPE_CACHE);
 
 	/*
 	 * If a page is a file cache, radix-tree replacement is very atomic
@@ -3283,7 +3290,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	 * and USED bit check in mem_cgroup_uncharge_page() will do enough
 	 * check. (see prepare_charge() also)
 	 */
-	if (PageAnon(used))
+	if (anon)
 		mem_cgroup_uncharge_page(used);
 	/*
 	 * At migration, we may charge account against cgroup which has no
@@ -3313,7 +3320,7 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,
 	/* fix accounting on old pages */
 	lock_page_cgroup(pc);
 	memcg = pc->mem_cgroup;
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -1);
+	mem_cgroup_charge_statistics(memcg, false, -1);
 	ClearPageCgroupUsed(pc);
 	unlock_page_cgroup(pc);
 
-- 
1.7.4.1



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

[PATCH v5] memcg: remove PCG_CACHE page_cgroup flag

[KAMEZAWA Hiroyuki]

On Wed, 25 Jan 2012 09:00:25 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Tue, 24 Jan 2012 19:09:47 +0100
> Michal Hocko <mhocko@suse.cz> wrote:
> 
> > On Tue 24-01-12 18:23:08, Michal Hocko wrote:
> > > On Tue 24-01-12 17:44:49, Johannes Weiner wrote:
> > > > On Tue, Jan 24, 2012 at 05:01:40PM +0100, Michal Hocko wrote:
> > > > > On Tue 24-01-12 15:54:11, Johannes Weiner wrote:
> > > > > > Hold on, I think this patch is still not complete: end_migration()
> > > > > > directly uses __mem_cgroup_uncharge_common() with the FORCE charge
> > > > > > type.  This will uncharge all migrated anon pages as cache, when it
> > > > > > should decide based on PageAnon(used), which is the page where
> > > > > > ->mapping is intact after migration.
> > > > > 
> > > > > You are right, I've missed that one as well. Anyway
> > > > > MEM_CGROUP_CHARGE_TYPE_FORCE is used only in mem_cgroup_end_migration
> > > > > these days and it got out of sync with its documentation (used by
> > > > > force_empty) quite some time ago (f817ed48). What about something like
> > > > > the following on top of the previous patch?
> > > > > --- 
> > > > > Should be foldet into the previous patch with the updated changelog:
> > > > > 
> > > > > Mapping of the unused page is not touched during migration (see
> > > > 
> > > > used one, not unused.  unused->mapping is globbered during migration.
> > > 
> > > Yes, you are right:
> > 
> > Sorry I haven't sent the most recent update. Here we go:
> > ---
> > Should be folded into the previous patch with the updated changelog:
> > 
> Thanks, I'll fold this into v5.
> -Kame
> 

v5 here
==
Subject: [PATCH v5] memcg: remove PCG_CACHE

We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
Here, "CACHE" means anonymous user pages (and SwapCache). This
doesn't include shmem.

Consdering callers, at charge/uncharge, the caller should know
what  the page is and we don't need to record it by using 1bit
per page.

This patch removes PCG_CACHE bit and make callers of
mem_cgroup_charge_statistics() to specify what the page is.

About page migration:
Mapping of the used page is not touched during migration (see
page_remove_rmap) so we can rely on it and push the correct charge type
down to __mem_cgroup_uncharge_common from end_migration for unused page.
The force flag was misleading was abused for skipping the needless
page_mapped() / PageCgroupMigration() check, as we know the unused page
is no longer mapped and cleared the migration flag just a few lines
up.  But doing the checks is no biggie and it's not worth adding another
flag just to skip them.

Changelog since v4
 - fixed a bug at page migration by Michal Hokko.

Changelog since v3
 - renamed a variable 'rss' to 'anon'

Changelog since v2
 - removed 'not_rss', added 'anon'
 - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
 - removed a patch to mem_cgroup_uncharge_cache
 - simplified comment.

Changelog since RFC.
 - rebased onto memcg-devel
 - rename 'file' to 'not_rss'
 - some cleanup and added comment.

Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
 include/linux/page_cgroup.h |    8 +-----
 mm/memcontrol.c             |   57 ++++++++++++++++++++++++-------------------
 2 files changed, 33 insertions(+), 32 deletions(-)

diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
index a2d1177..1060292 100644
--- a/include/linux/page_cgroup.h
+++ b/include/linux/page_cgroup.h
@@ -4,7 +4,6 @@
 enum {
 	/* flags for mem_cgroup */
 	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
-	PCG_CACHE, /* charged as cache */
 	PCG_USED, /* this object is in use. */
 	PCG_MIGRATION, /* under page migration */
 	/* flags for mem_cgroup and file and I/O status */
@@ -64,11 +63,6 @@ static inline void ClearPageCgroup##uname(struct page_cgroup *pc)	\
 static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
 	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
 
-/* Cache flag is set only once (at allocation) */
-TESTPCGFLAG(Cache, CACHE)
-CLEARPCGFLAG(Cache, CACHE)
-SETPCGFLAG(Cache, CACHE)
-
 TESTPCGFLAG(Used, USED)
 CLEARPCGFLAG(Used, USED)
 SETPCGFLAG(Used, USED)
@@ -85,7 +79,7 @@ static inline void lock_page_cgroup(struct page_cgroup *pc)
 {
 	/*
 	 * Don't take this lock in IRQ context.
-	 * This lock is for pc->mem_cgroup, USED, CACHE, MIGRATION
+	 * This lock is for pc->mem_cgroup, USED, MIGRATION
 	 */
 	bit_spin_lock(PCG_LOCK, &pc->flags);
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 1c56c5f..45dab06 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -670,15 +670,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
 }
 
 static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
-					 bool file, int nr_pages)
+					 bool anon, int nr_pages)
 {
 	preempt_disable();
 
-	if (file)
-		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
+	/*
+	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
+	 * counted as CACHE even if it's on ANON LRU.
+	 */
+	if (anon)
+		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
 				nr_pages);
 	else
-		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
+		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
 				nr_pages);
 
 	/* pagein of a big page is an event. So, ignore page size */
@@ -2405,6 +2409,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 				       struct page_cgroup *pc,
 				       enum charge_type ctype)
 {
+	bool anon;
+
 	lock_page_cgroup(pc);
 	if (unlikely(PageCgroupUsed(pc))) {
 		unlock_page_cgroup(pc);
@@ -2424,21 +2430,14 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
 	 * See mem_cgroup_add_lru_list(), etc.
  	 */
 	smp_wmb();
-	switch (ctype) {
-	case MEM_CGROUP_CHARGE_TYPE_CACHE:
-	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
-		SetPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
-		ClearPageCgroupCache(pc);
-		SetPageCgroupUsed(pc);
-		break;
-	default:
-		break;
-	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
+	SetPageCgroupUsed(pc);
+	if (ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
+		anon = true;
+	else
+		anon = false;
+
+	mem_cgroup_charge_statistics(memcg, anon, nr_pages);
 	unlock_page_cgroup(pc);
 	WARN_ON_ONCE(PageLRU(page));
 	/*
@@ -2503,6 +2502,7 @@ static int mem_cgroup_move_account(struct page *page,
 {
 	unsigned long flags;
 	int ret;
+	bool anon = PageAnon(page);
 
 	VM_BUG_ON(from == to);
 	VM_BUG_ON(PageLRU(page));
@@ -2531,14 +2531,14 @@ static int mem_cgroup_move_account(struct page *page,
 		__this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
 		preempt_enable();
 	}
-	mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(from, anon, -nr_pages);
 	if (uncharge)
 		/* This is not "cancel", but cancel_charge does all we need. */
 		__mem_cgroup_cancel_charge(from, nr_pages);
 
 	/* caller should have done css_get */
 	pc->mem_cgroup = to;
-	mem_cgroup_charge_statistics(to, PageCgroupCache(pc), nr_pages);
+	mem_cgroup_charge_statistics(to, anon, nr_pages);
 	/*
 	 * We charges against "to" which may not have any tasks. Then, "to"
 	 * can be under rmdir(). But in current implementation, caller of
@@ -2884,6 +2884,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 	struct mem_cgroup *memcg = NULL;
 	unsigned int nr_pages = 1;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (mem_cgroup_disabled())
 		return NULL;
@@ -2915,6 +2916,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 		/* See mem_cgroup_prepare_migration() */
 		if (page_mapped(page) || PageCgroupMigration(pc))
 			goto unlock_out;
+		anon = true;
 		break;
 	case MEM_CGROUP_CHARGE_TYPE_SWAPOUT:
 		if (!PageAnon(page)) {	/* Shared memory */
@@ -2922,12 +2924,14 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
 				goto unlock_out;
 		} else if (page_mapped(page)) /* Anon */
 				goto unlock_out;
+		anon = true;
 		break;
 	default:
+		anon = false;
 		break;
 	}
 
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -nr_pages);
+	mem_cgroup_charge_statistics(memcg, anon, -nr_pages);
 
 	ClearPageCgroupUsed(pc);
 	/*
@@ -3251,6 +3255,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
+	bool anon;
 
 	if (!memcg)
 		return;
@@ -3272,8 +3277,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	lock_page_cgroup(pc);
 	ClearPageCgroupMigration(pc);
 	unlock_page_cgroup(pc);
-
-	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
+	anon = PageAnon(used);
+	__mem_cgroup_uncharge_common(unused,
+		anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
+                     : MEM_CGROUP_CHARGE_TYPE_CACHE);
 
 	/*
 	 * If a page is a file cache, radix-tree replacement is very atomic
@@ -3283,7 +3290,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
 	 * and USED bit check in mem_cgroup_uncharge_page() will do enough
 	 * check. (see prepare_charge() also)
 	 */
-	if (PageAnon(used))
+	if (anon)
 		mem_cgroup_uncharge_page(used);
 	/*
 	 * At migration, we may charge account against cgroup which has no
@@ -3313,7 +3320,7 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,
 	/* fix accounting on old pages */
 	lock_page_cgroup(pc);
 	memcg = pc->mem_cgroup;
-	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -1);
+	mem_cgroup_charge_statistics(memcg, false, -1);
 	ClearPageCgroupUsed(pc);
 	unlock_page_cgroup(pc);
 
-- 
1.7.4.1

Re: [PATCH v5] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Wed 25-01-12 14:41:00, KAMEZAWA Hiroyuki wrote:
[...]
> Subject: [PATCH v5] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> About page migration:
> Mapping of the used page is not touched during migration (see
> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration for unused page.
> The force flag was misleading was abused for skipping the needless
> page_mapped() / PageCgroupMigration() check, as we know the unused page
> is no longer mapped and cleared the migration flag just a few lines
> up.  But doing the checks is no biggie and it's not worth adding another
> flag just to skip them.
> 
> Changelog since v4
>  - fixed a bug at page migration by Michal Hokko.

Would be more fair:
   - fixed a bug at page migration by Johannes Weiner

> Changelog since v3
>  - renamed a variable 'rss' to 'anon'
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Anyway it looks good now (unless I missed something ;)).

Acked-by: Michal Hocko <mhocko@suse.cz>

> ---
>  include/linux/page_cgroup.h |    8 +-----
>  mm/memcontrol.c             |   57 ++++++++++++++++++++++++-------------------
>  2 files changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
> index a2d1177..1060292 100644
> --- a/include/linux/page_cgroup.h
> +++ b/include/linux/page_cgroup.h
> @@ -4,7 +4,6 @@
>  enum {
>  	/* flags for mem_cgroup */
>  	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
> -	PCG_CACHE, /* charged as cache */
>  	PCG_USED, /* this object is in use. */
>  	PCG_MIGRATION, /* under page migration */
>  	/* flags for mem_cgroup and file and I/O status */
> @@ -64,11 +63,6 @@ static inline void ClearPageCgroup##uname(struct page_cgroup *pc)	\
>  static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
>  	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
>  
> -/* Cache flag is set only once (at allocation) */
> -TESTPCGFLAG(Cache, CACHE)
> -CLEARPCGFLAG(Cache, CACHE)
> -SETPCGFLAG(Cache, CACHE)
> -
>  TESTPCGFLAG(Used, USED)
>  CLEARPCGFLAG(Used, USED)
>  SETPCGFLAG(Used, USED)
> @@ -85,7 +79,7 @@ static inline void lock_page_cgroup(struct page_cgroup *pc)
>  {
>  	/*
>  	 * Don't take this lock in IRQ context.
> -	 * This lock is for pc->mem_cgroup, USED, CACHE, MIGRATION
> +	 * This lock is for pc->mem_cgroup, USED, MIGRATION
>  	 */
>  	bit_spin_lock(PCG_LOCK, &pc->flags);
>  }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1c56c5f..45dab06 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -670,15 +670,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
>  }
>  
>  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> -					 bool file, int nr_pages)
> +					 bool anon, int nr_pages)
>  {
>  	preempt_disable();
>  
> -	if (file)
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
> +	/*
> +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
> +	 * counted as CACHE even if it's on ANON LRU.
> +	 */
> +	if (anon)
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
>  				nr_pages);
>  	else
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
>  				nr_pages);
>  
>  	/* pagein of a big page is an event. So, ignore page size */
> @@ -2405,6 +2409,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  				       struct page_cgroup *pc,
>  				       enum charge_type ctype)
>  {
> +	bool anon;
> +
>  	lock_page_cgroup(pc);
>  	if (unlikely(PageCgroupUsed(pc))) {
>  		unlock_page_cgroup(pc);
> @@ -2424,21 +2430,14 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  	 * See mem_cgroup_add_lru_list(), etc.
>   	 */
>  	smp_wmb();
> -	switch (ctype) {
> -	case MEM_CGROUP_CHARGE_TYPE_CACHE:
> -	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
> -		SetPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
> -		ClearPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	default:
> -		break;
> -	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
> +	SetPageCgroupUsed(pc);
> +	if (ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
> +		anon = true;
> +	else
> +		anon = false;
> +
> +	mem_cgroup_charge_statistics(memcg, anon, nr_pages);
>  	unlock_page_cgroup(pc);
>  	WARN_ON_ONCE(PageLRU(page));
>  	/*
> @@ -2503,6 +2502,7 @@ static int mem_cgroup_move_account(struct page *page,
>  {
>  	unsigned long flags;
>  	int ret;
> +	bool anon = PageAnon(page);
>  
>  	VM_BUG_ON(from == to);
>  	VM_BUG_ON(PageLRU(page));
> @@ -2531,14 +2531,14 @@ static int mem_cgroup_move_account(struct page *page,
>  		__this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
>  		preempt_enable();
>  	}
> -	mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
> +	mem_cgroup_charge_statistics(from, anon, -nr_pages);
>  	if (uncharge)
>  		/* This is not "cancel", but cancel_charge does all we need. */
>  		__mem_cgroup_cancel_charge(from, nr_pages);
>  
>  	/* caller should have done css_get */
>  	pc->mem_cgroup = to;
> -	mem_cgroup_charge_statistics(to, PageCgroupCache(pc), nr_pages);
> +	mem_cgroup_charge_statistics(to, anon, nr_pages);
>  	/*
>  	 * We charges against "to" which may not have any tasks. Then, "to"
>  	 * can be under rmdir(). But in current implementation, caller of
> @@ -2884,6 +2884,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  	struct mem_cgroup *memcg = NULL;
>  	unsigned int nr_pages = 1;
>  	struct page_cgroup *pc;
> +	bool anon;
>  
>  	if (mem_cgroup_disabled())
>  		return NULL;
> @@ -2915,6 +2916,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  		/* See mem_cgroup_prepare_migration() */
>  		if (page_mapped(page) || PageCgroupMigration(pc))
>  			goto unlock_out;
> +		anon = true;
>  		break;
>  	case MEM_CGROUP_CHARGE_TYPE_SWAPOUT:
>  		if (!PageAnon(page)) {	/* Shared memory */
> @@ -2922,12 +2924,14 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  				goto unlock_out;
>  		} else if (page_mapped(page)) /* Anon */
>  				goto unlock_out;
> +		anon = true;
>  		break;
>  	default:
> +		anon = false;
>  		break;
>  	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -nr_pages);
> +	mem_cgroup_charge_statistics(memcg, anon, -nr_pages);
>  
>  	ClearPageCgroupUsed(pc);
>  	/*
> @@ -3251,6 +3255,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  {
>  	struct page *used, *unused;
>  	struct page_cgroup *pc;
> +	bool anon;
>  
>  	if (!memcg)
>  		return;
> @@ -3272,8 +3277,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  	lock_page_cgroup(pc);
>  	ClearPageCgroupMigration(pc);
>  	unlock_page_cgroup(pc);
> -
> -	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
> +	anon = PageAnon(used);
> +	__mem_cgroup_uncharge_common(unused,
> +		anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
> +                     : MEM_CGROUP_CHARGE_TYPE_CACHE);
>  
>  	/*
>  	 * If a page is a file cache, radix-tree replacement is very atomic
> @@ -3283,7 +3290,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  	 * and USED bit check in mem_cgroup_uncharge_page() will do enough
>  	 * check. (see prepare_charge() also)
>  	 */
> -	if (PageAnon(used))
> +	if (anon)
>  		mem_cgroup_uncharge_page(used);
>  	/*
>  	 * At migration, we may charge account against cgroup which has no
> @@ -3313,7 +3320,7 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,
>  	/* fix accounting on old pages */
>  	lock_page_cgroup(pc);
>  	memcg = pc->mem_cgroup;
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -1);
> +	mem_cgroup_charge_statistics(memcg, false, -1);
>  	ClearPageCgroupUsed(pc);
>  	unlock_page_cgroup(pc);
>  
> -- 
> 1.7.4.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

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

Re: [PATCH v5] memcg: remove PCG_CACHE page_cgroup flag

[Michal Hocko]

On Wed 25-01-12 14:41:00, KAMEZAWA Hiroyuki wrote:
[...]
> Subject: [PATCH v5] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> About page migration:
> Mapping of the used page is not touched during migration (see
> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration for unused page.
> The force flag was misleading was abused for skipping the needless
> page_mapped() / PageCgroupMigration() check, as we know the unused page
> is no longer mapped and cleared the migration flag just a few lines
> up.  But doing the checks is no biggie and it's not worth adding another
> flag just to skip them.
> 
> Changelog since v4
>  - fixed a bug at page migration by Michal Hokko.

Would be more fair:
   - fixed a bug at page migration by Johannes Weiner

> Changelog since v3
>  - renamed a variable 'rss' to 'anon'
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Anyway it looks good now (unless I missed something ;)).

Acked-by: Michal Hocko <mhocko@suse.cz>

> ---
>  include/linux/page_cgroup.h |    8 +-----
>  mm/memcontrol.c             |   57 ++++++++++++++++++++++++-------------------
>  2 files changed, 33 insertions(+), 32 deletions(-)
> 
> diff --git a/include/linux/page_cgroup.h b/include/linux/page_cgroup.h
> index a2d1177..1060292 100644
> --- a/include/linux/page_cgroup.h
> +++ b/include/linux/page_cgroup.h
> @@ -4,7 +4,6 @@
>  enum {
>  	/* flags for mem_cgroup */
>  	PCG_LOCK,  /* Lock for pc->mem_cgroup and following bits. */
> -	PCG_CACHE, /* charged as cache */
>  	PCG_USED, /* this object is in use. */
>  	PCG_MIGRATION, /* under page migration */
>  	/* flags for mem_cgroup and file and I/O status */
> @@ -64,11 +63,6 @@ static inline void ClearPageCgroup##uname(struct page_cgroup *pc)	\
>  static inline int TestClearPageCgroup##uname(struct page_cgroup *pc)	\
>  	{ return test_and_clear_bit(PCG_##lname, &pc->flags);  }
>  
> -/* Cache flag is set only once (at allocation) */
> -TESTPCGFLAG(Cache, CACHE)
> -CLEARPCGFLAG(Cache, CACHE)
> -SETPCGFLAG(Cache, CACHE)
> -
>  TESTPCGFLAG(Used, USED)
>  CLEARPCGFLAG(Used, USED)
>  SETPCGFLAG(Used, USED)
> @@ -85,7 +79,7 @@ static inline void lock_page_cgroup(struct page_cgroup *pc)
>  {
>  	/*
>  	 * Don't take this lock in IRQ context.
> -	 * This lock is for pc->mem_cgroup, USED, CACHE, MIGRATION
> +	 * This lock is for pc->mem_cgroup, USED, MIGRATION
>  	 */
>  	bit_spin_lock(PCG_LOCK, &pc->flags);
>  }
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1c56c5f..45dab06 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -670,15 +670,19 @@ static unsigned long mem_cgroup_read_events(struct mem_cgroup *memcg,
>  }
>  
>  static void mem_cgroup_charge_statistics(struct mem_cgroup *memcg,
> -					 bool file, int nr_pages)
> +					 bool anon, int nr_pages)
>  {
>  	preempt_disable();
>  
> -	if (file)
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
> +	/*
> +	 * Here, RSS means 'mapped anon' and anon's SwapCache. Shmem/tmpfs is
> +	 * counted as CACHE even if it's on ANON LRU.
> +	 */
> +	if (anon)
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
>  				nr_pages);
>  	else
> -		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_RSS],
> +		__this_cpu_add(memcg->stat->count[MEM_CGROUP_STAT_CACHE],
>  				nr_pages);
>  
>  	/* pagein of a big page is an event. So, ignore page size */
> @@ -2405,6 +2409,8 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  				       struct page_cgroup *pc,
>  				       enum charge_type ctype)
>  {
> +	bool anon;
> +
>  	lock_page_cgroup(pc);
>  	if (unlikely(PageCgroupUsed(pc))) {
>  		unlock_page_cgroup(pc);
> @@ -2424,21 +2430,14 @@ static void __mem_cgroup_commit_charge(struct mem_cgroup *memcg,
>  	 * See mem_cgroup_add_lru_list(), etc.
>   	 */
>  	smp_wmb();
> -	switch (ctype) {
> -	case MEM_CGROUP_CHARGE_TYPE_CACHE:
> -	case MEM_CGROUP_CHARGE_TYPE_SHMEM:
> -		SetPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	case MEM_CGROUP_CHARGE_TYPE_MAPPED:
> -		ClearPageCgroupCache(pc);
> -		SetPageCgroupUsed(pc);
> -		break;
> -	default:
> -		break;
> -	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), nr_pages);
> +	SetPageCgroupUsed(pc);
> +	if (ctype == MEM_CGROUP_CHARGE_TYPE_MAPPED)
> +		anon = true;
> +	else
> +		anon = false;
> +
> +	mem_cgroup_charge_statistics(memcg, anon, nr_pages);
>  	unlock_page_cgroup(pc);
>  	WARN_ON_ONCE(PageLRU(page));
>  	/*
> @@ -2503,6 +2502,7 @@ static int mem_cgroup_move_account(struct page *page,
>  {
>  	unsigned long flags;
>  	int ret;
> +	bool anon = PageAnon(page);
>  
>  	VM_BUG_ON(from == to);
>  	VM_BUG_ON(PageLRU(page));
> @@ -2531,14 +2531,14 @@ static int mem_cgroup_move_account(struct page *page,
>  		__this_cpu_inc(to->stat->count[MEM_CGROUP_STAT_FILE_MAPPED]);
>  		preempt_enable();
>  	}
> -	mem_cgroup_charge_statistics(from, PageCgroupCache(pc), -nr_pages);
> +	mem_cgroup_charge_statistics(from, anon, -nr_pages);
>  	if (uncharge)
>  		/* This is not "cancel", but cancel_charge does all we need. */
>  		__mem_cgroup_cancel_charge(from, nr_pages);
>  
>  	/* caller should have done css_get */
>  	pc->mem_cgroup = to;
> -	mem_cgroup_charge_statistics(to, PageCgroupCache(pc), nr_pages);
> +	mem_cgroup_charge_statistics(to, anon, nr_pages);
>  	/*
>  	 * We charges against "to" which may not have any tasks. Then, "to"
>  	 * can be under rmdir(). But in current implementation, caller of
> @@ -2884,6 +2884,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  	struct mem_cgroup *memcg = NULL;
>  	unsigned int nr_pages = 1;
>  	struct page_cgroup *pc;
> +	bool anon;
>  
>  	if (mem_cgroup_disabled())
>  		return NULL;
> @@ -2915,6 +2916,7 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  		/* See mem_cgroup_prepare_migration() */
>  		if (page_mapped(page) || PageCgroupMigration(pc))
>  			goto unlock_out;
> +		anon = true;
>  		break;
>  	case MEM_CGROUP_CHARGE_TYPE_SWAPOUT:
>  		if (!PageAnon(page)) {	/* Shared memory */
> @@ -2922,12 +2924,14 @@ __mem_cgroup_uncharge_common(struct page *page, enum charge_type ctype)
>  				goto unlock_out;
>  		} else if (page_mapped(page)) /* Anon */
>  				goto unlock_out;
> +		anon = true;
>  		break;
>  	default:
> +		anon = false;
>  		break;
>  	}
>  
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -nr_pages);
> +	mem_cgroup_charge_statistics(memcg, anon, -nr_pages);
>  
>  	ClearPageCgroupUsed(pc);
>  	/*
> @@ -3251,6 +3255,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  {
>  	struct page *used, *unused;
>  	struct page_cgroup *pc;
> +	bool anon;
>  
>  	if (!memcg)
>  		return;
> @@ -3272,8 +3277,10 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  	lock_page_cgroup(pc);
>  	ClearPageCgroupMigration(pc);
>  	unlock_page_cgroup(pc);
> -
> -	__mem_cgroup_uncharge_common(unused, MEM_CGROUP_CHARGE_TYPE_FORCE);
> +	anon = PageAnon(used);
> +	__mem_cgroup_uncharge_common(unused,
> +		anon ? MEM_CGROUP_CHARGE_TYPE_MAPPED
> +                     : MEM_CGROUP_CHARGE_TYPE_CACHE);
>  
>  	/*
>  	 * If a page is a file cache, radix-tree replacement is very atomic
> @@ -3283,7 +3290,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *memcg,
>  	 * and USED bit check in mem_cgroup_uncharge_page() will do enough
>  	 * check. (see prepare_charge() also)
>  	 */
> -	if (PageAnon(used))
> +	if (anon)
>  		mem_cgroup_uncharge_page(used);
>  	/*
>  	 * At migration, we may charge account against cgroup which has no
> @@ -3313,7 +3320,7 @@ void mem_cgroup_replace_page_cache(struct page *oldpage,
>  	/* fix accounting on old pages */
>  	lock_page_cgroup(pc);
>  	memcg = pc->mem_cgroup;
> -	mem_cgroup_charge_statistics(memcg, PageCgroupCache(pc), -1);
> +	mem_cgroup_charge_statistics(memcg, false, -1);
>  	ClearPageCgroupUsed(pc);
>  	unlock_page_cgroup(pc);
>  
> -- 
> 1.7.4.1
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Michal Hocko
SUSE Labs
SUSE LINUX s.r.o.
Lihovarska 1060/12
190 00 Praha 9    
Czech Republic

Re: [PATCH v5] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Wed, Jan 25, 2012 at 02:41:00PM +0900, KAMEZAWA Hiroyuki wrote:
> Subject: [PATCH v5] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> About page migration:
> Mapping of the used page is not touched during migration (see
> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration for unused page.
> The force flag was misleading was abused for skipping the needless
> page_mapped() / PageCgroupMigration() check, as we know the unused page
> is no longer mapped and cleared the migration flag just a few lines
> up.  But doing the checks is no biggie and it's not worth adding another
> flag just to skip them.
> 
> Changelog since v4
>  - fixed a bug at page migration by Michal Hokko.
> 
> Changelog since v3
>  - renamed a variable 'rss' to 'anon'
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

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

Re: [PATCH v5] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Wed, Jan 25, 2012 at 02:41:00PM +0900, KAMEZAWA Hiroyuki wrote:
> Subject: [PATCH v5] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> About page migration:
> Mapping of the used page is not touched during migration (see
> page_remove_rmap) so we can rely on it and push the correct charge type
> down to __mem_cgroup_uncharge_common from end_migration for unused page.
> The force flag was misleading was abused for skipping the needless
> page_mapped() / PageCgroupMigration() check, as we know the unused page
> is no longer mapped and cleared the migration flag just a few lines
> up.  But doing the checks is no biggie and it's not worth adding another
> flag just to skip them.
> 
> Changelog since v4
>  - fixed a bug at page migration by Michal Hokko.
> 
> Changelog since v3
>  - renamed a variable 'rss' to 'anon'
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v3] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Fri, Jan 20, 2012 at 12:26:58PM +0900, KAMEZAWA Hiroyuki wrote:
> I think this version is much simplified.
> 
> ==
> >From 5700a4fe9c581e1ebaa021ba6119dc8d921b024f Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH v3] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Apart from Michal's rss -> anon renaming suggestion, which I agree
with:

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

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

Re: [PATCH v3] memcg: remove PCG_CACHE page_cgroup flag

[Johannes Weiner]

On Fri, Jan 20, 2012 at 12:26:58PM +0900, KAMEZAWA Hiroyuki wrote:
> I think this version is much simplified.
> 
> ==
> >From 5700a4fe9c581e1ebaa021ba6119dc8d921b024f Mon Sep 17 00:00:00 2001
> From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> Date: Thu, 19 Jan 2012 17:09:41 +0900
> Subject: [PATCH v3] memcg: remove PCG_CACHE
> 
> We record 'the page is cache' by PCG_CACHE bit to page_cgroup.
> Here, "CACHE" means anonymous user pages (and SwapCache). This
> doesn't include shmem.
> 
> Consdering callers, at charge/uncharge, the caller should know
> what  the page is and we don't need to record it by using 1bit
> per page.
> 
> This patch removes PCG_CACHE bit and make callers of
> mem_cgroup_charge_statistics() to specify what the page is.
> 
> Changelog since v2
>  - removed 'not_rss', added 'anon'
>  - changed a meaning of arguments to mem_cgroup_charge_statisitcs()
>  - removed a patch to mem_cgroup_uncharge_cache
>  - simplified comment.
> 
> Changelog since RFC.
>  - rebased onto memcg-devel
>  - rename 'file' to 'not_rss'
>  - some cleanup and added comment.
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Apart from Michal's rss -> anon renaming suggestion, which I agree
with:

Acked-by: Johannes Weiner <hannes@cmpxchg.org>