[BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[Daisuke Nishimura]

Hi.

This is a fix for a problem which has bothered me for a month.

===
From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

In current implimentation, mem_cgroup_end_migration() decides whether the page
migration has succeeded or not by checking "oldpage->mapping".

But if we are tring to migrate a shmem swapcache, the page->mapping of it is
NULL from the begining, so the check would be invalid.
As a result, mem_cgroup_end_migration() assumes the migration has succeeded
even if it's not, so "newpage" would be freed while it's not uncharged.

This patch fixes it by passing mem_cgroup_end_migration() the result of the
page migration.

Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
---
 include/linux/memcontrol.h |    5 ++---
 mm/memcontrol.c            |    5 ++---
 mm/migrate.c               |    2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 159a076..275157b 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -93,7 +93,7 @@ extern int
 mem_cgroup_prepare_migration(struct page *page,
 	struct page *newpage, struct mem_cgroup **ptr);
 extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
-	struct page *oldpage, struct page *newpage);
+	struct page *oldpage, struct page *newpage, int result);
 
 /*
  * For memory reclaim.
@@ -231,8 +231,7 @@ mem_cgroup_prepare_migration(struct page *page, struct page *newpage,
 }
 
 static inline void mem_cgroup_end_migration(struct mem_cgroup *mem,
-					struct page *oldpage,
-					struct page *newpage)
+		struct page *oldpage, struct page *newpage, int result)
 {
 }
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 61678be..632d3bc 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2856,7 +2856,7 @@ int mem_cgroup_prepare_migration(struct page *page,
 
 /* remove redundant charge if migration failed*/
 void mem_cgroup_end_migration(struct mem_cgroup *mem,
-	struct page *oldpage, struct page *newpage)
+	struct page *oldpage, struct page *newpage, int result)
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
@@ -2865,8 +2865,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *mem,
 		return;
 	/* blocks rmdir() */
 	cgroup_exclude_rmdir(&mem->css);
