Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1445303 > unrolled thread
| Started by | Michal Hocko <mhocko@kernel.org> |
|---|---|
| First post | 2016-07-18 10:40 +0200 |
| Last post | 2016-07-20 08:50 +0200 |
| Articles | 20 on this page of 29 — 8 participants |
Back to article view | Back to linux.kernel
[RFC PATCH 0/2] mempool vs. page allocator interaction Michal Hocko <mhocko@kernel.org> - 2016-07-18 10:40 +0200
[RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Michal Hocko <mhocko@kernel.org> - 2016-07-18 10:50 +0200
[RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-07-18 10:50 +0200
Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-07-20 00:00 +0200
Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks NeilBrown <neilb@suse.de> - 2016-07-22 10:50 +0200
Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks NeilBrown <neilb@suse.com> - 2016-07-22 11:10 +0200
Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-07-22 11:20 +0200
Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks NeilBrown <neilb@suse.com> - 2016-07-23 02:20 +0200
Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-07-25 10:40 +0200
Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-07-25 21:30 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path David Rientjes <rientjes@google.com> - 2016-07-19 04:10 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Michal Hocko <mhocko@kernel.org> - 2016-07-19 09:50 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Johannes Weiner <hannes@cmpxchg.org> - 2016-07-19 16:00 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Michal Hocko <mhocko@kernel.org> - 2016-07-19 16:30 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Mikulas Patocka <mpatocka@redhat.com> - 2016-07-20 00:10 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path David Rientjes <rientjes@google.com> - 2016-07-19 22:50 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Michal Hocko <mhocko@kernel.org> - 2016-07-20 10:20 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path David Rientjes <rientjes@google.com> - 2016-07-20 23:10 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Michal Hocko <mhocko@kernel.org> - 2016-07-21 11:00 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Johannes Weiner <hannes@cmpxchg.org> - 2016-07-21 14:20 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Michal Hocko <mhocko@kernel.org> - 2016-07-21 17:00 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Johannes Weiner <hannes@cmpxchg.org> - 2016-07-21 17:30 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path NeilBrown <neilb@suse.com> - 2016-07-22 03:50 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Michal Hocko <mhocko@kernel.org> - 2016-07-22 08:40 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Vlastimil Babka <vbabka@suse.cz> - 2016-07-22 14:30 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Andrew Morton <akpm@linux-foundation.org> - 2016-07-22 21:50 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Vlastimil Babka <vbabka@suse.cz> - 2016-07-23 21:00 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Mikulas Patocka <mpatocka@redhat.com> - 2016-07-20 00:00 +0200
Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path Michal Hocko <mhocko@kernel.org> - 2016-07-20 08:50 +0200
Page 1 of 2 [1] 2 Next page →
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-18 10:40 +0200 |
| Subject | [RFC PATCH 0/2] mempool vs. page allocator interaction |
| Message-ID | <rWcfL-3Nz-13@gated-at.bofh.it> |
Hi,
there have been two issues identified when investigating dm-crypt
backed swap recently [1]. The first one looks like a regression from
f9054c70d28b ("mm, mempool: only set __GFP_NOMEMALLOC if there are free
elements") because swapout path can now deplete all the available memory
reserves. The first patch tries to address that issue by dropping
__GFP_NOMEMALLOC only to TIF_MEMDIE tasks.
The second issue is that dm writeout path which relies on mempool
allocator gets throttled by the direct reclaim in throttle_vm_writeout
which just makes the whole memory pressure problem even worse. The
patch2 just makes sure that we annotate mempool users to be throttled
less by PF_LESS_THROTTLE flag and prevent from throttle_vm_writeout for
that path. mempool users are usually the IO path and throttle them less
sounds like a reasonable way to go.
I do not have any more complicated dm setup available so I would
appreciate if dm people (CCed) could give these two a try.
Also it would be great to iron out concerns from David. He has posted a
deadlock stack trace [2] which has led to f9054c70d28b which is bio
allocation lockup because the TIF_MEMDIE process cannot make a forward
progress without access to memory reserve. This case should be fixed by
patch 1 AFAICS. There are other potential cases when the stuck mempool
is called from PF_MEMALLOC context and blocks the oom victim indirectly
(over a lock) but I believe those are much less likely and we have the
oom reaper to make a forward progress.
Sorry of pulling the discussion outside of the original email thread
but there were more lines of dicussion there and I felt discussing
particualr solution with its justification has a greater chance of
moving towards a solution. I am sending this as an RFC because this
needs a deep review as there might be other side effects I do not see
(especially about patch 2).
Any comments, suggestions are welcome.
---
[1] http://lkml.kernel.org/r/alpine.LRH.2.02.1607111027080.14327@file01.intranet.prod.int.rdu2.redhat.com
[2] http://lkml.kernel.org/r/alpine.DEB.2.10.1607131644590.92037@chino.kir.corp.google.com
[toc] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-18 10:50 +0200 |
| Subject | [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rWcpr-3TV-3@gated-at.bofh.it> |
| In reply to | #1445303 |
From: Michal Hocko <mhocko@suse.com>
There has been a report about OOM killer invoked when swapping out to
a dm-crypt device. The primary reason seems to be that the swapout
out IO managed to completely deplete memory reserves. Mikulas was
able to bisect and explained the issue by pointing to f9054c70d28b
("mm, mempool: only set __GFP_NOMEMALLOC if there are free elements").
The reason is that the swapout path is not throttled properly because
the md-raid layer needs to allocate from the generic_make_request path
which means it allocates from the PF_MEMALLOC context. dm layer uses
mempool_alloc in order to guarantee a forward progress which used to
inhibit access to memory reserves when using page allocator. This has
changed by f9054c70d28b ("mm, mempool: only set __GFP_NOMEMALLOC if
there are free elements") which has dropped the __GFP_NOMEMALLOC
protection when the memory pool is depleted.
If we are running out of memory and the only way forward to free memory
is to perform swapout we just keep consuming memory reserves rather than
throttling the mempool allocations and allowing the pending IO to
complete up to a moment when the memory is depleted completely and there
is no way forward but invoking the OOM killer. This is less than
optimal.
The original intention of f9054c70d28b was to help with the OOM
situations where the oom victim depends on mempool allocation to make a
forward progress. We can handle that case in a different way, though. We
can check whether the current task has access to memory reserves ad an
OOM victim (TIF_MEMDIE) and drop __GFP_NOMEMALLOC protection if the pool
is empty.
David Rientjes was objecting that such an approach wouldn't help if the
oom victim was blocked on a lock held by process doing mempool_alloc. This
is very similar to other oom deadlock situations and we have oom_reaper
to deal with them so it is reasonable to rely on the same mechanism
rather inventing a different one which has negative side effects.
Fixes: f9054c70d28b ("mm, mempool: only set __GFP_NOMEMALLOC if there are free elements")
Bisected-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/mempool.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/mm/mempool.c b/mm/mempool.c
index 8f65464da5de..ea26d75c8adf 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -322,20 +322,20 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
might_sleep_if(gfp_mask & __GFP_DIRECT_RECLAIM);
+ gfp_mask |= __GFP_NOMEMALLOC; /* don't allocate emergency reserves */
gfp_mask |= __GFP_NORETRY; /* don't loop in __alloc_pages */
gfp_mask |= __GFP_NOWARN; /* failures are OK */
gfp_temp = gfp_mask & ~(__GFP_DIRECT_RECLAIM|__GFP_IO);
repeat_alloc:
- if (likely(pool->curr_nr)) {
- /*
- * Don't allocate from emergency reserves if there are
- * elements available. This check is racy, but it will
- * be rechecked each loop.
- */
- gfp_temp |= __GFP_NOMEMALLOC;
- }
+ /*
+ * Make sure that the OOM victim will get access to memory reserves
+ * properly if there are no objects in the pool to prevent from
+ * livelocks.
+ */
+ if (!likely(pool->curr_nr) && test_thread_flag(TIF_MEMDIE))
+ gfp_temp &= ~__GFP_NOMEMALLOC;
element = pool->alloc(gfp_temp, pool->pool_data);
if (likely(element != NULL))
@@ -359,7 +359,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
* We use gfp mask w/o direct reclaim or IO for the first round. If
* alloc failed with that and @pool was empty, retry immediately.
*/
- if ((gfp_temp & ~__GFP_NOMEMALLOC) != gfp_mask) {
+ if ((gfp_temp & __GFP_DIRECT_RECLAIM) != (gfp_mask & __GFP_DIRECT_RECLAIM)) {
spin_unlock_irqrestore(&pool->lock, flags);
gfp_temp = gfp_mask;
goto repeat_alloc;
--
2.8.1
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-18 10:50 +0200 |
| Subject | [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks |
| Message-ID | <rWcps-3TV-19@gated-at.bofh.it> |
| In reply to | #1445305 |
From: Michal Hocko <mhocko@suse.com>
Mikulas has reported that a swap backed by dm-crypt doesn't work
properly because the swapout cannot make a sufficient forward progress
as the writeout path depends on dm_crypt worker which has to allocate
memory to perform the encryption. In order to guarantee a forward
progress it relies on the mempool allocator. mempool_alloc(), however,
prefers to use the underlying (usually page) allocator before it grabs
objects from the pool. Such an allocation can dive into the memory
reclaim and consequently to throttle_vm_writeout. If there are too many
dirty or pages under writeback it will get throttled even though it is
in fact a flusher to clear pending pages.
[ 345.352536] kworker/u4:0 D ffff88003df7f438 10488 6 2 0x00000000
[ 345.352536] Workqueue: kcryptd kcryptd_crypt [dm_crypt]
[ 345.352536] ffff88003df7f438 ffff88003e5d0380 ffff88003e5d0380 ffff88003e5d8e80
[ 345.352536] ffff88003dfb3240 ffff88003df73240 ffff88003df80000 ffff88003df7f470
[ 345.352536] ffff88003e5d0380 ffff88003e5d0380 ffff88003df7f828 ffff88003df7f450
[ 345.352536] Call Trace:
[ 345.352536] [<ffffffff818d466c>] schedule+0x3c/0x90
[ 345.352536] [<ffffffff818d96a8>] schedule_timeout+0x1d8/0x360
[ 345.352536] [<ffffffff81135e40>] ? detach_if_pending+0x1c0/0x1c0
[ 345.352536] [<ffffffff811407c3>] ? ktime_get+0xb3/0x150
[ 345.352536] [<ffffffff811958cf>] ? __delayacct_blkio_start+0x1f/0x30
[ 345.352536] [<ffffffff818d39e4>] io_schedule_timeout+0xa4/0x110
[ 345.352536] [<ffffffff8121d886>] congestion_wait+0x86/0x1f0
[ 345.352536] [<ffffffff810fdf40>] ? prepare_to_wait_event+0xf0/0xf0
[ 345.352536] [<ffffffff812061d4>] throttle_vm_writeout+0x44/0xd0
[ 345.352536] [<ffffffff81211533>] shrink_zone_memcg+0x613/0x720
[ 345.352536] [<ffffffff81211720>] shrink_zone+0xe0/0x300
[ 345.352536] [<ffffffff81211aed>] do_try_to_free_pages+0x1ad/0x450
[ 345.352536] [<ffffffff81211e7f>] try_to_free_pages+0xef/0x300
[ 345.352536] [<ffffffff811fef19>] __alloc_pages_nodemask+0x879/0x1210
[ 345.352536] [<ffffffff810e8080>] ? sched_clock_cpu+0x90/0xc0
[ 345.352536] [<ffffffff8125a8d1>] alloc_pages_current+0xa1/0x1f0
[ 345.352536] [<ffffffff81265ef5>] ? new_slab+0x3f5/0x6a0
[ 345.352536] [<ffffffff81265dd7>] new_slab+0x2d7/0x6a0
[ 345.352536] [<ffffffff810e7f87>] ? sched_clock_local+0x17/0x80
[ 345.352536] [<ffffffff812678cb>] ___slab_alloc+0x3fb/0x5c0
[ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30
[ 345.352536] [<ffffffff810e7f87>] ? sched_clock_local+0x17/0x80
[ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30
[ 345.352536] [<ffffffff81267ae1>] __slab_alloc+0x51/0x90
[ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30
[ 345.352536] [<ffffffff81267d9b>] kmem_cache_alloc+0x27b/0x310
[ 345.352536] [<ffffffff811f71bd>] mempool_alloc_slab+0x1d/0x30
[ 345.352536] [<ffffffff811f6f11>] mempool_alloc+0x91/0x230
[ 345.352536] [<ffffffff8141a02d>] bio_alloc_bioset+0xbd/0x260
[ 345.352536] [<ffffffffc02f1a54>] kcryptd_crypt+0x114/0x3b0 [dm_crypt]
Memory pools are usually used for the writeback paths and it doesn't
really make much sense to throttle them just because there are too many
dirty/writeback pages. The main purpose of throttle_vm_writeout is to
make sure that the pageout path doesn't generate too much dirty data.
Considering that we are in mempool path which performs __GFP_NORETRY
requests the risk shouldn't be really high.
Fix this by ensuring that mempool users will get PF_LESS_THROTTLE and
that such processes are not throttled in throttle_vm_writeout. They can
still get throttled due to current_may_throttle() sleeps but that should
happen when the backing device itself is congested which sounds like a
proper reaction.
Please note that the bonus given by domain_dirty_limits() alone is not
sufficient because at least dm-crypt has to double buffer each page
under writeback so this won't be sufficient to prevent from being
throttled.
There are other users of the flag but they are in the writeout path so
this looks like a proper thing for them as well.
Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/mempool.c | 19 +++++++++++++++----
mm/page-writeback.c | 3 +++
2 files changed, 18 insertions(+), 4 deletions(-)
diff --git a/mm/mempool.c b/mm/mempool.c
index ea26d75c8adf..916e95c4192c 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -310,7 +310,8 @@ EXPORT_SYMBOL(mempool_resize);
*/
void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
{
- void *element;
+ unsigned int pflags = current->flags;
+ void *element = NULL;
unsigned long flags;
wait_queue_t wait;
gfp_t gfp_temp;
@@ -328,6 +329,12 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
gfp_temp = gfp_mask & ~(__GFP_DIRECT_RECLAIM|__GFP_IO);
+ /*
+ * Make sure that the allocation doesn't get throttled during the
+ * reclaim
+ */
+ if (gfpflags_allow_blocking(gfp_mask))
+ current->flags |= PF_LESS_THROTTLE;
repeat_alloc:
/*
* Make sure that the OOM victim will get access to memory reserves
@@ -339,7 +346,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
element = pool->alloc(gfp_temp, pool->pool_data);
if (likely(element != NULL))
- return element;
+ goto out;
spin_lock_irqsave(&pool->lock, flags);
if (likely(pool->curr_nr)) {
@@ -352,7 +359,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
* for debugging.
*/
kmemleak_update_trace(element);
- return element;
+ goto out;
}
/*
@@ -369,7 +376,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
/* We must not sleep if !__GFP_DIRECT_RECLAIM */
if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
spin_unlock_irqrestore(&pool->lock, flags);
- return NULL;
+ goto out;
}
/* Let's wait for someone else to return an element to @pool */
@@ -386,6 +393,10 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
finish_wait(&pool->wait, &wait);
goto repeat_alloc;
+out:
+ if (gfpflags_allow_blocking(gfp_mask))
+ tsk_restore_flags(current, pflags, PF_LESS_THROTTLE);
+ return element;
}
EXPORT_SYMBOL(mempool_alloc);
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index 7fbb2d008078..a37661f1a11b 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -1971,6 +1971,9 @@ void throttle_vm_writeout(gfp_t gfp_mask)
unsigned long background_thresh;
unsigned long dirty_thresh;
+ if (current->flags & PF_LESS_THROTTLE)
+ return;
+
for ( ; ; ) {
global_dirty_limits(&background_thresh, &dirty_thresh);
dirty_thresh = hard_dirty_limit(&global_wb_domain, dirty_thresh);
--
2.8.1
[toc] | [prev] | [next] | [standalone]
| From | Mikulas Patocka <mpatocka@redhat.com> |
|---|---|
| Date | 2016-07-20 00:00 +0200 |
| Subject | Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks |
| Message-ID | <rWLdv-Zu-11@gated-at.bofh.it> |
| In reply to | #1445310 |
On Mon, 18 Jul 2016, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> Mikulas has reported that a swap backed by dm-crypt doesn't work
> properly because the swapout cannot make a sufficient forward progress
> as the writeout path depends on dm_crypt worker which has to allocate
> memory to perform the encryption. In order to guarantee a forward
> progress it relies on the mempool allocator. mempool_alloc(), however,
> prefers to use the underlying (usually page) allocator before it grabs
> objects from the pool. Such an allocation can dive into the memory
> reclaim and consequently to throttle_vm_writeout. If there are too many
> dirty or pages under writeback it will get throttled even though it is
> in fact a flusher to clear pending pages.
>
> [ 345.352536] kworker/u4:0 D ffff88003df7f438 10488 6 2 0x00000000
> [ 345.352536] Workqueue: kcryptd kcryptd_crypt [dm_crypt]
> [ 345.352536] ffff88003df7f438 ffff88003e5d0380 ffff88003e5d0380 ffff88003e5d8e80
> [ 345.352536] ffff88003dfb3240 ffff88003df73240 ffff88003df80000 ffff88003df7f470
> [ 345.352536] ffff88003e5d0380 ffff88003e5d0380 ffff88003df7f828 ffff88003df7f450
> [ 345.352536] Call Trace:
> [ 345.352536] [<ffffffff818d466c>] schedule+0x3c/0x90
> [ 345.352536] [<ffffffff818d96a8>] schedule_timeout+0x1d8/0x360
> [ 345.352536] [<ffffffff81135e40>] ? detach_if_pending+0x1c0/0x1c0
> [ 345.352536] [<ffffffff811407c3>] ? ktime_get+0xb3/0x150
> [ 345.352536] [<ffffffff811958cf>] ? __delayacct_blkio_start+0x1f/0x30
> [ 345.352536] [<ffffffff818d39e4>] io_schedule_timeout+0xa4/0x110
> [ 345.352536] [<ffffffff8121d886>] congestion_wait+0x86/0x1f0
> [ 345.352536] [<ffffffff810fdf40>] ? prepare_to_wait_event+0xf0/0xf0
> [ 345.352536] [<ffffffff812061d4>] throttle_vm_writeout+0x44/0xd0
> [ 345.352536] [<ffffffff81211533>] shrink_zone_memcg+0x613/0x720
> [ 345.352536] [<ffffffff81211720>] shrink_zone+0xe0/0x300
> [ 345.352536] [<ffffffff81211aed>] do_try_to_free_pages+0x1ad/0x450
> [ 345.352536] [<ffffffff81211e7f>] try_to_free_pages+0xef/0x300
> [ 345.352536] [<ffffffff811fef19>] __alloc_pages_nodemask+0x879/0x1210
> [ 345.352536] [<ffffffff810e8080>] ? sched_clock_cpu+0x90/0xc0
> [ 345.352536] [<ffffffff8125a8d1>] alloc_pages_current+0xa1/0x1f0
> [ 345.352536] [<ffffffff81265ef5>] ? new_slab+0x3f5/0x6a0
> [ 345.352536] [<ffffffff81265dd7>] new_slab+0x2d7/0x6a0
> [ 345.352536] [<ffffffff810e7f87>] ? sched_clock_local+0x17/0x80
> [ 345.352536] [<ffffffff812678cb>] ___slab_alloc+0x3fb/0x5c0
> [ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30
> [ 345.352536] [<ffffffff810e7f87>] ? sched_clock_local+0x17/0x80
> [ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30
> [ 345.352536] [<ffffffff81267ae1>] __slab_alloc+0x51/0x90
> [ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30
> [ 345.352536] [<ffffffff81267d9b>] kmem_cache_alloc+0x27b/0x310
> [ 345.352536] [<ffffffff811f71bd>] mempool_alloc_slab+0x1d/0x30
> [ 345.352536] [<ffffffff811f6f11>] mempool_alloc+0x91/0x230
> [ 345.352536] [<ffffffff8141a02d>] bio_alloc_bioset+0xbd/0x260
> [ 345.352536] [<ffffffffc02f1a54>] kcryptd_crypt+0x114/0x3b0 [dm_crypt]
>
> Memory pools are usually used for the writeback paths and it doesn't
> really make much sense to throttle them just because there are too many
> dirty/writeback pages. The main purpose of throttle_vm_writeout is to
> make sure that the pageout path doesn't generate too much dirty data.
> Considering that we are in mempool path which performs __GFP_NORETRY
> requests the risk shouldn't be really high.
>
> Fix this by ensuring that mempool users will get PF_LESS_THROTTLE and
> that such processes are not throttled in throttle_vm_writeout. They can
> still get throttled due to current_may_throttle() sleeps but that should
> happen when the backing device itself is congested which sounds like a
> proper reaction.
>
> Please note that the bonus given by domain_dirty_limits() alone is not
> sufficient because at least dm-crypt has to double buffer each page
> under writeback so this won't be sufficient to prevent from being
> throttled.
>
> There are other users of the flag but they are in the writeout path so
> this looks like a proper thing for them as well.
>
> Reported-by: Mikulas Patocka <mpatocka@redhat.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Reviewed-by: Mikulas Patocka <mpatocka@redhat.com>
Tested-by: Mikulas Patocka <mpatocka@redhat.com>
> ---
> mm/mempool.c | 19 +++++++++++++++----
> mm/page-writeback.c | 3 +++
> 2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/mm/mempool.c b/mm/mempool.c
> index ea26d75c8adf..916e95c4192c 100644
> --- a/mm/mempool.c
> +++ b/mm/mempool.c
> @@ -310,7 +310,8 @@ EXPORT_SYMBOL(mempool_resize);
> */
> void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> {
> - void *element;
> + unsigned int pflags = current->flags;
> + void *element = NULL;
> unsigned long flags;
> wait_queue_t wait;
> gfp_t gfp_temp;
> @@ -328,6 +329,12 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
>
> gfp_temp = gfp_mask & ~(__GFP_DIRECT_RECLAIM|__GFP_IO);
>
> + /*
> + * Make sure that the allocation doesn't get throttled during the
> + * reclaim
> + */
> + if (gfpflags_allow_blocking(gfp_mask))
> + current->flags |= PF_LESS_THROTTLE;
> repeat_alloc:
> /*
> * Make sure that the OOM victim will get access to memory reserves
> @@ -339,7 +346,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
>
> element = pool->alloc(gfp_temp, pool->pool_data);
> if (likely(element != NULL))
> - return element;
> + goto out;
>
> spin_lock_irqsave(&pool->lock, flags);
> if (likely(pool->curr_nr)) {
> @@ -352,7 +359,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> * for debugging.
> */
> kmemleak_update_trace(element);
> - return element;
> + goto out;
> }
>
> /*
> @@ -369,7 +376,7 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
> /* We must not sleep if !__GFP_DIRECT_RECLAIM */
> if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
> spin_unlock_irqrestore(&pool->lock, flags);
> - return NULL;
> + goto out;
> }
>
> /* Let's wait for someone else to return an element to @pool */
> @@ -386,6 +393,10 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
>
> finish_wait(&pool->wait, &wait);
> goto repeat_alloc;
> +out:
> + if (gfpflags_allow_blocking(gfp_mask))
> + tsk_restore_flags(current, pflags, PF_LESS_THROTTLE);
> + return element;
> }
> EXPORT_SYMBOL(mempool_alloc);
>
> diff --git a/mm/page-writeback.c b/mm/page-writeback.c
> index 7fbb2d008078..a37661f1a11b 100644
> --- a/mm/page-writeback.c
> +++ b/mm/page-writeback.c
> @@ -1971,6 +1971,9 @@ void throttle_vm_writeout(gfp_t gfp_mask)
> unsigned long background_thresh;
> unsigned long dirty_thresh;
>
> + if (current->flags & PF_LESS_THROTTLE)
> + return;
> +
> for ( ; ; ) {
> global_dirty_limits(&background_thresh, &dirty_thresh);
> dirty_thresh = hard_dirty_limit(&global_wb_domain, dirty_thresh);
> --
> 2.8.1
>
[toc] | [prev] | [next] | [standalone]
| From | NeilBrown <neilb@suse.de> |
|---|---|
| Date | 2016-07-22 10:50 +0200 |
| Subject | Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks |
| Message-ID | <rXEjE-3pq-5@gated-at.bofh.it> |
| In reply to | #1445310 |
[Multipart message — attachments visible in raw view] — view raw
On Mon, Jul 18 2016, Michal Hocko wrote: > From: Michal Hocko <mhocko@suse.com> > > Mikulas has reported that a swap backed by dm-crypt doesn't work > properly because the swapout cannot make a sufficient forward progress > as the writeout path depends on dm_crypt worker which has to allocate > memory to perform the encryption. In order to guarantee a forward > progress it relies on the mempool allocator. mempool_alloc(), however, > prefers to use the underlying (usually page) allocator before it grabs > objects from the pool. Such an allocation can dive into the memory > reclaim and consequently to throttle_vm_writeout. That's just broken. I used to think mempool should always use the pre-allocated reserves first. That is surely the most logical course of action. Otherwise that memory is just sitting there doing nothing useful. I spoke to Nick Piggin about this some years ago and he pointed out that the kmalloc allocation paths are much better optimized for low overhead when there is plenty of memory. They can just pluck a free block of a per-CPU list without taking any locks. By contrast, accessing the preallocated pool always requires a spinlock. So it makes lots of sense to prefer the underlying allocator if it can provide a quick response. If it cannot, the sensible thing is to use the pool, or wait for the pool to be replenished. So the allocator should never wait at all, never enter reclaim, never throttle. Looking at the current code, __GFP_DIRECT_RECLAIM is disabled the first time through, but if the pool is empty, direct-reclaim is allowed on the next attempt. Presumably this is where the throttling comes in ?? I suspect that it really shouldn't do that. It should leave kswapd to do reclaim (so __GFP_KSWAPD_RECLAIM is appropriate) and only wait in mempool_alloc where pool->wait can wake it up. If I'm following the code properly, the stack trace below can only happen if the first pool->alloc() attempt, with direct-reclaim disabled, fails and the pool is empty, so mempool_alloc() calls prepare_to_wait() and io_schedule_timeout(). I suspect the timeout *doesn't* fire (5 seconds is along time) so it gets woken up when there is something in the pool. It then loops around and tries pool->alloc() again, even though there is something in the pool. This might be justified if that ->alloc would never block, but obviously it does. I would very strongly recommend just changing mempool_alloc() to permanently mask out __GFP_DIRECT_RECLAIM. Quite separately I don't think PF_LESS_THROTTLE is at all appropriate. It is "LESS" throttle, not "NO" throttle, but you have made throttle_vm_writeout never throttle PF_LESS_THROTTLE threads. The purpose of that flag is to allow a thread to dirty a page-cache page as part of cleaning another page-cache page. So it makes sense for loop and sometimes for nfsd. It would make sense for dm-crypt if it was putting the encrypted version in the page cache. But if dm-crypt is just allocating a transient page (which I think it is), then a mempool should be sufficient (and we should make sure it is sufficient) and access to an extra 10% (or whatever) of the page cache isn't justified. Thanks, NeilBrown If there are too many > dirty or pages under writeback it will get throttled even though it is > in fact a flusher to clear pending pages. > > [ 345.352536] kworker/u4:0 D ffff88003df7f438 10488 6 2 0x00000000 > [ 345.352536] Workqueue: kcryptd kcryptd_crypt [dm_crypt] > [ 345.352536] ffff88003df7f438 ffff88003e5d0380 ffff88003e5d0380 ffff88003e5d8e80 > [ 345.352536] ffff88003dfb3240 ffff88003df73240 ffff88003df80000 ffff88003df7f470 > [ 345.352536] ffff88003e5d0380 ffff88003e5d0380 ffff88003df7f828 ffff88003df7f450 > [ 345.352536] Call Trace: > [ 345.352536] [<ffffffff818d466c>] schedule+0x3c/0x90 > [ 345.352536] [<ffffffff818d96a8>] schedule_timeout+0x1d8/0x360 > [ 345.352536] [<ffffffff81135e40>] ? detach_if_pending+0x1c0/0x1c0 > [ 345.352536] [<ffffffff811407c3>] ? ktime_get+0xb3/0x150 > [ 345.352536] [<ffffffff811958cf>] ? __delayacct_blkio_start+0x1f/0x30 > [ 345.352536] [<ffffffff818d39e4>] io_schedule_timeout+0xa4/0x110 > [ 345.352536] [<ffffffff8121d886>] congestion_wait+0x86/0x1f0 > [ 345.352536] [<ffffffff810fdf40>] ? prepare_to_wait_event+0xf0/0xf0 > [ 345.352536] [<ffffffff812061d4>] throttle_vm_writeout+0x44/0xd0 > [ 345.352536] [<ffffffff81211533>] shrink_zone_memcg+0x613/0x720 > [ 345.352536] [<ffffffff81211720>] shrink_zone+0xe0/0x300 > [ 345.352536] [<ffffffff81211aed>] do_try_to_free_pages+0x1ad/0x450 > [ 345.352536] [<ffffffff81211e7f>] try_to_free_pages+0xef/0x300 > [ 345.352536] [<ffffffff811fef19>] __alloc_pages_nodemask+0x879/0x1210 > [ 345.352536] [<ffffffff810e8080>] ? sched_clock_cpu+0x90/0xc0 > [ 345.352536] [<ffffffff8125a8d1>] alloc_pages_current+0xa1/0x1f0 > [ 345.352536] [<ffffffff81265ef5>] ? new_slab+0x3f5/0x6a0 > [ 345.352536] [<ffffffff81265dd7>] new_slab+0x2d7/0x6a0 > [ 345.352536] [<ffffffff810e7f87>] ? sched_clock_local+0x17/0x80 > [ 345.352536] [<ffffffff812678cb>] ___slab_alloc+0x3fb/0x5c0 > [ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30 > [ 345.352536] [<ffffffff810e7f87>] ? sched_clock_local+0x17/0x80 > [ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30 > [ 345.352536] [<ffffffff81267ae1>] __slab_alloc+0x51/0x90 > [ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30 > [ 345.352536] [<ffffffff81267d9b>] kmem_cache_alloc+0x27b/0x310 > [ 345.352536] [<ffffffff811f71bd>] mempool_alloc_slab+0x1d/0x30 > [ 345.352536] [<ffffffff811f6f11>] mempool_alloc+0x91/0x230 > [ 345.352536] [<ffffffff8141a02d>] bio_alloc_bioset+0xbd/0x260 > [ 345.352536] [<ffffffffc02f1a54>] kcryptd_crypt+0x114/0x3b0 [dm_crypt]
[toc] | [prev] | [next] | [standalone]
| From | NeilBrown <neilb@suse.com> |
|---|---|
| Date | 2016-07-22 11:10 +0200 |
| Subject | Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks |
| Message-ID | <rXED0-3Mr-23@gated-at.bofh.it> |
| In reply to | #1448471 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, Jul 22 2016, NeilBrown wrote: > > Looking at the current code, __GFP_DIRECT_RECLAIM is disabled the first > time through, but if the pool is empty, direct-reclaim is allowed on the > next attempt. Presumably this is where the throttling comes in ?? I > suspect that it really shouldn't do that. It should leave kswapd to do > reclaim (so __GFP_KSWAPD_RECLAIM is appropriate) and only wait in > mempool_alloc where pool->wait can wake it up. Actually, thinking about the kswapd connection, it might make sense for mempool_alloc() to wait in the relevant pgdata->pfmemalloc_wait as well as waiting on pool->wait. What way it should be able to proceed as soon as any memory is available. I don't know what the correct 'pgdata' is though. Just a thought, NeilBrown
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-22 11:20 +0200 |
| Subject | Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks |
| Message-ID | <rXEMF-3Qd-21@gated-at.bofh.it> |
| In reply to | #1448471 |
On Fri 22-07-16 18:46:57, Neil Brown wrote: > On Mon, Jul 18 2016, Michal Hocko wrote: > > > From: Michal Hocko <mhocko@suse.com> > > > > Mikulas has reported that a swap backed by dm-crypt doesn't work > > properly because the swapout cannot make a sufficient forward progress > > as the writeout path depends on dm_crypt worker which has to allocate > > memory to perform the encryption. In order to guarantee a forward > > progress it relies on the mempool allocator. mempool_alloc(), however, > > prefers to use the underlying (usually page) allocator before it grabs > > objects from the pool. Such an allocation can dive into the memory > > reclaim and consequently to throttle_vm_writeout. > > That's just broken. > I used to think mempool should always use the pre-allocated reserves > first. That is surely the most logical course of action. Otherwise > that memory is just sitting there doing nothing useful. > > I spoke to Nick Piggin about this some years ago and he pointed out that > the kmalloc allocation paths are much better optimized for low overhead > when there is plenty of memory. They can just pluck a free block of a > per-CPU list without taking any locks. By contrast, accessing the > preallocated pool always requires a spinlock. > > So it makes lots of sense to prefer the underlying allocator if it can > provide a quick response. If it cannot, the sensible thing is to use > the pool, or wait for the pool to be replenished. > > So the allocator should never wait at all, never enter reclaim, never > throttle. > > Looking at the current code, __GFP_DIRECT_RECLAIM is disabled the first > time through, but if the pool is empty, direct-reclaim is allowed on the > next attempt. Presumably this is where the throttling comes in ?? Yes that is correct. > I suspect that it really shouldn't do that. It should leave kswapd to > do reclaim (so __GFP_KSWAPD_RECLAIM is appropriate) and only wait in > mempool_alloc where pool->wait can wake it up. Mikulas was already suggesting that and my concern was that this would give up prematurely even under mild page cache load when there are many clean page cache pages. If we just back off and rely on kswapd which might get stuck on the writeout then the IO throughput can be reduced I believe which would make the whole memory pressure just worse. So I am not sure this is a good idea in general. I completely agree with you that the mempool request shouldn't be throttled unless there is a strong reason for that. More on that below. > If I'm following the code properly, the stack trace below can only > happen if the first pool->alloc() attempt, with direct-reclaim disabled, > fails and the pool is empty, so mempool_alloc() calls prepare_to_wait() > and io_schedule_timeout(). mempool_alloc retries immediatelly without any sleep after the first no-reclaim attempt. > I suspect the timeout *doesn't* fire (5 seconds is along time) so it > gets woken up when there is something in the pool. It then loops around > and tries pool->alloc() again, even though there is something in the > pool. This might be justified if that ->alloc would never block, but > obviously it does. > > I would very strongly recommend just changing mempool_alloc() to > permanently mask out __GFP_DIRECT_RECLAIM. > > Quite separately I don't think PF_LESS_THROTTLE is at all appropriate. > It is "LESS" throttle, not "NO" throttle, but you have made > throttle_vm_writeout never throttle PF_LESS_THROTTLE threads. Yes that is correct. But it still allows to throttle on congestion: shrink_inactive_list: /* * Stall direct reclaim for IO completions if underlying BDIs or zone * is congested. Allow kswapd to continue until it starts encountering * unqueued dirty pages or cycling through the LRU too quickly. */ if (!sc->hibernation_mode && !current_is_kswapd() && current_may_throttle()) wait_iff_congested(pgdat, BLK_RW_ASYNC, HZ/10); My thinking was that throttle_vm_writeout is there to prevent from dirtying too many pages from the reclaim the context. PF_LESS_THROTTLE is part of the writeout so throttling it on too many dirty pages is questionable (well we get some bias but that is not really reliable). It still makes sense to throttle when the backing device is congested because the writeout path wouldn't make much progress anyway and we also do not want to cycle through LRU lists too quickly in that case. Or is this assumption wrong for nfsd_vfs_write? Can it cause unbounded dirtying of memory? > The purpose of that flag is to allow a thread to dirty a page-cache page > as part of cleaning another page-cache page. > So it makes sense for loop and sometimes for nfsd. It would make sense > for dm-crypt if it was putting the encrypted version in the page cache. > But if dm-crypt is just allocating a transient page (which I think it > is), then a mempool should be sufficient (and we should make sure it is > sufficient) and access to an extra 10% (or whatever) of the page cache > isn't justified. If you think that PF_LESS_THROTTLE (ab)use in mempool_alloc is not appropriate then would a PF_MEMPOOL be any better? Thanks! -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | NeilBrown <neilb@suse.com> |
|---|---|
| Date | 2016-07-23 02:20 +0200 |
| Subject | Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks |
| Message-ID | <rXSPE-4m9-11@gated-at.bofh.it> |
| In reply to | #1448492 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, Jul 22 2016, Michal Hocko wrote:
> On Fri 22-07-16 18:46:57, Neil Brown wrote:
>> On Mon, Jul 18 2016, Michal Hocko wrote:
>>
>> > From: Michal Hocko <mhocko@suse.com>
>> >
>> > Mikulas has reported that a swap backed by dm-crypt doesn't work
>> > properly because the swapout cannot make a sufficient forward progress
>> > as the writeout path depends on dm_crypt worker which has to allocate
>> > memory to perform the encryption. In order to guarantee a forward
>> > progress it relies on the mempool allocator. mempool_alloc(), however,
>> > prefers to use the underlying (usually page) allocator before it grabs
>> > objects from the pool. Such an allocation can dive into the memory
>> > reclaim and consequently to throttle_vm_writeout.
>>
>> That's just broken.
>> I used to think mempool should always use the pre-allocated reserves
>> first. That is surely the most logical course of action. Otherwise
>> that memory is just sitting there doing nothing useful.
>>
>> I spoke to Nick Piggin about this some years ago and he pointed out that
>> the kmalloc allocation paths are much better optimized for low overhead
>> when there is plenty of memory. They can just pluck a free block of a
>> per-CPU list without taking any locks. By contrast, accessing the
>> preallocated pool always requires a spinlock.
>>
>> So it makes lots of sense to prefer the underlying allocator if it can
>> provide a quick response. If it cannot, the sensible thing is to use
>> the pool, or wait for the pool to be replenished.
>>
>> So the allocator should never wait at all, never enter reclaim, never
>> throttle.
>>
>> Looking at the current code, __GFP_DIRECT_RECLAIM is disabled the first
>> time through, but if the pool is empty, direct-reclaim is allowed on the
>> next attempt. Presumably this is where the throttling comes in ??
>
> Yes that is correct.
>
>> I suspect that it really shouldn't do that. It should leave kswapd to
>> do reclaim (so __GFP_KSWAPD_RECLAIM is appropriate) and only wait in
>> mempool_alloc where pool->wait can wake it up.
>
> Mikulas was already suggesting that and my concern was that this would
> give up prematurely even under mild page cache load when there are many
> clean page cache pages.
That's a valid point - freeing up clean pages is a reasonable thing for
a mempool allocator to try to do.
> If we just back off and rely on kswapd which
> might get stuck on the writeout then the IO throughput can be reduced
If I were king of MM, I would make a decree to be proclaimed throughout
the land
kswapd must never sleep except when it explicitly chooses to
Maybe that is impractical, but having firm rules like that would go a
long way to make it possible to actually understand and reason about how
MM works. As it is, there seems to be a tendency to put bandaids over
bandaids.
> I believe which would make the whole memory pressure just worse. So I am
> not sure this is a good idea in general. I completely agree with you
> that the mempool request shouldn't be throttled unless there is a strong
> reason for that. More on that below.
>
>> If I'm following the code properly, the stack trace below can only
>> happen if the first pool->alloc() attempt, with direct-reclaim disabled,
>> fails and the pool is empty, so mempool_alloc() calls prepare_to_wait()
>> and io_schedule_timeout().
>
> mempool_alloc retries immediatelly without any sleep after the first
> no-reclaim attempt.
I missed that ... I see it now... I wonder if anyone has contemplated
using some modern programming techniques like, maybe, a "while" loop in
there..
Something like the below...
>
>> I suspect the timeout *doesn't* fire (5 seconds is along time) so it
>> gets woken up when there is something in the pool. It then loops around
>> and tries pool->alloc() again, even though there is something in the
>> pool. This might be justified if that ->alloc would never block, but
>> obviously it does.
>>
>> I would very strongly recommend just changing mempool_alloc() to
>> permanently mask out __GFP_DIRECT_RECLAIM.
>>
>> Quite separately I don't think PF_LESS_THROTTLE is at all appropriate.
>> It is "LESS" throttle, not "NO" throttle, but you have made
>> throttle_vm_writeout never throttle PF_LESS_THROTTLE threads.
>
> Yes that is correct. But it still allows to throttle on congestion:
> shrink_inactive_list:
> /*
> * Stall direct reclaim for IO completions if underlying BDIs or zone
> * is congested. Allow kswapd to continue until it starts encountering
> * unqueued dirty pages or cycling through the LRU too quickly.
> */
> if (!sc->hibernation_mode && !current_is_kswapd() &&
> current_may_throttle())
> wait_iff_congested(pgdat, BLK_RW_ASYNC, HZ/10);
>
> My thinking was that throttle_vm_writeout is there to prevent from
> dirtying too many pages from the reclaim the context. PF_LESS_THROTTLE
> is part of the writeout so throttling it on too many dirty pages is
> questionable (well we get some bias but that is not really reliable). It
> still makes sense to throttle when the backing device is congested
> because the writeout path wouldn't make much progress anyway and we also
> do not want to cycle through LRU lists too quickly in that case.
"dirtying ... from the reclaim context" ??? What does that mean?
According to
Commit: 26eecbf3543b ("[PATCH] vm: pageout throttling")
From the history tree, the purpose of throttle_vm_writeout() is to
limit the amount of memory that is concurrently under I/O.
That seems strange to me because I thought it was the responsibility of
each backing device to impose a limit - a maximum queue size of some
sort.
I remember when NFS didn't impose a limit and you could end up with lots
of memory in NFS write-back, and very long latencies could result.
So I wonder what throttle_vm_writeout() really achieves these days. Is
it just a bandaid that no-one is brave enough to remove?
I guess it could play a role in balancing the freeing of clean pages,
which can be done instantly, against dirty pages, which require
writeback. Without some throttling, might all clean pages being cleaned
too quickly, just trashing our read caches?
>
> Or is this assumption wrong for nfsd_vfs_write? Can it cause unbounded
> dirtying of memory?
In most cases, nfsd it just like any other application and needs to be
throttled like any other application when it writes too much data.
The only time nfsd *needs* PF_LESS_THROTTLE when when a loop-back mount
is active. When the same page cache is the source and destination of
writes.
So nfsd needs to be able to dirty a few more pages when nothing else
can due to high dirty count. Otherwise it deadlocks.
The main use of PF_LESS_THROTTLE is in zone_dirty_limit() and
domain_dirty_limits() where an extra 25% is allowed to overcome this
deadlock.
The use of PF_LESS_THROTTLE in current_may_throttle() in vmscan.c is to
avoid a live-lock. A key premise is that nfsd only allocates unbounded
memory when it is writing to the page cache. So it only needs to be
throttled when the backing device it is writing to is congested. It is
particularly important that it *doesn't* get throttled just because an
NFS backing device is congested, because nfsd might be trying to clear
that congestion.
In general, callers of try_to_free_pages() might get throttled when any
backing device is congested. This is a reasonable default when we don't
know what they are allocating memory for. When we do know the purpose of
the allocation, we can be more cautious about throttling.
If a thread is allocating just to dirty pages for a given backing
device, we only need to throttle the allocation if the backing device is
congested. Any further throttling needed happens in
balance_dirty_pages().
If a thread is only making transient allocations, ones which will be
freed shortly afterwards (not, for example, put in a cache), then I
don't think it needs to be throttled at all. I think this universally
applies to mempools.
In the case of dm_crypt, if it is writing too fast it will eventually be
throttled in generic_make_request when the underlying device has a full
queue and so blocks waiting for requests to be completed, and thus parts
of them returned to the mempool.
>
>> The purpose of that flag is to allow a thread to dirty a page-cache page
>> as part of cleaning another page-cache page.
>> So it makes sense for loop and sometimes for nfsd. It would make sense
>> for dm-crypt if it was putting the encrypted version in the page cache.
>> But if dm-crypt is just allocating a transient page (which I think it
>> is), then a mempool should be sufficient (and we should make sure it is
>> sufficient) and access to an extra 10% (or whatever) of the page cache
>> isn't justified.
>
> If you think that PF_LESS_THROTTLE (ab)use in mempool_alloc is not
> appropriate then would a PF_MEMPOOL be any better?
Why a PF rather than a GFP flag?
NFSD uses a PF because there is no GFP interface for filesystem write.
But mempool can pass down a GFP flag, so I think it should.
The meaning of the flag is, in my opinion, that a 'transient' allocation
is being requested. i.e. an allocation which will be used for a single
purpose for a short amount of time and will then be freed. In
particularly it will never be placed in a cache, and if it is ever
placed on a queue, that is certain to be a queue with an upper bound on
the size and with guaranteed forward progress in the face of memory
pressure.
Any allocation request for a use case with those properties should be
allowed to set GFP_TRANSIENT (for example) with the effect that the
allocation will not be throttled.
A key point with the name is to identify the purpose of the flag, not a
specific use case (mempool) which we want it for.
At least, that is what I think we should do today...
NeilBrown
>
> Thanks!
> --
> Michal Hocko
> SUSE Labs
diff --git a/mm/mempool.c b/mm/mempool.c
index 8f65464da5de..2dded8c1b9d7 100644
--- a/mm/mempool.c
+++ b/mm/mempool.c
@@ -313,7 +313,6 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
void *element;
unsigned long flags;
wait_queue_t wait;
- gfp_t gfp_temp;
/* If oom killed, memory reserves are essential to prevent livelock */
VM_WARN_ON_ONCE(gfp_mask & __GFP_NOMEMALLOC);
@@ -325,67 +324,47 @@ void *mempool_alloc(mempool_t *pool, gfp_t gfp_mask)
gfp_mask |= __GFP_NORETRY; /* don't loop in __alloc_pages */
gfp_mask |= __GFP_NOWARN; /* failures are OK */
- gfp_temp = gfp_mask & ~(__GFP_DIRECT_RECLAIM|__GFP_IO);
+ element = pool->alloc(gfp_mask & ~(__GFP_DIRECT_RECLAIM|__GFP_IO),
+ pool->pool_data);
-repeat_alloc:
- if (likely(pool->curr_nr)) {
- /*
- * Don't allocate from emergency reserves if there are
- * elements available. This check is racy, but it will
- * be rechecked each loop.
- */
- gfp_temp |= __GFP_NOMEMALLOC;
- }
+ while (!element) {
+ spin_lock_irqsave(&pool->lock, flags);
+ if (likely(pool->curr_nr)) {
+ element = remove_element(pool, gfp_mask);
+ spin_unlock_irqrestore(&pool->lock, flags);
+ /* paired with rmb in mempool_free(), read comment there */
+ smp_wmb();
+ /*
+ * Update the allocation stack trace as this is more useful
+ * for debugging.
+ */
+ kmemleak_update_trace(element);
+ break;
+ }
+
+ /* We must not sleep if !__GFP_DIRECT_RECLAIM */
+ if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
+ spin_unlock_irqrestore(&pool->lock, flags);
+ break;
+ }
- element = pool->alloc(gfp_temp, pool->pool_data);
- if (likely(element != NULL))
- return element;
+ /* Let's wait for someone else to return an element to @pool */
+ init_wait(&wait);
+ prepare_to_wait(&pool->wait, &wait, TASK_UNINTERRUPTIBLE);
- spin_lock_irqsave(&pool->lock, flags);
- if (likely(pool->curr_nr)) {
- element = remove_element(pool, gfp_temp);
spin_unlock_irqrestore(&pool->lock, flags);
- /* paired with rmb in mempool_free(), read comment there */
- smp_wmb();
+
/*
- * Update the allocation stack trace as this is more useful
- * for debugging.
+ * FIXME: this should be io_schedule(). The timeout is there as a
+ * workaround for some DM problems in 2.6.18.
*/
- kmemleak_update_trace(element);
- return element;
- }
+ io_schedule_timeout(5*HZ);
- /*
- * We use gfp mask w/o direct reclaim or IO for the first round. If
- * alloc failed with that and @pool was empty, retry immediately.
- */
- if ((gfp_temp & ~__GFP_NOMEMALLOC) != gfp_mask) {
- spin_unlock_irqrestore(&pool->lock, flags);
- gfp_temp = gfp_mask;
- goto repeat_alloc;
- }
- gfp_temp = gfp_mask;
+ finish_wait(&pool->wait, &wait);
- /* We must not sleep if !__GFP_DIRECT_RECLAIM */
- if (!(gfp_mask & __GFP_DIRECT_RECLAIM)) {
- spin_unlock_irqrestore(&pool->lock, flags);
- return NULL;
+ element = pool->alloc(gfp_mask, pool->pool_data);
}
-
- /* Let's wait for someone else to return an element to @pool */
- init_wait(&wait);
- prepare_to_wait(&pool->wait, &wait, TASK_UNINTERRUPTIBLE);
-
- spin_unlock_irqrestore(&pool->lock, flags);
-
- /*
- * FIXME: this should be io_schedule(). The timeout is there as a
- * workaround for some DM problems in 2.6.18.
- */
- io_schedule_timeout(5*HZ);
-
- finish_wait(&pool->wait, &wait);
- goto repeat_alloc;
+ return element;
}
EXPORT_SYMBOL(mempool_alloc);
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-25 10:40 +0200 |
| Subject | Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks |
| Message-ID | <rYJAC-2kh-25@gated-at.bofh.it> |
| In reply to | #1448848 |
On Sat 23-07-16 10:12:24, NeilBrown wrote:
> On Fri, Jul 22 2016, Michal Hocko wrote:
[...]
> > If we just back off and rely on kswapd which
> > might get stuck on the writeout then the IO throughput can be reduced
>
> If I were king of MM, I would make a decree to be proclaimed throughout
> the land
> kswapd must never sleep except when it explicitly chooses to
>
> Maybe that is impractical, but having firm rules like that would go a
> long way to make it possible to actually understand and reason about how
> MM works. As it is, there seems to be a tendency to put bandaids over
> bandaids.
Ohh, I would definitely wish for this to be more clear but as it turned
out over time there are quite some interdependencies between MM/FS/IO
layers which make the picture really blur. If there is a brave soul to
make that more clear without breaking any of that it would be really
cool ;)
> > I believe which would make the whole memory pressure just worse. So I am
> > not sure this is a good idea in general. I completely agree with you
> > that the mempool request shouldn't be throttled unless there is a strong
> > reason for that. More on that below.
> >
> >> If I'm following the code properly, the stack trace below can only
> >> happen if the first pool->alloc() attempt, with direct-reclaim disabled,
> >> fails and the pool is empty, so mempool_alloc() calls prepare_to_wait()
> >> and io_schedule_timeout().
> >
> > mempool_alloc retries immediatelly without any sleep after the first
> > no-reclaim attempt.
>
> I missed that ... I see it now... I wonder if anyone has contemplated
> using some modern programming techniques like, maybe, a "while" loop in
> there..
> Something like the below...
Heh, why not, the code could definitely see some more love. Care to send
a proper patch so that we are not mixing two different things here.
> >> I suspect the timeout *doesn't* fire (5 seconds is along time) so it
> >> gets woken up when there is something in the pool. It then loops around
> >> and tries pool->alloc() again, even though there is something in the
> >> pool. This might be justified if that ->alloc would never block, but
> >> obviously it does.
> >>
> >> I would very strongly recommend just changing mempool_alloc() to
> >> permanently mask out __GFP_DIRECT_RECLAIM.
> >>
> >> Quite separately I don't think PF_LESS_THROTTLE is at all appropriate.
> >> It is "LESS" throttle, not "NO" throttle, but you have made
> >> throttle_vm_writeout never throttle PF_LESS_THROTTLE threads.
> >
> > Yes that is correct. But it still allows to throttle on congestion:
> > shrink_inactive_list:
> > /*
> > * Stall direct reclaim for IO completions if underlying BDIs or zone
> > * is congested. Allow kswapd to continue until it starts encountering
> > * unqueued dirty pages or cycling through the LRU too quickly.
> > */
> > if (!sc->hibernation_mode && !current_is_kswapd() &&
> > current_may_throttle())
> > wait_iff_congested(pgdat, BLK_RW_ASYNC, HZ/10);
> >
> > My thinking was that throttle_vm_writeout is there to prevent from
> > dirtying too many pages from the reclaim the context. PF_LESS_THROTTLE
> > is part of the writeout so throttling it on too many dirty pages is
> > questionable (well we get some bias but that is not really reliable). It
> > still makes sense to throttle when the backing device is congested
> > because the writeout path wouldn't make much progress anyway and we also
> > do not want to cycle through LRU lists too quickly in that case.
>
> "dirtying ... from the reclaim context" ??? What does that mean?
Say you would cause a swapout from the reclaim context. You would
effectively dirty that anon page until it gets written down to the
storage.
> According to
> Commit: 26eecbf3543b ("[PATCH] vm: pageout throttling")
> From the history tree, the purpose of throttle_vm_writeout() is to
> limit the amount of memory that is concurrently under I/O.
> That seems strange to me because I thought it was the responsibility of
> each backing device to impose a limit - a maximum queue size of some
> sort.
We do throttle on the congestion during the reclaim so in some
sense this is already implemented but I am not really sure that is
sufficient. Maybe this is something to re-evaluate because
wait_iff_congested came in much later after throttle_vm_writeout. Let me
think about it some more.
> I remember when NFS didn't impose a limit and you could end up with lots
> of memory in NFS write-back, and very long latencies could result.
>
> So I wonder what throttle_vm_writeout() really achieves these days. Is
> it just a bandaid that no-one is brave enough to remove?
Maybe yes. It is sitting there quietly and you do not know about it
until it bites. Like in this particular case.
> I guess it could play a role in balancing the freeing of clean pages,
> which can be done instantly, against dirty pages, which require
> writeback. Without some throttling, might all clean pages being cleaned
> too quickly, just trashing our read caches?
I do not see how that would happen. kswapd has its reclaim targets
depending on watermarks and direct reclaim has SWAP_CLUSTER_MAX. So none
of them should go too wild and reclaim way too many clean pages.
> > Or is this assumption wrong for nfsd_vfs_write? Can it cause unbounded
> > dirtying of memory?
>
> In most cases, nfsd it just like any other application and needs to be
> throttled like any other application when it writes too much data.
> The only time nfsd *needs* PF_LESS_THROTTLE when when a loop-back mount
> is active. When the same page cache is the source and destination of
> writes.
> So nfsd needs to be able to dirty a few more pages when nothing else
> can due to high dirty count. Otherwise it deadlocks.
> The main use of PF_LESS_THROTTLE is in zone_dirty_limit() and
> domain_dirty_limits() where an extra 25% is allowed to overcome this
> deadlock.
>
> The use of PF_LESS_THROTTLE in current_may_throttle() in vmscan.c is to
> avoid a live-lock. A key premise is that nfsd only allocates unbounded
> memory when it is writing to the page cache. So it only needs to be
> throttled when the backing device it is writing to is congested. It is
> particularly important that it *doesn't* get throttled just because an
> NFS backing device is congested, because nfsd might be trying to clear
> that congestion.
Thanks for the clarification. IIUC then removing throttle_vm_writeout
for the nfsd writeout should be harmless as well, right?
> In general, callers of try_to_free_pages() might get throttled when any
> backing device is congested. This is a reasonable default when we don't
> know what they are allocating memory for. When we do know the purpose of
> the allocation, we can be more cautious about throttling.
>
> If a thread is allocating just to dirty pages for a given backing
> device, we only need to throttle the allocation if the backing device is
> congested. Any further throttling needed happens in
> balance_dirty_pages().
>
> If a thread is only making transient allocations, ones which will be
> freed shortly afterwards (not, for example, put in a cache), then I
> don't think it needs to be throttled at all. I think this universally
> applies to mempools.
> In the case of dm_crypt, if it is writing too fast it will eventually be
> throttled in generic_make_request when the underlying device has a full
> queue and so blocks waiting for requests to be completed, and thus parts
> of them returned to the mempool.
Makes sense to me.
> >> The purpose of that flag is to allow a thread to dirty a page-cache page
> >> as part of cleaning another page-cache page.
> >> So it makes sense for loop and sometimes for nfsd. It would make sense
> >> for dm-crypt if it was putting the encrypted version in the page cache.
> >> But if dm-crypt is just allocating a transient page (which I think it
> >> is), then a mempool should be sufficient (and we should make sure it is
> >> sufficient) and access to an extra 10% (or whatever) of the page cache
> >> isn't justified.
> >
> > If you think that PF_LESS_THROTTLE (ab)use in mempool_alloc is not
> > appropriate then would a PF_MEMPOOL be any better?
>
> Why a PF rather than a GFP flag?
Well, short answer is that gfp masks are almost depleted.
> NFSD uses a PF because there is no GFP interface for filesystem write.
> But mempool can pass down a GFP flag, so I think it should.
> The meaning of the flag is, in my opinion, that a 'transient' allocation
> is being requested. i.e. an allocation which will be used for a single
> purpose for a short amount of time and will then be freed. In
> particularly it will never be placed in a cache, and if it is ever
> placed on a queue, that is certain to be a queue with an upper bound on
> the size and with guaranteed forward progress in the face of memory
> pressure.
> Any allocation request for a use case with those properties should be
> allowed to set GFP_TRANSIENT (for example) with the effect that the
> allocation will not be throttled.
> A key point with the name is to identify the purpose of the flag, not a
> specific use case (mempool) which we want it for.
Agreed. But let's first explore throttle_vm_writeout and its potential
removal.
Thanks!
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-25 21:30 +0200 |
| Subject | Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks |
| Message-ID | <rYTJD-8ug-21@gated-at.bofh.it> |
| In reply to | #1449367 |
[CC Marcelo who might remember other details for the loads which made
him to add this code - see the patch changelog for more context]
On Mon 25-07-16 10:32:47, Michal Hocko wrote:
> On Sat 23-07-16 10:12:24, NeilBrown wrote:
[...]
> > So I wonder what throttle_vm_writeout() really achieves these days. Is
> > it just a bandaid that no-one is brave enough to remove?
>
> Maybe yes. It is sitting there quietly and you do not know about it
> until it bites. Like in this particular case.
So I was playing with this today and tried to provoke throttle_vm_writeout
and couldn't hit that path with my pretty much default IO stack. I
probably need a more complex IO setup like dm-crypt or something that
basically have to double buffer every page in the writeout for some
time.
Anyway I believe that the throttle_vm_writeout is just a relict from the
past which just survived after many other changes in the reclaim path. I
fully realize my testing is quite poor and I would really appreciate if
Mikulas could try to retest with his more complex IO setups but let me
post a patch with the changelog so that we can at least reason about the
justification. In principle the reclaim path should have sufficient
throttling already and if that is not the case then we should
consolidate the remaining rather than have yet another one.
Thoughts?
---
From 0d950d64e3c59061f7cca71fe5877d4e430499c9 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 25 Jul 2016 14:18:54 +0200
Subject: [PATCH] mm, vmscan: get rid of throttle_vm_writeout
throttle_vm_writeout has been introduced back in 2005 to fix OOMs caused
by excessive pageout activity during the reclaim. Too many pages could
be put under writeback therefore LRUs would be full of unreclaimable pages
until the IO completes and in turn the OOM killer could be invoked.
There have been some important changes introduced since then in the
reclaim path though. Writers are throttled by balance_dirty_pages
when initiating the buffered IO and later during the memory pressure,
the direct reclaim is throttled by wait_iff_congested if the node is
considered congested by dirty pages on LRUs and the underlying bdi
is congested by the queued IO. The kswapd is throttled as well if it
encounters pages marked for immediate reclaim or under writeback which
signals that that there are too many pages under writeback already.
Another important aspect is that we do not issue any IO from the direct
reclaim context anymore. In a heavy parallel load this could queue a lot
of IO which would be very scattered and thus unefficient which would
just make the problem worse.
This three mechanisms should throttle and keep the amount of IO in a
steady state even under heavy IO and memory pressure so yet another
throttling point doesn't really seem helpful. Quite contrary, Mikulas
Patocka has reported that swap backed by dm-crypt doesn't work properly
because the swapout IO cannot make sufficient progress as the writeout
path depends on dm_crypt worker which has to allocate memory to perform
the encryption. In order to guarantee a forward progress it relies
on the mempool allocator. mempool_alloc(), however, prefers to use
the underlying (usually page) allocator before it grabs objects from
the pool. Such an allocation can dive into the memory reclaim and
consequently to throttle_vm_writeout. If there are too many dirty or
pages under writeback it will get throttled even though it is in fact a
flusher to clear pending pages.
[ 345.352536] kworker/u4:0 D ffff88003df7f438 10488 6 2 0x00000000
[ 345.352536] Workqueue: kcryptd kcryptd_crypt [dm_crypt]
[ 345.352536] ffff88003df7f438 ffff88003e5d0380 ffff88003e5d0380 ffff88003e5d8e80
[ 345.352536] ffff88003dfb3240 ffff88003df73240 ffff88003df80000 ffff88003df7f470
[ 345.352536] ffff88003e5d0380 ffff88003e5d0380 ffff88003df7f828 ffff88003df7f450
[ 345.352536] Call Trace:
[ 345.352536] [<ffffffff818d466c>] schedule+0x3c/0x90
[ 345.352536] [<ffffffff818d96a8>] schedule_timeout+0x1d8/0x360
[ 345.352536] [<ffffffff81135e40>] ? detach_if_pending+0x1c0/0x1c0
[ 345.352536] [<ffffffff811407c3>] ? ktime_get+0xb3/0x150
[ 345.352536] [<ffffffff811958cf>] ? __delayacct_blkio_start+0x1f/0x30
[ 345.352536] [<ffffffff818d39e4>] io_schedule_timeout+0xa4/0x110
[ 345.352536] [<ffffffff8121d886>] congestion_wait+0x86/0x1f0
[ 345.352536] [<ffffffff810fdf40>] ? prepare_to_wait_event+0xf0/0xf0
[ 345.352536] [<ffffffff812061d4>] throttle_vm_writeout+0x44/0xd0
[ 345.352536] [<ffffffff81211533>] shrink_zone_memcg+0x613/0x720
[ 345.352536] [<ffffffff81211720>] shrink_zone+0xe0/0x300
[ 345.352536] [<ffffffff81211aed>] do_try_to_free_pages+0x1ad/0x450
[ 345.352536] [<ffffffff81211e7f>] try_to_free_pages+0xef/0x300
[ 345.352536] [<ffffffff811fef19>] __alloc_pages_nodemask+0x879/0x1210
[ 345.352536] [<ffffffff810e8080>] ? sched_clock_cpu+0x90/0xc0
[ 345.352536] [<ffffffff8125a8d1>] alloc_pages_current+0xa1/0x1f0
[ 345.352536] [<ffffffff81265ef5>] ? new_slab+0x3f5/0x6a0
[ 345.352536] [<ffffffff81265dd7>] new_slab+0x2d7/0x6a0
[ 345.352536] [<ffffffff810e7f87>] ? sched_clock_local+0x17/0x80
[ 345.352536] [<ffffffff812678cb>] ___slab_alloc+0x3fb/0x5c0
[ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30
[ 345.352536] [<ffffffff810e7f87>] ? sched_clock_local+0x17/0x80
[ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30
[ 345.352536] [<ffffffff81267ae1>] __slab_alloc+0x51/0x90
[ 345.352536] [<ffffffff811f71bd>] ? mempool_alloc_slab+0x1d/0x30
[ 345.352536] [<ffffffff81267d9b>] kmem_cache_alloc+0x27b/0x310
[ 345.352536] [<ffffffff811f71bd>] mempool_alloc_slab+0x1d/0x30
[ 345.352536] [<ffffffff811f6f11>] mempool_alloc+0x91/0x230
[ 345.352536] [<ffffffff8141a02d>] bio_alloc_bioset+0xbd/0x260
[ 345.352536] [<ffffffffc02f1a54>] kcryptd_crypt+0x114/0x3b0 [dm_crypt]
Let's just drop throttle_vm_writeout altogether. It is not very much
helpful anymore.
I have tried to test a potential writeback IO runaway similar to the one
described in the original patch which has introduced that [1]. Small
virtual machine (512MB RAM, 4 CPUs, 2G of swap space and disk image on a
rather slow NFS in a sync mode on the host) with 8 parallel writers each
writing 1G worth of data. As soon as the pagecache fills up and the
direct reclaim hits then I start anon memory consumer in a loop
(allocating 300M and exiting after populating it) in the background
to make the memory pressure even stronger as well as to disrupt the
steady state for the IO. The direct reclaim is throttled because of the
congestion as well as kswapd hitting congestion_wait due to nr_immediate
but throttle_vm_writeout doesn't ever trigger the sleep throughout
the test. Dirty+writeback are close to nr_dirty_threshold with some
fluctuations caused by the anon consumer.
[1] https://www2.kernel.org/pub/linux/kernel/people/akpm/patches/2.6/2.6.9-rc1/2.6.9-rc1-mm3/broken-out/vm-pageout-throttling.patch
Cc: Marcelo Tosatti <mtosatti@redhat.com>
Reported-by: Mikulas Patocka <mpatocka@redhat.com>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
include/linux/writeback.h | 1 -
mm/page-writeback.c | 30 ------------------------------
mm/vmscan.c | 2 --
3 files changed, 33 deletions(-)
diff --git a/include/linux/writeback.h b/include/linux/writeback.h
index 44b4422ae57f..f67a992cdf89 100644
--- a/include/linux/writeback.h
+++ b/include/linux/writeback.h
@@ -319,7 +319,6 @@ void laptop_mode_timer_fn(unsigned long data);
#else
static inline void laptop_sync_completion(void) { }
#endif
-void throttle_vm_writeout(gfp_t gfp_mask);
bool node_dirty_ok(struct pglist_data *pgdat);
int wb_domain_init(struct wb_domain *dom, gfp_t gfp);
#ifdef CONFIG_CGROUP_WRITEBACK
diff --git a/mm/page-writeback.c b/mm/page-writeback.c
index b82303a9e67d..2828d6ca1451 100644
--- a/mm/page-writeback.c
+++ b/mm/page-writeback.c
@@ -1962,36 +1962,6 @@ bool wb_over_bg_thresh(struct bdi_writeback *wb)
return false;
}
-void throttle_vm_writeout(gfp_t gfp_mask)
-{
- unsigned long background_thresh;
- unsigned long dirty_thresh;
-
- for ( ; ; ) {
- global_dirty_limits(&background_thresh, &dirty_thresh);
- dirty_thresh = hard_dirty_limit(&global_wb_domain, dirty_thresh);
-
- /*
- * Boost the allowable dirty threshold a bit for page
- * allocators so they don't get DoS'ed by heavy writers
- */
- dirty_thresh += dirty_thresh / 10; /* wheeee... */
-
- if (global_node_page_state(NR_UNSTABLE_NFS) +
- global_node_page_state(NR_WRITEBACK) <= dirty_thresh)
- break;
- congestion_wait(BLK_RW_ASYNC, HZ/10);
-
- /*
- * The caller might hold locks which can prevent IO completion
- * or progress in the filesystem. So we cannot just sit here
- * waiting for IO to complete.
- */
- if ((gfp_mask & (__GFP_FS|__GFP_IO)) != (__GFP_FS|__GFP_IO))
- break;
- }
-}
-
/*
* sysctl handler for /proc/sys/vm/dirty_writeback_centisecs
*/
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 0294ab34f475..0f35ed30e35b 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2410,8 +2410,6 @@ static void shrink_node_memcg(struct pglist_data *pgdat, struct mem_cgroup *memc
if (inactive_list_is_low(lruvec, false, sc))
shrink_active_list(SWAP_CLUSTER_MAX, lruvec,
sc, LRU_ACTIVE_ANON);
-
- throttle_vm_writeout(sc->gfp_mask);
}
/* Use reclaim/compaction for costly allocs or under memory pressure */
--
2.8.1
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | David Rientjes <rientjes@google.com> |
|---|---|
| Date | 2016-07-19 04:10 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rWsDT-68O-5@gated-at.bofh.it> |
| In reply to | #1445305 |
On Mon, 18 Jul 2016, Michal Hocko wrote: > David Rientjes was objecting that such an approach wouldn't help if the > oom victim was blocked on a lock held by process doing mempool_alloc. This > is very similar to other oom deadlock situations and we have oom_reaper > to deal with them so it is reasonable to rely on the same mechanism > rather inventing a different one which has negative side effects. > Right, this causes oom livelock as described in the aforementioned thread: the oom victim is waiting on a mutex that is held by a thread doing mempool_alloc(). The oom reaper is not guaranteed to free any memory, so nothing on the system can allocate memory from the page allocator. I think the better solution here is to allow mempool_alloc() users to set __GFP_NOMEMALLOC if they are in a context which allows them to deplete memory reserves.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-19 09:50 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rWxWW-1ci-13@gated-at.bofh.it> |
| In reply to | #1445991 |
On Mon 18-07-16 19:00:57, David Rientjes wrote: > On Mon, 18 Jul 2016, Michal Hocko wrote: > > > David Rientjes was objecting that such an approach wouldn't help if the > > oom victim was blocked on a lock held by process doing mempool_alloc. This > > is very similar to other oom deadlock situations and we have oom_reaper > > to deal with them so it is reasonable to rely on the same mechanism > > rather inventing a different one which has negative side effects. > > > > Right, this causes oom livelock as described in the aforementioned thread: > the oom victim is waiting on a mutex that is held by a thread doing > mempool_alloc(). The backtrace you have provided: schedule schedule_timeout io_schedule_timeout mempool_alloc __split_and_process_bio dm_request generic_make_request submit_bio mpage_readpages ext4_readpages __do_page_cache_readahead ra_submit filemap_fault handle_mm_fault __do_page_fault do_page_fault page_fault is not PF_MEMALLOC context AFAICS so clearing __GFP_NOMEMALLOC for such a task will not help unless that task has TIF_MEMDIE. Could you provide a trace where the PF_MEMALLOC context holding a lock cannot make a forward progress? > The oom reaper is not guaranteed to free any memory, so > nothing on the system can allocate memory from the page allocator. Sure, there is no guarantee but as I've said earlier, 1) oom_reaper will allow to select another victim in many cases and 2) such a deadlock is no different from any other where the victim cannot continue because of another context blocking a lock while waiting for memory. Tweaking mempool allocator to potentially catch such a case in a different way doesn't sound right in principle, not to mention this is other dangerous side effects. > I think the better solution here is to allow mempool_alloc() users to set > __GFP_NOMEMALLOC if they are in a context which allows them to deplete > memory reserves. I am not really sure about that. I agree with Johannes [1] that this is bending mempool allocator into an undesirable direction because the point of the mempool is to have its own reliably reusable memory reserves. Now I am even not sure whether TIF_MEMDIE exception is a good way forward and a plain revert is more appropriate. Let's CC Johannes. The patch is [2]. [1] http://lkml.kernel.org/r/20160718151445.GB14604@cmpxchg.org [2] http://lkml.kernel.org/r/1468831285-27242-1-git-send-email-mhocko@kernel.org -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Johannes Weiner <hannes@cmpxchg.org> |
|---|---|
| Date | 2016-07-19 16:00 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rWDIZ-4IM-3@gated-at.bofh.it> |
| In reply to | #1445305 |
On Mon, Jul 18, 2016 at 10:41:24AM +0200, Michal Hocko wrote:
> The original intention of f9054c70d28b was to help with the OOM
> situations where the oom victim depends on mempool allocation to make a
> forward progress. We can handle that case in a different way, though. We
> can check whether the current task has access to memory reserves ad an
> OOM victim (TIF_MEMDIE) and drop __GFP_NOMEMALLOC protection if the pool
> is empty.
>
> David Rientjes was objecting that such an approach wouldn't help if the
> oom victim was blocked on a lock held by process doing mempool_alloc. This
> is very similar to other oom deadlock situations and we have oom_reaper
> to deal with them so it is reasonable to rely on the same mechanism
> rather inventing a different one which has negative side effects.
I don't understand how this scenario wouldn't be a flat-out bug.
Mempool guarantees forward progress by having all necessary memory
objects for the guaranteed operation in reserve. Think about it this
way: you should be able to delete the pool->alloc() call entirely and
still make reliable forward progress. It would kill concurrency and be
super slow, but how could it be affected by a system OOM situation?
If our mempool_alloc() is waiting for an object that an OOM victim is
holding, where could that OOM victim get stuck before giving it back?
As I asked in the previous thread, surely you wouldn't do a mempool
allocation first and then rely on an unguarded page allocation to make
forward progress, right? It would defeat the purpose of using mempools
in the first place. And surely the OOM victim wouldn't be waiting for
a lock that somebody doing mempool_alloc() *against the same mempool*
is holding. That'd be an obvious ABBA deadlock.
So maybe I'm just dense, but could somebody please outline the exact
deadlock diagram? Who is doing what, and how are they getting stuck?
cpu0: cpu1:
mempool_alloc(pool0)
mempool_alloc(pool0)
wait for cpu1
not allocating memory - would defeat mempool
not taking locks held by cpu0* - would ABBA
???
mempool_free(pool0)
Thanks
* or any other task that does mempool_alloc(pool0) before unlock
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-19 16:30 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rWEc2-57V-15@gated-at.bofh.it> |
| In reply to | #1446493 |
On Tue 19-07-16 09:54:26, Johannes Weiner wrote: > On Mon, Jul 18, 2016 at 10:41:24AM +0200, Michal Hocko wrote: > > The original intention of f9054c70d28b was to help with the OOM > > situations where the oom victim depends on mempool allocation to make a > > forward progress. We can handle that case in a different way, though. We > > can check whether the current task has access to memory reserves ad an > > OOM victim (TIF_MEMDIE) and drop __GFP_NOMEMALLOC protection if the pool > > is empty. > > > > David Rientjes was objecting that such an approach wouldn't help if the > > oom victim was blocked on a lock held by process doing mempool_alloc. This > > is very similar to other oom deadlock situations and we have oom_reaper > > to deal with them so it is reasonable to rely on the same mechanism > > rather inventing a different one which has negative side effects. > > I don't understand how this scenario wouldn't be a flat-out bug. > > Mempool guarantees forward progress by having all necessary memory > objects for the guaranteed operation in reserve. Think about it this > way: you should be able to delete the pool->alloc() call entirely and > still make reliable forward progress. It would kill concurrency and be > super slow, but how could it be affected by a system OOM situation? Yes this is my understanding of the mempool usage as well. It is much harder to check whether mempool users are really behaving and they do not request more than the pre allocated pool allows them, though. That would be a bug in the consumer not the mempool as such of course. My original understanding of f9054c70d28b was that it acts as a prevention for issues where the OOM victim loops inside the mempool_alloc not doing reasonable progress because those who should refill the pool are stuck for some reason (aka assume that not all mempool users are behaving or they have unexpected dependencies like WQ without WQ_MEM_RECLAIM and similar). My thinking was that the victim has access to memory reserves by default so it sounds reasonable to preserve this access also when it is in the mempool_alloc. Therefore I wanted to preserve that particular logic and came up with this patch which should be safer than f9054c70d28b. But the more I am thinking about it the more it sounds like papering over a bug somewhere else. So I guess we should just go and revert f9054c70d28b and get back to David's lockup and investigate what exactly went wrong and why. The current form of f9054c70d28b is simply too dangerous. -- Michal Hocko SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Mikulas Patocka <mpatocka@redhat.com> |
|---|---|
| Date | 2016-07-20 00:10 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rWLnb-1hJ-9@gated-at.bofh.it> |
| In reply to | #1446522 |
On Tue, 19 Jul 2016, Michal Hocko wrote: > On Tue 19-07-16 09:54:26, Johannes Weiner wrote: > > On Mon, Jul 18, 2016 at 10:41:24AM +0200, Michal Hocko wrote: > > > The original intention of f9054c70d28b was to help with the OOM > > > situations where the oom victim depends on mempool allocation to make a > > > forward progress. We can handle that case in a different way, though. We > > > can check whether the current task has access to memory reserves ad an > > > OOM victim (TIF_MEMDIE) and drop __GFP_NOMEMALLOC protection if the pool > > > is empty. > > > > > > David Rientjes was objecting that such an approach wouldn't help if the > > > oom victim was blocked on a lock held by process doing mempool_alloc. This > > > is very similar to other oom deadlock situations and we have oom_reaper > > > to deal with them so it is reasonable to rely on the same mechanism > > > rather inventing a different one which has negative side effects. > > > > I don't understand how this scenario wouldn't be a flat-out bug. > > > > Mempool guarantees forward progress by having all necessary memory > > objects for the guaranteed operation in reserve. Think about it this > > way: you should be able to delete the pool->alloc() call entirely and > > still make reliable forward progress. It would kill concurrency and be > > super slow, but how could it be affected by a system OOM situation? > > Yes this is my understanding of the mempool usage as well. It is much Yes, that's correct. > harder to check whether mempool users are really behaving and they do > not request more than the pre allocated pool allows them, though. That > would be a bug in the consumer not the mempool as such of course. > > My original understanding of f9054c70d28b was that it acts as > a prevention for issues where the OOM victim loops inside the > mempool_alloc not doing reasonable progress because those who should > refill the pool are stuck for some reason (aka assume that not all > mempool users are behaving or they have unexpected dependencies like WQ > without WQ_MEM_RECLAIM and similar). David Rientjes didn't tell us what is the configuration of his servers, we don't know what dm targets and block device drivers is he using, we don't know how they are connected - so it not really possible to know what happened for him. Mikulas > My thinking was that the victim has access to memory reserves by default > so it sounds reasonable to preserve this access also when it is in the > mempool_alloc. Therefore I wanted to preserve that particular logic and > came up with this patch which should be safer than f9054c70d28b. But the > more I am thinking about it the more it sounds like papering over a bug > somewhere else. > > So I guess we should just go and revert f9054c70d28b and get back to > David's lockup and investigate what exactly went wrong and why. The > current form of f9054c70d28b is simply too dangerous. > -- > Michal Hocko > SUSE Labs >
[toc] | [prev] | [next] | [standalone]
| From | David Rientjes <rientjes@google.com> |
|---|---|
| Date | 2016-07-19 22:50 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rWK7L-kN-5@gated-at.bofh.it> |
| In reply to | #1446493 |
On Tue, 19 Jul 2016, Johannes Weiner wrote: > Mempool guarantees forward progress by having all necessary memory > objects for the guaranteed operation in reserve. Think about it this > way: you should be able to delete the pool->alloc() call entirely and > still make reliable forward progress. It would kill concurrency and be > super slow, but how could it be affected by a system OOM situation? > > If our mempool_alloc() is waiting for an object that an OOM victim is > holding, where could that OOM victim get stuck before giving it back? > As I asked in the previous thread, surely you wouldn't do a mempool > allocation first and then rely on an unguarded page allocation to make > forward progress, right? It would defeat the purpose of using mempools > in the first place. And surely the OOM victim wouldn't be waiting for > a lock that somebody doing mempool_alloc() *against the same mempool* > is holding. That'd be an obvious ABBA deadlock. > > So maybe I'm just dense, but could somebody please outline the exact > deadlock diagram? Who is doing what, and how are they getting stuck? > > cpu0: cpu1: > mempool_alloc(pool0) > mempool_alloc(pool0) > wait for cpu1 > not allocating memory - would defeat mempool > not taking locks held by cpu0* - would ABBA > ??? > mempool_free(pool0) > > Thanks > > * or any other task that does mempool_alloc(pool0) before unlock > I'm approaching this from a perspective of any possible mempool usage, not with any single current user in mind. Any mempool_alloc() user that then takes a contended mutex can do this. An example: taskA taskB taskC ----- ----- ----- mempool_alloc(a) mutex_lock(b) mutex_lock(b) mempool_alloc(a) Imagine the mempool_alloc() done by taskA depleting all free elements so we rely on it to do mempool_free() before any other mempool allocator can be guaranteed. If taskC is oom killed, or has PF_MEMALLOC set, it cannot access memory reserves from the page allocator if __GFP_NOMEMALLOC is automatic in mempool_alloc(). This livelocks the page allocator for all processes. taskB in this case need only stall after taking mutex_lock() successfully; that could be because of the oom livelock, it is contended on another mutex held by an allocator, etc. Obviously taskB stalling while holding a mutex that is contended by a mempool user holding an element is not preferred, but it's possible. (A simplified version is also possible with 0-size mempools, which are also allowed.) My point is that I don't think we should be forcing any behavior wrt memory reserves as part of the mempool implementation. In the above, taskC mempool_alloc() would succeed and not livelock unless __GFP_NOMEMALLOC is forced. The mempool_alloc() user may construct their set of gfp flags as appropriate just like any other memory allocator in the kernel. The alternative would be to ensure no mempool users ever take a lock that another thread can hold while contending another mutex or allocating memory itself.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-20 10:20 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rWUTw-7mO-19@gated-at.bofh.it> |
| In reply to | #1446727 |
On Tue 19-07-16 13:45:52, David Rientjes wrote:
> On Tue, 19 Jul 2016, Johannes Weiner wrote:
>
> > Mempool guarantees forward progress by having all necessary memory
> > objects for the guaranteed operation in reserve. Think about it this
> > way: you should be able to delete the pool->alloc() call entirely and
> > still make reliable forward progress. It would kill concurrency and be
> > super slow, but how could it be affected by a system OOM situation?
> >
> > If our mempool_alloc() is waiting for an object that an OOM victim is
> > holding, where could that OOM victim get stuck before giving it back?
> > As I asked in the previous thread, surely you wouldn't do a mempool
> > allocation first and then rely on an unguarded page allocation to make
> > forward progress, right? It would defeat the purpose of using mempools
> > in the first place. And surely the OOM victim wouldn't be waiting for
> > a lock that somebody doing mempool_alloc() *against the same mempool*
> > is holding. That'd be an obvious ABBA deadlock.
> >
> > So maybe I'm just dense, but could somebody please outline the exact
> > deadlock diagram? Who is doing what, and how are they getting stuck?
> >
> > cpu0: cpu1:
> > mempool_alloc(pool0)
> > mempool_alloc(pool0)
> > wait for cpu1
> > not allocating memory - would defeat mempool
> > not taking locks held by cpu0* - would ABBA
> > ???
> > mempool_free(pool0)
> >
> > Thanks
> >
> > * or any other task that does mempool_alloc(pool0) before unlock
> >
>
> I'm approaching this from a perspective of any possible mempool usage, not
> with any single current user in mind.
>
> Any mempool_alloc() user that then takes a contended mutex can do this.
> An example:
>
> taskA taskB taskC
> ----- ----- -----
> mempool_alloc(a)
> mutex_lock(b)
> mutex_lock(b)
> mempool_alloc(a)
>
> Imagine the mempool_alloc() done by taskA depleting all free elements so
> we rely on it to do mempool_free() before any other mempool allocator can
> be guaranteed.
>
> If taskC is oom killed, or has PF_MEMALLOC set, it cannot access memory
> reserves from the page allocator if __GFP_NOMEMALLOC is automatic in
> mempool_alloc(). This livelocks the page allocator for all processes.
>
> taskB in this case need only stall after taking mutex_lock() successfully;
> that could be because of the oom livelock, it is contended on another
> mutex held by an allocator, etc.
But that falls down to the deadlock described by Johannes above because
then the mempool user would _depend_ on an "unguarded page allocation"
via that particular lock and that is a bug.
> Obviously taskB stalling while holding a mutex that is contended by a
> mempool user holding an element is not preferred, but it's possible. (A
> simplified version is also possible with 0-size mempools, which are also
> allowed.)
>
> My point is that I don't think we should be forcing any behavior wrt
> memory reserves as part of the mempool implementation.
Isn't the reserve management the whole point of the mempool approach?
> In the above,
> taskC mempool_alloc() would succeed and not livelock unless
> __GFP_NOMEMALLOC is forced.
Or it would get stuck because even page allocator memory reserves got
depleted. Without any way to throttle there is no guarantee to make
further progress. In fact this is not a theoretical situation. It has
been observed with the swap over dm-crypt and there shouldn't be any
lock dependeces you are describing above there AFAIU.
> The mempool_alloc() user may construct their
> set of gfp flags as appropriate just like any other memory allocator in
> the kernel.
So which users of mempool_alloc would benefit from not having
__GFP_NOMEMALLOC and why?
> The alternative would be to ensure no mempool users ever take a lock that
> another thread can hold while contending another mutex or allocating
> memory itself.
I am not sure how can we enforce that but surely that would detect a
clear mempool usage bug. Lockdep could be probably extended to do so.
Anway, I feel we are looping in a circle. We have a clear regression
caused by your patch. It might solve some oom livelock you are seeing
but there are only very dim details about it and the patch might very
well paper over a bug in mempool usage somewhere else. We definitely
need more details to know that better.
That being said, f9054c70d28b ("mm, mempool: only set __GFP_NOMEMALLOC
if there are free elements") should be either reverted or
http://lkml.kernel.org/r/1468831285-27242-1-git-send-email-mhocko@kernel.org
should be applied as a temporal workaround because it would make a
lockup less likely for now until we find out more about your issue.
Does that sound like a way forward?
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | David Rientjes <rientjes@google.com> |
|---|---|
| Date | 2016-07-20 23:10 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rX6UG-6zY-5@gated-at.bofh.it> |
| In reply to | #1447077 |
On Wed, 20 Jul 2016, Michal Hocko wrote: > > Any mempool_alloc() user that then takes a contended mutex can do this. > > An example: > > > > taskA taskB taskC > > ----- ----- ----- > > mempool_alloc(a) > > mutex_lock(b) > > mutex_lock(b) > > mempool_alloc(a) > > > > Imagine the mempool_alloc() done by taskA depleting all free elements so > > we rely on it to do mempool_free() before any other mempool allocator can > > be guaranteed. > > > > If taskC is oom killed, or has PF_MEMALLOC set, it cannot access memory > > reserves from the page allocator if __GFP_NOMEMALLOC is automatic in > > mempool_alloc(). This livelocks the page allocator for all processes. > > > > taskB in this case need only stall after taking mutex_lock() successfully; > > that could be because of the oom livelock, it is contended on another > > mutex held by an allocator, etc. > > But that falls down to the deadlock described by Johannes above because > then the mempool user would _depend_ on an "unguarded page allocation" > via that particular lock and that is a bug. > It becomes a deadlock because of mempool_alloc(a) forcing __GFP_NOMEMALLOC, I agree. For that not to be the case, it must be required that between mempool_alloc() and mempool_free() that we take no mutex that may be held by any other thread on the system, in any context, that is allocating memory. If that's a caller's bug as you describe it, and only enabled by mempool_alloc() forcing __GFP_NOMEMALLOC, then please add the relevant lockdep detection, which would be trivial to add, so we can determine if any users are unsafe and prevent this issue in the future. The overwhelming goal here should be to prevent possible problems in the future especially if an API does not allow you to opt-out of the behavior. > > My point is that I don't think we should be forcing any behavior wrt > > memory reserves as part of the mempool implementation. > > Isn't the reserve management the whole point of the mempool approach? > No, the whole point is to maintain the freelist of elements that are guaranteed; my suggestion is that we cannot make that guarantee if we are blocked from freeing elements. It's trivial to fix by allowing __GFP_NOMEMALLOC from the caller in cases where you cannot possibly be blocked by an oom victim. > Or it would get stuck because even page allocator memory reserves got > depleted. Without any way to throttle there is no guarantee to make > further progress. In fact this is not a theoretical situation. It has > been observed with the swap over dm-crypt and there shouldn't be any > lock dependeces you are describing above there AFAIU. > They should do mempool_alloc(__GFP_NOMEMALLOC), no argument. > > The mempool_alloc() user may construct their > > set of gfp flags as appropriate just like any other memory allocator in > > the kernel. > > So which users of mempool_alloc would benefit from not having > __GFP_NOMEMALLOC and why? > Any mempool_alloc() user that would be blocked on returning the element back to the freelist by an oom condition. I think the dm-crypt case is quite unique on how it is able to deplete memory reserves. > Anway, I feel we are looping in a circle. We have a clear regression > caused by your patch. It might solve some oom livelock you are seeing > but there are only very dim details about it and the patch might very > well paper over a bug in mempool usage somewhere else. We definitely > need more details to know that better. > What is the objection to allowing __GFP_NOMEMALLOC from the caller with clear documentation on how to use it? It can be described to not allow depletion of memory reserves with the caveat that the caller must ensure mempool_free() cannot be blocked in lowmem situations.
[toc] | [prev] | [next] | [standalone]
| From | Michal Hocko <mhocko@kernel.org> |
|---|---|
| Date | 2016-07-21 11:00 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rXhZL-57r-15@gated-at.bofh.it> |
| In reply to | #1447471 |
On Wed 20-07-16 14:06:26, David Rientjes wrote:
> On Wed, 20 Jul 2016, Michal Hocko wrote:
>
> > > Any mempool_alloc() user that then takes a contended mutex can do this.
> > > An example:
> > >
> > > taskA taskB taskC
> > > ----- ----- -----
> > > mempool_alloc(a)
> > > mutex_lock(b)
> > > mutex_lock(b)
> > > mempool_alloc(a)
> > >
> > > Imagine the mempool_alloc() done by taskA depleting all free elements so
> > > we rely on it to do mempool_free() before any other mempool allocator can
> > > be guaranteed.
> > >
> > > If taskC is oom killed, or has PF_MEMALLOC set, it cannot access memory
> > > reserves from the page allocator if __GFP_NOMEMALLOC is automatic in
> > > mempool_alloc(). This livelocks the page allocator for all processes.
> > >
> > > taskB in this case need only stall after taking mutex_lock() successfully;
> > > that could be because of the oom livelock, it is contended on another
> > > mutex held by an allocator, etc.
> >
> > But that falls down to the deadlock described by Johannes above because
> > then the mempool user would _depend_ on an "unguarded page allocation"
> > via that particular lock and that is a bug.
> >
>
> It becomes a deadlock because of mempool_alloc(a) forcing
> __GFP_NOMEMALLOC, I agree.
>
> For that not to be the case, it must be required that between
> mempool_alloc() and mempool_free() that we take no mutex that may be held
> by any other thread on the system, in any context, that is allocating
> memory. If that's a caller's bug as you describe it, and only enabled by
> mempool_alloc() forcing __GFP_NOMEMALLOC, then please add the relevant
> lockdep detection, which would be trivial to add, so we can determine if
> any users are unsafe and prevent this issue in the future.
I am sorry but I am neither familiar with the lockdep internals nor I
have a time to add this support.
> The
> overwhelming goal here should be to prevent possible problems in the
> future especially if an API does not allow you to opt-out of the behavior.
The __GFP_NOMEMALLOC enforcement is there since b84a35be0285 ("[PATCH]
mempool: NOMEMALLOC and NORETRY") so more than 10 years ago. So I think
it is quite reasonable to expect that users are familiar with this fact
and handle it properly in the vast majority cases. In fact mempool
deadlocks are really rare.
[...]
> > Or it would get stuck because even page allocator memory reserves got
> > depleted. Without any way to throttle there is no guarantee to make
> > further progress. In fact this is not a theoretical situation. It has
> > been observed with the swap over dm-crypt and there shouldn't be any
> > lock dependeces you are describing above there AFAIU.
> >
>
> They should do mempool_alloc(__GFP_NOMEMALLOC), no argument.
How that would be any different from any other mempool user which can be
invoked from the swap out path - aka any other IO path?
> What is the objection to allowing __GFP_NOMEMALLOC from the caller with
> clear documentation on how to use it? It can be described to not allow
> depletion of memory reserves with the caveat that the caller must ensure
> mempool_free() cannot be blocked in lowmem situations.
Look, there are
$ git grep mempool_alloc | wc -l
304
many users of this API and we do not want to flip the default behavior
which is there for more than 10 years. So far you have been arguing
about potential deadlocks and haven't shown any particular path which
would have a direct or indirect dependency between mempool and normal
allocator and it wouldn't be a bug. As the matter of fact the change
we are discussing here causes a regression. If you want to change the
semantic of mempool allocator then you are absolutely free to do so. In
a separate patch which would be discussed with IO people and other
users, though. But we _absolutely_ want to fix the regression first
and have a simple fix for 4.6 and 4.7 backports. At this moment there
are revert and patch 1 on the table. The later one should make your
backtrace happy and should be only as a temporal fix until we find out
what is actually misbehaving on your systems. If you are not interested
to pursue that way I will simply go with the revert.
--
Michal Hocko
SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | Johannes Weiner <hannes@cmpxchg.org> |
|---|---|
| Date | 2016-07-21 14:20 +0200 |
| Subject | Re: [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path |
| Message-ID | <rXl7j-7jq-5@gated-at.bofh.it> |
| In reply to | #1447721 |
On Thu, Jul 21, 2016 at 10:52:03AM +0200, Michal Hocko wrote: > Look, there are > $ git grep mempool_alloc | wc -l > 304 > > many users of this API and we do not want to flip the default behavior > which is there for more than 10 years. So far you have been arguing > about potential deadlocks and haven't shown any particular path which > would have a direct or indirect dependency between mempool and normal > allocator and it wouldn't be a bug. As the matter of fact the change > we are discussing here causes a regression. If you want to change the > semantic of mempool allocator then you are absolutely free to do so. In > a separate patch which would be discussed with IO people and other > users, though. But we _absolutely_ want to fix the regression first > and have a simple fix for 4.6 and 4.7 backports. At this moment there > are revert and patch 1 on the table. The later one should make your > backtrace happy and should be only as a temporal fix until we find out > what is actually misbehaving on your systems. If you are not interested > to pursue that way I will simply go with the revert. +1 It's very unlikely that decade-old mempool semantics are suddenly a fundamental livelock problem, when all the evidence we have is one hang and vague speculation. Given that the patch causes regressions, and that the bug is most likely elsewhere anyway, a full revert rather than merely-less-invasive mempool changes makes the most sense to me.
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web