lkml.org 
[lkml]   [2011]   [Jan]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [BUGFIX][PATCH] memcg: fix memory migration of shmem swapcache
    From
    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/

    \
     
     \ /
      Last update: 2011-01-06 03:51    [W:0.032 / U:1.832 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site