Re: [PATCH 02/10] mm, compaction: report compaction as contended only due to lock contention

[Minchan Kim]

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.