* [PATCH v2 0/2] memcg: migration clean up @ 2011-02-20 15:17 Minchan Kim 2011-02-20 15:17 ` [PATCH v2 1/2] memcg: remove unnecessary BUG_ON Minchan Kim 2011-02-20 15:17 ` [PATCH v2 2/2] memcg: remove charge variable in unmap_and_move Minchan Kim 0 siblings, 2 replies; 6+ messages in thread From: Minchan Kim @ 2011-02-20 15:17 UTC (permalink / raw) To: Andrew Morton Cc: linux-mm, LKML, Johannes Weiner, Balbir Singh, KAMEZAWA Hiroyuki, Daisuke Nishimura, Minchan Kim This patch cleans up memcg migration. This patch sent Tue, Jan 11, 2011 but maybe Andrew was very busy so lost. I resend with Acked-by and rebased on mmotm-2011-02-04. Minchan Kim (2): memcg: remove unnecessary BUG_ON memcg: remove charge variable in unmap_and_move mm/memcontrol.c | 1 + mm/migrate.c | 10 +++------- 2 files changed, 4 insertions(+), 7 deletions(-) -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] memcg: remove unnecessary BUG_ON 2011-02-20 15:17 [PATCH v2 0/2] memcg: migration clean up Minchan Kim @ 2011-02-20 15:17 ` Minchan Kim 2011-02-21 13:04 ` Johannes Weiner 2011-02-20 15:17 ` [PATCH v2 2/2] memcg: remove charge variable in unmap_and_move Minchan Kim 1 sibling, 1 reply; 6+ messages in thread From: Minchan Kim @ 2011-02-20 15:17 UTC (permalink / raw) To: Andrew Morton Cc: linux-mm, LKML, Johannes Weiner, Balbir Singh, KAMEZAWA Hiroyuki, Daisuke Nishimura, Minchan Kim Now memcg in unmap_and_move checks BUG_ON of charge. But mem_cgroup_prepare_migration returns either 0 or -ENOMEM. If it returns -ENOMEM, it jumps out unlock without the check. If it returns 0, it can pass BUG_ON. So it's meaningless. Let's remove it. Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Balbir Singh <balbir@linux.vnet.ibm.com> Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Signed-off-by: Minchan Kim <minchan.kim@gmail.com> --- mm/migrate.c | 1 - 1 files changed, 0 insertions(+), 1 deletions(-) diff --git a/mm/migrate.c b/mm/migrate.c index eb083a6..2abc9c9 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -683,7 +683,6 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private, rc = -ENOMEM; goto unlock; } - BUG_ON(charge); if (PageWriteback(page)) { if (!force || !sync) -- 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 internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] memcg: remove unnecessary BUG_ON 2011-02-20 15:17 ` [PATCH v2 1/2] memcg: remove unnecessary BUG_ON Minchan Kim @ 2011-02-21 13:04 ` Johannes Weiner 2011-02-21 14:14 ` Minchan Kim 0 siblings, 1 reply; 6+ messages in thread From: Johannes Weiner @ 2011-02-21 13:04 UTC (permalink / raw) To: Minchan Kim Cc: Andrew Morton, linux-mm, LKML, Balbir Singh, KAMEZAWA Hiroyuki, Daisuke Nishimura On Mon, Feb 21, 2011 at 12:17:17AM +0900, Minchan Kim wrote: > Now memcg in unmap_and_move checks BUG_ON of charge. > But mem_cgroup_prepare_migration returns either 0 or -ENOMEM. > If it returns -ENOMEM, it jumps out unlock without the check. > If it returns 0, it can pass BUG_ON. So it's meaningless. > Let's remove it. > > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: Balbir Singh <balbir@linux.vnet.ibm.com> > Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> > Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com> > --- > mm/migrate.c | 1 - > 1 files changed, 0 insertions(+), 1 deletions(-) > > diff --git a/mm/migrate.c b/mm/migrate.c > index eb083a6..2abc9c9 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -683,7 +683,6 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private, > rc = -ENOMEM; > goto unlock; > } > - BUG_ON(charge); You remove this assertion of the mem_cgroup_prepare_migration() return value but only add a comment about the expectations in the next patch. Could you write a full-blown kerneldoc on mem_cgroup_prepare_migration and remove this BUG_ON() in the same patch? -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] memcg: remove unnecessary BUG_ON 2011-02-21 13:04 ` Johannes Weiner @ 2011-02-21 14:14 ` Minchan Kim 0 siblings, 0 replies; 6+ messages in thread From: Minchan Kim @ 2011-02-21 14:14 UTC (permalink / raw) To: Johannes Weiner Cc: Andrew Morton, linux-mm, LKML, Balbir Singh, KAMEZAWA Hiroyuki, Daisuke Nishimura On Mon, Feb 21, 2011 at 10:04 PM, Johannes Weiner <hannes@cmpxchg.org> wrote: > On Mon, Feb 21, 2011 at 12:17:17AM +0900, Minchan Kim wrote: >> Now memcg in unmap_and_move checks BUG_ON of charge. >> But mem_cgroup_prepare_migration returns either 0 or -ENOMEM. >> If it returns -ENOMEM, it jumps out unlock without the check. >> If it returns 0, it can pass BUG_ON. So it's meaningless. >> Let's remove it. >> >> Cc: Johannes Weiner <hannes@cmpxchg.org> >> Cc: Balbir Singh <balbir@linux.vnet.ibm.com> >> Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> >> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> >> Signed-off-by: Minchan Kim <minchan.kim@gmail.com> >> --- >> mm/migrate.c | 1 - >> 1 files changed, 0 insertions(+), 1 deletions(-) >> >> diff --git a/mm/migrate.c b/mm/migrate.c >> index eb083a6..2abc9c9 100644 >> --- a/mm/migrate.c >> +++ b/mm/migrate.c >> @@ -683,7 +683,6 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private, >> rc = -ENOMEM; >> goto unlock; >> } >> - BUG_ON(charge); > > You remove this assertion of the mem_cgroup_prepare_migration() return > value but only add a comment about the expectations in the next patch. > > Could you write a full-blown kerneldoc on mem_cgroup_prepare_migration > and remove this BUG_ON() in the same patch? > Okay. I could. -- 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 internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] memcg: remove charge variable in unmap_and_move 2011-02-20 15:17 [PATCH v2 0/2] memcg: migration clean up Minchan Kim 2011-02-20 15:17 ` [PATCH v2 1/2] memcg: remove unnecessary BUG_ON Minchan Kim @ 2011-02-20 15:17 ` Minchan Kim 2011-02-21 13:10 ` Johannes Weiner 1 sibling, 1 reply; 6+ messages in thread From: Minchan Kim @ 2011-02-20 15:17 UTC (permalink / raw) To: Andrew Morton Cc: linux-mm, LKML, Johannes Weiner, Balbir Singh, KAMEZAWA Hiroyuki, Daisuke Nishimura, Minchan Kim memcg charge/uncharge could be handled by mem_cgroup_[prepare/end] migration itself so charge local variable in unmap_and_move lost the role since we introduced 01b1ae63c2. In addition, the variable name is not good like below. 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); .. } So let's remove unnecessary and confusing variable. Cc: Johannes Weiner <hannes@cmpxchg.org> Cc: Balbir Singh <balbir@linux.vnet.ibm.com> Suggested-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> Signed-off-by: Minchan Kim <minchan.kim@gmail.com> --- mm/memcontrol.c | 1 + mm/migrate.c | 9 +++------ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 8a97571..3c91d5c 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -2873,6 +2873,7 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry, /* * Before starting migration, account PAGE_SIZE to mem_cgroup that the old * page belongs to. + * Note: Should not return -EAGAIN. unmap_and_move depens on it. */ int mem_cgroup_prepare_migration(struct page *page, struct page *newpage, struct mem_cgroup **ptr, gfp_t gfp_mask) diff --git a/mm/migrate.c b/mm/migrate.c index 2abc9c9..37055d0 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -622,7 +622,6 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private, int *result = NULL; struct page *newpage = get_new_page(page, private, &result); int remap_swapcache = 1; - int charge = 0; struct mem_cgroup *mem; struct anon_vma *anon_vma = NULL; @@ -637,7 +636,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private, if (unlikely(split_huge_page(page))) goto move_newpage; - /* prepare cgroup just returns 0 or -ENOMEM */ + /* mem_cgroup_prepage_migration never returns -EAGAIN */ rc = -EAGAIN; if (!trylock_page(page)) { @@ -678,8 +677,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private, } /* charge against new page */ - charge = mem_cgroup_prepare_migration(page, newpage, &mem, GFP_KERNEL); - if (charge == -ENOMEM) { + if (mem_cgroup_prepare_migration(page, newpage, &mem, GFP_KERNEL)) { rc = -ENOMEM; goto unlock; } @@ -766,8 +764,7 @@ skip_unmap: drop_anon_vma(anon_vma); uncharge: - if (!charge) - mem_cgroup_end_migration(mem, page, newpage, rc == 0); + 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 internet charges in Canada: sign http://stopthemeter.ca/ Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a> ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] memcg: remove charge variable in unmap_and_move 2011-02-20 15:17 ` [PATCH v2 2/2] memcg: remove charge variable in unmap_and_move Minchan Kim @ 2011-02-21 13:10 ` Johannes Weiner 0 siblings, 0 replies; 6+ messages in thread From: Johannes Weiner @ 2011-02-21 13:10 UTC (permalink / raw) To: Minchan Kim Cc: Andrew Morton, linux-mm, LKML, Balbir Singh, KAMEZAWA Hiroyuki, Daisuke Nishimura On Mon, Feb 21, 2011 at 12:17:18AM +0900, Minchan Kim wrote: > memcg charge/uncharge could be handled by mem_cgroup_[prepare/end] > migration itself so charge local variable in unmap_and_move lost the role > since we introduced 01b1ae63c2. > > In addition, the variable name is not good like below. > > 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); > .. > } > > So let's remove unnecessary and confusing variable. > > Cc: Johannes Weiner <hannes@cmpxchg.org> > Cc: Balbir Singh <balbir@linux.vnet.ibm.com> > Suggested-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> > Acked-by: Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> > Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> > Signed-off-by: Minchan Kim <minchan.kim@gmail.com> > --- > mm/memcontrol.c | 1 + > mm/migrate.c | 9 +++------ > 2 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 8a97571..3c91d5c 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -2873,6 +2873,7 @@ static inline int mem_cgroup_move_swap_account(swp_entry_t entry, > /* > * Before starting migration, account PAGE_SIZE to mem_cgroup that the old > * page belongs to. > + * Note: Should not return -EAGAIN. unmap_and_move depens on it. > */ > int mem_cgroup_prepare_migration(struct page *page, > struct page *newpage, struct mem_cgroup **ptr, gfp_t gfp_mask) > diff --git a/mm/migrate.c b/mm/migrate.c > index 2abc9c9..37055d0 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -622,7 +622,6 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private, > int *result = NULL; > struct page *newpage = get_new_page(page, private, &result); > int remap_swapcache = 1; > - int charge = 0; > struct mem_cgroup *mem; > struct anon_vma *anon_vma = NULL; > > @@ -637,7 +636,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private, > if (unlikely(split_huge_page(page))) > goto move_newpage; > > - /* prepare cgroup just returns 0 or -ENOMEM */ > + /* mem_cgroup_prepage_migration never returns -EAGAIN */ > rc = -EAGAIN; I really don't like this. Why should we depend on that? > if (!trylock_page(page)) { > @@ -678,8 +677,7 @@ static int unmap_and_move(new_page_t get_new_page, unsigned long private, > } > > /* charge against new page */ > - charge = mem_cgroup_prepare_migration(page, newpage, &mem, GFP_KERNEL); > - if (charge == -ENOMEM) { > + if (mem_cgroup_prepare_migration(page, newpage, &mem, GFP_KERNEL)) { > rc = -ENOMEM; > goto unlock; Couldn't we make unmap_and_move completely oblivious of the specific value and just do rc = mem_cgroup_prepare_migration() if (rc) goto unlock; at this point? I think mem_cgroup_prepare_migration should be rather free to signal pretty much any error and it is up to migrate_pages() to handle them correctly. -- 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> ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-02-21 14:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-02-20 15:17 [PATCH v2 0/2] memcg: migration clean up Minchan Kim 2011-02-20 15:17 ` [PATCH v2 1/2] memcg: remove unnecessary BUG_ON Minchan Kim 2011-02-21 13:04 ` Johannes Weiner 2011-02-21 14:14 ` Minchan Kim 2011-02-20 15:17 ` [PATCH v2 2/2] memcg: remove charge variable in unmap_and_move Minchan Kim 2011-02-21 13:10 ` Johannes Weiner
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).