Messages in this thread | | | Date | Thu, 6 Jan 2011 11:49:36 +0900 | Subject | Re: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache | From | 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. >> > >> > 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> Reviewed-by: Minchan Kim <minchan.kim@gmail.com>
Below minor nitpick.
> --- > 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);
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? I don't want to bother you since Kame already acked it so I will depend on your decision.
-- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |