Re: [RFC 13/13] mm, compaction: fix and improve watermark handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 05/16/2016 11:25 AM, Michal Hocko wrote:
On Tue 10-05-16 09:36:03, Vlastimil Babka wrote:
Compaction has been using watermark checks when deciding whether it was
successful, and whether compaction is at all suitable. There are few problems
with these checks.

- __compact_finished() uses low watermark in a check that has to pass if
   the direct compaction is to finish and allocation should succeed. This is
   too pessimistic, as the allocation will typically use min watermark. It
   may happen that during compaction, we drop below the low watermark (due to
   parallel activity), but still form the target high-order page. By checking
   against low watermark, we might needlessly continue compaction. After this
   patch, the check uses direct compactor's alloc_flags to determine the
   watermark, which is effectively the min watermark.

OK, this makes some sense. It would be great if we could have at least
some clarification why the low wmark has been used previously. Probably
Mel can remember?

- __compaction_suitable has the same issue in the check whether the allocation
   is already supposed to succeed and we don't need to compact. Fix it the same
   way.

- __compaction_suitable() then checks the low watermark plus a (2 << order) gap
   to decide if there's enough free memory to perform compaction. This check

And this was a real head scratcher when I started looking into the
compaction recently. Why do we need to be above low watermark to even
start compaction.

Hmm, above you said you're fine with low wmark (maybe after clarification). I don't know why it was used, can only guess.

Compaction uses additional memory only for a short
period of time and then releases the already migrated pages.

As for the 2 << order gap. I can imagine that e.g. order-5 compaction (32 pages) isolates 20 pages for migration and starts looking for free pages. It collects 19 free pages and then reaches an order-4 free page. Splitting that page to collect it would result in 19+16=35 pages isolated, thus exceed the 1 << order gap, and fail. With 2 << order gap, chances of this happening are reduced.

   uses direct compactor's alloc_flags, but that's wrong. If alloc_flags doesn't
   include ALLOC_CMA, we might fail the check, even though the freepage
   isolation isn't restricted outside of CMA pageblocks. On the other hand,
   alloc_flags may indicate access to memory reserves, making compaction proceed
   and then fail watermark check during freepage isolation, which doesn't pass
   alloc_flags. The fix here is to use fixed ALLOC_CMA flags in the
   __compaction_suitable() check.

This makes my head hurt. Whut?

I'll try to explain better next time.

- __isolate_free_page uses low watermark check to decide if free page can be
   isolated. It also doesn't use ALLOC_CMA, so add it for the same reasons.

Why do we check the watermark at all? What would happen if this obscure
if (!is_migrate_isolate(mt)) was gone? I remember I put some tracing
there and it never hit for me even when I was testing close to OOM
conditions. Maybe an earlier check bailed out but this code path looks
really obscure so it should either deserve a large fat comment or to
die.

The check is there so that compaction doesn't exhaust memory below reserves during its work, just like any other non-privileged allocation.

- The use of low watermark checks in __compaction_suitable() and
   __isolate_free_page does perhaps make sense for high-order allocations where
   more freepages increase the chance of success, and we can typically fail
   with some order-0 fallback when the system is struggling. But for low-order
   allocation, forming the page should not be that hard. So using low watermark
   here might just prevent compaction from even trying, and eventually lead to
   OOM killer even if we are above min watermarks. So after this patch, we use
   min watermark for non-costly orders in these checks, by passing the
   alloc_flags parameter to split_page() and __isolate_free_page().

OK, so if IIUC costly high order requests even shouldn't try when we are
below watermark (unless they are __GFP_REPEAT which would get us to a
stronger compaction mode/priority) and that would reclaim us over low
wmark and go on. Is that what you are saying? This makes some sense but
then let's have a _single_ place to check the watermak please. This
checks at few different levels is just subtle as hell and error prone
likewise.

What single place then? The situation might change dynamically so passing the initial __compaction_suitable() check doesn't guarantee that enough free pages are still available when it comes to isolating freepages. Your testing that never hit it shows that this is rare, but do we want to risk compaction making an OOM situation worse?

To sum up, after this patch, the kernel should in some situations finish
successful direct compaction sooner, prevent compaction from starting when it's
not needed, proceed with compaction when free memory is in CMA pageblocks, and
for non-costly orders, prevent OOM killing or excessive reclaim when free
memory is between the min and low watermarks.

Could you please split this patch into three(?) parts. One to remove as many
wmark checks as possible, move low wmark to min for !costly high orders
and finally the cma part which I fail to understand...

Sure, although I'm not yet convinced we can remove any checks.

Thanks!


--
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>



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]