Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1390130 > unrolled thread

Re: [PATCH 09/14] mm: use compaction feedback for thp backoff conditions

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 09/14] mm: use compaction feedback for thp backoff  conditions Michal Hocko <mhocko@kernel.org> - 2016-04-28 14:40 +0200
    Re: [PATCH 09/14] mm: use compaction feedback for thp backoff  conditions Vlastimil Babka <vbabka@suse.cz> - 2016-04-29 11:20 +0200
      Re: [PATCH 09/14] mm: use compaction feedback for thp backoff  conditions Michal Hocko <mhocko@kernel.org> - 2016-04-29 11:30 +0200

#1390130 — Re: [PATCH 09/14] mm: use compaction feedback for thp backoff conditions

On Thu 28-04-16 10:53:18, Vlastimil Babka wrote:
> On 04/20/2016 09:47 PM, Michal Hocko wrote:
> >From: Michal Hocko <mhocko@suse.com>
> >
> >THP requests skip the direct reclaim if the compaction is either
> >deferred or contended to reduce stalls which wouldn't help the
> >allocation success anyway. These checks are ignoring other potential
> >feedback modes which we have available now.
> >
> >It clearly doesn't make much sense to go and reclaim few pages if the
> >previous compaction has failed.
> >
> >We can also simplify the check by using compaction_withdrawn which
> >checks for both COMPACT_CONTENDED and COMPACT_DEFERRED. This check
> >is however covering more reasons why the compaction was withdrawn.
> >None of them should be a problem for the THP case though.
> >
> >It is safe to back of if we see COMPACT_SKIPPED because that means
> >that compaction_suitable failed and a single round of the reclaim is
> >unlikely to make any difference here. We would have to be close to
> >the low watermark to reclaim enough and even then there is no guarantee
> >that the compaction would make any progress while the direct reclaim
> >would have caused the stall.
> >
> >COMPACT_PARTIAL_SKIPPED is slightly different because that means that we
> >have only seen a part of the zone so a retry would make some sense. But
> >it would be a compaction retry not a reclaim retry to perform. We are
> >not doing that and that might indeed lead to situations where THP fails
> >but this should happen only rarely and it would be really hard to
> >measure.
> >
> >Signed-off-by: Michal Hocko <mhocko@suse.com>
> 
> THP's don't compact by default in page fault path anymore, so we don't need
> to restrict them even more. And hopefully we'll replace the
> is_thp_gfp_mask() hack with something better soon, so this might be just
> extra code churn. But I don't feel strongly enough to nack it.

My main point was to simplify the code and get rid of as much compaction
specific hacks as possible. We might very well drop this later on but it
would be at least less code to grasp through. I do not have any problem
with dropping this but I think this shouldn't collide with other patches
much so reducing the number of lines is worth it.

-- 
Michal Hocko
SUSE Labs

[toc] | [next] | [standalone]

#1390917

On 04/28/2016 02:35 PM, Michal Hocko wrote:
> On Thu 28-04-16 10:53:18, Vlastimil Babka wrote:
>> On 04/20/2016 09:47 PM, Michal Hocko wrote:
>>> From: Michal Hocko <mhocko@suse.com>
>>>
>>> THP requests skip the direct reclaim if the compaction is either
>>> deferred or contended to reduce stalls which wouldn't help the
>>> allocation success anyway. These checks are ignoring other potential
>>> feedback modes which we have available now.
>>>
>>> It clearly doesn't make much sense to go and reclaim few pages if the
>>> previous compaction has failed.
>>>
>>> We can also simplify the check by using compaction_withdrawn which
>>> checks for both COMPACT_CONTENDED and COMPACT_DEFERRED. This check
>>> is however covering more reasons why the compaction was withdrawn.
>>> None of them should be a problem for the THP case though.
>>>
>>> It is safe to back of if we see COMPACT_SKIPPED because that means
>>> that compaction_suitable failed and a single round of the reclaim is
>>> unlikely to make any difference here. We would have to be close to

Hmm this is actually incorrect, as should_continue_reclaim() will keep 
shrink_zone() going as much as needed for compaction to become enabled, 
so it doesn't reclaim just SWAP_CLUSTER_MAX.

>>> the low watermark to reclaim enough and even then there is no guarantee
>>> that the compaction would make any progress while the direct reclaim
>>> would have caused the stall.
>>>
>>> COMPACT_PARTIAL_SKIPPED is slightly different because that means that we
>>> have only seen a part of the zone so a retry would make some sense. But
>>> it would be a compaction retry not a reclaim retry to perform. We are
>>> not doing that and that might indeed lead to situations where THP fails
>>> but this should happen only rarely and it would be really hard to
>>> measure.
>>>
>>> Signed-off-by: Michal Hocko <mhocko@suse.com>
>>
>> THP's don't compact by default in page fault path anymore, so we don't need
>> to restrict them even more. And hopefully we'll replace the
>> is_thp_gfp_mask() hack with something better soon, so this might be just
>> extra code churn. But I don't feel strongly enough to nack it.
>
> My main point was to simplify the code and get rid of as much compaction
> specific hacks as possible. We might very well drop this later on but it
> would be at least less code to grasp through. I do not have any problem
> with dropping this but I think this shouldn't collide with other patches
> much so reducing the number of lines is worth it.

I just realized it also affects khugepaged, and not just THP page 
faults, so it may potentially cripple THP's completely. My main issue is 
that the reasons to bail out includes COMPACT_SKIPPED, and for a wrong 
reason (see the comment above). It also goes against the comment below 
the noretry label:

  * High-order allocations do not necessarily loop after direct reclaim
  * and reclaim/compaction depends on compaction being called after
  * reclaim so call directly if necessary.

Given that THP's are large, I expect reclaim would indeed be quite often 
necessary before compaction, and the first optimistic async compaction 
attempt will just return SKIPPED. After this patch, there will be no 
more reclaim/compaction attempts for THP's, including khugepaged. And 
given the change of THP page fault defaults, even crippling that path 
should no longer be necessary.

So I would just drop this for now indeed.

[toc] | [prev] | [next] | [standalone]

#1390924

On Fri 29-04-16 11:16:44, Vlastimil Babka wrote:
> On 04/28/2016 02:35 PM, Michal Hocko wrote:
[...]
> >My main point was to simplify the code and get rid of as much compaction
> >specific hacks as possible. We might very well drop this later on but it
> >would be at least less code to grasp through. I do not have any problem
> >with dropping this but I think this shouldn't collide with other patches
> >much so reducing the number of lines is worth it.

Good point, I have completely missed this part.

> I just realized it also affects khugepaged, and not just THP page faults, so
> it may potentially cripple THP's completely. My main issue is that the
> reasons to bail out includes COMPACT_SKIPPED, and for a wrong reason (see
> the comment above). It also goes against the comment below the noretry
> label:
> 
>  * High-order allocations do not necessarily loop after direct reclaim
>  * and reclaim/compaction depends on compaction being called after
>  * reclaim so call directly if necessary.
> 
> Given that THP's are large, I expect reclaim would indeed be quite often
> necessary before compaction, and the first optimistic async compaction
> attempt will just return SKIPPED. After this patch, there will be no more
> reclaim/compaction attempts for THP's, including khugepaged. And given the
> change of THP page fault defaults, even crippling that path should no longer
> be necessary.
> 
> So I would just drop this for now indeed.

Agreed, thanks for catching this. Andrew, could you drop this patch
please? It was supposed to be a mere clean up without any effect on the
oom detection.

Thanks!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web