Re: [patch 2/2 v2]compaction: check lock contention first before taking lock

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

On Wed, 12 Sep 2012 11:55:35 +0100
Mel Gorman <mgorman@suse.de> wrote:

> On Tue, Sep 11, 2012 at 04:43:30PM -0700, Andrew Morton wrote:
> > On Mon, 10 Sep 2012 09:18:50 +0800
> > Shaohua Li <shli@kernel.org> wrote:
> > 
> > > isolate_migratepages_range will take zone->lru_lock first and check if the lock
> > > is contented, if yes, it will release the lock. This isn't efficient. If the
> > > lock is truly contented, a lock/unlock pair will increase the lock contention.
> > > We'd better check if the lock is contended first. compact_trylock_irqsave
> > > perfectly meets the requirement.
> > > 
> > > V2:
> > > leave cond_resched() pointed out by Mel.
> > > 
> > > Signed-off-by: Shaohua Li <shli@fusionio.com>
> > > ---
> > >  mm/compaction.c |    5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > Index: linux/mm/compaction.c
> > > ===================================================================
> > > --- linux.orig/mm/compaction.c	2012-09-10 08:49:40.377869710 +0800
> > > +++ linux/mm/compaction.c	2012-09-10 08:53:10.295230575 +0800
> > > @@ -295,8 +295,9 @@ isolate_migratepages_range(struct zone *
> > >  
> > >  	/* Time to isolate some pages for migration */
> > >  	cond_resched();
> > > -	spin_lock_irqsave(&zone->lru_lock, flags);
> > > -	locked = true;
> > > +	locked = compact_trylock_irqsave(&zone->lru_lock, &flags, cc);
> > > +	if (!locked)
> > > +		return 0;
> > >  	for (; low_pfn < end_pfn; low_pfn++) {
> > >  		struct page *page;
> > 
> > Geeze that compact_checklock_irqsave stuff is naaaasty.
> > 
> 
> The intention is to avoid THP allocations getting stuck in compaction.c
> for ages due to spinlock contention. It's always better for those to
> fail quickly. If compact_trylock_irqsave is improved it must still be
> able to do this.

So there's an implicit two-level prioritization here.  But between what
and what?

It all sounds a bit hack/bandaidy?

> 
> > There is no relationship between the concepts "user
> > pressed ^C" and "this device driver or subsystem wants a high-order
> > allocation".
> > 
> 
> hmm, I see your point. The fatal signal check is "hidden" but this was to
> preseve the existing behaviour prior to commit [c67fe375: mm: compaction:
> Abort async compaction if locks are contended or taking too long]. The
> fatal_signal_check could be deleted from compact_trylock_irqsave() but
> then it should be checked in the isolate_migratepages_range() at the
> very least. How about this?

hm, well, actually, I chose ^C as an example of something which might
set need_resched().  How about `There is no relationship between the
concepts "this process exceeded its timeslice" and "this device driver
or subsystem wants a high-order allocation"'.

--
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>