On Wed, 12 Sep 2012 02:48:40 +0200
Andrea Arcangeli <aarcange@xxxxxxxxxx> wrote:
> On Tue, Sep 11, 2012 at 04:34:55PM -0700, Andrew Morton wrote:
> > On Mon, 10 Sep 2012 09:18:30 +0800
> > Shaohua Li <shli@xxxxxxxxxx> wrote:
> >
> > > isolate_migratepages_range() might isolate none pages, for example, when
> > > zone->lru_lock is contended and compaction is async. In this case, we should
> > > abort compaction, otherwise, compact_zone will run a useless loop and make
> > > zone->lru_lock is even contended.
> > >
> > > ...
> > >
> > > @@ -838,12 +838,14 @@ static unsigned long compact_zone_order(
> > > .migratetype = allocflags_to_migratetype(gfp_mask),
> > > .zone = zone,
> > > .sync = sync,
> > > - .contended = contended,
> > > };
> > > INIT_LIST_HEAD(&cc.freepages);
> > > INIT_LIST_HEAD(&cc.migratepages);
> > >
> > > - return compact_zone(zone, &cc);
> > > + ret = compact_zone(zone, &cc);
> > > + if (contended)
> > > + *contended = cc.contended;
> > > + return ret;
> > > }
> > >
> >
> > From a quick read, `contended' is never NULL here. And defining the
>
> "contended" pointer can be null with some caller so the if is
> needed. The inner code was checking it before. This is also why we
> couldn't use *contended for the loop break bugfix, because contended
> could have been null at times.
Confused. I can see only two call sites:
__alloc_pages_slowpath
->__alloc_pages_direct_compact
->try_to_compact_pages
->compact_zone_order
and in both cases, `contended' points at valid storage.
> Now cc.contended is always available so
> we can use that to fix the bug and it's self documenting as well.
>
> > interface so that `contended' must be a valid pointer is a good change,
> > IMO - it results in simpler and faster code.
>
> Agreed.
OK, I'll slip this in there:
--- a/mm/compaction.c~mm-compaction-abort-compaction-loop-if-lock-is-contended-or-run-too-long-fix
+++ a/mm/compaction.c
@@ -909,8 +909,7 @@ static unsigned long compact_zone_order(
INIT_LIST_HEAD(&cc.migratepages);
ret = compact_zone(zone, &cc);
- if (contended)
- *contended = cc.contended;
+ *contended = cc.contended;
return ret;
}
> > Alas, try_to_compact_pages()'s kerneldoc altogether forgets to describe
> > this argument. Mel's
> > mm-compaction-capture-a-suitable-high-order-page-immediately-when-it-is-made-available.patch
> > adds a `pages' arg and forgets to document that as well.
poke poke
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>