Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753175AbaFWXeT (ORCPT ); Mon, 23 Jun 2014 19:34:19 -0400 Received: from lgeamrelo01.lge.com ([156.147.1.125]:35927 "EHLO lgeamrelo01.lge.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752260AbaFWXeS (ORCPT ); Mon, 23 Jun 2014 19:34:18 -0400 X-Original-SENDERIP: 10.177.220.169 X-Original-MAILFROM: minchan@kernel.org Date: Tue, 24 Jun 2014 08:35:07 +0900 From: Minchan Kim To: Zhang Yanfei Cc: Vlastimil Babka , linux-mm@kvack.org, Andrew Morton , David Rientjes , Mel Gorman , Joonsoo Kim , Michal Nazarewicz , Naoya Horiguchi , Christoph Lameter , Rik van Riel , linux-kernel@vger.kernel.org Subject: Re: [PATCH v3 05/13] mm, compaction: report compaction as contended only due to lock contention Message-ID: <20140623233507.GF15594@bbox> References: <1403279383-5862-1-git-send-email-vbabka@suse.cz> <1403279383-5862-6-git-send-email-vbabka@suse.cz> <20140623013903.GA12413@bbox> <53A7EB9B.5000406@cn.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <53A7EB9B.5000406@cn.fujitsu.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hello Zhang, On Mon, Jun 23, 2014 at 04:55:55PM +0800, Zhang Yanfei wrote: > Hello Minchan, > > On 06/23/2014 09:39 AM, Minchan Kim wrote: > > Hello Vlastimil, > > > > On Fri, Jun 20, 2014 at 05:49:35PM +0200, Vlastimil Babka wrote: > >> Async compaction aborts when it detects zone lock contention or need_resched() > >> is true. David Rientjes has reported that in practice, most direct async > >> compactions for THP allocation abort due to need_resched(). This means that a > >> second direct compaction is never attempted, which might be OK for a page > >> fault, but khugepaged is intended to attempt a sync compaction in such case and > >> in these cases it won't. > >> > >> This patch replaces "bool contended" in compact_control with an enum that > >> distinguieshes between aborting due to need_resched() and aborting due to lock > >> contention. This allows propagating the abort through all compaction functions > >> as before, but declaring the direct compaction as contended only when lock > >> contention has been detected. > >> > >> A second problem is that try_to_compact_pages() did not act upon the reported > >> contention (both need_resched() or lock contention) and could proceed with > >> another zone from the zonelist. When need_resched() is true, that means > >> initializing another zone compaction, only to check again need_resched() in > >> isolate_migratepages() and aborting. For zone lock contention, the unintended > >> consequence is that the contended status reported back to the allocator > >> is decided from the last zone where compaction was attempted, which is rather > >> arbitrary. > >> > >> This patch fixes the problem in the following way: > >> - need_resched() being true after async compaction returned from a zone means > >> that further zones should not be tried. We do a cond_resched() so that we > >> do not hog the CPU, and abort. "contended" is reported as false, since we > >> did not fail due to lock contention. > >> - aborting zone compaction due to lock contention means we can still try > >> another zone, since it has different locks. We report back "contended" as > >> true only if *all* zones where compaction was attempted, it aborted due to > >> lock contention. > >> > >> As a result of these fixes, khugepaged will proceed with second sync compaction > >> as intended, when the preceding async compaction aborted due to need_resched(). > >> Page fault compactions aborting due to need_resched() will spare some cycles > >> previously wasted by initializing another zone compaction only to abort again. > >> Lock contention will be reported only when compaction in all zones aborted due > >> to lock contention, and therefore it's not a good idea to try again after > >> reclaim. > >> > >> Reported-by: David Rientjes > >> Signed-off-by: Vlastimil Babka > >> Cc: Minchan Kim > >> Cc: Mel Gorman > >> Cc: Joonsoo Kim > >> Cc: Michal Nazarewicz > >> Cc: Naoya Horiguchi > >> Cc: Christoph Lameter > >> Cc: Rik van Riel > >> --- > >> mm/compaction.c | 48 +++++++++++++++++++++++++++++++++++++++--------- > >> mm/internal.h | 15 +++++++++++---- > >> 2 files changed, 50 insertions(+), 13 deletions(-) > >> > >> diff --git a/mm/compaction.c b/mm/compaction.c > >> index ebe30c9..e8cfac9 100644 > >> --- a/mm/compaction.c > >> +++ b/mm/compaction.c > >> @@ -180,9 +180,14 @@ static void update_pageblock_skip(struct compact_control *cc, > >> } > >> #endif /* CONFIG_COMPACTION */ > >> > >> -static inline bool should_release_lock(spinlock_t *lock) > >> +enum compact_contended should_release_lock(spinlock_t *lock) > >> { > >> - return need_resched() || spin_is_contended(lock); > >> + if (spin_is_contended(lock)) > >> + return COMPACT_CONTENDED_LOCK; > >> + else if (need_resched()) > >> + return COMPACT_CONTENDED_SCHED; > >> + else > >> + return COMPACT_CONTENDED_NONE; > > > > If you want to raise priority of lock contention than need_resched > > intentionally, please write it down on comment. > > > >> } > >> > >> /* > >> @@ -197,7 +202,9 @@ static inline bool should_release_lock(spinlock_t *lock) > >> static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags, > >> bool locked, struct compact_control *cc) > >> { > >> - if (should_release_lock(lock)) { > >> + enum compact_contended contended = should_release_lock(lock); > >> + > >> + if (contended) { > >> if (locked) { > >> spin_unlock_irqrestore(lock, *flags); > >> locked = false; > >> @@ -205,7 +212,7 @@ static bool compact_checklock_irqsave(spinlock_t *lock, unsigned long *flags, > >> > >> /* async aborts if taking too long or contended */ > >> if (cc->mode == MIGRATE_ASYNC) { > >> - cc->contended = true; > >> + cc->contended = contended; > >> return false; > >> } > > > > > >> > >> @@ -231,7 +238,7 @@ static inline bool compact_should_abort(struct compact_control *cc) > >> /* async compaction aborts if contended */ > >> if (need_resched()) { > >> if (cc->mode == MIGRATE_ASYNC) { > >> - cc->contended = true; > >> + cc->contended = COMPACT_CONTENDED_SCHED; > >> return true; > >> } > >> > >> @@ -1101,7 +1108,8 @@ static unsigned long compact_zone_order(struct zone *zone, int order, > >> VM_BUG_ON(!list_empty(&cc.freepages)); > >> VM_BUG_ON(!list_empty(&cc.migratepages)); > >> > >> - *contended = cc.contended; > >> + /* We only signal lock contention back to the allocator */ > >> + *contended = cc.contended == COMPACT_CONTENDED_LOCK; > > > > Please write down *WHY* as well as your intention we can know by looking at code. > > > >> return ret; > >> } > >> > >> @@ -1132,6 +1140,7 @@ unsigned long try_to_compact_pages(struct zonelist *zonelist, > >> struct zone *zone; > >> int rc = COMPACT_SKIPPED; > >> int alloc_flags = 0; > >> + bool all_zones_contended = true; > >> > >> /* Check if the GFP flags allow compaction */ > >> if (!order || !may_enter_fs || !may_perform_io) > >> @@ -1146,6 +1155,7 @@ unsigned long try_to_compact_pages(struct zonelist *zonelist, > >> for_each_zone_zonelist_nodemask(zone, z, zonelist, high_zoneidx, > >> nodemask) { > >> int status; > >> + bool zone_contended; > >> > >> if (compaction_deferred(zone, order)) > >> continue; > >> @@ -1153,8 +1163,9 @@ unsigned long try_to_compact_pages(struct zonelist *zonelist, > >> *deferred = false; > >> > >> status = compact_zone_order(zone, order, gfp_mask, mode, > >> - contended); > >> + &zone_contended); > >> rc = max(status, rc); > >> + all_zones_contended &= zone_contended; > >> > >> /* If a normal allocation would succeed, stop compacting */ > >> if (zone_watermark_ok(zone, order, low_wmark_pages(zone), 0, > >> @@ -1168,12 +1179,31 @@ unsigned long try_to_compact_pages(struct zonelist *zonelist, > >> * succeeding after all, it will be reset. > >> */ > >> defer_compaction(zone, order); > >> + /* > >> + * If we stopped compacting due to need_resched(), do > >> + * not try further zones and yield the CPU. > >> + */ > > > > For what? It would make your claim more clear. > > > >> + if (need_resched()) { > > > > compact_zone_order returns true state of contended only if it was lock contention > > so it couldn't return true state of contended by need_resched so you made > > need_resched check in here. It's fragile to me because it could be not a result > > from ahead compact_zone_order call. More clear thing is compact_zone_order > > should return zone_contended as enum, not bool and in here, you could check it. > > > > It means you could return enum in compact_zone_order and make the result bool > > in try_to_compact_pages. > > > >> + /* > >> + * We might not have tried all the zones, so > >> + * be conservative and assume they are not > >> + * all lock contended. > >> + */ > >> + all_zones_contended = false; > >> + cond_resched(); > >> + break; > >> + } > >> } > >> } > >> > >> - /* If at least one zone wasn't deferred, we count a compaction stall */ > >> - if (!*deferred) > >> + /* > >> + * If at least one zone wasn't deferred, we count a compaction stall > >> + * and we report if all zones that were tried were contended. > >> + */ > >> + if (!*deferred) { > >> count_compact_event(COMPACTSTALL); > >> + *contended = all_zones_contended; > > > > Why don't you initialize contended as *false* in function's intro? > > > >> + } > >> > >> return rc; > >> } > >> diff --git a/mm/internal.h b/mm/internal.h > >> index a1b651b..2c187d2 100644 > >> --- a/mm/internal.h > >> +++ b/mm/internal.h > >> @@ -117,6 +117,13 @@ extern int user_min_free_kbytes; > >> > >> #if defined CONFIG_COMPACTION || defined CONFIG_CMA > >> > >> +/* Used to signal whether compaction detected need_sched() or lock contention */ > >> +enum compact_contended { > >> + COMPACT_CONTENDED_NONE = 0, /* no contention detected */ > >> + COMPACT_CONTENDED_SCHED, /* need_sched() was true */ > >> + COMPACT_CONTENDED_LOCK, /* zone lock or lru_lock was contended */ > >> +}; > >> + > >> /* > >> * in mm/compaction.c > >> */ > >> @@ -144,10 +151,10 @@ struct compact_control { > >> int order; /* order a direct compactor needs */ > >> int migratetype; /* MOVABLE, RECLAIMABLE etc */ > >> struct zone *zone; > >> - bool contended; /* True if a lock was contended, or > >> - * need_resched() true during async > >> - * compaction > >> - */ > >> + enum compact_contended contended; /* Signal need_sched() or lock > >> + * contention detected during > >> + * compaction > >> + */ > >> }; > >> > >> unsigned long > >> -- > > > > Anyway, most big concern is that you are changing current behavior as > > I said earlier. > > > > Old behavior in THP page fault when it consumes own timeslot was just > > abort and fallback 4K page but with your patch, new behavior is > > take a rest when it founds need_resched and goes to another round with > > async, not sync compaction. I'm not sure we need another round with > > async compaction at the cost of increasing latency rather than fallback > > 4 page. > > I don't see the new behavior works like what you said. If need_resched > is true, it calls cond_resched() and after a rest it just breaks the loop. > Why there is another round with async compact? One example goes Old: page fault huge page allocation __alloc_pages_slowpath __alloc_pages_direct_compact compact_zone_order isolate_migratepages compact_checklock_irqsave need_resched is true cc->contended = true; return ISOLATE_ABORT return COMPACT_PARTIAL with *contented = cc.contended; COMPACTFAIL if (contended_compaction && gfp_mask & __GFP_NO_KSWAPD) goto nopage; New: page fault huge page allocation __alloc_pages_slowpath __alloc_pages_direct_compact compact_zone_order isolate_migratepages compact_unlock_should_abort need_resched is true cc->contended = COMPACT_CONTENDED_SCHED; return true; return ISOLATE_ABORT return COMPACT_PARTIAL with *contended = cc.contended == COMPACT_CONTENDED_LOCK (1) COMPACTFAIL if (contended_compaction && gfp_mask & __GFP_NO_KSWAPD) no goto nopage because contended_compaction was false by (1) __alloc_pages_direct_reclaim if (should_alloc_retry) else __alloc_pages_direct_compact again with ASYNC_MODE > > Thanks. > > > > > It might be okay if the VMA has MADV_HUGEPAGE which is good hint to > > indicate non-temporal VMA so latency would be trade-off but it's not > > for temporal big memory allocation in HUGEPAGE_ALWAYS system. > > > > If you really want to go this, could you show us numbers? > > > > 1. How many could we can be successful in direct compaction by this patch? > > 2. How long could we increase latency for temporal allocation > > for HUGEPAGE_ALWAYS system? > > > > > -- > Thanks. > Zhang Yanfei > > -- > 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: email@kvack.org -- Kind regards, Minchan Kim -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/