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, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxxx 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@xxxxxxxxx";> email@xxxxxxxxx </a>