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>