Re: [patch 1/2 v2]compaction: abort compaction loop if lock is contended or run too long

[Andrew Morton <akpm@linux-foundation.org>]

On Wed, 12 Sep 2012 02:48:40 +0200
Andrea Arcangeli <aarcange@redhat.com> 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@kernel.org> 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@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>