Message-ID: <53A41F54.8000501@suse.cz>
Date:	Fri, 20 Jun 2014 13:47:32 +0200
From:	Vlastimil Babka <vbabka@...e.cz>
To:	Minchan Kim <minchan@...nel.org>
CC:	David Rientjes <rientjes@...gle.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org,
	Andrew Morton <akpm@...ux-foundation.org>,
	Greg Thelen <gthelen@...gle.com>, Mel Gorman <mgorman@...e.de>,
	Joonsoo Kim <iamjoonsoo.kim@....com>,
	Michal Nazarewicz <mina86@...a86.com>,
	Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
	Christoph Lameter <cl@...ux.com>,
	Rik van Riel <riel@...hat.com>
Subject: Re: [PATCH 02/10] mm, compaction: report compaction as contended
 only due to lock contention

On 06/13/2014 04:40 AM, Minchan Kim wrote:
> On Thu, Jun 12, 2014 at 04:02:04PM +0200, Vlastimil Babka wrote:
>> On 06/12/2014 01:49 AM, Minchan Kim wrote:
>> >On Wed, Jun 11, 2014 at 02:22:30PM +0200, Vlastimil Babka wrote:
>> >>On 06/11/2014 03:10 AM, Minchan Kim wrote:
>> >>>On Mon, Jun 09, 2014 at 11:26:14AM +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 hugepaged 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
>> >>>>contantion has been detected.
>> >>>>
>> >>>>As a result, hugepaged will proceed with second sync compaction as intended,
>> >>>>when the preceding async compaction aborted due to need_resched().
>> >>>
>> >>>You said "second direct compaction is never attempted, which might be OK
>> >>>for a page fault" and said "hugepagd is intented to attempt a sync compaction"
>> >>>so I feel you want to handle khugepaged so special unlike other direct compact
>> >>>(ex, page fault).
>> >>
>> >>Well khugepaged is my primary concern, but I imagine there are other
>> >>direct compaction users besides THP page fault and khugepaged.
>> >>
>> >>>By this patch, direct compaction take care only lock contention, not rescheduling
>> >>>so that pop questions.
>> >>>
>> >>>Is it okay not to consider need_resched in direct compaction really?
>> >>
>> >>It still considers need_resched() to back of from async compaction.
>> >>It's only about signaling contended_compaction back to
>> >>__alloc_pages_slowpath(). There's this code executed after the
>> >>first, async compaction fails:
>> >>
>> >>/*
>> >>  * It can become very expensive to allocate transparent hugepages at
>> >>  * fault, so use asynchronous memory compaction for THP unless it is
>> >>  * khugepaged trying to collapse.
>> >>  */
>> >>if (!(gfp_mask & __GFP_NO_KSWAPD) || (current->flags & PF_KTHREAD))
>> >>         migration_mode = MIGRATE_SYNC_LIGHT;
>> >>
>> >>/*
>> >>  * If compaction is deferred for high-order allocations, it is because
>> >>  * sync compaction recently failed. In this is the case and the caller
>> >>  * requested a movable allocation that does not heavily disrupt the
>> >>  * system then fail the allocation instead of entering direct reclaim.
>> >>  */
>> >>if ((deferred_compaction || contended_compaction) &&
>> >>                                         (gfp_mask & __GFP_NO_KSWAPD))
>> >>         goto nopage;
>> >>
>> >>Both THP page fault and khugepaged use __GFP_NO_KSWAPD. The first
>> >>if() decides whether the second attempt will be sync (for
>> >>khugepaged) or async (page fault). The second if() decides that if
>> >>compaction was contended, then there won't be any second attempt
>> >>(and reclaim) at all. Counting need_resched() as contended in this
>> >>case is bad for khugepaged. Even for page fault it means no direct
>> >
>> >I agree khugepaged shouldn't count on need_resched, even lock contention
>> >because it was a result from admin's decision.
>> >If it hurts system performance, he should adjust knobs for khugepaged.
>> >
>> >>reclaim and a second async compaction. David says need_resched()
>> >>occurs so often then it is a poor heuristic to decide this.
>> >
>> >But page fault is a bit different. Inherently, high-order allocation
>> >(ie, above PAGE_ALLOC_COSTLY_ORDER) is fragile so all of the caller
>> >shoud keep in mind that and prepare second plan(ex, 4K allocation)
>> >so direct reclaim/compaction should take care of latency rather than
>> >success ratio.
>> 
>> Yes it's a rather delicate balance. But the plan is now to try
>> balance this differently than using need_resched.
>> 
>> >If need_resched in second attempt(ie, synchronous compaction) is almost
>> >true, it means the process consumed his timeslice so it shouldn't be
>> >greedy and gives a CPU resource to others.
>> 
>> Synchronous compaction uses cond_resched() so that's fine I think?
> 
> Sorry for being not clear. I post for the clarification before taking
> a rest in holiday. :)
> 
> When THP page fault occurs and found rescheduling while doing async
> direct compaction, it goes "nopage" and fall-backed to 4K page.
> It's good to me.
> 
> Another topic: I couldn't find any cond_resched. Anyway, it could be
> another patch.
> 

Thanks for the explanation. I'll include a cond_resched() at the level of
try_to_compact_pages() where it fits better, so it's not necessary in the place you
suggested. This should solve the "don't be greedy" problem. I will not yet include
the "bail out for latency" part because we are now slowly moving towards removing
need_resched() as a condition for stopping compaction, and this would on the contrary
extend it to prevent direct reclaim as well. David's data suggests that compaction often
bails out due to need_resched(), so this would reduce the amount of direct reclaim and I
don't want to touch that area in this series :)
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists