* [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache @ 2011-01-05 4:00 Daisuke Nishimura 2011-01-05 4:48 ` Minchan Kim 2011-01-05 11:58 ` Johannes Weiner 0 siblings, 2 replies; 15+ messages in thread From: Daisuke Nishimura @ 2011-01-05 4:00 UTC (permalink / raw) To: Andrew Morton Cc: Balbir Singh, KAMEZAWA Hiroyuki, Daisuke Nishimura, LKML, linux-mm 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> ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache 2011-01-05 4:00 [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache Daisuke Nishimura @ 2011-01-05 4:48 ` Minchan Kim 2011-01-05 6:47 ` Daisuke Nishimura 2011-01-05 11:58 ` Johannes Weiner 1 sibling, 1 reply; 15+ messages in thread From: Minchan Kim @ 2011-01-05 4:48 UTC (permalink / raw) To: Daisuke Nishimura Cc: Andrew Morton, Balbir Singh, KAMEZAWA Hiroyuki, LKML, linux-mm 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache 2011-01-05 4:48 ` Minchan Kim @ 2011-01-05 6:47 ` Daisuke Nishimura 2011-01-05 7:18 ` Minchan Kim 2011-01-06 0:52 ` KAMEZAWA Hiroyuki 0 siblings, 2 replies; 15+ messages in thread From: Daisuke Nishimura @ 2011-01-05 6:47 UTC (permalink / raw) To: Minchan Kim Cc: Andrew Morton, Balbir Singh, KAMEZAWA Hiroyuki, LKML, linux-mm, 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache 2011-01-05 6:47 ` Daisuke Nishimura @ 2011-01-05 7:18 ` Minchan Kim 2011-01-06 0:52 ` KAMEZAWA Hiroyuki 1 sibling, 0 replies; 15+ messages in thread From: Minchan Kim @ 2011-01-05 7:18 UTC (permalink / raw) To: Daisuke Nishimura Cc: Andrew Morton, Balbir Singh, KAMEZAWA Hiroyuki, LKML, linux-mm 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache 2011-01-05 6:47 ` Daisuke Nishimura 2011-01-05 7:18 ` Minchan Kim @ 2011-01-06 0:52 ` KAMEZAWA Hiroyuki 2011-01-06 2:51 ` Minchan Kim 1 sibling, 1 reply; 15+ messages in thread From: KAMEZAWA Hiroyuki @ 2011-01-06 0:52 UTC (permalink / raw) To: Daisuke Nishimura Cc: Minchan Kim, Andrew Morton, Balbir Singh, LKML, linux-mm 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache 2011-01-06 0:52 ` KAMEZAWA Hiroyuki @ 2011-01-06 2:51 ` Minchan Kim 0 siblings, 0 replies; 15+ messages in thread From: Minchan Kim @ 2011-01-06 2:51 UTC (permalink / raw) To: KAMEZAWA Hiroyuki Cc: Daisuke Nishimura, Andrew Morton, Balbir Singh, LKML, linux-mm 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache 2011-01-05 4:00 [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache Daisuke Nishimura 2011-01-05 4:48 ` Minchan Kim @ 2011-01-05 11:58 ` Johannes Weiner 2011-01-06 1:09 ` Daisuke Nishimura 1 sibling, 1 reply; 15+ messages in thread From: Johannes Weiner @ 2011-01-05 11:58 UTC (permalink / raw) To: Daisuke Nishimura Cc: Andrew Morton, Balbir Singh, KAMEZAWA Hiroyuki, LKML, linux-mm 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache 2011-01-05 11:58 ` Johannes Weiner @ 2011-01-06 1:09 ` Daisuke Nishimura 2011-01-06 1:55 ` KAMEZAWA Hiroyuki 2011-01-06 2:49 ` Minchan Kim 0 siblings, 2 replies; 15+ messages in thread From: Daisuke Nishimura @ 2011-01-06 1:09 UTC (permalink / raw) To: Johannes Weiner Cc: Andrew Morton, Balbir Singh, KAMEZAWA Hiroyuki, LKML, linux-mm, 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> ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache 2011-01-06 1:09 ` Daisuke Nishimura @ 2011-01-06 1:55 ` KAMEZAWA Hiroyuki 2011-01-06 2:49 ` Minchan Kim 1 sibling, 0 replies; 15+ messages in thread From: KAMEZAWA Hiroyuki @ 2011-01-06 1:55 UTC (permalink / raw) To: Daisuke Nishimura Cc: Johannes Weiner, Andrew Morton, Balbir Singh, LKML, linux-mm 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache 2011-01-06 1:09 ` Daisuke Nishimura 2011-01-06 1:55 ` KAMEZAWA Hiroyuki @ 2011-01-06 2:49 ` Minchan Kim 2011-01-06 3:34 ` [BUGFIX][PATCH v3] " Daisuke Nishimura 1 sibling, 1 reply; 15+ messages in thread From: Minchan Kim @ 2011-01-06 2:49 UTC (permalink / raw) To: Daisuke Nishimura Cc: Johannes Weiner, Andrew Morton, Balbir Singh, KAMEZAWA Hiroyuki, LKML, linux-mm 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [BUGFIX][PATCH v3] memcg: fix memory migration of shmem swapcache 2011-01-06 2:49 ` Minchan Kim @ 2011-01-06 3:34 ` Daisuke Nishimura 2011-01-06 5:42 ` Balbir Singh 0 siblings, 1 reply; 15+ messages in thread From: Daisuke Nishimura @ 2011-01-06 3:34 UTC (permalink / raw) To: Minchan Kim Cc: Daisuke Nishimura, Johannes Weiner, Andrew Morton, Balbir Singh, KAMEZAWA Hiroyuki, LKML, linux-mm > > 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> ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH v3] memcg: fix memory migration of shmem swapcache 2011-01-06 3:34 ` [BUGFIX][PATCH v3] " Daisuke Nishimura @ 2011-01-06 5:42 ` Balbir Singh 2011-01-06 6:29 ` [BUGFIX][PATCH v4] " Daisuke Nishimura 0 siblings, 1 reply; 15+ messages in thread From: Balbir Singh @ 2011-01-06 5:42 UTC (permalink / raw) To: Daisuke Nishimura Cc: Minchan Kim, Johannes Weiner, Andrew Morton, KAMEZAWA Hiroyuki, LKML, linux-mm * 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [BUGFIX][PATCH v4] memcg: fix memory migration of shmem swapcache 2011-01-06 5:42 ` Balbir Singh @ 2011-01-06 6:29 ` Daisuke Nishimura 2011-01-08 9:26 ` Johannes Weiner 2011-01-10 9:05 ` Balbir Singh 0 siblings, 2 replies; 15+ messages in thread From: Daisuke Nishimura @ 2011-01-06 6:29 UTC (permalink / raw) To: balbir Cc: Minchan Kim, Johannes Weiner, Andrew Morton, KAMEZAWA Hiroyuki, LKML, linux-mm, 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> ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH v4] memcg: fix memory migration of shmem swapcache 2011-01-06 6:29 ` [BUGFIX][PATCH v4] " Daisuke Nishimura @ 2011-01-08 9:26 ` Johannes Weiner 2011-01-10 9:05 ` Balbir Singh 1 sibling, 0 replies; 15+ messages in thread From: Johannes Weiner @ 2011-01-08 9:26 UTC (permalink / raw) To: Daisuke Nishimura Cc: balbir, Minchan Kim, Andrew Morton, KAMEZAWA Hiroyuki, LKML, linux-mm 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [BUGFIX][PATCH v4] memcg: fix memory migration of shmem swapcache 2011-01-06 6:29 ` [BUGFIX][PATCH v4] " Daisuke Nishimura 2011-01-08 9:26 ` Johannes Weiner @ 2011-01-10 9:05 ` Balbir Singh 1 sibling, 0 replies; 15+ messages in thread From: Balbir Singh @ 2011-01-10 9:05 UTC (permalink / raw) To: Daisuke Nishimura Cc: Minchan Kim, Johannes Weiner, Andrew Morton, KAMEZAWA Hiroyuki, LKML, linux-mm * 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2011-01-10 9:05 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-01-05 4:00 [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache Daisuke Nishimura 2011-01-05 4:48 ` Minchan Kim 2011-01-05 6:47 ` Daisuke Nishimura 2011-01-05 7:18 ` Minchan Kim 2011-01-06 0:52 ` KAMEZAWA Hiroyuki 2011-01-06 2:51 ` Minchan Kim 2011-01-05 11:58 ` Johannes Weiner 2011-01-06 1:09 ` Daisuke Nishimura 2011-01-06 1:55 ` KAMEZAWA Hiroyuki 2011-01-06 2:49 ` Minchan Kim 2011-01-06 3:34 ` [BUGFIX][PATCH v3] " Daisuke Nishimura 2011-01-06 5:42 ` Balbir Singh 2011-01-06 6:29 ` [BUGFIX][PATCH v4] " Daisuke Nishimura 2011-01-08 9:26 ` Johannes Weiner 2011-01-10 9:05 ` Balbir Singh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).