-	/* at migration success, oldpage->mapping is NULL. */
-	if (oldpage->mapping) {
+	if (result) {
 		used = oldpage;
 		unused = newpage;
 	} else {
diff --git a/mm/migrate.c b/mm/migrate.c
index 6ae8a66..9a5704a 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -756,7 +756,7 @@ rcu_unlock:
 		rcu_read_unlock();
 uncharge:
 	if (!charge)
-		mem_cgroup_end_migration(mem, page, newpage);
+		mem_cgroup_end_migration(mem, page, newpage, rc);
 unlock:
 	unlock_page(page);
 
-- 
1.7.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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[Minchan Kim]

Hi,

On Wed, Jan 5, 2011 at 1:00 PM, Daisuke Nishimura
<nishimura@mxp.nes.nec.co.jp> wrote:
> Hi.
>
> This is a fix for a problem which has bothered me for a month.
>
> ===
> From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
>
> In current implimentation, mem_cgroup_end_migration() decides whether the page
> migration has succeeded or not by checking "oldpage->mapping".
>
> But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> NULL from the begining, so the check would be invalid.
> As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> even if it's not, so "newpage" would be freed while it's not uncharged.
>
> This patch fixes it by passing mem_cgroup_end_migration() the result of the
> page migration.
>
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>

Nice catch. I don't oppose the patch.
But as looking the code in unmap_and_move, I feel part of mem cgroup
migrate is rather awkward.

int unmap_and_move()
{
   charge = mem_cgroup_prepare_migration(xxx);
   ..
   BUG_ON(charge); <-- BUG if it is charged?
   ..
uncharge:
   if (!charge)    <-- why do we have to uncharge !charge?
      mem_group_end_migration(xxx);
   ..
}

'charge' local variable isn't good. How about changing "uncharge" or whatever?
Of course, It would be another patch.
If you don't mind, I will send the patch or you may send the patch.

Thanks,

-- 
Kind regards,
Minchan Kim

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[Daisuke Nishimura]

On Wed, 5 Jan 2011 13:48:50 +0900
Minchan Kim <minchan.kim@gmail.com> wrote:

> Hi,
> 
> On Wed, Jan 5, 2011 at 1:00 PM, Daisuke Nishimura
> <nishimura@mxp.nes.nec.co.jp> wrote:
> > Hi.
> >
> > This is a fix for a problem which has bothered me for a month.
> >
> > ===
> > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> >
> > In current implimentation, mem_cgroup_end_migration() decides whether the page
> > migration has succeeded or not by checking "oldpage->mapping".
> >
> > But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> > NULL from the begining, so the check would be invalid.
> > As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> > even if it's not, so "newpage" would be freed while it's not uncharged.
> >
> > This patch fixes it by passing mem_cgroup_end_migration() the result of the
> > page migration.
> >
> > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> 
> Nice catch. I don't oppose the patch.
Thank you for your review.

> But as looking the code in unmap_and_move, I feel part of mem cgroup
> migrate is rather awkward.
> 
> int unmap_and_move()
> {
>    charge = mem_cgroup_prepare_migration(xxx);
>    ..
>    BUG_ON(charge); <-- BUG if it is charged?
>    ..
> uncharge:
>    if (!charge)    <-- why do we have to uncharge !charge?
>       mem_group_end_migration(xxx);
>    ..
> }
> 
> 'charge' local variable isn't good. How about changing "uncharge" or whatever?
hmm, I agree that current code seems a bit confusing, but I can't think of
better name to imply the result of 'charge'.

And considering more, I can't understand why we need to check "if (!charge)"
before mem_cgroup_end_migration() becase it must be always true and, IMHO,
mem_cgroup_end_migration() should do all necesarry checks to avoid double uncharge.
So, I think this local variable can be removed completely.

	rc = mem_cgroup_prepare_migration(..);
	if (rc == -ENOMEM)
		goto unlock;
	BUG_ON(rc);
	..
uncharge:
	mem_cgroup_end_migration(..);

KAMEZAWA-san, what do you think ?

> Of course, It would be another patch.
Yes.

> If you don't mind, I will send the patch or you may send the patch.
> 
I'll leave it to you, but anyway, please do it after this patch has merged.
it will conflict with this patch.


Thanks,
Daisuke Nishimura.

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[Minchan Kim]

On Wed, Jan 5, 2011 at 3:47 PM, Daisuke Nishimura
<nishimura@mxp.nes.nec.co.jp> wrote:
> On Wed, 5 Jan 2011 13:48:50 +0900
> Minchan Kim <minchan.kim@gmail.com> wrote:
>
>> Hi,
>>
>> On Wed, Jan 5, 2011 at 1:00 PM, Daisuke Nishimura
>> <nishimura@mxp.nes.nec.co.jp> wrote:
>> > Hi.
>> >
>> > This is a fix for a problem which has bothered me for a month.
>> >
>> > ===
>> > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
>> >
>> > In current implimentation, mem_cgroup_end_migration() decides whether the page
>> > migration has succeeded or not by checking "oldpage->mapping".
>> >
>> > But if we are tring to migrate a shmem swapcache, the page->mapping of it is
>> > NULL from the begining, so the check would be invalid.
>> > As a result, mem_cgroup_end_migration() assumes the migration has succeeded
>> > even if it's not, so "newpage" would be freed while it's not uncharged

Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[Johannes Weiner]

On Wed, Jan 05, 2011 at 01:00:20PM +0900, Daisuke Nishimura wrote:
> In current implimentation, mem_cgroup_end_migration() decides whether the page
> migration has succeeded or not by checking "oldpage->mapping".
> 
> But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> NULL from the begining, so the check would be invalid.
> As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> even if it's not, so "newpage" would be freed while it's not uncharged.
> 
> This patch fixes it by passing mem_cgroup_end_migration() the result of the
> page migration.

Are there other users that rely on unused->mapping being NULL after
migration?

If so, aren't they prone to misinterpreting this for shmem swapcache
as well?

If not, wouldn't it be better to remove that page->mapping = NULL from
migrate_page_copy() altogether?  I think it's an ugly exception where
the outcome of PageAnon() is not meaningful for an LRU page.

To your patch:

> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2856,7 +2856,7 @@ int mem_cgroup_prepare_migration(struct page *page,
>  
>  /* remove redundant charge if migration failed*/
>  void mem_cgroup_end_migration(struct mem_cgroup *mem,
> -	struct page *oldpage, struct page *newpage)
> +	struct page *oldpage, struct page *newpage, int result)
>  {
>  	struct page *used, *unused;
>  	struct page_cgroup *pc;
> @@ -2865,8 +2865,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *mem,
>  		return;
>  	/* blocks rmdir() */
>  	cgroup_exclude_rmdir(&mem->css);
> -	/* at migration success, oldpage->mapping is NULL. */
> -	if (oldpage->mapping) {
> +	if (result) {

Since this function does not really need more than a boolean value,
wouldn't it make the code more obvious if the parameter was `bool
success'?

	if (!success) {
>  		used = oldpage;
>  		unused = newpage;
>  	} else {

Minor nit, though.  I agree with the patch in general.

	Hannes

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[KAMEZAWA Hiroyuki]

On Wed, 5 Jan 2011 15:47:48 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:

> On Wed, 5 Jan 2011 13:48:50 +0900
> Minchan Kim <minchan.kim@gmail.com> wrote:
> 
> > Hi,
> > 
> > On Wed, Jan 5, 2011 at 1:00 PM, Daisuke Nishimura
> > <nishimura@mxp.nes.nec.co.jp> wrote:
> > > Hi.
> > >
> > > This is a fix for a problem which has bothered me for a month.
> > >
> > > ===
> > > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > >
> > > In current implimentation, mem_cgroup_end_migration() decides whether the page
> > > migration has succeeded or not by checking "oldpage->mapping".
> > >
> > > But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> > > NULL from the begining, so the check would be invalid.
> > > As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> > > even if it's not, so "newpage" would be freed while it's not uncharged.
> > >
> > > This patch fixes it by passing mem_cgroup_end_migration() the result of the
> > > page migration.
> > >
> > > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> > Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> > 
> > Nice catch. I don't oppose the patch.
> Thank you for your review.
> 

Nice catch.


> > But as looking the code in unmap_and_move, I feel part of mem cgroup
> > migrate is rather awkward.
> > 
> > int unmap_and_move()
> > {
> >    charge = mem_cgroup_prepare_migration(xxx);
> >    ..
> >    BUG_ON(charge); <-- BUG if it is charged?
> >    ..
> > uncharge:
> >    if (!charge)    <-- why do we have to uncharge !charge?
> >       mem_group_end_migration(xxx);
> >    ..
> > }
> > 
> > 'charge' local variable isn't good. How about changing "uncharge" or whatever?
> hmm, I agree that current code seems a bit confusing, but I can't think of
> better name to imply the result of 'charge'.
> 
> And considering more, I can't understand why we need to check "if (!charge)"
> before mem_cgroup_end_migration() becase it must be always true and, IMHO,
> mem_cgroup_end_migration() should do all necesarry checks to avoid double uncharge.

ok, please remove it.
Before this commit, http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=01b1ae63c2270cbacfd43fea94578c17950eb548;hp=bced0520fe462bb94021dcabd32e99630c171be2

"mem" is not passed as argument and this was the reason for the vairable "charge".

We can check "charge is in moving" by checking "mem == NULL".


> So, I think this local variable can be removed completely.
> 
> 	rc = mem_cgroup_prepare_migration(..);
> 	if (rc == -ENOMEM)
> 		goto unlock;
> 	BUG_ON(rc);
> 	..
> uncharge:
> 	mem_cgroup_end_migration(..);
> 
> KAMEZAWA-san, what do you think ?
> 

seems ok.

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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[Daisuke Nishimura]

On Wed, 5 Jan 2011 12:58:40 +0100
Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Wed, Jan 05, 2011 at 01:00:20PM +0900, Daisuke Nishimura wrote:
> > In current implimentation, mem_cgroup_end_migration() decides whether the page
> > migration has succeeded or not by checking "oldpage->mapping".
> > 
> > But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> > NULL from the begining, so the check would be invalid.
> > As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> > even if it's not, so "newpage" would be freed while it's not uncharged.
> > 
> > This patch fixes it by passing mem_cgroup_end_migration() the result of the
> > page migration.
> 
> Are there other users that rely on unused->mapping being NULL after
> migration?
> 
As long as I can see, no.

> If so, aren't they prone to misinterpreting this for shmem swapcache
> as well?
> 
> If not, wouldn't it be better to remove that page->mapping = NULL from
> migrate_page_copy() altogether?  I think it's an ugly exception where
> the outcome of PageAnon() is not meaningful for an LRU page.
> 
IIUC, oldpage will be freed on success of page migration, so we hit bad_page
check at freeing the page unless we clear oldpage->mapping, 

> To your patch:
> 
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -2856,7 +2856,7 @@ int mem_cgroup_prepare_migration(struct page *page,
> >  
> >  /* remove redundant charge if migration failed*/
> >  void mem_cgroup_end_migration(struct mem_cgroup *mem,
> > -	struct page *oldpage, struct page *newpage)
> > +	struct page *oldpage, struct page *newpage, int result)
> >  {
> >  	struct page *used, *unused;
> >  	struct page_cgroup *pc;
> > @@ -2865,8 +2865,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *mem,
> >  		return;
> >  	/* blocks rmdir() */
> >  	cgroup_exclude_rmdir(&mem->css);
> > -	/* at migration success, oldpage->mapping is NULL. */
> > -	if (oldpage->mapping) {
> > +	if (result) {
> 
> Since this function does not really need more than a boolean value,
> wouldn't it make the code more obvious if the parameter was `bool
> success'?
> 
> 	if (!success) {
> >  		used = oldpage;
> >  		unused = newpage;
> >  	} else {
> 
> Minor nit, though.  I agree with the patch in general.
> 
Thank you for your review.
How about this ?

===
From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

In current implimentation, mem_cgroup_end_migration() decides whether the page
migration has succeeded or not by checking "oldpage->mapping".

But if we are tring to migrate a shmem swapcache, the page->mapping of it is
NULL from the begining, so the check would be invalid.
As a result, mem_cgroup_end_migration() assumes the migration has succeeded
even if it's not, so "newpage" would be freed while it's not uncharged.

This patch fixes it by passing mem_cgroup_end_migration() the result of the
page migration.

Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
---
 include/linux/memcontrol.h |    5 ++---
 mm/memcontrol.c            |    5 ++---
 mm/migrate.c               |    2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 159a076..cc5a8fd 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -93,7 +93,7 @@ extern int
 mem_cgroup_prepare_migration(struct page *page,
 	struct page *newpage, struct mem_cgroup **ptr);
 extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
-	struct page *oldpage, struct page *newpage);
+	struct page *oldpage, struct page *newpage, bool success);
 
 /*
  * For memory reclaim.
@@ -231,8 +231,7 @@ mem_cgroup_prepare_migration(struct page *page, struct page *newpage,
 }
 
 static inline void mem_cgroup_end_migration(struct mem_cgroup *mem,
-					struct page *oldpage,
-					struct page *newpage)
+		struct page *oldpage, struct page *newpage, bool success)
 {
 }
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 61678be..fbecd02 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2856,7 +2856,7 @@ int mem_cgroup_prepare_migration(struct page *page,
 
 /* remove redundant charge if migration failed*/
 void mem_cgroup_end_migration(struct mem_cgroup *mem,
-	struct page *oldpage, struct page *newpage)
+	struct page *oldpage, struct page *newpage, bool success)
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
@@ -2865,8 +2865,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *mem,
 		return;
 	/* blocks rmdir() */
 	cgroup_exclude_rmdir(&mem->css);
-	/* at migration success, oldpage->mapping is NULL. */
-	if (oldpage->mapping) {
+	if (!success) {
 		used = oldpage;
 		unused = newpage;
 	} else {
diff --git a/mm/migrate.c b/mm/migrate.c
index 6ae8a66..be66b23 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -756,7 +756,7 @@ rcu_unlock:
 		rcu_read_unlock();
 uncharge:
 	if (!charge)
-		mem_cgroup_end_migration(mem, page, newpage);
+		mem_cgroup_end_migration(mem, page, newpage, rc == 0);
 unlock:
 	unlock_page(page);
 
-- 
1.7.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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[KAMEZAWA Hiroyuki]

On Thu, 6 Jan 2011 10:09:23 +0900
Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:

> ===
> From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> 
> In current implimentation, mem_cgroup_end_migration() decides whether the page
> migration has succeeded or not by checking "oldpage->mapping".
> 
> But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> NULL from the begining, so the check would be invalid.
> As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> even if it's not, so "newpage" would be freed while it's not uncharged.
> 
> This patch fixes it by passing mem_cgroup_end_migration() the result of the
> page migration.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[Minchan Kim]

On Thu, Jan 6, 2011 at 10:09 AM, Daisuke Nishimura
<nishimura@mxp.nes.nec.co.jp> wrote:
> On Wed, 5 Jan 2011 12:58:40 +0100
> Johannes Weiner <hannes@cmpxchg.org> wrote:
>
>> On Wed, Jan 05, 2011 at 01:00:20PM +0900, Daisuke Nishimura wrote:
>> > In current implimentation, mem_cgroup_end_migration() decides whether the page
>> > migration has succeeded or not by checking "oldpage->mapping".
>> >
>> > But if we are tring to migrate a shmem swapcache, the page->mapping of it is
>> > NULL from the begining, so the check would be invalid.
>> > As a result, mem_cgroup_end_migration() assumes the migration has succeeded
>> > even if it's not, so "newpage" would be freed while it's not uncharged

Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache

[Minchan Kim]

On Thu, Jan 6, 2011 at 9:52 AM, KAMEZAWA Hiroyuki
<kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Wed, 5 Jan 2011 15:47:48 +0900
> Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
>
>> On Wed, 5 Jan 2011 13:48:50 +0900
>> Minchan Kim <minchan.kim@gmail.com> wrote:
>>
>> > Hi,
>> >
>> > On Wed, Jan 5, 2011 at 1:00 PM, Daisuke Nishimura
>> > <nishimura@mxp.nes.nec.co.jp> wrote:
>> > > Hi.
>> > >
>> > > This is a fix for a problem which has bothered me for a month.
>> > >
>> > > ===
>> > > From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
>> > >
>> > > In current implimentation, mem_cgroup_end_migration() decides whether the page
>> > > migration has succeeded or not by checking "oldpage->mapping".
>> > >
>> > > But if we are tring to migrate a shmem swapcache, the page->mapping of it is
>> > > NULL from the begining, so the check would be invalid.
>> > > As a result, mem_cgroup_end_migration() assumes the migration has succeeded
>> > > even if it's not, so "newpage" would be freed while it's not uncharged.
>> > >
>> > > This patch fixes it by passing mem_cgroup_end_migration() the result of the
>> > > page migration.
>> > >
>> > > Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
>> > Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
>> >
>> > Nice catch. I don't oppose the patch.
>> Thank you for your review.
>>
>
> Nice catch.
>
>
>> > But as looking the code in unmap_and_move, I feel part of mem cgroup
>> > migrate is rather awkward.
>> >
>> > int unmap_and_move()
>> > {
>> >    charge = mem_cgroup_prepare_migration(xxx);
>> >    ..
>> >    BUG_ON(charge); <-- BUG if it is charged?
>> >    ..
>> > uncharge:
>> >    if (!charge)    <-- why do we have to uncharge !charge?
>> >       mem_group_end_migration(xxx);
>> >    ..
>> > }
>> >
>> > 'charge' local variable isn't good. How about changing "uncharge" or whatever?
>> hmm, I agree that current code seems a bit confusing, but I can't think of
>> better name to imply the result of 'charge'.
>>
>> And considering more, I can't understand why we need to check "if (!charge)"
>> before mem_cgroup_end_migration() becase it must be always true and, IMHO,
>> mem_cgroup_end_migration() should do all necesarry checks to avoid double uncharge.
>
> ok, please remove it.
> Before this commit, http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=01b1ae63c2270cbacfd43fea94578c17950eb548;hp=bced0520fe462bb94021dcabd32e99630c171be2
>
> "mem" is not passed as argument and this was the reason for the vairable "charge".
>
> We can check "charge is in moving" by checking "mem == NULL".

I will send the patch after Andrew picks Daisuke's patch up.
Thanks.

-- 
Kind regards,
Minchan Kim

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[BUGFIX][PATCH v3] memcg: fix memory migration of shmem swapcache

[Daisuke Nishimura]

> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index 159a076..cc5a8fd 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -93,7 +93,7 @@ extern int
> > A mem_cgroup_prepare_migration(struct page *page,
> > A  A  A  A struct page *newpage, struct mem_cgroup **ptr);
> > A extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
> > - A  A  A  struct page *oldpage, struct page *newpage);
> > + A  A  A  struct page *oldpage, struct page *newpage, bool success);
> 
> The term "success" implies present or future tense.
> The event this variable cares about in the past so "succeed" might be
> a more appropriate term.
> Sorry to be picky about the English but there is an important
> distinction here since we don't have any comment about the variable.
> 
> Am I being too fussy?
Not at all. Your comments are very helpful to make the code more readable :)

===
From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

In current implimentation, mem_cgroup_end_migration() decides whether the page
migration has succeeded or not by checking "oldpage->mapping".

But if we are tring to migrate a shmem swapcache, the page->mapping of it is
NULL from the begining, so the check would be invalid.
As a result, mem_cgroup_end_migration() assumes the migration has succeeded
even if it's not, so "newpage" would be freed while it's not uncharged.

This patch fixes it by passing mem_cgroup_end_migration() the result of the
page migration.

Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
v2->v3
  - s/success/succeed

v1->v2
  - pass mem_cgroup_end_migration() "bool" instead of "int".

 include/linux/memcontrol.h |    5 ++---
 mm/memcontrol.c            |    5 ++---
 mm/migrate.c               |    2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 159a076..9f52b57 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -93,7 +93,7 @@ extern int
 mem_cgroup_prepare_migration(struct page *page,
 	struct page *newpage, struct mem_cgroup **ptr);
 extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
-	struct page *oldpage, struct page *newpage);
+	struct page *oldpage, struct page *newpage, bool succeed);
 
 /*
  * For memory reclaim.
@@ -231,8 +231,7 @@ mem_cgroup_prepare_migration(struct page *page, struct page *newpage,
 }
 
 static inline void mem_cgroup_end_migration(struct mem_cgroup *mem,
-					struct page *oldpage,
-					struct page *newpage)
+		struct page *oldpage, struct page *newpage, bool succeed)
 {
 }
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 61678be..71a39bc 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2856,7 +2856,7 @@ int mem_cgroup_prepare_migration(struct page *page,
 
 /* remove redundant charge if migration failed*/
 void mem_cgroup_end_migration(struct mem_cgroup *mem,
-	struct page *oldpage, struct page *newpage)
+	struct page *oldpage, struct page *newpage, bool succeed)
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
@@ -2865,8 +2865,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *mem,
 		return;
 	/* blocks rmdir() */
 	cgroup_exclude_rmdir(&mem->css);
-	/* at migration success, oldpage->mapping is NULL. */
-	if (oldpage->mapping) {
+	if (!succeed) {
 		used = oldpage;
 		unused = newpage;
 	} else {
diff --git a/mm/migrate.c b/mm/migrate.c
index 6ae8a66..be66b23 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -756,7 +756,7 @@ rcu_unlock:
 		rcu_read_unlock();
 uncharge:
 	if (!charge)
-		mem_cgroup_end_migration(mem, page, newpage);
+		mem_cgroup_end_migration(mem, page, newpage, rc == 0);
 unlock:
 	unlock_page(page);
 
-- 
1.7.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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH v3] memcg: fix memory migration of shmem swapcache

[Balbir Singh]

* nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2011-01-06 12:34:15]:

> > > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > > index 159a076..cc5a8fd 100644
> > > --- a/include/linux/memcontrol.h
> > > +++ b/include/linux/memcontrol.h
> > > @@ -93,7 +93,7 @@ extern int
> > >  mem_cgroup_prepare_migration(struct page *page,
> > >        struct page *newpage, struct mem_cgroup **ptr);
> > >  extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
> > > -       struct page *oldpage, struct page *newpage);
> > > +       struct page *oldpage, struct page *newpage, bool success);
> > 
> > The term "success" implies present or future tense.
> > The event this variable cares about in the past so "succeed" might be
> > a more appropriate term.
> > Sorry to be picky about the English but there is an important
> > distinction here since we don't have any comment about the variable.
> > 
> > Am I being too fussy?
> Not at all. Your comments are very helpful to make the code more readable :)
> 
> ===
> From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> 
> In current implimentation, mem_cgroup_end_migration() decides whether the page
> migration has succeeded or not by checking "oldpage->mapping".
> 
> But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> NULL from the begining, so the check would be invalid.
> As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> even if it's not, so "newpage" would be freed while it's not uncharged.
> 
> This patch fixes it by passing mem_cgroup_end_migration() the result of the
> page migration.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> ---
> v2->v3
>   - s/success/succeed
> 
> v1->v2
>   - pass mem_cgroup_end_migration() "bool" instead of "int".
> 
>  include/linux/memcontrol.h |    5 ++---
>  mm/memcontrol.c            |    5 ++---
>  mm/migrate.c               |    2 +-
>  3 files changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 159a076..9f52b57 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -93,7 +93,7 @@ extern int
>  mem_cgroup_prepare_migration(struct page *page,
>  	struct page *newpage, struct mem_cgroup **ptr);
>  extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
> -	struct page *oldpage, struct page *newpage);
> +	struct page *oldpage, struct page *newpage, bool succeed);

