Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1383701 > unrolled thread
| Started by | Michal Hocko <mhocko@kernel.org> |
|---|---|
| First post | 2016-04-20 21:50 +0200 |
| Last post | 2016-05-12 13:10 +0200 |
| Articles | 20 on this page of 52 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
[PATCH 07/14] mm, compaction: Simplify __alloc_pages_direct_compact feedback interface Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
[PATCH 03/14] mm, compaction: cover all compaction mode in compact_zone Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
[PATCH 10/14] mm, oom: rework oom detection Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
[PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders Vlastimil Babka <vbabka@suse.cz> - 2016-04-28 11:00 +0200
Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders Michal Hocko <mhocko@kernel.org> - 2016-04-28 14:40 +0200
Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-04 08:30 +0200
Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders Michal Hocko <mhocko@kernel.org> - 2016-05-04 11:10 +0200
Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders Joonsoo Kim <js1304@gmail.com> - 2016-05-04 17:20 +0200
Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders Michal Hocko <mhocko@kernel.org> - 2016-05-04 21:30 +0200
[PATCH 11/14] mm: throttle on IO only when there are too many dirty and writeback pages Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
[PATCH 12/14] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
Re: [PATCH 12/14] mm, oom: protect !costly allocations some more Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-04 08:10 +0200
Re: [PATCH 12/14] mm, oom: protect !costly allocations some more Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-04 08:40 +0200
Re: [PATCH 12/14] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-05-04 11:00 +0200
Re: [PATCH 12/14] mm, oom: protect !costly allocations some more Joonsoo Kim <js1304@gmail.com> - 2016-05-04 17:00 +0200
Re: [PATCH 12/14] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-05-04 20:20 +0200
Re: [PATCH 12/14] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-05-04 11:00 +0200
Re: [PATCH 12/14] mm, oom: protect !costly allocations some more Joonsoo Kim <js1304@gmail.com> - 2016-05-04 16:40 +0200
Re: [PATCH 12/14] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-05-04 20:30 +0200
[PATCH 01/14] vmscan: consider classzone_idx in compaction_ready Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
Re: [PATCH 01/14] vmscan: consider classzone_idx in compaction_ready "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-04-21 06:40 +0200
Re: [PATCH 01/14] vmscan: consider classzone_idx in compaction_ready Michal Hocko <mhocko@kernel.org> - 2016-05-04 16:00 +0200
[PATCH 05/14] mm, compaction: distinguish between full and partial COMPACT_COMPLETE Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
Re: [PATCH 05/14] mm, compaction: distinguish between full and partial COMPACT_COMPLETE "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-04-21 08:40 +0200
[PATCH 08/14] mm, compaction: Abstract compaction feedback to helpers Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
Re: [PATCH 08/14] mm, compaction: Abstract compaction feedback to helpers Vlastimil Babka <vbabka@suse.cz> - 2016-04-28 10:50 +0200
[PATCH 09/14] mm: use compaction feedback for thp backoff conditions Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
Re: [PATCH 09/14] mm: use compaction feedback for thp backoff conditions "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-04-21 09:10 +0200
Re: [PATCH 09/14] mm: use compaction feedback for thp backoff conditions Vlastimil Babka <vbabka@suse.cz> - 2016-04-28 11:00 +0200
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
[PATCH 13/14] mm: consider compaction feedback also for costly allocation Michal Hocko <mhocko@kernel.org> - 2016-04-20 21:50 +0200
[PATCH 04/14] mm, compaction: distinguish COMPACT_DEFERRED from COMPACT_SKIPPED Michal Hocko <mhocko@kernel.org> - 2016-04-20 22:00 +0200
[PATCH 02/14] mm, compaction: change COMPACT_ constants into enum Michal Hocko <mhocko@kernel.org> - 2016-04-20 22:00 +0200
Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-04 07:50 +0200
Re: [PATCH 0.14] oom detection rework v6 Vlastimil Babka <vbabka@suse.cz> - 2016-05-04 10:20 +0200
Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-04 10:40 +0200
Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-04 11:00 +0200
Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-04 10:50 +0200
Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <js1304@gmail.com> - 2016-05-04 16:40 +0200
Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-04 20:20 +0200
Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <js1304@gmail.com> - 2016-05-10 08:50 +0200
Re: [PATCH 0.14] oom detection rework v6 Vlastimil Babka <vbabka@suse.cz> - 2016-05-10 09:10 +0200
Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <js1304@gmail.com> - 2016-05-10 10:10 +0200
Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-10 11:50 +0200
Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-10 11:50 +0200
Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-12 04:30 +0200
Re: [PATCH 0.14] oom detection rework v6 Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-05-12 07:20 +0200
Re: [PATCH 0.14] oom detection rework v6 Michal Hocko <mhocko@kernel.org> - 2016-05-12 13:10 +0200
Page 1 of 3 [1] 2 3 Next page →
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-04-20 21:50 +0200 |
| Subject | [PATCH 0.14] oom detection rework v6 |
| Message-ID | <rq6im-4F2-9@gated-at.bofh.it> |
Hi,
This is v6 of the series. The previous version was posted [1]. The
code hasn't changed much since then. I have found one old standing
bug (patch 1) which just got much more severe and visible with this
series. Other than that I have reorganized the series and put the
compaction feedback abstraction to the front just in case we find out
that parts of the series would have to be reverted later on for some
reason. The premature oom killer invocation reported by Hugh [2] seems
to be addressed.
We have discussed this series at LSF/MM summit in Raleigh and there
didn't seem to be any concerns/objections to go on with the patch set
and target it for the next merge window.
Motivation:
As pointed by Linus [3][4] relying on zone_reclaimable as a way to
communicate the reclaim progress is rater dubious. I tend to agree,
not only it is really obscure, it is not hard to imagine cases where a
single page freed in the loop keeps all the reclaimers looping without
getting any progress because their gfp_mask wouldn't allow to get that
page anyway (e.g. single GFP_ATOMIC alloc and free loop). This is rather
rare so it doesn't happen in the practice but the current logic which we
have is rather obscure and hard to follow a also non-deterministic.
This is an attempt to make the OOM detection more deterministic and
easier to follow because each reclaimer basically tracks its own
progress which is implemented at the page allocator layer rather spread
out between the allocator and the reclaim. The more on the implementation
is described in the first patch.
I have tested several different scenarios but it should be clear that
testing OOM killer is quite hard to be representative. There is usually
a tiny gap between almost OOM and full blown OOM which is often time
sensitive. Anyway, I have tested the following 2 scenarios and I would
appreciate if there are more to test.
Testing environment: a virtual machine with 2G of RAM and 2CPUs without
any swap to make the OOM more deterministic.
1) 2 writers (each doing dd with 4M blocks to an xfs partition with 1G
file size, removes the files and starts over again) running in
parallel for 10s to build up a lot of dirty pages when 100 parallel
mem_eaters (anon private populated mmap which waits until it gets
signal) with 80M each.
This causes an OOM flood of course and I have compared both patched
and unpatched kernels. The test is considered finished after there
are no OOM conditions detected. This should tell us whether there are
any excessive kills or some of them premature (e.g. due to dirty pages):
I have performed two runs this time each after a fresh boot.
* base kernel
$ grep "Out of memory:" base-oom-run1.log | wc -l
78
$ grep "Out of memory:" base-oom-run2.log | wc -l
78
$ grep "Kill process" base-oom-run1.log | tail -n1
[ 91.391203] Out of memory: Kill process 3061 (mem_eater) score 39 or sacrifice child
$ grep "Kill process" base-oom-run2.log | tail -n1
[ 82.141919] Out of memory: Kill process 3086 (mem_eater) score 39 or sacrifice child
$ grep "DMA32 free:" base-oom-run1.log | sed 's@.*free:\([0-9]*\)kB.*@\1@' | calc_min_max.awk
min: 5376.00 max: 6776.00 avg: 5530.75 std: 166.50 nr: 61
$ grep "DMA32 free:" base-oom-run2.log | sed 's@.*free:\([0-9]*\)kB.*@\1@' | calc_min_max.awk
min: 5416.00 max: 5608.00 avg: 5514.15 std: 42.94 nr: 52
$ grep "DMA32.*all_unreclaimable? no" base-oom-run1.log | wc -l
1
$ grep "DMA32.*all_unreclaimable? no" base-oom-run2.log | wc -l
3
* patched kernel
$ grep "Out of memory:" patched-oom-run1.log | wc -l
78
miso@tiehlicka /mnt/share/devel/miso/kvm $ grep "Out of memory:" patched-oom-run2.log | wc -l
77
e grep "Kill process" patched-oom-run1.log | tail -n1
[ 497.317732] Out of memory: Kill process 3108 (mem_eater) score 39 or sacrifice child
$ grep "Kill process" patched-oom-run2.log | tail -n1
[ 316.169920] Out of memory: Kill process 3093 (mem_eater) score 39 or sacrifice child
$ grep "DMA32 free:" patched-oom-run1.log | sed 's@.*free:\([0-9]*\)kB.*@\1@' | calc_min_max.awk
min: 5420.00 max: 5808.00 avg: 5513.90 std: 60.45 nr: 78
$ grep "DMA32 free:" patched-oom-run2.log | sed 's@.*free:\([0-9]*\)kB.*@\1@' | calc_min_max.awk
min: 5380.00 max: 6384.00 avg: 5520.94 std: 136.84 nr: 77
e grep "DMA32.*all_unreclaimable? no" patched-oom-run1.log | wc -l
2
$ grep "DMA32.*all_unreclaimable? no" patched-oom-run2.log | wc -l
3
The patched kernel run noticeably longer while invoking OOM killer same
number of times. This means that the original implementation is much
more aggressive and triggers the OOM killer sooner. free pages stats
show that neither kernels went OOM too early most of the time, though. I
guess the difference is in the backoff when retries without any progress
do sleep for a while if there is memory under writeback or dirty which
is highly likely considering the parallel IO.
Both kernels have seen races where zone wasn't marked unreclaimable
and we still hit the OOM killer. This is most likely a race where
a task managed to exit between the last allocation attempt and the oom
killer invocation.
2) 2 writers again with 10s of run and then 10 mem_eaters to consume as much
memory as possible without triggering the OOM killer. This required a lot
of tuning but I've considered 3 consecutive runs in three different boots
without OOM as a success.
* base kernel
size=$(awk '/MemFree/{printf "%dK", ($2/10)-(16*1024)}' /proc/meminfo)
* patched kernel
size=$(awk '/MemFree/{printf "%dK", ($2/10)-(12*1024)}' /proc/meminfo)
That means 40M more memory was usable without triggering OOM killer. The
base kernel sometimes managed to handle the same as patched but it
wasn't consistent and failed in at least on of the 3 runs. This seems
like a minor improvement.
I was testing also GPF_REPEAT costly requests (hughetlb) with fragmented
memory and under memory pressure. The results are in patch 11 where the
logic is implemented. In short I can see huge improvement there.
I am certainly interested in other usecases as well as well as any
feedback. Especially those which require higher order requests.
* Changes since v5
- added "vmscan: consider classzone_idx in compaction_ready"
- added "mm, oom, compaction: prevent from should_compact_retry looping
for ever for costly orders"
- acked-bys from Vlastimil
- integrated feedback from review
* Changes since v4
- dropped __GFP_REPEAT for costly allocation as it is now replaced by
the compaction based feedback logic
- !costly high order requests are retried based on the compaction feedback
- compaction feedback has been tweaked to give us an useful information
to make decisions in the page allocator
- rebased on the current mmotm-2016-04-01-16-24 with the previous version
of the rework reverted
* Changes since v3
- factor out the new heuristic into its own function as suggested by
Johannes (no functional changes)
* Changes since v2
- rebased on top of mmotm-2015-11-25-17-08 which includes
wait_iff_congested related changes which needed refresh in
patch#1 and patch#2
- use zone_page_state_snapshot for NR_FREE_PAGES per David
- shrink_zones doesn't need to return anything per David
- retested because the major kernel version has changed since
the last time (4.2 -> 4.3 based kernel + mmotm patches)
* Changes since v1
- backoff calculation was de-obfuscated by using DIV_ROUND_UP
- __GFP_NOFAIL high order migh fail fixed - theoretical bug
[1] http://lkml.kernel.org/r/1459855533-4600-1-git-send-email-mhocko@kernel.org
[2] http://lkml.kernel.org/r/alpine.LSU.2.11.1602241832160.15564@eggly.anvils
[3] http://lkml.kernel.org/r/CA+55aFwapaED7JV6zm-NVkP-jKie+eQ1vDXWrKD=SkbshZSgmw@mail.gmail.com
[4] http://lkml.kernel.org/r/CA+55aFxwg=vS2nrXsQhAUzPQDGb8aQpZi0M7UUh21ftBo-z46Q@mail.gmail.com
[toc] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-04-20 21:50 +0200 |
| Subject | [PATCH 07/14] mm, compaction: Simplify __alloc_pages_direct_compact feedback interface |
| Message-ID | <rq6im-4F2-11@gated-at.bofh.it> |
| In reply to | #1383701 |
From: Michal Hocko <mhocko@suse.com>
__alloc_pages_direct_compact communicates potential back off by two
variables:
- deferred_compaction tells that the compaction returned
COMPACT_DEFERRED
- contended_compaction is set when there is a contention on
zone->lock resp. zone->lru_lock locks
__alloc_pages_slowpath then backs of for THP allocation requests to
prevent from long stalls. This is rather messy and it would be much
cleaner to return a single compact result value and hide all the nasty
details into __alloc_pages_direct_compact.
This patch shouldn't introduce any functional changes.
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/page_alloc.c | 67 ++++++++++++++++++++++++++-------------------------------
1 file changed, 31 insertions(+), 36 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 06af8a757d52..350d13f3709b 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2944,29 +2944,21 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
static struct page *
__alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
int alloc_flags, const struct alloc_context *ac,
- enum migrate_mode mode, int *contended_compaction,
- bool *deferred_compaction)
+ enum migrate_mode mode, enum compact_result *compact_result)
{
- enum compact_result compact_result;
struct page *page;
+ int contended_compaction;
if (!order)
return NULL;
current->flags |= PF_MEMALLOC;
- compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
- mode, contended_compaction);
+ *compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
+ mode, &contended_compaction);
current->flags &= ~PF_MEMALLOC;
- switch (compact_result) {
- case COMPACT_DEFERRED:
- *deferred_compaction = true;
- /* fall-through */
- case COMPACT_SKIPPED:
+ if (*compact_result <= COMPACT_INACTIVE)
return NULL;
- default:
- break;
- }
/*
* At least in one zone compaction wasn't deferred or skipped, so let's
@@ -2992,6 +2984,24 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
*/
count_vm_event(COMPACTFAIL);
+ /*
+ * In all zones where compaction was attempted (and not
+ * deferred or skipped), lock contention has been detected.
+ * For THP allocation we do not want to disrupt the others
+ * so we fallback to base pages instead.
+ */
+ if (contended_compaction == COMPACT_CONTENDED_LOCK)
+ *compact_result = COMPACT_CONTENDED;
+
+ /*
+ * If compaction was aborted due to need_resched(), we do not
+ * want to further increase allocation latency, unless it is
+ * khugepaged trying to collapse.
+ */
+ if (contended_compaction == COMPACT_CONTENDED_SCHED
+ && !(current->flags & PF_KTHREAD))
+ *compact_result = COMPACT_CONTENDED;
+
cond_resched();
return NULL;
@@ -3000,8 +3010,7 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
static inline struct page *
__alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
int alloc_flags, const struct alloc_context *ac,
- enum migrate_mode mode, int *contended_compaction,
- bool *deferred_compaction)
+ enum migrate_mode mode, enum compact_result *compact_result)
{
return NULL;
}
@@ -3146,8 +3155,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
unsigned long pages_reclaimed = 0;
unsigned long did_some_progress;
enum migrate_mode migration_mode = MIGRATE_ASYNC;
- bool deferred_compaction = false;
- int contended_compaction = COMPACT_CONTENDED_NONE;
+ enum compact_result compact_result;
/*
* In the slowpath, we sanity check order to avoid ever trying to
@@ -3245,8 +3253,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
*/
page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags, ac,
migration_mode,
- &contended_compaction,
- &deferred_compaction);
+ &compact_result);
if (page)
goto got_pg;
@@ -3259,25 +3266,14 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
* to heavily disrupt the system, so we fail the allocation
* instead of entering direct reclaim.
*/
- if (deferred_compaction)
- goto nopage;
-
- /*
- * In all zones where compaction was attempted (and not
- * deferred or skipped), lock contention has been detected.
- * For THP allocation we do not want to disrupt the others
- * so we fallback to base pages instead.
- */
- if (contended_compaction == COMPACT_CONTENDED_LOCK)
+ if (compact_result == COMPACT_DEFERRED)
goto nopage;
/*
- * If compaction was aborted due to need_resched(), we do not
- * want to further increase allocation latency, unless it is
- * khugepaged trying to collapse.
+ * Compaction is contended so rather back off than cause
+ * excessive stalls.
*/
- if (contended_compaction == COMPACT_CONTENDED_SCHED
- && !(current->flags & PF_KTHREAD))
+ if(compact_result == COMPACT_CONTENDED)
goto nopage;
}
@@ -3325,8 +3321,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
*/
page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
ac, migration_mode,
- &contended_compaction,
- &deferred_compaction);
+ &compact_result);
if (page)
goto got_pg;
nopage:
--
2.8.0.rc3
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-04-20 21:50 +0200 |
| Subject | [PATCH 03/14] mm, compaction: cover all compaction mode in compact_zone |
| Message-ID | <rq6in-4F2-17@gated-at.bofh.it> |
| In reply to | #1383701 |
From: Michal Hocko <mhocko@suse.com>
the compiler is complaining after "mm, compaction: change COMPACT_
constants into enum"
mm/compaction.c: In function ‘compact_zone’:
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_DEFERRED’ not handled in switch [-Wswitch]
switch (ret) {
^
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_COMPLETE’ not handled in switch [-Wswitch]
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_NO_SUITABLE_PAGE’ not handled in switch [-Wswitch]
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_NOT_SUITABLE_ZONE’ not handled in switch [-Wswitch]
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_CONTENDED’ not handled in switch [-Wswitch]
compaction_suitable is allowed to return only COMPACT_PARTIAL,
COMPACT_SKIPPED and COMPACT_CONTINUE so other cases are simply
impossible. Put a VM_BUG_ON to catch an impossible return value.
Signed-off-by: Michal Hocko <mhocko@suse.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>
---
mm/compaction.c | 13 +++++--------
1 file changed, 5 insertions(+), 8 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index 8ae7b1c46c72..b06de27b7f72 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1433,15 +1433,12 @@ static enum compact_result compact_zone(struct zone *zone, struct compact_contro
ret = compaction_suitable(zone, cc->order, cc->alloc_flags,
cc->classzone_idx);
- switch (ret) {
- case COMPACT_PARTIAL:
- case COMPACT_SKIPPED:
- /* Compaction is likely to fail */
+ /* Compaction is likely to fail */
+ if (ret == COMPACT_PARTIAL || ret == COMPACT_SKIPPED)
return ret;
- case COMPACT_CONTINUE:
- /* Fall through to compaction */
- ;
- }
+
+ /* huh, compaction_suitable is returning something unexpected */
+ VM_BUG_ON(ret != COMPACT_CONTINUE);
/*
* Clear pageblock skip if there were failures recently and compaction
--
2.8.0.rc3
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-04-20 21:50 +0200 |
| Subject | [PATCH 10/14] mm, oom: rework oom detection |
| Message-ID | <rq6in-4F2-23@gated-at.bofh.it> |
| In reply to | #1383701 |
From: Michal Hocko <mhocko@suse.com>
__alloc_pages_slowpath has traditionally relied on the direct reclaim
and did_some_progress as an indicator that it makes sense to retry
allocation rather than declaring OOM. shrink_zones had to rely on
zone_reclaimable if shrink_zone didn't make any progress to prevent
from a premature OOM killer invocation - the LRU might be full of dirty
or writeback pages and direct reclaim cannot clean those up.
zone_reclaimable allows to rescan the reclaimable lists several
times and restart if a page is freed. This is really subtle behavior
and it might lead to a livelock when a single freed page keeps allocator
looping but the current task will not be able to allocate that single
page. OOM killer would be more appropriate than looping without any
progress for unbounded amount of time.
This patch changes OOM detection logic and pulls it out from shrink_zone
which is too low to be appropriate for any high level decisions such as OOM
which is per zonelist property. It is __alloc_pages_slowpath which knows
how many attempts have been done and what was the progress so far
therefore it is more appropriate to implement this logic.
The new heuristic is implemented in should_reclaim_retry helper called
from __alloc_pages_slowpath. It tries to be more deterministic and
easier to follow. It builds on an assumption that retrying makes sense
only if the currently reclaimable memory + free pages would allow the
current allocation request to succeed (as per __zone_watermark_ok) at
least for one zone in the usable zonelist.
This alone wouldn't be sufficient, though, because the writeback might
get stuck and reclaimable pages might be pinned for a really long time
or even depend on the current allocation context. Therefore there is a
backoff mechanism implemented which reduces the reclaim target after
each reclaim round without any progress. This means that we should
eventually converge to only NR_FREE_PAGES as the target and fail on the
wmark check and proceed to OOM. The backoff is simple and linear with
1/16 of the reclaimable pages for each round without any progress. We
are optimistic and reset counter for successful reclaim rounds.
Costly high order pages mostly preserve their semantic and those without
__GFP_REPEAT fail right away while those which have the flag set will
back off after the amount of reclaimable pages reaches equivalent of the
requested order. The only difference is that if there was no progress
during the reclaim we rely on zone watermark check. This is more logical
thing to do than previous 1<<order attempts which were a result of
zone_reclaimable faking the progress.
[vdavydov@virtuozzo.com: check classzone_idx for shrink_zone]
[hannes@cmpxchg.org: separate the heuristic into should_reclaim_retry]
[rientjes@google.com: use zone_page_state_snapshot for NR_FREE_PAGES]
[rientjes@google.com: shrink_zones doesn't need to return anything]
Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
include/linux/swap.h | 1 +
mm/page_alloc.c | 100 ++++++++++++++++++++++++++++++++++++++++++++++-----
mm/vmscan.c | 25 +++----------
3 files changed, 97 insertions(+), 29 deletions(-)
diff --git a/include/linux/swap.h b/include/linux/swap.h
index d18b65c53dbb..b14a2bb33514 100644
--- a/include/linux/swap.h
+++ b/include/linux/swap.h
@@ -316,6 +316,7 @@ extern void lru_cache_add_active_or_unevictable(struct page *page,
struct vm_area_struct *vma);
/* linux/mm/vmscan.c */
+extern unsigned long zone_reclaimable_pages(struct zone *zone);
extern unsigned long try_to_free_pages(struct zonelist *zonelist, int order,
gfp_t gfp_mask, nodemask_t *mask);
extern int __isolate_lru_page(struct page *page, isolate_mode_t mode);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d551fe326c33..38302c2041a3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3145,6 +3145,77 @@ static inline bool is_thp_gfp_mask(gfp_t gfp_mask)
return (gfp_mask & (GFP_TRANSHUGE | __GFP_KSWAPD_RECLAIM)) == GFP_TRANSHUGE;
}
+/*
+ * Maximum number of reclaim retries without any progress before OOM killer
+ * is consider as the only way to move forward.
+ */
+#define MAX_RECLAIM_RETRIES 16
+
+/*
+ * Checks whether it makes sense to retry the reclaim to make a forward progress
+ * for the given allocation request.
+ * The reclaim feedback represented by did_some_progress (any progress during
+ * the last reclaim round), pages_reclaimed (cumulative number of reclaimed
+ * pages) and no_progress_loops (number of reclaim rounds without any progress
+ * in a row) is considered as well as the reclaimable pages on the applicable
+ * zone list (with a backoff mechanism which is a function of no_progress_loops).
+ *
+ * Returns true if a retry is viable or false to enter the oom path.
+ */
+static inline bool
+should_reclaim_retry(gfp_t gfp_mask, unsigned order,
+ struct alloc_context *ac, int alloc_flags,
+ bool did_some_progress, unsigned long pages_reclaimed,
+ int no_progress_loops)
+{
+ struct zone *zone;
+ struct zoneref *z;
+
+ /*
+ * Make sure we converge to OOM if we cannot make any progress
+ * several times in the row.
+ */
+ if (no_progress_loops > MAX_RECLAIM_RETRIES)
+ return false;
+
+ if (order > PAGE_ALLOC_COSTLY_ORDER) {
+ if (pages_reclaimed >= (1<<order))
+ return false;
+
+ if (did_some_progress)
+ return true;
+ }
+
+ /*
+ * Keep reclaiming pages while there is a chance this will lead somewhere.
+ * If none of the target zones can satisfy our allocation request even
+ * if all reclaimable pages are considered then we are screwed and have
+ * to go OOM.
+ */
+ for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
+ ac->nodemask) {
+ unsigned long available;
+
+ available = zone_reclaimable_pages(zone);
+ available -= DIV_ROUND_UP(no_progress_loops * available,
+ MAX_RECLAIM_RETRIES);
+ available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
+
+ /*
+ * Would the allocation succeed if we reclaimed the whole
+ * available?
+ */
+ if (__zone_watermark_ok(zone, order, min_wmark_pages(zone),
+ ac->high_zoneidx, alloc_flags, available)) {
+ /* Wait for some write requests to complete then retry */
+ wait_iff_congested(zone, BLK_RW_ASYNC, HZ/50);
+ return true;
+ }
+ }
+
+ return false;
+}
+
static inline struct page *
__alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
struct alloc_context *ac)
@@ -3156,6 +3227,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
unsigned long did_some_progress;
enum migrate_mode migration_mode = MIGRATE_ASYNC;
enum compact_result compact_result;
+ int no_progress_loops = 0;
/*
* In the slowpath, we sanity check order to avoid ever trying to
@@ -3284,23 +3356,35 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
if (gfp_mask & __GFP_NORETRY)
goto noretry;
- /* Keep reclaiming pages as long as there is reasonable progress */
- pages_reclaimed += did_some_progress;
- if ((did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER) ||
- ((gfp_mask & __GFP_REPEAT) && pages_reclaimed < (1 << order))) {
- /* Wait for some write requests to complete then retry */
- wait_iff_congested(ac->preferred_zone, BLK_RW_ASYNC, HZ/50);
- goto retry;
+ /*
+ * Do not retry costly high order allocations unless they are
+ * __GFP_REPEAT
+ */
+ if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
+ goto noretry;
+
+ if (did_some_progress) {
+ no_progress_loops = 0;
+ pages_reclaimed += did_some_progress;
+ } else {
+ no_progress_loops++;
}
+ if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
+ did_some_progress > 0, pages_reclaimed,
+ no_progress_loops))
+ goto retry;
+
/* Reclaim has failed us, start killing things */
page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
if (page)
goto got_pg;
/* Retry as long as the OOM killer is making progress */
- if (did_some_progress)
+ if (did_some_progress) {
+ no_progress_loops = 0;
goto retry;
+ }
noretry:
/*
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 3e6347e2a5fc..a2ba60aa7b88 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -191,7 +191,7 @@ static bool sane_reclaim(struct scan_control *sc)
}
#endif
-static unsigned long zone_reclaimable_pages(struct zone *zone)
+unsigned long zone_reclaimable_pages(struct zone *zone)
{
unsigned long nr;
@@ -2530,10 +2530,8 @@ static inline bool compaction_ready(struct zone *zone, int order, int classzone_
*
* If a zone is deemed to be full of pinned pages then just give it a light
* scan then give up on it.
- *
- * Returns true if a zone was reclaimable.
*/
-static bool shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
+static void shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
{
struct zoneref *z;
struct zone *zone;
@@ -2541,7 +2539,6 @@ static bool shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
unsigned long nr_soft_scanned;
gfp_t orig_mask;
enum zone_type requested_highidx = gfp_zone(sc->gfp_mask);
- bool reclaimable = false;
/*
* If the number of buffer_heads in the machine exceeds the maximum
@@ -2606,17 +2603,10 @@ static bool shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
&nr_soft_scanned);
sc->nr_reclaimed += nr_soft_reclaimed;
sc->nr_scanned += nr_soft_scanned;
- if (nr_soft_reclaimed)
- reclaimable = true;
/* need some check for avoid more shrink_zone() */
}
- if (shrink_zone(zone, sc, zone_idx(zone) == classzone_idx))
- reclaimable = true;
-
- if (global_reclaim(sc) &&
- !reclaimable && zone_reclaimable(zone))
- reclaimable = true;
+ shrink_zone(zone, sc, zone_idx(zone) == classzone_idx);
}
/*
@@ -2624,8 +2614,6 @@ static bool shrink_zones(struct zonelist *zonelist, struct scan_control *sc)
* promoted it to __GFP_HIGHMEM.
*/
sc->gfp_mask = orig_mask;
-
- return reclaimable;
}
/*
@@ -2650,7 +2638,6 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
int initial_priority = sc->priority;
unsigned long total_scanned = 0;
unsigned long writeback_threshold;
- bool zones_reclaimable;
retry:
delayacct_freepages_start();
@@ -2661,7 +2648,7 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
vmpressure_prio(sc->gfp_mask, sc->target_mem_cgroup,
sc->priority);
sc->nr_scanned = 0;
- zones_reclaimable = shrink_zones(zonelist, sc);
+ shrink_zones(zonelist, sc);
total_scanned += sc->nr_scanned;
if (sc->nr_reclaimed >= sc->nr_to_reclaim)
@@ -2708,10 +2695,6 @@ static unsigned long do_try_to_free_pages(struct zonelist *zonelist,
goto retry;
}
- /* Any of the zones still reclaimable? Don't OOM. */
- if (zones_reclaimable)
- return 1;
-
return 0;
}
--
2.8.0.rc3
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-04-20 21:50 +0200 |
| Subject | [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders |
| Message-ID | <rq6in-4F2-25@gated-at.bofh.it> |
| In reply to | #1383701 |
From: Michal Hocko <mhocko@suse.com>
"mm: consider compaction feedback also for costly allocation" has
removed the upper bound for the reclaim/compaction retries based on the
number of reclaimed pages for costly orders. While this is desirable
the patch did miss a mis interaction between reclaim, compaction and the
retry logic. The direct reclaim tries to get zones over min watermark
while compaction backs off and returns COMPACT_SKIPPED when all zones
are below low watermark + 1<<order gap. If we are getting really close
to OOM then __compaction_suitable can keep returning COMPACT_SKIPPED a
high order request (e.g. hugetlb order-9) while the reclaim is not able
to release enough pages to get us over low watermark. The reclaim is
still able to make some progress (usually trashing over few remaining
pages) so we are not able to break out from the loop.
I have seen this happening with the same test described in "mm: consider
compaction feedback also for costly allocation" on a swapless system.
The original problem got resolved by "vmscan: consider classzone_idx in
compaction_ready" but it shows how things might go wrong when we
approach the oom event horizont.
The reason why compaction requires being over low rather than min
watermark is not clear to me. This check was there essentially since
56de7263fcf3 ("mm: compaction: direct compact when a high-order
allocation fails"). It is clearly an implementation detail though and we
shouldn't pull it into the generic retry logic while we should be able
to cope with such eventuality. The only place in should_compact_retry
where we retry without any upper bound is for compaction_withdrawn()
case.
Introduce compaction_zonelist_suitable function which checks the given
zonelist and returns true only if there is at least one zone which would
would unblock __compaction_suitable if more memory got reclaimed. In
this implementation it checks __compaction_suitable with NR_FREE_PAGES
plus part of the reclaimable memory as the target for the watermark check.
The reclaimable memory is reduced linearly by the allocation order. The
idea is that we do not want to reclaim all the remaining memory for a
single allocation request just unblock __compaction_suitable which
doesn't guarantee we will make a further progress.
The new helper is then used if compaction_withdrawn() feedback was
provided so we do not retry if there is no outlook for a further
progress. !costly requests shouldn't be affected much - e.g. order-2
pages would require to have at least 64kB on the reclaimable LRUs while
order-9 would need at least 32M which should be enough to not lock up.
[vbabka@suse.cz: fix classzone_idx vs. high_zoneidx usage in
compaction_zonelist_suitable]
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
include/linux/compaction.h | 4 ++++
include/linux/mmzone.h | 3 +++
mm/compaction.c | 42 +++++++++++++++++++++++++++++++++++++++---
mm/page_alloc.c | 18 +++++++++++-------
4 files changed, 57 insertions(+), 10 deletions(-)
diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index a002ca55c513..7bbdbf729757 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -142,6 +142,10 @@ static inline bool compaction_withdrawn(enum compact_result result)
return false;
}
+
+bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
+ int alloc_flags);
+
extern int kcompactd_run(int nid);
extern void kcompactd_stop(int nid);
extern void wakeup_kcompactd(pg_data_t *pgdat, int order, int classzone_idx);
diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
index 150c6049f961..0bf13c7cd8cd 100644
--- a/include/linux/mmzone.h
+++ b/include/linux/mmzone.h
@@ -746,6 +746,9 @@ static inline bool is_dev_zone(const struct zone *zone)
extern struct mutex zonelists_mutex;
void build_all_zonelists(pg_data_t *pgdat, struct zone *zone);
void wakeup_kswapd(struct zone *zone, int order, enum zone_type classzone_idx);
+bool __zone_watermark_ok(struct zone *z, unsigned int order,
+ unsigned long mark, int classzone_idx, int alloc_flags,
+ long free_pages);
bool zone_watermark_ok(struct zone *z, unsigned int order,
unsigned long mark, int classzone_idx, int alloc_flags);
bool zone_watermark_ok_safe(struct zone *z, unsigned int order,
diff --git a/mm/compaction.c b/mm/compaction.c
index e2e487cea5ea..0a7ca578af97 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1369,7 +1369,8 @@ static enum compact_result compact_finished(struct zone *zone,
* COMPACT_CONTINUE - If compaction should run now
*/
static enum compact_result __compaction_suitable(struct zone *zone, int order,
- int alloc_flags, int classzone_idx)
+ int alloc_flags, int classzone_idx,
+ unsigned long wmark_target)
{
int fragindex;
unsigned long watermark;
@@ -1392,7 +1393,8 @@ static enum compact_result __compaction_suitable(struct zone *zone, int order,
* allocated and for a short time, the footprint is higher
*/
watermark += (2UL << order);
- if (!zone_watermark_ok(zone, 0, watermark, classzone_idx, alloc_flags))
+ if (!__zone_watermark_ok(zone, 0, watermark, classzone_idx,
+ alloc_flags, wmark_target))
return COMPACT_SKIPPED;
/*
@@ -1418,7 +1420,8 @@ enum compact_result compaction_suitable(struct zone *zone, int order,
{
enum compact_result ret;
- ret = __compaction_suitable(zone, order, alloc_flags, classzone_idx);
+ ret = __compaction_suitable(zone, order, alloc_flags, classzone_idx,
+ zone_page_state(zone, NR_FREE_PAGES));
trace_mm_compaction_suitable(zone, order, ret);
if (ret == COMPACT_NOT_SUITABLE_ZONE)
ret = COMPACT_SKIPPED;
@@ -1426,6 +1429,39 @@ enum compact_result compaction_suitable(struct zone *zone, int order,
return ret;
}
+bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
+ int alloc_flags)
+{
+ struct zone *zone;
+ struct zoneref *z;
+
+ /*
+ * Make sure at least one zone would pass __compaction_suitable if we continue
+ * retrying the reclaim.
+ */
+ for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
+ ac->nodemask) {
+ unsigned long available;
+ enum compact_result compact_result;
+
+ /*
+ * Do not consider all the reclaimable memory because we do not
+ * want to trash just for a single high order allocation which
+ * is even not guaranteed to appear even if __compaction_suitable
+ * is happy about the watermark check.
+ */
+ available = zone_reclaimable_pages(zone) / order;
+ available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
+ compact_result = __compaction_suitable(zone, order, alloc_flags,
+ ac->classzone_idx, available);
+ if (compact_result != COMPACT_SKIPPED &&
+ compact_result != COMPACT_NOT_SUITABLE_ZONE)
+ return true;
+ }
+
+ return false;
+}
+
static enum compact_result compact_zone(struct zone *zone, struct compact_control *cc)
{
enum compact_result ret;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index d5a938f12554..6757d6df2160 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2526,7 +2526,7 @@ static inline bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
* one free page of a suitable size. Checking now avoids taking the zone lock
* to check in the allocation paths if no pages are free.
*/
-static bool __zone_watermark_ok(struct zone *z, unsigned int order,
+bool __zone_watermark_ok(struct zone *z, unsigned int order,
unsigned long mark, int classzone_idx, int alloc_flags,
long free_pages)
{
@@ -3015,8 +3015,8 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
}
static inline bool
-should_compact_retry(unsigned int order, enum compact_result compact_result,
- enum migrate_mode *migrate_mode,
+should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
+ enum compact_result compact_result, enum migrate_mode *migrate_mode,
int compaction_retries)
{
int max_retries = MAX_COMPACT_RETRIES;
@@ -3040,9 +3040,11 @@ should_compact_retry(unsigned int order, enum compact_result compact_result,
/*
* make sure the compaction wasn't deferred or didn't bail out early
* due to locks contention before we declare that we should give up.
+ * But do not retry if the given zonelist is not suitable for
+ * compaction.
*/
if (compaction_withdrawn(compact_result))
- return true;
+ return compaction_zonelist_suitable(ac, order, alloc_flags);
/*
* !costly requests are much more important than __GFP_REPEAT
@@ -3069,7 +3071,8 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
}
static inline bool
-should_compact_retry(unsigned int order, enum compact_result compact_result,
+should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
+ enum compact_result compact_result,
enum migrate_mode *migrate_mode,
int compaction_retries)
{
@@ -3464,8 +3467,9 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
* of free memory (see __compaction_suitable)
*/
if (did_some_progress > 0 &&
- should_compact_retry(order, compact_result,
- &migration_mode, compaction_retries))
+ should_compact_retry(ac, order, alloc_flags,
+ compact_result, &migration_mode,
+ compaction_retries))
goto retry;
/* Reclaim has failed us, start killing things */
--
2.8.0.rc3
[toc] | [prev] | [next] | [standalone]
| From | Vlastimil Babka <vbabka@suse.cz> |
|---|---|
| Date | 2016-04-28 11:00 +0200 |
| Subject | Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders |
| Message-ID | <rsPXI-hT-11@gated-at.bofh.it> |
| In reply to | #1383705 |
On 04/20/2016 09:47 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> "mm: consider compaction feedback also for costly allocation" has
> removed the upper bound for the reclaim/compaction retries based on the
> number of reclaimed pages for costly orders. While this is desirable
> the patch did miss a mis interaction between reclaim, compaction and the
> retry logic.
Hmm perhaps reversing the order of patches 13 and 14 would be a bit
safer wrt future bisections then? Add compaction_zonelist_suitable()
first with the reasoning, and then immediately use it in the other patch.
> The direct reclaim tries to get zones over min watermark
> while compaction backs off and returns COMPACT_SKIPPED when all zones
> are below low watermark + 1<<order gap. If we are getting really close
> to OOM then __compaction_suitable can keep returning COMPACT_SKIPPED a
> high order request (e.g. hugetlb order-9) while the reclaim is not able
> to release enough pages to get us over low watermark. The reclaim is
> still able to make some progress (usually trashing over few remaining
> pages) so we are not able to break out from the loop.
>
> I have seen this happening with the same test described in "mm: consider
> compaction feedback also for costly allocation" on a swapless system.
> The original problem got resolved by "vmscan: consider classzone_idx in
> compaction_ready" but it shows how things might go wrong when we
> approach the oom event horizont.
>
> The reason why compaction requires being over low rather than min
> watermark is not clear to me. This check was there essentially since
> 56de7263fcf3 ("mm: compaction: direct compact when a high-order
> allocation fails"). It is clearly an implementation detail though and we
> shouldn't pull it into the generic retry logic while we should be able
> to cope with such eventuality. The only place in should_compact_retry
> where we retry without any upper bound is for compaction_withdrawn()
> case.
>
> Introduce compaction_zonelist_suitable function which checks the given
> zonelist and returns true only if there is at least one zone which would
> would unblock __compaction_suitable if more memory got reclaimed. In
> this implementation it checks __compaction_suitable with NR_FREE_PAGES
> plus part of the reclaimable memory as the target for the watermark check.
> The reclaimable memory is reduced linearly by the allocation order. The
> idea is that we do not want to reclaim all the remaining memory for a
> single allocation request just unblock __compaction_suitable which
> doesn't guarantee we will make a further progress.
>
> The new helper is then used if compaction_withdrawn() feedback was
> provided so we do not retry if there is no outlook for a further
> progress. !costly requests shouldn't be affected much - e.g. order-2
> pages would require to have at least 64kB on the reclaimable LRUs while
> order-9 would need at least 32M which should be enough to not lock up.
>
> [vbabka@suse.cz: fix classzone_idx vs. high_zoneidx usage in
> compaction_zonelist_suitable]
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-04-28 14:40 +0200 |
| Subject | Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders |
| Message-ID | <rsToB-36j-3@gated-at.bofh.it> |
| In reply to | #1389896 |
On Thu 28-04-16 10:59:22, Vlastimil Babka wrote: > On 04/20/2016 09:47 PM, Michal Hocko wrote: > >From: Michal Hocko <mhocko@suse.com> > > > >"mm: consider compaction feedback also for costly allocation" has > >removed the upper bound for the reclaim/compaction retries based on the > >number of reclaimed pages for costly orders. While this is desirable > >the patch did miss a mis interaction between reclaim, compaction and the > >retry logic. > > Hmm perhaps reversing the order of patches 13 and 14 would be a bit safer > wrt future bisections then? Add compaction_zonelist_suitable() first with > the reasoning, and then immediately use it in the other patch. Hmm, I do not think the risk is high. This would require the allocate GFP_REPEAT large orders to the last drop which is not usual. I found the ordering more logical to argue about because this patch will be mostly noop for costly orders without 13 and !costly allocations retry endlessly anyway. So I would prefer this ordering even though there is a window where an extreme load can lockup. I do not expect people shooting their head during bisection. [...] > > > >[vbabka@suse.cz: fix classzone_idx vs. high_zoneidx usage in > >compaction_zonelist_suitable] > >Signed-off-by: Michal Hocko <mhocko@suse.com> > > Acked-by: Vlastimil Babka <vbabka@suse.cz> Thanks! -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <iamjoonsoo.kim@lge.com> |
|---|---|
| Date | 2016-05-04 08:30 +0200 |
| Subject | Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders |
| Message-ID | <ruYtQ-71w-19@gated-at.bofh.it> |
| In reply to | #1383705 |
On Wed, Apr 20, 2016 at 03:47:27PM -0400, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> "mm: consider compaction feedback also for costly allocation" has
> removed the upper bound for the reclaim/compaction retries based on the
> number of reclaimed pages for costly orders. While this is desirable
> the patch did miss a mis interaction between reclaim, compaction and the
> retry logic. The direct reclaim tries to get zones over min watermark
> while compaction backs off and returns COMPACT_SKIPPED when all zones
> are below low watermark + 1<<order gap. If we are getting really close
> to OOM then __compaction_suitable can keep returning COMPACT_SKIPPED a
> high order request (e.g. hugetlb order-9) while the reclaim is not able
> to release enough pages to get us over low watermark. The reclaim is
> still able to make some progress (usually trashing over few remaining
> pages) so we are not able to break out from the loop.
>
> I have seen this happening with the same test described in "mm: consider
> compaction feedback also for costly allocation" on a swapless system.
> The original problem got resolved by "vmscan: consider classzone_idx in
> compaction_ready" but it shows how things might go wrong when we
> approach the oom event horizont.
>
> The reason why compaction requires being over low rather than min
> watermark is not clear to me. This check was there essentially since
> 56de7263fcf3 ("mm: compaction: direct compact when a high-order
> allocation fails"). It is clearly an implementation detail though and we
> shouldn't pull it into the generic retry logic while we should be able
> to cope with such eventuality. The only place in should_compact_retry
> where we retry without any upper bound is for compaction_withdrawn()
> case.
>
> Introduce compaction_zonelist_suitable function which checks the given
> zonelist and returns true only if there is at least one zone which would
> would unblock __compaction_suitable if more memory got reclaimed. In
> this implementation it checks __compaction_suitable with NR_FREE_PAGES
> plus part of the reclaimable memory as the target for the watermark check.
> The reclaimable memory is reduced linearly by the allocation order. The
> idea is that we do not want to reclaim all the remaining memory for a
> single allocation request just unblock __compaction_suitable which
> doesn't guarantee we will make a further progress.
>
> The new helper is then used if compaction_withdrawn() feedback was
> provided so we do not retry if there is no outlook for a further
> progress. !costly requests shouldn't be affected much - e.g. order-2
> pages would require to have at least 64kB on the reclaimable LRUs while
> order-9 would need at least 32M which should be enough to not lock up.
>
> [vbabka@suse.cz: fix classzone_idx vs. high_zoneidx usage in
> compaction_zonelist_suitable]
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> include/linux/compaction.h | 4 ++++
> include/linux/mmzone.h | 3 +++
> mm/compaction.c | 42 +++++++++++++++++++++++++++++++++++++++---
> mm/page_alloc.c | 18 +++++++++++-------
> 4 files changed, 57 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index a002ca55c513..7bbdbf729757 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -142,6 +142,10 @@ static inline bool compaction_withdrawn(enum compact_result result)
> return false;
> }
>
> +
> +bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
> + int alloc_flags);
> +
> extern int kcompactd_run(int nid);
> extern void kcompactd_stop(int nid);
> extern void wakeup_kcompactd(pg_data_t *pgdat, int order, int classzone_idx);
> diff --git a/include/linux/mmzone.h b/include/linux/mmzone.h
> index 150c6049f961..0bf13c7cd8cd 100644
> --- a/include/linux/mmzone.h
> +++ b/include/linux/mmzone.h
> @@ -746,6 +746,9 @@ static inline bool is_dev_zone(const struct zone *zone)
> extern struct mutex zonelists_mutex;
> void build_all_zonelists(pg_data_t *pgdat, struct zone *zone);
> void wakeup_kswapd(struct zone *zone, int order, enum zone_type classzone_idx);
> +bool __zone_watermark_ok(struct zone *z, unsigned int order,
> + unsigned long mark, int classzone_idx, int alloc_flags,
> + long free_pages);
> bool zone_watermark_ok(struct zone *z, unsigned int order,
> unsigned long mark, int classzone_idx, int alloc_flags);
> bool zone_watermark_ok_safe(struct zone *z, unsigned int order,
> diff --git a/mm/compaction.c b/mm/compaction.c
> index e2e487cea5ea..0a7ca578af97 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1369,7 +1369,8 @@ static enum compact_result compact_finished(struct zone *zone,
> * COMPACT_CONTINUE - If compaction should run now
> */
> static enum compact_result __compaction_suitable(struct zone *zone, int order,
> - int alloc_flags, int classzone_idx)
> + int alloc_flags, int classzone_idx,
> + unsigned long wmark_target)
> {
> int fragindex;
> unsigned long watermark;
> @@ -1392,7 +1393,8 @@ static enum compact_result __compaction_suitable(struct zone *zone, int order,
> * allocated and for a short time, the footprint is higher
> */
> watermark += (2UL << order);
> - if (!zone_watermark_ok(zone, 0, watermark, classzone_idx, alloc_flags))
> + if (!__zone_watermark_ok(zone, 0, watermark, classzone_idx,
> + alloc_flags, wmark_target))
> return COMPACT_SKIPPED;
>
> /*
> @@ -1418,7 +1420,8 @@ enum compact_result compaction_suitable(struct zone *zone, int order,
> {
> enum compact_result ret;
>
> - ret = __compaction_suitable(zone, order, alloc_flags, classzone_idx);
> + ret = __compaction_suitable(zone, order, alloc_flags, classzone_idx,
> + zone_page_state(zone, NR_FREE_PAGES));
> trace_mm_compaction_suitable(zone, order, ret);
> if (ret == COMPACT_NOT_SUITABLE_ZONE)
> ret = COMPACT_SKIPPED;
> @@ -1426,6 +1429,39 @@ enum compact_result compaction_suitable(struct zone *zone, int order,
> return ret;
> }
>
> +bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
> + int alloc_flags)
> +{
> + struct zone *zone;
> + struct zoneref *z;
> +
> + /*
> + * Make sure at least one zone would pass __compaction_suitable if we continue
> + * retrying the reclaim.
> + */
> + for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> + ac->nodemask) {
> + unsigned long available;
> + enum compact_result compact_result;
> +
> + /*
> + * Do not consider all the reclaimable memory because we do not
> + * want to trash just for a single high order allocation which
> + * is even not guaranteed to appear even if __compaction_suitable
> + * is happy about the watermark check.
> + */
> + available = zone_reclaimable_pages(zone) / order;
I can't understand why '/ order' is needed here. Think about specific
example.
zone_reclaimable_pages = 100 MB
NR_FREE_PAGES = 20 MB
watermark = 40 MB
order = 10
I think that compaction should run in this situation and your logic
doesn't. We should be conservative when guessing not to do something
prematurely.
> + available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> + compact_result = __compaction_suitable(zone, order, alloc_flags,
> + ac->classzone_idx, available);
It misses tracepoint in compaction_suitable().
> + if (compact_result != COMPACT_SKIPPED &&
> + compact_result != COMPACT_NOT_SUITABLE_ZONE)
It's undesirable to use COMPACT_NOT_SUITABLE_ZONE here. It is just for
detailed tracepoint output.
> + return true;
> + }
> +
> + return false;
> +}
> +
> static enum compact_result compact_zone(struct zone *zone, struct compact_control *cc)
> {
> enum compact_result ret;
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index d5a938f12554..6757d6df2160 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2526,7 +2526,7 @@ static inline bool should_fail_alloc_page(gfp_t gfp_mask, unsigned int order)
> * one free page of a suitable size. Checking now avoids taking the zone lock
> * to check in the allocation paths if no pages are free.
> */
> -static bool __zone_watermark_ok(struct zone *z, unsigned int order,
> +bool __zone_watermark_ok(struct zone *z, unsigned int order,
> unsigned long mark, int classzone_idx, int alloc_flags,
> long free_pages)
> {
> @@ -3015,8 +3015,8 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> }
>
> static inline bool
> -should_compact_retry(unsigned int order, enum compact_result compact_result,
> - enum migrate_mode *migrate_mode,
> +should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> + enum compact_result compact_result, enum migrate_mode *migrate_mode,
> int compaction_retries)
> {
> int max_retries = MAX_COMPACT_RETRIES;
> @@ -3040,9 +3040,11 @@ should_compact_retry(unsigned int order, enum compact_result compact_result,
> /*
> * make sure the compaction wasn't deferred or didn't bail out early
> * due to locks contention before we declare that we should give up.
> + * But do not retry if the given zonelist is not suitable for
> + * compaction.
> */
> if (compaction_withdrawn(compact_result))
> - return true;
> + return compaction_zonelist_suitable(ac, order, alloc_flags);
I think that compaction_zonelist_suitable() should be checked first.
If compaction_zonelist_suitable() returns false, it's useless to
retry since it means that compaction cannot run if all reclaimable
pages are reclaimed. Logic should be as following.
if (!compaction_zonelist_suitable())
return false;
if (compaction_withdrawn())
return true;
....
Thanks.
>
> /*
> * !costly requests are much more important than __GFP_REPEAT
> @@ -3069,7 +3071,8 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> }
>
> static inline bool
> -should_compact_retry(unsigned int order, enum compact_result compact_result,
> +should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
> + enum compact_result compact_result,
> enum migrate_mode *migrate_mode,
> int compaction_retries)
> {
> @@ -3464,8 +3467,9 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> * of free memory (see __compaction_suitable)
> */
> if (did_some_progress > 0 &&
> - should_compact_retry(order, compact_result,
> - &migration_mode, compaction_retries))
> + should_compact_retry(ac, order, alloc_flags,
> + compact_result, &migration_mode,
> + compaction_retries))
> goto retry;
>
> /* Reclaim has failed us, start killing things */
> --
> 2.8.0.rc3
>
> --
> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-05-04 11:10 +0200 |
| Subject | Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders |
| Message-ID | <rv0YH-17R-17@gated-at.bofh.it> |
| In reply to | #1394017 |
On Wed 04-05-16 15:27:48, Joonsoo Kim wrote:
> On Wed, Apr 20, 2016 at 03:47:27PM -0400, Michal Hocko wrote:
[...]
> > +bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
> > + int alloc_flags)
> > +{
> > + struct zone *zone;
> > + struct zoneref *z;
> > +
> > + /*
> > + * Make sure at least one zone would pass __compaction_suitable if we continue
> > + * retrying the reclaim.
> > + */
> > + for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> > + ac->nodemask) {
> > + unsigned long available;
> > + enum compact_result compact_result;
> > +
> > + /*
> > + * Do not consider all the reclaimable memory because we do not
> > + * want to trash just for a single high order allocation which
> > + * is even not guaranteed to appear even if __compaction_suitable
> > + * is happy about the watermark check.
> > + */
> > + available = zone_reclaimable_pages(zone) / order;
>
> I can't understand why '/ order' is needed here. Think about specific
> example.
>
> zone_reclaimable_pages = 100 MB
> NR_FREE_PAGES = 20 MB
> watermark = 40 MB
> order = 10
>
> I think that compaction should run in this situation and your logic
> doesn't. We should be conservative when guessing not to do something
> prematurely.
I do not mind changing this. But pushing really hard on reclaim for
order-10 pages doesn't sound like a good idea. So we should somehow
reduce the target. I am open for any better suggestions.
> > + available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> > + compact_result = __compaction_suitable(zone, order, alloc_flags,
> > + ac->classzone_idx, available);
>
> It misses tracepoint in compaction_suitable().
Why do you think the check would be useful. I have considered it more
confusing than halpful to I have intentionally not added it.
>
> > + if (compact_result != COMPACT_SKIPPED &&
> > + compact_result != COMPACT_NOT_SUITABLE_ZONE)
>
> It's undesirable to use COMPACT_NOT_SUITABLE_ZONE here. It is just for
> detailed tracepoint output.
Well this is a compaction code so I considered it acceptable. If you
consider it a big deal I can extract a wrapper and hide this detail.
[...]
> > @@ -3040,9 +3040,11 @@ should_compact_retry(unsigned int order, enum compact_result compact_result,
> > /*
> > * make sure the compaction wasn't deferred or didn't bail out early
> > * due to locks contention before we declare that we should give up.
> > + * But do not retry if the given zonelist is not suitable for
> > + * compaction.
> > */
> > if (compaction_withdrawn(compact_result))
> > - return true;
> > + return compaction_zonelist_suitable(ac, order, alloc_flags);
>
> I think that compaction_zonelist_suitable() should be checked first.
> If compaction_zonelist_suitable() returns false, it's useless to
> retry since it means that compaction cannot run if all reclaimable
> pages are reclaimed. Logic should be as following.
>
> if (!compaction_zonelist_suitable())
> return false;
>
> if (compaction_withdrawn())
> return true;
That is certainly an option as well. The logic above is that I really
wanted to have a terminal condition when compaction can return
compaction_withdrawn for ever basically. Normally we are bound by a
number of successful reclaim rounds. Before we go an change there I
would like to see where it makes real change though.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <js1304@gmail.com> |
|---|---|
| Date | 2016-05-04 17:20 +0200 |
| Subject | Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders |
| Message-ID | <rv6KJ-6s5-15@gated-at.bofh.it> |
| In reply to | #1394105 |
2016-05-04 18:04 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 04-05-16 15:27:48, Joonsoo Kim wrote:
>> On Wed, Apr 20, 2016 at 03:47:27PM -0400, Michal Hocko wrote:
> [...]
>> > +bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
>> > + int alloc_flags)
>> > +{
>> > + struct zone *zone;
>> > + struct zoneref *z;
>> > +
>> > + /*
>> > + * Make sure at least one zone would pass __compaction_suitable if we continue
>> > + * retrying the reclaim.
>> > + */
>> > + for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
>> > + ac->nodemask) {
>> > + unsigned long available;
>> > + enum compact_result compact_result;
>> > +
>> > + /*
>> > + * Do not consider all the reclaimable memory because we do not
>> > + * want to trash just for a single high order allocation which
>> > + * is even not guaranteed to appear even if __compaction_suitable
>> > + * is happy about the watermark check.
>> > + */
>> > + available = zone_reclaimable_pages(zone) / order;
>>
>> I can't understand why '/ order' is needed here. Think about specific
>> example.
>>
>> zone_reclaimable_pages = 100 MB
>> NR_FREE_PAGES = 20 MB
>> watermark = 40 MB
>> order = 10
>>
>> I think that compaction should run in this situation and your logic
>> doesn't. We should be conservative when guessing not to do something
>> prematurely.
>
> I do not mind changing this. But pushing really hard on reclaim for
> order-10 pages doesn't sound like a good idea. So we should somehow
> reduce the target. I am open for any better suggestions.
If the situation is changed to order-2, it doesn't look good, either.
I think that some reduction on zone_reclaimable_page() are needed since
it's not possible to free all of them in certain case. But, reduction by order
doesn't make any sense. if we need to consider order to guess probability of
compaction, it should be considered in __compaction_suitable() instead of
reduction from here.
I think that following code that is used in should_reclaim_retry() would be
good for here.
available -= DIV_ROUND_UP(no_progress_loops * available, MAX_RECLAIM_RETRIES)
Any thought?
>> > + available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
>> > + compact_result = __compaction_suitable(zone, order, alloc_flags,
>> > + ac->classzone_idx, available);
>>
>> It misses tracepoint in compaction_suitable().
>
> Why do you think the check would be useful. I have considered it more
> confusing than halpful to I have intentionally not added it.
What confusing do you have in mind?
If we try to analyze OOM, we need to know why should_compact_retry()
return false and and tracepoint here could be helpful.
>>
>> > + if (compact_result != COMPACT_SKIPPED &&
>> > + compact_result != COMPACT_NOT_SUITABLE_ZONE)
>>
>> It's undesirable to use COMPACT_NOT_SUITABLE_ZONE here. It is just for
>> detailed tracepoint output.
>
> Well this is a compaction code so I considered it acceptable. If you
> consider it a big deal I can extract a wrapper and hide this detail.
It is not a big deal.
> [...]
>
>> > @@ -3040,9 +3040,11 @@ should_compact_retry(unsigned int order, enum compact_result compact_result,
>> > /*
>> > * make sure the compaction wasn't deferred or didn't bail out early
>> > * due to locks contention before we declare that we should give up.
>> > + * But do not retry if the given zonelist is not suitable for
>> > + * compaction.
>> > */
>> > if (compaction_withdrawn(compact_result))
>> > - return true;
>> > + return compaction_zonelist_suitable(ac, order, alloc_flags);
>>
>> I think that compaction_zonelist_suitable() should be checked first.
>> If compaction_zonelist_suitable() returns false, it's useless to
>> retry since it means that compaction cannot run if all reclaimable
>> pages are reclaimed. Logic should be as following.
>>
>> if (!compaction_zonelist_suitable())
>> return false;
>>
>> if (compaction_withdrawn())
>> return true;
>
> That is certainly an option as well. The logic above is that I really
> wanted to have a terminal condition when compaction can return
> compaction_withdrawn for ever basically. Normally we are bound by a
> number of successful reclaim rounds. Before we go an change there I
> would like to see where it makes real change though.
It would not make real change because !compaction_withdrawn() and
!compaction_zonelist_suitable() case doesn't happen easily.
But, change makes code more understandable so it's worth doing, IMO.
Thanks.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-05-04 21:30 +0200 |
| Subject | Re: [PATCH 14/14] mm, oom, compaction: prevent from should_compact_retry looping for ever for costly orders |
| Message-ID | <rvaEH-1v3-25@gated-at.bofh.it> |
| In reply to | #1394434 |
On Thu 05-05-16 00:14:51, Joonsoo Kim wrote:
> 2016-05-04 18:04 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > On Wed 04-05-16 15:27:48, Joonsoo Kim wrote:
> >> On Wed, Apr 20, 2016 at 03:47:27PM -0400, Michal Hocko wrote:
> > [...]
> >> > +bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
> >> > + int alloc_flags)
> >> > +{
> >> > + struct zone *zone;
> >> > + struct zoneref *z;
> >> > +
> >> > + /*
> >> > + * Make sure at least one zone would pass __compaction_suitable if we continue
> >> > + * retrying the reclaim.
> >> > + */
> >> > + for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
> >> > + ac->nodemask) {
> >> > + unsigned long available;
> >> > + enum compact_result compact_result;
> >> > +
> >> > + /*
> >> > + * Do not consider all the reclaimable memory because we do not
> >> > + * want to trash just for a single high order allocation which
> >> > + * is even not guaranteed to appear even if __compaction_suitable
> >> > + * is happy about the watermark check.
> >> > + */
> >> > + available = zone_reclaimable_pages(zone) / order;
> >>
> >> I can't understand why '/ order' is needed here. Think about specific
> >> example.
> >>
> >> zone_reclaimable_pages = 100 MB
> >> NR_FREE_PAGES = 20 MB
> >> watermark = 40 MB
> >> order = 10
> >>
> >> I think that compaction should run in this situation and your logic
> >> doesn't. We should be conservative when guessing not to do something
> >> prematurely.
> >
> > I do not mind changing this. But pushing really hard on reclaim for
> > order-10 pages doesn't sound like a good idea. So we should somehow
> > reduce the target. I am open for any better suggestions.
>
> If the situation is changed to order-2, it doesn't look good, either.
Why not? If we are not able to get over compaction_suitable watermark
check after we consider half of the reclaimable memory then we are really
close to oom. This will trigger only when the reclaimable LRUs are
really _tiny_. We are (very roughly) talking about:
low_wmark + 2<<order >= NR_FREE_PAGES + reclaimable/order - 1<<order
where low_wmark would be close to NR_FREE_PAGES so in the end we are asking
for order * 3<<order >= reclaimable and that sounds quite conservative
to me. Originally I wanted much more aggressive back off.
> I think that some reduction on zone_reclaimable_page() are needed since
> it's not possible to free all of them in certain case. But, reduction by order
> doesn't make any sense. if we need to consider order to guess probability of
> compaction, it should be considered in __compaction_suitable() instead of
> reduction from here.
I do agree that a more clever algorithm would be better and I also agree
that __compaction_suitable would be a better place for such a better
algorithm. I just wanted to have something simple first and more as a
safety net to stop endless retries (this has proven to work before I
found the real culprit compaction_ready patch). A more rigorous approach
would require a much deeper analysis what the actual compaction capacity
of the reclaimable memory really is. This is a quite hard problem and I
am not really convinced it is really needed.
> I think that following code that is used in should_reclaim_retry() would be
> good for here.
>
> available -= DIV_ROUND_UP(no_progress_loops * available, MAX_RECLAIM_RETRIES)
>
> Any thought?
I would prefer not to mix reclaim retry logic in here. Moreover it can
be argued that this is a kind of arbitrary as well because it has no
relevance to the compaction capacity of the reclaimable memory. If I
have to chose then I would rather go with simpler calculation than
something that is complex and we are even not sure it works any better.
> >> > + available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> >> > + compact_result = __compaction_suitable(zone, order, alloc_flags,
> >> > + ac->classzone_idx, available);
> >>
> >> It misses tracepoint in compaction_suitable().
> >
> > Why do you think the check would be useful. I have considered it more
> > confusing than halpful to I have intentionally not added it.
>
> What confusing do you have in mind?
> If we try to analyze OOM, we need to know why should_compact_retry()
> return false and and tracepoint here could be helpful.
Because then you can easily confuse compaction_suitable from the
compaction decisions and the allocation retries. This code patch
definitely deserves a specific trace point and I plan to prepare one
along with others in the allocation path.
[...]
> >> > @@ -3040,9 +3040,11 @@ should_compact_retry(unsigned int order, enum compact_result compact_result,
> >> > /*
> >> > * make sure the compaction wasn't deferred or didn't bail out early
> >> > * due to locks contention before we declare that we should give up.
> >> > + * But do not retry if the given zonelist is not suitable for
> >> > + * compaction.
> >> > */
> >> > if (compaction_withdrawn(compact_result))
> >> > - return true;
> >> > + return compaction_zonelist_suitable(ac, order, alloc_flags);
> >>
> >> I think that compaction_zonelist_suitable() should be checked first.
> >> If compaction_zonelist_suitable() returns false, it's useless to
> >> retry since it means that compaction cannot run if all reclaimable
> >> pages are reclaimed. Logic should be as following.
> >>
> >> if (!compaction_zonelist_suitable())
> >> return false;
> >>
> >> if (compaction_withdrawn())
> >> return true;
> >
> > That is certainly an option as well. The logic above is that I really
> > wanted to have a terminal condition when compaction can return
> > compaction_withdrawn for ever basically. Normally we are bound by a
> > number of successful reclaim rounds. Before we go an change there I
> > would like to see where it makes real change though.
>
> It would not make real change because !compaction_withdrawn() and
> !compaction_zonelist_suitable() case doesn't happen easily.
>
> But, change makes code more understandable so it's worth doing, IMO.
I dunno. I might be really biased here but I consider the current
ordering more appropriate for the reasons described above. Act as a
terminal condition for potentially endless compaction_withdrawn() rather
than a terminal condition on its own. Anyway I am not really sure this
is something crucial or do you consider this particular part really
important? I would prefer to not sneak last minute changes before the
upcoming merge windown just for readability which is even non-trivial.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-04-20 21:50 +0200 |
| Subject | [PATCH 11/14] mm: throttle on IO only when there are too many dirty and writeback pages |
| Message-ID | <rq6in-4F2-29@gated-at.bofh.it> |
| In reply to | #1383701 |
From: Michal Hocko <mhocko@suse.com>
wait_iff_congested has been used to throttle allocator before it retried
another round of direct reclaim to allow the writeback to make some
progress and prevent reclaim from looping over dirty/writeback pages
without making any progress. We used to do congestion_wait before
0e093d99763e ("writeback: do not sleep on the congestion queue if
there are no congested BDIs or if significant congestion is not being
encountered in the current zone") but that led to undesirable stalls
and sleeping for the full timeout even when the BDI wasn't congested.
Hence wait_iff_congested was used instead. But it seems that even
wait_iff_congested doesn't work as expected. We might have a small file
LRU list with all pages dirty/writeback and yet the bdi is not congested
so this is just a cond_resched in the end and can end up triggering pre
mature OOM.
This patch replaces the unconditional wait_iff_congested by
congestion_wait which is executed only if we _know_ that the last round
of direct reclaim didn't make any progress and dirty+writeback pages are
more than a half of the reclaimable pages on the zone which might be
usable for our target allocation. This shouldn't reintroduce stalls
fixed by 0e093d99763e because congestion_wait is called only when we
are getting hopeless when sleeping is a better choice than OOM with many
pages under IO.
We have to preserve logic introduced by 373ccbe59270 ("mm, vmstat: allow
WQ concurrency to discover memory reclaim doesn't make any progress")
into the __alloc_pages_slowpath now that wait_iff_congested is not
used anymore. As the only remaining user of wait_iff_congested is
shrink_inactive_list we can remove the WQ specific short sleep from
wait_iff_congested because the sleep is needed to be done only once in
the allocation retry cycle.
Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/backing-dev.c | 20 +++-----------------
mm/page_alloc.c | 39 ++++++++++++++++++++++++++++++++++++---
2 files changed, 39 insertions(+), 20 deletions(-)
diff --git a/mm/backing-dev.c b/mm/backing-dev.c
index bfbd7096b6ed..08e3a58628ed 100644
--- a/mm/backing-dev.c
+++ b/mm/backing-dev.c
@@ -957,9 +957,8 @@ EXPORT_SYMBOL(congestion_wait);
* jiffies for either a BDI to exit congestion of the given @sync queue
* or a write to complete.
*
- * In the absence of zone congestion, a short sleep or a cond_resched is
- * performed to yield the processor and to allow other subsystems to make
- * a forward progress.
+ * In the absence of zone congestion, cond_resched() is called to yield
+ * the processor if necessary but otherwise does not sleep.
*
* The return value is 0 if the sleep is for the full timeout. Otherwise,
* it is the number of jiffies that were still remaining when the function
@@ -979,20 +978,7 @@ long wait_iff_congested(struct zone *zone, int sync, long timeout)
*/
if (atomic_read(&nr_wb_congested[sync]) == 0 ||
!test_bit(ZONE_CONGESTED, &zone->flags)) {
-
- /*
- * Memory allocation/reclaim might be called from a WQ
- * context and the current implementation of the WQ
- * concurrency control doesn't recognize that a particular
- * WQ is congested if the worker thread is looping without
- * ever sleeping. Therefore we have to do a short sleep
- * here rather than calling cond_resched().
- */
- if (current->flags & PF_WQ_WORKER)
- schedule_timeout_uninterruptible(1);
- else
- cond_resched();
-
+ cond_resched();
/* In case we scheduled, work out time remaining */
ret = timeout - (jiffies - start);
if (ret < 0)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 38302c2041a3..3b78936eca70 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3195,8 +3195,9 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
for_each_zone_zonelist_nodemask(zone, z, ac->zonelist, ac->high_zoneidx,
ac->nodemask) {
unsigned long available;
+ unsigned long reclaimable;
- available = zone_reclaimable_pages(zone);
+ available = reclaimable = zone_reclaimable_pages(zone);
available -= DIV_ROUND_UP(no_progress_loops * available,
MAX_RECLAIM_RETRIES);
available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
@@ -3207,8 +3208,40 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
*/
if (__zone_watermark_ok(zone, order, min_wmark_pages(zone),
ac->high_zoneidx, alloc_flags, available)) {
- /* Wait for some write requests to complete then retry */
- wait_iff_congested(zone, BLK_RW_ASYNC, HZ/50);
+ /*
+ * If we didn't make any progress and have a lot of
+ * dirty + writeback pages then we should wait for
+ * an IO to complete to slow down the reclaim and
+ * prevent from pre mature OOM
+ */
+ if (!did_some_progress) {
+ unsigned long writeback;
+ unsigned long dirty;
+
+ writeback = zone_page_state_snapshot(zone,
+ NR_WRITEBACK);
+ dirty = zone_page_state_snapshot(zone, NR_FILE_DIRTY);
+
+ if (2*(writeback + dirty) > reclaimable) {
+ congestion_wait(BLK_RW_ASYNC, HZ/10);
+ return true;
+ }
+ }
+
+ /*
+ * Memory allocation/reclaim might be called from a WQ
+ * context and the current implementation of the WQ
+ * concurrency control doesn't recognize that
+ * a particular WQ is congested if the worker thread is
+ * looping without ever sleeping. Therefore we have to
+ * do a short sleep here rather than calling
+ * cond_resched().
+ */
+ if (current->flags & PF_WQ_WORKER)
+ schedule_timeout_uninterruptible(1);
+ else
+ cond_resched();
+
return true;
}
}
--
2.8.0.rc3
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-04-20 21:50 +0200 |
| Subject | [PATCH 12/14] mm, oom: protect !costly allocations some more |
| Message-ID | <rq6in-4F2-33@gated-at.bofh.it> |
| In reply to | #1383701 |
From: Michal Hocko <mhocko@suse.com>
should_reclaim_retry will give up retries for higher order allocations
if none of the eligible zones has any requested or higher order pages
available even if we pass the watermak check for order-0. This is done
because there is no guarantee that the reclaimable and currently free
pages will form the required order.
This can, however, lead to situations were the high-order request (e.g.
order-2 required for the stack allocation during fork) will trigger
OOM too early - e.g. after the first reclaim/compaction round. Such a
system would have to be highly fragmented and there is no guarantee
further reclaim/compaction attempts would help but at least make sure
that the compaction was active before we go OOM and keep retrying even
if should_reclaim_retry tells us to oom if
- the last compaction round backed off or
- we haven't completed at least MAX_COMPACT_RETRIES active
compaction rounds.
The first rule ensures that the very last attempt for compaction
was not ignored while the second guarantees that the compaction has done
some work. Multiple retries might be needed to prevent occasional
pigggy backing of other contexts to steal the compacted pages before
the current context manages to retry to allocate them.
compaction_failed() is taken as a final word from the compaction that
the retry doesn't make much sense. We have to be careful though because
the first compaction round is MIGRATE_ASYNC which is rather weak as it
ignores pages under writeback and gives up too easily in other
situations. We therefore have to make sure that MIGRATE_SYNC_LIGHT mode
has been used before we give up. With this logic in place we do not have
to increase the migration mode unconditionally and rather do it only if
the compaction failed for the weaker mode. A nice side effect is that
the stronger migration mode is used only when really needed so this has
a potential of smaller latencies in some cases.
Please note that the compaction doesn't tell us much about how
successful it was when returning compaction_made_progress so we just
have to blindly trust that another retry is worthwhile and cap the
number to something reasonable to guarantee a convergence.
If the given number of successful retries is not sufficient for a
reasonable workloads we should focus on the collected compaction
tracepoints data and try to address the issue in the compaction code.
If this is not feasible we can increase the retries limit.
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/page_alloc.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 77 insertions(+), 10 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3b78936eca70..bb4df1be0d43 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2939,6 +2939,13 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
return page;
}
+
+/*
+ * Maximum number of compaction retries wit a progress before OOM
+ * killer is consider as the only way to move forward.
+ */
+#define MAX_COMPACT_RETRIES 16
+
#ifdef CONFIG_COMPACTION
/* Try memory compaction for high-order allocations before reclaim */
static struct page *
@@ -3006,6 +3013,43 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
return NULL;
}
+
+static inline bool
+should_compact_retry(unsigned int order, enum compact_result compact_result,
+ enum migrate_mode *migrate_mode,
+ int compaction_retries)
+{
+ if (!order)
+ return false;
+
+ /*
+ * compaction considers all the zone as desperately out of memory
+ * so it doesn't really make much sense to retry except when the
+ * failure could be caused by weak migration mode.
+ */
+ if (compaction_failed(compact_result)) {
+ if (*migrate_mode == MIGRATE_ASYNC) {
+ *migrate_mode = MIGRATE_SYNC_LIGHT;
+ return true;
+ }
+ return false;
+ }
+
+ /*
+ * !costly allocations are really important and we have to make sure
+ * the compaction wasn't deferred or didn't bail out early due to locks
+ * contention before we go OOM. Still cap the reclaim retry loops with
+ * progress to prevent from looping forever and potential trashing.
+ */
+ if (order <= PAGE_ALLOC_COSTLY_ORDER) {
+ if (compaction_withdrawn(compact_result))
+ return true;
+ if (compaction_retries <= MAX_COMPACT_RETRIES)
+ return true;
+ }
+
+ return false;
+}
#else
static inline struct page *
__alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
@@ -3014,6 +3058,14 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
{
return NULL;
}
+
+static inline bool
+should_compact_retry(unsigned int order, enum compact_result compact_result,
+ enum migrate_mode *migrate_mode,
+ int compaction_retries)
+{
+ return false;
+}
#endif /* CONFIG_COMPACTION */
/* Perform direct synchronous page reclaim */
@@ -3260,6 +3312,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
unsigned long did_some_progress;
enum migrate_mode migration_mode = MIGRATE_ASYNC;
enum compact_result compact_result;
+ int compaction_retries = 0;
int no_progress_loops = 0;
/*
@@ -3371,13 +3424,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
compaction_failed(compact_result)))
goto nopage;
- /*
- * 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 (!is_thp_gfp_mask(gfp_mask) || (current->flags & PF_KTHREAD))
- migration_mode = MIGRATE_SYNC_LIGHT;
+ if (order && compaction_made_progress(compact_result))
+ compaction_retries++;
/* Try direct reclaim and then allocating */
page = __alloc_pages_direct_reclaim(gfp_mask, order, alloc_flags, ac,
@@ -3408,6 +3456,17 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
no_progress_loops))
goto retry;
+ /*
+ * It doesn't make any sense to retry for the compaction if the order-0
+ * reclaim is not able to make any progress because the current
+ * implementation of the compaction depends on the sufficient amount
+ * of free memory (see __compaction_suitable)
+ */
+ if (did_some_progress > 0 &&
+ should_compact_retry(order, compact_result,
+ &migration_mode, compaction_retries))
+ goto retry;
+
/* Reclaim has failed us, start killing things */
page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
if (page)
@@ -3421,10 +3480,18 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
noretry:
/*
- * 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
+ * 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.
+ * 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. All other requests should tolerate
+ * at least light sync migration.
*/
+ if (is_thp_gfp_mask(gfp_mask) && !(current->flags & PF_KTHREAD))
+ migration_mode = MIGRATE_ASYNC;
+ else
+ migration_mode = MIGRATE_SYNC_LIGHT;
page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
ac, migration_mode,
&compact_result);
--
2.8.0.rc3
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <iamjoonsoo.kim@lge.com> |
|---|---|
| Date | 2016-05-04 08:10 +0200 |
| Subject | Re: [PATCH 12/14] mm, oom: protect !costly allocations some more |
| Message-ID | <ruYau-6P7-13@gated-at.bofh.it> |
| In reply to | #1383708 |
On Wed, Apr 20, 2016 at 03:47:25PM -0400, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> should_reclaim_retry will give up retries for higher order allocations
> if none of the eligible zones has any requested or higher order pages
> available even if we pass the watermak check for order-0. This is done
> because there is no guarantee that the reclaimable and currently free
> pages will form the required order.
>
> This can, however, lead to situations were the high-order request (e.g.
> order-2 required for the stack allocation during fork) will trigger
> OOM too early - e.g. after the first reclaim/compaction round. Such a
> system would have to be highly fragmented and there is no guarantee
> further reclaim/compaction attempts would help but at least make sure
> that the compaction was active before we go OOM and keep retrying even
> if should_reclaim_retry tells us to oom if
> - the last compaction round backed off or
> - we haven't completed at least MAX_COMPACT_RETRIES active
> compaction rounds.
>
> The first rule ensures that the very last attempt for compaction
> was not ignored while the second guarantees that the compaction has done
> some work. Multiple retries might be needed to prevent occasional
> pigggy backing of other contexts to steal the compacted pages before
> the current context manages to retry to allocate them.
>
> compaction_failed() is taken as a final word from the compaction that
> the retry doesn't make much sense. We have to be careful though because
> the first compaction round is MIGRATE_ASYNC which is rather weak as it
> ignores pages under writeback and gives up too easily in other
> situations. We therefore have to make sure that MIGRATE_SYNC_LIGHT mode
> has been used before we give up. With this logic in place we do not have
> to increase the migration mode unconditionally and rather do it only if
> the compaction failed for the weaker mode. A nice side effect is that
> the stronger migration mode is used only when really needed so this has
> a potential of smaller latencies in some cases.
>
> Please note that the compaction doesn't tell us much about how
> successful it was when returning compaction_made_progress so we just
> have to blindly trust that another retry is worthwhile and cap the
> number to something reasonable to guarantee a convergence.
>
> If the given number of successful retries is not sufficient for a
> reasonable workloads we should focus on the collected compaction
> tracepoints data and try to address the issue in the compaction code.
> If this is not feasible we can increase the retries limit.
>
> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
> mm/page_alloc.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 77 insertions(+), 10 deletions(-)
>
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 3b78936eca70..bb4df1be0d43 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2939,6 +2939,13 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
> return page;
> }
>
> +
> +/*
> + * Maximum number of compaction retries wit a progress before OOM
> + * killer is consider as the only way to move forward.
> + */
> +#define MAX_COMPACT_RETRIES 16
> +
> #ifdef CONFIG_COMPACTION
> /* Try memory compaction for high-order allocations before reclaim */
> static struct page *
> @@ -3006,6 +3013,43 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>
> return NULL;
> }
> +
> +static inline bool
> +should_compact_retry(unsigned int order, enum compact_result compact_result,
> + enum migrate_mode *migrate_mode,
> + int compaction_retries)
> +{
> + if (!order)
> + return false;
> +
> + /*
> + * compaction considers all the zone as desperately out of memory
> + * so it doesn't really make much sense to retry except when the
> + * failure could be caused by weak migration mode.
> + */
> + if (compaction_failed(compact_result)) {
IIUC, this compaction_failed() means that at least one zone is
compacted and failed. This is not same with your assumption in the
comment. If compaction is done and failed on ZONE_DMA, it would be
premature decision.
> + if (*migrate_mode == MIGRATE_ASYNC) {
> + *migrate_mode = MIGRATE_SYNC_LIGHT;
> + return true;
> + }
> + return false;
> + }
> +
> + /*
> + * !costly allocations are really important and we have to make sure
> + * the compaction wasn't deferred or didn't bail out early due to locks
> + * contention before we go OOM. Still cap the reclaim retry loops with
> + * progress to prevent from looping forever and potential trashing.
> + */
> + if (order <= PAGE_ALLOC_COSTLY_ORDER) {
> + if (compaction_withdrawn(compact_result))
> + return true;
> + if (compaction_retries <= MAX_COMPACT_RETRIES)
> + return true;
> + }
> +
> + return false;
> +}
> #else
> static inline struct page *
> __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> @@ -3014,6 +3058,14 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> {
> return NULL;
> }
> +
> +static inline bool
> +should_compact_retry(unsigned int order, enum compact_result compact_result,
> + enum migrate_mode *migrate_mode,
> + int compaction_retries)
> +{
> + return false;
> +}
> #endif /* CONFIG_COMPACTION */
>
> /* Perform direct synchronous page reclaim */
> @@ -3260,6 +3312,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> unsigned long did_some_progress;
> enum migrate_mode migration_mode = MIGRATE_ASYNC;
> enum compact_result compact_result;
> + int compaction_retries = 0;
> int no_progress_loops = 0;
>
> /*
> @@ -3371,13 +3424,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> compaction_failed(compact_result)))
> goto nopage;
>
> - /*
> - * 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 (!is_thp_gfp_mask(gfp_mask) || (current->flags & PF_KTHREAD))
> - migration_mode = MIGRATE_SYNC_LIGHT;
> + if (order && compaction_made_progress(compact_result))
> + compaction_retries++;
>
> /* Try direct reclaim and then allocating */
> page = __alloc_pages_direct_reclaim(gfp_mask, order, alloc_flags, ac,
> @@ -3408,6 +3456,17 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> no_progress_loops))
> goto retry;
>
> + /*
> + * It doesn't make any sense to retry for the compaction if the order-0
> + * reclaim is not able to make any progress because the current
> + * implementation of the compaction depends on the sufficient amount
> + * of free memory (see __compaction_suitable)
> + */
> + if (did_some_progress > 0 &&
> + should_compact_retry(order, compact_result,
> + &migration_mode, compaction_retries))
Checking did_some_progress on each round have subtle corner case. Think
about following situation.
round, compaction, did_some_progress, compaction
0, defer, 1
0, defer, 1
0, defer, 1
0, defer, 1
0, defer, 0
In this case, compaction has enough chance to succeed since freepages
increase, but, compaction will not be triggered.
Thanks.
> + goto retry;
> +
> /* Reclaim has failed us, start killing things */
> page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
> if (page)
> @@ -3421,10 +3480,18 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>
> noretry:
> /*
> - * 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
> + * 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.
> + * 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. All other requests should tolerate
> + * at least light sync migration.
> */
> + if (is_thp_gfp_mask(gfp_mask) && !(current->flags & PF_KTHREAD))
> + migration_mode = MIGRATE_ASYNC;
> + else
> + migration_mode = MIGRATE_SYNC_LIGHT;
> page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
> ac, migration_mode,
> &compact_result);
> --
> 2.8.0.rc3
>
> --
> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <iamjoonsoo.kim@lge.com> |
|---|---|
| Date | 2016-05-04 08:40 +0200 |
| Subject | Re: [PATCH 12/14] mm, oom: protect !costly allocations some more |
| Message-ID | <ruYDv-77h-5@gated-at.bofh.it> |
| In reply to | #1393982 |
On Wed, May 04, 2016 at 03:01:24PM +0900, Joonsoo Kim wrote:
> On Wed, Apr 20, 2016 at 03:47:25PM -0400, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> >
> > should_reclaim_retry will give up retries for higher order allocations
> > if none of the eligible zones has any requested or higher order pages
> > available even if we pass the watermak check for order-0. This is done
> > because there is no guarantee that the reclaimable and currently free
> > pages will form the required order.
> >
> > This can, however, lead to situations were the high-order request (e.g.
> > order-2 required for the stack allocation during fork) will trigger
> > OOM too early - e.g. after the first reclaim/compaction round. Such a
> > system would have to be highly fragmented and there is no guarantee
> > further reclaim/compaction attempts would help but at least make sure
> > that the compaction was active before we go OOM and keep retrying even
> > if should_reclaim_retry tells us to oom if
> > - the last compaction round backed off or
> > - we haven't completed at least MAX_COMPACT_RETRIES active
> > compaction rounds.
> >
> > The first rule ensures that the very last attempt for compaction
> > was not ignored while the second guarantees that the compaction has done
> > some work. Multiple retries might be needed to prevent occasional
> > pigggy backing of other contexts to steal the compacted pages before
> > the current context manages to retry to allocate them.
> >
> > compaction_failed() is taken as a final word from the compaction that
> > the retry doesn't make much sense. We have to be careful though because
> > the first compaction round is MIGRATE_ASYNC which is rather weak as it
> > ignores pages under writeback and gives up too easily in other
> > situations. We therefore have to make sure that MIGRATE_SYNC_LIGHT mode
> > has been used before we give up. With this logic in place we do not have
> > to increase the migration mode unconditionally and rather do it only if
> > the compaction failed for the weaker mode. A nice side effect is that
> > the stronger migration mode is used only when really needed so this has
> > a potential of smaller latencies in some cases.
> >
> > Please note that the compaction doesn't tell us much about how
> > successful it was when returning compaction_made_progress so we just
> > have to blindly trust that another retry is worthwhile and cap the
> > number to something reasonable to guarantee a convergence.
> >
> > If the given number of successful retries is not sufficient for a
> > reasonable workloads we should focus on the collected compaction
> > tracepoints data and try to address the issue in the compaction code.
> > If this is not feasible we can increase the retries limit.
> >
> > Acked-by: Vlastimil Babka <vbabka@suse.cz>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> > mm/page_alloc.c | 87 ++++++++++++++++++++++++++++++++++++++++++++++++++-------
> > 1 file changed, 77 insertions(+), 10 deletions(-)
> >
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 3b78936eca70..bb4df1be0d43 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -2939,6 +2939,13 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
> > return page;
> > }
> >
> > +
> > +/*
> > + * Maximum number of compaction retries wit a progress before OOM
> > + * killer is consider as the only way to move forward.
> > + */
> > +#define MAX_COMPACT_RETRIES 16
> > +
> > #ifdef CONFIG_COMPACTION
> > /* Try memory compaction for high-order allocations before reclaim */
> > static struct page *
> > @@ -3006,6 +3013,43 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> >
> > return NULL;
> > }
> > +
> > +static inline bool
> > +should_compact_retry(unsigned int order, enum compact_result compact_result,
> > + enum migrate_mode *migrate_mode,
> > + int compaction_retries)
> > +{
> > + if (!order)
> > + return false;
> > +
> > + /*
> > + * compaction considers all the zone as desperately out of memory
> > + * so it doesn't really make much sense to retry except when the
> > + * failure could be caused by weak migration mode.
> > + */
> > + if (compaction_failed(compact_result)) {
>
> IIUC, this compaction_failed() means that at least one zone is
> compacted and failed. This is not same with your assumption in the
> comment. If compaction is done and failed on ZONE_DMA, it would be
> premature decision.
>
> > + if (*migrate_mode == MIGRATE_ASYNC) {
> > + *migrate_mode = MIGRATE_SYNC_LIGHT;
> > + return true;
> > + }
> > + return false;
> > + }
> > +
> > + /*
> > + * !costly allocations are really important and we have to make sure
> > + * the compaction wasn't deferred or didn't bail out early due to locks
> > + * contention before we go OOM. Still cap the reclaim retry loops with
> > + * progress to prevent from looping forever and potential trashing.
> > + */
> > + if (order <= PAGE_ALLOC_COSTLY_ORDER) {
> > + if (compaction_withdrawn(compact_result))
> > + return true;
> > + if (compaction_retries <= MAX_COMPACT_RETRIES)
> > + return true;
> > + }
> > +
> > + return false;
> > +}
> > #else
> > static inline struct page *
> > __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> > @@ -3014,6 +3058,14 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> > {
> > return NULL;
> > }
> > +
> > +static inline bool
> > +should_compact_retry(unsigned int order, enum compact_result compact_result,
> > + enum migrate_mode *migrate_mode,
> > + int compaction_retries)
> > +{
> > + return false;
> > +}
> > #endif /* CONFIG_COMPACTION */
> >
> > /* Perform direct synchronous page reclaim */
> > @@ -3260,6 +3312,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > unsigned long did_some_progress;
> > enum migrate_mode migration_mode = MIGRATE_ASYNC;
> > enum compact_result compact_result;
> > + int compaction_retries = 0;
> > int no_progress_loops = 0;
> >
> > /*
> > @@ -3371,13 +3424,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > compaction_failed(compact_result)))
> > goto nopage;
> >
> > - /*
> > - * 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 (!is_thp_gfp_mask(gfp_mask) || (current->flags & PF_KTHREAD))
> > - migration_mode = MIGRATE_SYNC_LIGHT;
> > + if (order && compaction_made_progress(compact_result))
> > + compaction_retries++;
> >
> > /* Try direct reclaim and then allocating */
> > page = __alloc_pages_direct_reclaim(gfp_mask, order, alloc_flags, ac,
> > @@ -3408,6 +3456,17 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> > no_progress_loops))
> > goto retry;
> >
> > + /*
> > + * It doesn't make any sense to retry for the compaction if the order-0
> > + * reclaim is not able to make any progress because the current
> > + * implementation of the compaction depends on the sufficient amount
> > + * of free memory (see __compaction_suitable)
> > + */
> > + if (did_some_progress > 0 &&
> > + should_compact_retry(order, compact_result,
> > + &migration_mode, compaction_retries))
>
> Checking did_some_progress on each round have subtle corner case. Think
> about following situation.
>
> round, compaction, did_some_progress, compaction
> 0, defer, 1
> 0, defer, 1
> 0, defer, 1
> 0, defer, 1
> 0, defer, 0
Oops...Example should be below one.
0, defer, 1
1, defer, 1
2, defer, 1
3, defer, 1
4, defer, 0
>
> In this case, compaction has enough chance to succeed since freepages
> increase, but, compaction will not be triggered.
>
>
> Thanks.
>
> > + goto retry;
> > +
> > /* Reclaim has failed us, start killing things */
> > page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
> > if (page)
> > @@ -3421,10 +3480,18 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >
> > noretry:
> > /*
> > - * 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
> > + * 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.
> > + * 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. All other requests should tolerate
> > + * at least light sync migration.
> > */
> > + if (is_thp_gfp_mask(gfp_mask) && !(current->flags & PF_KTHREAD))
> > + migration_mode = MIGRATE_ASYNC;
> > + else
> > + migration_mode = MIGRATE_SYNC_LIGHT;
> > page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
> > ac, migration_mode,
> > &compact_result);
> > --
> > 2.8.0.rc3
> >
> > --
> > 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
> --
> 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: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-05-04 11:00 +0200 |
| Subject | Re: [PATCH 12/14] mm, oom: protect !costly allocations some more |
| Message-ID | <rv0P1-HK-17@gated-at.bofh.it> |
| In reply to | #1394019 |
On Wed 04-05-16 15:31:12, Joonsoo Kim wrote: > On Wed, May 04, 2016 at 03:01:24PM +0900, Joonsoo Kim wrote: > > On Wed, Apr 20, 2016 at 03:47:25PM -0400, Michal Hocko wrote: [...] > > > @@ -3408,6 +3456,17 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, > > > no_progress_loops)) > > > goto retry; > > > > > > + /* > > > + * It doesn't make any sense to retry for the compaction if the order-0 > > > + * reclaim is not able to make any progress because the current > > > + * implementation of the compaction depends on the sufficient amount > > > + * of free memory (see __compaction_suitable) > > > + */ > > > + if (did_some_progress > 0 && > > > + should_compact_retry(order, compact_result, > > > + &migration_mode, compaction_retries)) > > > > Checking did_some_progress on each round have subtle corner case. Think > > about following situation. > > > > round, compaction, did_some_progress, compaction > > 0, defer, 1 > > 0, defer, 1 > > 0, defer, 1 > > 0, defer, 1 > > 0, defer, 0 > > Oops...Example should be below one. > > 0, defer, 1 > 1, defer, 1 > 2, defer, 1 > 3, defer, 1 > 4, defer, 0 I am not sure I understand. The point of the check is that if the reclaim doesn't make _any_ progress then checking the result of the compaction after it didn't lead to a successful allocation just doesn't make any sense. If the compaction deferred all the time then we have a bug in the compaction. Vlastimil is already working on a code which should make the compaction more ready for !costly requests but that is a separate topic IMO. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <js1304@gmail.com> |
|---|---|
| Date | 2016-05-04 17:00 +0200 |
| Subject | Re: [PATCH 12/14] mm, oom: protect !costly allocations some more |
| Message-ID | <rv6ro-5XA-23@gated-at.bofh.it> |
| In reply to | #1394099 |
2016-05-04 17:56 GMT+09:00 Michal Hocko <mhocko@kernel.org>: > On Wed 04-05-16 15:31:12, Joonsoo Kim wrote: >> On Wed, May 04, 2016 at 03:01:24PM +0900, Joonsoo Kim wrote: >> > On Wed, Apr 20, 2016 at 03:47:25PM -0400, Michal Hocko wrote: > [...] >> > > @@ -3408,6 +3456,17 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, >> > > no_progress_loops)) >> > > goto retry; >> > > >> > > + /* >> > > + * It doesn't make any sense to retry for the compaction if the order-0 >> > > + * reclaim is not able to make any progress because the current >> > > + * implementation of the compaction depends on the sufficient amount >> > > + * of free memory (see __compaction_suitable) >> > > + */ >> > > + if (did_some_progress > 0 && >> > > + should_compact_retry(order, compact_result, >> > > + &migration_mode, compaction_retries)) >> > >> > Checking did_some_progress on each round have subtle corner case. Think >> > about following situation. >> > >> > round, compaction, did_some_progress, compaction >> > 0, defer, 1 >> > 0, defer, 1 >> > 0, defer, 1 >> > 0, defer, 1 >> > 0, defer, 0 >> >> Oops...Example should be below one. >> >> 0, defer, 1 >> 1, defer, 1 >> 2, defer, 1 >> 3, defer, 1 >> 4, defer, 0 > > I am not sure I understand. The point of the check is that if the > reclaim doesn't make _any_ progress then checking the result of the > compaction after it didn't lead to a successful allocation just doesn't > make any sense. Even if this round (#4) doesn't reclaim any pages, previous rounds (#0, #1, #2, #3) would reclaim enough pages to succeed future compaction attempt. Thanks.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-05-04 20:20 +0200 |
| Subject | Re: [PATCH 12/14] mm, oom: protect !costly allocations some more |
| Message-ID | <rv9yX-DZ-23@gated-at.bofh.it> |
| In reply to | #1394421 |
On Wed 04-05-16 23:57:50, Joonsoo Kim wrote: > 2016-05-04 17:56 GMT+09:00 Michal Hocko <mhocko@kernel.org>: > > On Wed 04-05-16 15:31:12, Joonsoo Kim wrote: > >> On Wed, May 04, 2016 at 03:01:24PM +0900, Joonsoo Kim wrote: > >> > On Wed, Apr 20, 2016 at 03:47:25PM -0400, Michal Hocko wrote: > > [...] > >> > > @@ -3408,6 +3456,17 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, > >> > > no_progress_loops)) > >> > > goto retry; > >> > > > >> > > + /* > >> > > + * It doesn't make any sense to retry for the compaction if the order-0 > >> > > + * reclaim is not able to make any progress because the current > >> > > + * implementation of the compaction depends on the sufficient amount > >> > > + * of free memory (see __compaction_suitable) > >> > > + */ > >> > > + if (did_some_progress > 0 && > >> > > + should_compact_retry(order, compact_result, > >> > > + &migration_mode, compaction_retries)) > >> > > >> > Checking did_some_progress on each round have subtle corner case. Think > >> > about following situation. > >> > > >> > round, compaction, did_some_progress, compaction > >> > 0, defer, 1 > >> > 0, defer, 1 > >> > 0, defer, 1 > >> > 0, defer, 1 > >> > 0, defer, 0 > >> > >> Oops...Example should be below one. > >> > >> 0, defer, 1 > >> 1, defer, 1 > >> 2, defer, 1 > >> 3, defer, 1 > >> 4, defer, 0 > > > > I am not sure I understand. The point of the check is that if the > > reclaim doesn't make _any_ progress then checking the result of the > > compaction after it didn't lead to a successful allocation just doesn't > > make any sense. > > Even if this round (#4) doesn't reclaim any pages, previous rounds > (#0, #1, #2, #3) would reclaim enough pages to succeed future > compaction attempt. Then the compaction shouldn't back off and I would consider it a compaction bug. I haven't see this happening though. Vlastimil is already working on patches which would simply guarantee that really important allocations will not defer. So unless I can see an example of a real issue with this I think it is just a theoretical issue which shouldn't block the patch as is. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-05-04 11:00 +0200 |
| Subject | Re: [PATCH 12/14] mm, oom: protect !costly allocations some more |
| Message-ID | <rv0P0-HK-3@gated-at.bofh.it> |
| In reply to | #1393982 |
On Wed 04-05-16 15:01:24, Joonsoo Kim wrote:
> On Wed, Apr 20, 2016 at 03:47:25PM -0400, Michal Hocko wrote:
[...]
Please try to trim your responses it makes it much easier to follow the
discussion
> > +static inline bool
> > +should_compact_retry(unsigned int order, enum compact_result compact_result,
> > + enum migrate_mode *migrate_mode,
> > + int compaction_retries)
> > +{
> > + if (!order)
> > + return false;
> > +
> > + /*
> > + * compaction considers all the zone as desperately out of memory
> > + * so it doesn't really make much sense to retry except when the
> > + * failure could be caused by weak migration mode.
> > + */
> > + if (compaction_failed(compact_result)) {
>
> IIUC, this compaction_failed() means that at least one zone is
> compacted and failed. This is not same with your assumption in the
> comment. If compaction is done and failed on ZONE_DMA, it would be
> premature decision.
Not really, because if other zones are making some progress then their
result will override COMPACT_COMPLETE
[...]
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Joonsoo Kim <js1304@gmail.com> |
|---|---|
| Date | 2016-05-04 16:40 +0200 |
| Subject | Re: [PATCH 12/14] mm, oom: protect !costly allocations some more |
| Message-ID | <rv682-5L8-29@gated-at.bofh.it> |
| In reply to | #1394095 |
2016-05-04 17:53 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 04-05-16 15:01:24, Joonsoo Kim wrote:
>> On Wed, Apr 20, 2016 at 03:47:25PM -0400, Michal Hocko wrote:
> [...]
>
> Please try to trim your responses it makes it much easier to follow the
> discussion
Okay.
>> > +static inline bool
>> > +should_compact_retry(unsigned int order, enum compact_result compact_result,
>> > + enum migrate_mode *migrate_mode,
>> > + int compaction_retries)
>> > +{
>> > + if (!order)
>> > + return false;
>> > +
>> > + /*
>> > + * compaction considers all the zone as desperately out of memory
>> > + * so it doesn't really make much sense to retry except when the
>> > + * failure could be caused by weak migration mode.
>> > + */
>> > + if (compaction_failed(compact_result)) {
>>
>> IIUC, this compaction_failed() means that at least one zone is
>> compacted and failed. This is not same with your assumption in the
>> comment. If compaction is done and failed on ZONE_DMA, it would be
>> premature decision.
>
> Not really, because if other zones are making some progress then their
> result will override COMPACT_COMPLETE
Think about the situation that DMA zone fails to compact and
the other zones are deferred or skipped. In this case, COMPACT_COMPLETE
will be returned as a final result and should_compact_retry() return false.
I don't think that it means all the zones are desperately out of memory.
Thanks.
[toc] | [prev] | [next] | [standalone]
Page 1 of 3 [1] 2 3 Next page →
Back to top | Article view | linux.kernel
csiph-web