Re: [PATCH 1/7] memcg : comment, style fixes for recent patch ofmove_parent

[Johannes Weiner]

On Mon, Jan 24, 2011 at 08:14:22PM +0900, Hiroyuki Kamezawa wrote:
> 2011/1/24 Johannes Weiner <hannes@xxxxxxxxxxx>:
> > On Mon, Jan 24, 2011 at 07:15:35PM +0900, KAMEZAWA Hiroyuki wrote:
> >> On Mon, 24 Jan 2011 11:14:02 +0100
> >> Johannes Weiner <hannes@xxxxxxxxxxx> wrote:
> >>
> >> > On Fri, Jan 21, 2011 at 03:37:26PM +0900, KAMEZAWA Hiroyuki wrote:
> >> > > From: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> >> > >
> >> > > A fix for 987eba66e0e6aa654d60881a14731a353ee0acb4
> >> > >
> >> > > A clean up for mem_cgroup_move_parent().
> >> > >  - remove unnecessary initialization of local variable.
> >> > >  - rename charge_size -> page_size
> >> > >  - remove unnecessary (wrong) comment.
> >> > >
> >> > > Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@xxxxxxxxxxxxxx>
> >> > > ---
> >> > >  mm/memcontrol.c |   17 +++++++++--------
> >> > >  1 file changed, 9 insertions(+), 8 deletions(-)
> >> > >
> >> > > Index: mmotm-0107/mm/memcontrol.c
> >> > > ===================================================================
> >> > > --- mmotm-0107.orig/mm/memcontrol.c
> >> > > +++ mmotm-0107/mm/memcontrol.c
> >> > > @@ -2265,7 +2265,7 @@ static int mem_cgroup_move_parent(struct
> >> > >   struct cgroup *cg = child->css.cgroup;
> >> > >   struct cgroup *pcg = cg->parent;
> >> > >   struct mem_cgroup *parent;
> >> > > - int charge = PAGE_SIZE;
> >> > > + int page_size;
> >> > >   unsigned long flags;
> >> > >   int ret;
> >> > >
> >> > > @@ -2278,22 +2278,23 @@ static int mem_cgroup_move_parent(struct
> >> > >           goto out;
> >> > >   if (isolate_lru_page(page))
> >> > >           goto put;
> >> > > - /* The page is isolated from LRU and we have no race with splitting */
> >> > > - charge = PAGE_SIZE << compound_order(page);
> >> > > +
> >> > > + page_size = PAGE_SIZE << compound_order(page);
> >> >
> >> > Okay, so you remove the wrong comment, but that does not make the code
> >> > right.  What protects compound_order from reading garbage because the
> >> > page is currently splitting?
> >> >
> >>
> >> ==
> >> static int mem_cgroup_move_account(struct page_cgroup *pc,
> >>                 struct mem_cgroup *from, struct mem_cgroup *to,
> >>                 bool uncharge, int charge_size)
> >> {
> >>         int ret = -EINVAL;
> >>         unsigned long flags;
> >>
> >>         if ((charge_size > PAGE_SIZE) && !PageTransHuge(pc->page))
> >>                 return -EBUSY;
> >> ==
> >>
> >> This is called under compound_lock(). Then, if someone breaks THP,
> >> -EBUSY and retry.
> >
> > This charge_size contains exactly the garbage you just read from an
> > unprotected compound_order().  It could be anything if the page is
> > split concurrently.
> 
> Then, my recent fix to LRU accounting which use compound_order() is racy, too ?

In lru add/delete/move/rotate?  No, that should be safe because we
have the lru lock there and __split_huge_page_refcount() takes the
lock as well.

> I'll replace compound_order() with
>   if (PageTransHuge(page))
>       size = HPAGE_SIZE.
> 
> Does this work ?

Yes, I think this should work.  This gives a sane size for try_charge
and we still catch a split under the compound_lock later in
move_account as you described above.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/