Sorry for nit-picking but succeed is not as good as succeeded,
successful, successful_migration or migration_ok

> 
>  /*
>   * For memory reclaim.
> @@ -231,8 +231,7 @@ mem_cgroup_prepare_migration(struct page *page, struct page *newpage,
>  }
> 
>  static inline void mem_cgroup_end_migration(struct mem_cgroup *mem,
> -					struct page *oldpage,
> -					struct page *newpage)
> +		struct page *oldpage, struct page *newpage, bool succeed)
>  {
>  }
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 61678be..71a39bc 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2856,7 +2856,7 @@ int mem_cgroup_prepare_migration(struct page *page,
> 
>  /* remove redundant charge if migration failed*/
>  void mem_cgroup_end_migration(struct mem_cgroup *mem,
> -	struct page *oldpage, struct page *newpage)
> +	struct page *oldpage, struct page *newpage, bool succeed)
>  {
>  	struct page *used, *unused;
>  	struct page_cgroup *pc;
> @@ -2865,8 +2865,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *mem,
>  		return;
>  	/* blocks rmdir() */
>  	cgroup_exclude_rmdir(&mem->css);
> -	/* at migration success, oldpage->mapping is NULL. */
> -	if (oldpage->mapping) {
> +	if (!succeed) {
>  		used = oldpage;
>  		unused = newpage;
>  	} else {
> diff --git a/mm/migrate.c b/mm/migrate.c
> index 6ae8a66..be66b23 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -756,7 +756,7 @@ rcu_unlock:
>  		rcu_read_unlock();
>  uncharge:
>  	if (!charge)
> -		mem_cgroup_end_migration(mem, page, newpage);
> +		mem_cgroup_end_migration(mem, page, newpage, rc == 0);
>  unlock:
>  	unlock_page(page);
> 
> -- 
> 1.7.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 policy in Canada: sign http://dissolvethecrtc.ca/
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
	Three Cheers,
	Balbir

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[BUGFIX][PATCH v4] memcg: fix memory migration of shmem swapcache

[Daisuke Nishimura]

> Sorry for nit-picking but succeed is not as good as succeeded,
> successful, successful_migration or migration_ok
> 
OK, I use "migration_ok".

===
From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>

In current implimentation, mem_cgroup_end_migration() decides whether the page
migration has succeeded or not by checking "oldpage->mapping".

But if we are tring to migrate a shmem swapcache, the page->mapping of it is
NULL from the begining, so the check would be invalid.
As a result, mem_cgroup_end_migration() assumes the migration has succeeded
even if it's not, so "newpage" would be freed while it's not uncharged.

This patch fixes it by passing mem_cgroup_end_migration() the result of the
page migration.

Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
---
v3->v4
  - s/succeed/migration_ok
v2->v3
  - s/success/succeed
v1->v2
  - pass mem_cgroup_end_migration() "bool" instead of "int".

 include/linux/memcontrol.h |    5 ++---
 mm/memcontrol.c            |    5 ++---
 mm/migrate.c               |    2 +-
 3 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 159a076..769c318 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -93,7 +93,7 @@ extern int
 mem_cgroup_prepare_migration(struct page *page,
 	struct page *newpage, struct mem_cgroup **ptr);
 extern void mem_cgroup_end_migration(struct mem_cgroup *mem,
-	struct page *oldpage, struct page *newpage);
+	struct page *oldpage, struct page *newpage, bool migration_ok);
 
 /*
  * For memory reclaim.
@@ -231,8 +231,7 @@ mem_cgroup_prepare_migration(struct page *page, struct page *newpage,
 }
 
 static inline void mem_cgroup_end_migration(struct mem_cgroup *mem,
-					struct page *oldpage,
-					struct page *newpage)
+		struct page *oldpage, struct page *newpage, bool migration_ok)
 {
 }
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 61678be..c35f817 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2856,7 +2856,7 @@ int mem_cgroup_prepare_migration(struct page *page,
 
 /* remove redundant charge if migration failed*/
 void mem_cgroup_end_migration(struct mem_cgroup *mem,
-	struct page *oldpage, struct page *newpage)
+	struct page *oldpage, struct page *newpage, bool migration_ok)
 {
 	struct page *used, *unused;
 	struct page_cgroup *pc;
@@ -2865,8 +2865,7 @@ void mem_cgroup_end_migration(struct mem_cgroup *mem,
 		return;
 	/* blocks rmdir() */
 	cgroup_exclude_rmdir(&mem->css);
-	/* at migration success, oldpage->mapping is NULL. */
-	if (oldpage->mapping) {
+	if (!migration_ok) {
 		used = oldpage;
 		unused = newpage;
 	} else {
diff --git a/mm/migrate.c b/mm/migrate.c
index 6ae8a66..be66b23 100644
--- a/mm/migrate.c
+++ b/mm/migrate.c
@@ -756,7 +756,7 @@ rcu_unlock:
 		rcu_read_unlock();
 uncharge:
 	if (!charge)
-		mem_cgroup_end_migration(mem, page, newpage);
+		mem_cgroup_end_migration(mem, page, newpage, rc == 0);
 unlock:
 	unlock_page(page);
 
-- 
1.7.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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH v4] memcg: fix memory migration of shmem swapcache

[Johannes Weiner]

On Thu, Jan 06, 2011 at 03:29:11PM +0900, Daisuke Nishimura wrote:
> In current implimentation, mem_cgroup_end_migration() decides whether the page
> migration has succeeded or not by checking "oldpage->mapping".
> 
> But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> NULL from the begining, so the check would be invalid.
> As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> even if it's not, so "newpage" would be freed while it's not uncharged.
> 
> This patch fixes it by passing mem_cgroup_end_migration() the result of the
> page migration.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Reviewed-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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [BUGFIX][PATCH v4] memcg: fix memory migration of shmem swapcache

[Balbir Singh]

* nishimura@mxp.nes.nec.co.jp <nishimura@mxp.nes.nec.co.jp> [2011-01-06 15:29:11]:

> > Sorry for nit-picking but succeed is not as good as succeeded,
> > successful, successful_migration or migration_ok
> > 
> OK, I use "migration_ok".
> 
> ===
> From: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> 
> In current implimentation, mem_cgroup_end_migration() decides whether the page
> migration has succeeded or not by checking "oldpage->mapping".
> 
> But if we are tring to migrate a shmem swapcache, the page->mapping of it is
> NULL from the begining, so the check would be invalid.
> As a result, mem_cgroup_end_migration() assumes the migration has succeeded
> even if it's not, so "newpage" would be freed while it's not uncharged.
> 
> This patch fixes it by passing mem_cgroup_end_migration() the result of the
> page migration.
> 
> Signed-off-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>
> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>


Acked-by: Balbir Singh <balbir@linux.vnet.ibm.com>
 
-- 
	Three Cheers,
	Balbir

--
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 policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>