On Wed, 12 Sep 2012 11:55:35 +0100
Mel Gorman <mgorman@xxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>