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

Groups > linux.kernel > #1445303 > unrolled thread

[RFC PATCH 0/2] mempool vs. page allocator interaction

Started byMichal Hocko <mhocko@kernel.org>
First post2016-07-18 10:40 +0200
Last post2016-07-20 08:50 +0200
Articles 20 on this page of 46 — 8 participants

Back to article view | Back to linux.kernel

Contents

  [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 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE  tasks Michal Hocko <mhocko@kernel.org> - 2016-07-26 09:10 +0200
                Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks NeilBrown <neilb@suse.com> - 2016-07-27 05:50 +0200
                  Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-07-27 20:30 +0200
                    Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks NeilBrown <neilb@suse.com> - 2016-07-27 23:40 +0200
                      Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-07-28 09:20 +0200
                        Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-08-03 15:00 +0200
                          Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-08-03 17:50 +0200
                            Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-08-04 20:50 +0200
              Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE  tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-07-26 00:00 +0200
                Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE  tasks Michal Hocko <mhocko@kernel.org> - 2016-07-26 09:30 +0200
                Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks NeilBrown <neilb@suse.com> - 2016-07-27 06:10 +0200
                  Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-07-27 16:30 +0200
                    Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-07-27 20:50 +0200
                      Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-08-03 16:30 +0200
                        Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Michal Hocko <mhocko@kernel.org> - 2016-08-03 16:50 +0200
                          Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle  PF_LESS_THROTTLE tasks Mikulas Patocka <mpatocka@redhat.com> - 2016-08-04 20:50 +0200
                    Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks NeilBrown <neilb@suse.com> - 2016-07-27 23:40 +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 3  [1] 2 3  Next page →

#1445303 — [RFC PATCH 0/2] mempool vs. page allocator interaction

FromMichal Hocko <mhocko@kernel.org>
Date2016-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]

#1445305 — [RFC PATCH 1/2] mempool: do not consume memory reserves from the reclaim path

FromMichal Hocko <mhocko@kernel.org>
Date2016-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]

#1445310 — [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMichal Hocko <mhocko@kernel.org>
Date2016-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]

#1446743 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMikulas Patocka <mpatocka@redhat.com>
Date2016-07-20 00:00 +0200
SubjectRe: [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]

#1448471 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromNeilBrown <neilb@suse.de>
Date2016-07-22 10:50 +0200
SubjectRe: [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]

#1448486 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromNeilBrown <neilb@suse.com>
Date2016-07-22 11:10 +0200
SubjectRe: [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]

#1448492 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMichal Hocko <mhocko@kernel.org>
Date2016-07-22 11:20 +0200
SubjectRe: [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]

#1448848 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromNeilBrown <neilb@suse.com>
Date2016-07-23 02:20 +0200
SubjectRe: [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]

#1449367 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMichal Hocko <mhocko@kernel.org>
Date2016-07-25 10:40 +0200
SubjectRe: [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]

#1449767 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMichal Hocko <mhocko@kernel.org>
Date2016-07-25 21:30 +0200
SubjectRe: [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]

#1450429 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMichal Hocko <mhocko@kernel.org>
Date2016-07-26 09:10 +0200
SubjectRe: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<rZ4F4-7pO-25@gated-at.bofh.it>
In reply to#1449767
On Mon 25-07-16 21:23:44, Michal Hocko wrote:
> [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:
[...]
> 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.

And I forgot another throttling point. should_reclaim_retry which is the
main logic to decide whether we go OOM or not has a congestion_wait if
there are too many dirty/writeback pages. That should give the IO
subsystem some time to finish the IO.

> 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

-- 
Michal Hocko
SUSE Labs

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

#1451026 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromNeilBrown <neilb@suse.com>
Date2016-07-27 05:50 +0200
SubjectRe: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<rZo14-2xs-7@gated-at.bofh.it>
In reply to#1449367

[Multipart message — attachments visible in raw view] — view raw

On Mon, Jul 25 2016, Michal Hocko wrote:

> On Sat 23-07-16 10:12:24, NeilBrown wrote:

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

Just need that comprehensive regression-test-suite and off we go....


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

I should probably figure out how swap really works.  I have vague ideas
which are probably missing important details...
Isn't the first step that the page gets moved into the swap-cache - and
marked dirty I guess.  Then it gets written out and the page is marked
'clean'.
Then further memory pressure might push it out of the cache, or an early
re-use would pull it back from the cache.
If so, then "dirtying in reclaim context" could also be described as
"moving into the swap cache" - yes?  So should there be a limit on dirty
pages in the swap cache just like there is for dirty pages in any
filesystem (the max_dirty_ratio thing) ??
Maybe there is?

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

Certainly shouldn't hurt from the perspective of nfsd.

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

Really?  We have 26.

pagemap has a cute hack to store both GFP flags and other flag bits in
the one 32 it number per address_space.  'struct address_space' could
afford an extra 32 number I think.

radix_tree_root adds 3 'tag' flags to the gfp_mask.
There is 16bits of free space in radix_tree_node (between 'offset' and
'count').  That space on the root node could store a record of which tags
are set anywhere.  Or would that extra memory de-ref be a killer?

I think we'd end up with cleaner code if we removed the cute-hacks.  And
we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
need all those 26).

Thanks,
NeilBrown

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

#1451455 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMichal Hocko <mhocko@kernel.org>
Date2016-07-27 20:30 +0200
SubjectRe: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<rZBKF-2Zd-13@gated-at.bofh.it>
In reply to#1451026
On Wed 27-07-16 13:43:35, NeilBrown wrote:
> On Mon, Jul 25 2016, Michal Hocko wrote:
> 
> > On Sat 23-07-16 10:12:24, NeilBrown wrote:
[...]
> >> > 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.
> 
> I should probably figure out how swap really works.  I have vague ideas
> which are probably missing important details...
> Isn't the first step that the page gets moved into the swap-cache - and
> marked dirty I guess.  Then it gets written out and the page is marked
> 'clean'.
> Then further memory pressure might push it out of the cache, or an early
> re-use would pull it back from the cache.
> If so, then "dirtying in reclaim context" could also be described as
> "moving into the swap cache" - yes?

Yes that is basically correct

> So should there be a limit on dirty
> pages in the swap cache just like there is for dirty pages in any
> filesystem (the max_dirty_ratio thing) ??
> Maybe there is?

There is no limit AFAIK. We are relying that the reclaim is throttled
when necessary.
 
> >> 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?
> 
> Certainly shouldn't hurt from the perspective of nfsd.
> 
> >> >> 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.
> 
> Really?  We have 26.
> 
> pagemap has a cute hack to store both GFP flags and other flag bits in
> the one 32 it number per address_space.  'struct address_space' could
> afford an extra 32 number I think.
> 
> radix_tree_root adds 3 'tag' flags to the gfp_mask.
> There is 16bits of free space in radix_tree_node (between 'offset' and
> 'count').  That space on the root node could store a record of which tags
> are set anywhere.  Or would that extra memory de-ref be a killer?

Yes these are reasons why adding new gfp flags is more complicated.

> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> need all those 26).

Well, maybe we are able to remove those hacks, I wouldn't definitely
be opposed.  But right now I am not even convinced that the mempool
specific gfp flags is the right way to go.

-- 
Michal Hocko
SUSE Labs

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

#1451527 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromNeilBrown <neilb@suse.com>
Date2016-07-27 23:40 +0200
SubjectRe: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<rZEIy-4S2-1@gated-at.bofh.it>
In reply to#1451455

[Multipart message — attachments visible in raw view] — view raw

On Thu, Jul 28 2016, Michal Hocko wrote:

> On Wed 27-07-16 13:43:35, NeilBrown wrote:
>> On Mon, Jul 25 2016, Michal Hocko wrote:
>> 
>> > On Sat 23-07-16 10:12:24, NeilBrown wrote:
> [...]
>> So should there be a limit on dirty
>> pages in the swap cache just like there is for dirty pages in any
>> filesystem (the max_dirty_ratio thing) ??
>> Maybe there is?
>
> There is no limit AFAIK. We are relying that the reclaim is throttled
> when necessary.

Is that a bit indirect?  It is hard to tell without a clear big-picture.
Something to keep in mind anyway.

>
>> I think we'd end up with cleaner code if we removed the cute-hacks.  And
>> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
>> need all those 26).
>
> Well, maybe we are able to remove those hacks, I wouldn't definitely
> be opposed.  But right now I am not even convinced that the mempool
> specific gfp flags is the right way to go.

I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
transient-allocation gfp flag, which would be quite useful for mempool.

Can you give more details on why using a gfp flag isn't your first choice
for guiding what happens when the system is trying to get a free page
:-?

Thanks,
NeilBrown

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

#1451757 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMichal Hocko <mhocko@kernel.org>
Date2016-07-28 09:20 +0200
SubjectRe: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<rZNLP-2UX-1@gated-at.bofh.it>
In reply to#1451527
On Thu 28-07-16 07:33:19, NeilBrown wrote:
> On Thu, Jul 28 2016, Michal Hocko wrote:
> 
> > On Wed 27-07-16 13:43:35, NeilBrown wrote:
> >> On Mon, Jul 25 2016, Michal Hocko wrote:
> >> 
> >> > On Sat 23-07-16 10:12:24, NeilBrown wrote:
> > [...]
> >> So should there be a limit on dirty
> >> pages in the swap cache just like there is for dirty pages in any
> >> filesystem (the max_dirty_ratio thing) ??
> >> Maybe there is?
> >
> > There is no limit AFAIK. We are relying that the reclaim is throttled
> > when necessary.
> 
> Is that a bit indirect?

Yes it is. Dunno, how much of a problem is that, though.

> It is hard to tell without a clear big-picture.
> Something to keep in mind anyway.
> 
> >
> >> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> >> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> >> need all those 26).
> >
> > Well, maybe we are able to remove those hacks, I wouldn't definitely
> > be opposed.  But right now I am not even convinced that the mempool
> > specific gfp flags is the right way to go.
> 
> I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
> transient-allocation gfp flag, which would be quite useful for mempool.
> 
> Can you give more details on why using a gfp flag isn't your first choice
> for guiding what happens when the system is trying to get a free page
> :-?

If we get rid of throttle_vm_writeout then I guess it might turn out to
be unnecessary. There are other places which will still throttle but I
believe those should be kept regardless of who is doing the allocation
because they are helping the LRU scanning sane. I might be wrong here
and bailing out from the reclaim rather than waiting would turn out
better for some users but I would like to see whether the first approach
works reasonably well.
-- 
Michal Hocko
SUSE Labs

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

#1455786 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMikulas Patocka <mpatocka@redhat.com>
Date2016-08-03 15:00 +0200
SubjectRe: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<s23W9-2D9-7@gated-at.bofh.it>
In reply to#1451757

On Thu, 28 Jul 2016, Michal Hocko wrote:

> > >> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> > >> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> > >> need all those 26).
> > >
> > > Well, maybe we are able to remove those hacks, I wouldn't definitely
> > > be opposed.  But right now I am not even convinced that the mempool
> > > specific gfp flags is the right way to go.
> > 
> > I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
> > transient-allocation gfp flag, which would be quite useful for mempool.
> > 
> > Can you give more details on why using a gfp flag isn't your first choice
> > for guiding what happens when the system is trying to get a free page
> > :-?
> 
> If we get rid of throttle_vm_writeout then I guess it might turn out to
> be unnecessary. There are other places which will still throttle but I
> believe those should be kept regardless of who is doing the allocation
> because they are helping the LRU scanning sane. I might be wrong here
> and bailing out from the reclaim rather than waiting would turn out
> better for some users but I would like to see whether the first approach
> works reasonably well.

If we are swapping to a dm-crypt device, the dm-crypt device is congested 
and the underlying block device is not congested, we should not throttle 
mempool allocations made from the dm-crypt workqueue. Not even a little 
bit.

So, I think, mempool_alloc should set PF_NO_THROTTLE (or 
__GFP_NO_THROTTLE).

Mikulas

> -- 
> Michal Hocko
> SUSE Labs
>

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

#1455868 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMichal Hocko <mhocko@kernel.org>
Date2016-08-03 17:50 +0200
SubjectRe: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<s26AF-4lT-3@gated-at.bofh.it>
In reply to#1455786
On Wed 03-08-16 08:53:25, Mikulas Patocka wrote:
> 
> 
> On Thu, 28 Jul 2016, Michal Hocko wrote:
> 
> > > >> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> > > >> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> > > >> need all those 26).
> > > >
> > > > Well, maybe we are able to remove those hacks, I wouldn't definitely
> > > > be opposed.  But right now I am not even convinced that the mempool
> > > > specific gfp flags is the right way to go.
> > > 
> > > I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
> > > transient-allocation gfp flag, which would be quite useful for mempool.
> > > 
> > > Can you give more details on why using a gfp flag isn't your first choice
> > > for guiding what happens when the system is trying to get a free page
> > > :-?
> > 
> > If we get rid of throttle_vm_writeout then I guess it might turn out to
> > be unnecessary. There are other places which will still throttle but I
> > believe those should be kept regardless of who is doing the allocation
> > because they are helping the LRU scanning sane. I might be wrong here
> > and bailing out from the reclaim rather than waiting would turn out
> > better for some users but I would like to see whether the first approach
> > works reasonably well.
> 
> If we are swapping to a dm-crypt device, the dm-crypt device is congested 
> and the underlying block device is not congested, we should not throttle 
> mempool allocations made from the dm-crypt workqueue. Not even a little 
> bit.

But the device congestion is not the only condition required for the
throttling. The pgdat has also be marked congested which means that the
LRU page scanner bumped into dirty/writeback/pg_reclaim pages at the
tail of the LRU. That should only happen if we are rotating LRUs too
quickly. AFAIU the reclaim shouldn't allow free ticket scanning in that
situation.

> So, I think, mempool_alloc should set PF_NO_THROTTLE (or 
> __GFP_NO_THROTTLE).

As I've said earlier that would probably require to bail out from the
reclaim if we detect a potential pgdat congestion. What do you think
Mel?
-- 
Michal Hocko
SUSE Labs

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

#1456627 — Re: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMikulas Patocka <mpatocka@redhat.com>
Date2016-08-04 20:50 +0200
SubjectRe: [dm-devel] [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<s2vSp-4OE-1@gated-at.bofh.it>
In reply to#1455868

On Wed, 3 Aug 2016, Michal Hocko wrote:

> On Wed 03-08-16 08:53:25, Mikulas Patocka wrote:
> > 
> > 
> > On Thu, 28 Jul 2016, Michal Hocko wrote:
> > 
> > > > >> I think we'd end up with cleaner code if we removed the cute-hacks.  And
> > > > >> we'd be able to use 6 more GFP flags!!  (though I do wonder if we really
> > > > >> need all those 26).
> > > > >
> > > > > Well, maybe we are able to remove those hacks, I wouldn't definitely
> > > > > be opposed.  But right now I am not even convinced that the mempool
> > > > > specific gfp flags is the right way to go.
> > > > 
> > > > I'm not suggesting a mempool-specific gfp flag.  I'm suggesting a
> > > > transient-allocation gfp flag, which would be quite useful for mempool.
> > > > 
> > > > Can you give more details on why using a gfp flag isn't your first choice
> > > > for guiding what happens when the system is trying to get a free page
> > > > :-?
> > > 
> > > If we get rid of throttle_vm_writeout then I guess it might turn out to
> > > be unnecessary. There are other places which will still throttle but I
> > > believe those should be kept regardless of who is doing the allocation
> > > because they are helping the LRU scanning sane. I might be wrong here
> > > and bailing out from the reclaim rather than waiting would turn out
> > > better for some users but I would like to see whether the first approach
> > > works reasonably well.
> > 
> > If we are swapping to a dm-crypt device, the dm-crypt device is congested 
> > and the underlying block device is not congested, we should not throttle 
> > mempool allocations made from the dm-crypt workqueue. Not even a little 
> > bit.
> 
> But the device congestion is not the only condition required for the
> throttling. The pgdat has also be marked congested which means that the
> LRU page scanner bumped into dirty/writeback/pg_reclaim pages at the
> tail of the LRU. That should only happen if we are rotating LRUs too
> quickly. AFAIU the reclaim shouldn't allow free ticket scanning in that
> situation.

The obvious problem here is that mempool allocations should sleep in 
mempool_alloc() on &pool->wait (until someone returns some entries into 
the mempool), they should not sleep inside the page allocator.

Mikulas

> > So, I think, mempool_alloc should set PF_NO_THROTTLE (or 
> > __GFP_NO_THROTTLE).
> 
> As I've said earlier that would probably require to bail out from the
> reclaim if we detect a potential pgdat congestion. What do you think
> Mel?
> -- 
> Michal Hocko
> SUSE Labs
>

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

#1450036 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMikulas Patocka <mpatocka@redhat.com>
Date2016-07-26 00:00 +0200
SubjectRe: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<rYW4N-1xV-21@gated-at.bofh.it>
In reply to#1448848

On Sat, 23 Jul 2016, NeilBrown wrote:

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

Device mapper doesn't impose any limit for in-flight bios.

Some simple device mapper targets (such as linear or stripe) pass bio 
directly to the underlying device with generic_make_request, so if the 
underlying device's request limit is reached, the target's request routine 
waits.

However, complex dm targets (such as dm-crypt, dm-mirror, dm-thin) pass 
bios to a workqueue that processes them. And since there is no limit on 
the number of workqueue entries, there is no limit on the number of 
in-flight bios.

I've seen a case when I had a HPFS filesystem on dm-crypt. I wrote to the 
filesystem, there was about 2GB dirty data. The HPFS filesystem used 
512-byte bios. dm-crypt allocates one temporary page for each incoming 
bio. So, there were 4M bios in flight, each bio allocated 4k temporary 
page - that is attempted 16GB allocation. It didn't trigger OOM condition 
(because mempool allocations don't ever trigger it), but it temporarily 
exhausted all computer's memory.

I've made some patches that limit in-flight bios for device mapper in the 
past, but there were not integrated into upstream.

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

No, it won't be throttled.

dm-crypt does:
1. pass the bio to the encryption workqueue
2. allocate the outgoing bio and allocate temporary pages for the 
   encrypted data
3. do the encryption
4. pass the bio to the writer thread
5. submit the write request with generic_make_request

So, if the underlying block device is throttled, it stalls the writer 
thread, but it doesn't stall the encryption threads and it doesn't stall 
the caller that submits the bios to dm-crypt.

There can be really high number of in-flight bios for dm-crypt.

Mikulas

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

#1450436 — Re: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks

FromMichal Hocko <mhocko@kernel.org>
Date2016-07-26 09:30 +0200
SubjectRe: [RFC PATCH 2/2] mm, mempool: do not throttle PF_LESS_THROTTLE tasks
Message-ID<rZ4Yp-7w9-7@gated-at.bofh.it>
In reply to#1450036
On Mon 25-07-16 17:52:17, Mikulas Patocka wrote:
> 
> 
> On Sat, 23 Jul 2016, NeilBrown wrote:
> 
> > "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.
> 
> Device mapper doesn't impose any limit for in-flight bios.
> 
> Some simple device mapper targets (such as linear or stripe) pass bio 
> directly to the underlying device with generic_make_request, so if the 
> underlying device's request limit is reached, the target's request routine 
> waits.
> 
> However, complex dm targets (such as dm-crypt, dm-mirror, dm-thin) pass 
> bios to a workqueue that processes them. And since there is no limit on 
> the number of workqueue entries, there is no limit on the number of 
> in-flight bios.
> 
> I've seen a case when I had a HPFS filesystem on dm-crypt. I wrote to the 
> filesystem, there was about 2GB dirty data. The HPFS filesystem used 
> 512-byte bios. dm-crypt allocates one temporary page for each incoming 
> bio. So, there were 4M bios in flight, each bio allocated 4k temporary 
> page - that is attempted 16GB allocation. It didn't trigger OOM condition 
> (because mempool allocations don't ever trigger it), but it temporarily 
> exhausted all computer's memory.

OK, that is certainly not good and something that throttle_vm_writeout
aimed at protecting from. It is a little bit poor protection because
it might fire much more earlier than necessary. Shouldn't those workers
simply backoff when the underlying bdi is congested? It wouldn't help
to queue more IO when the bdi is hammered already.
 
> I've made some patches that limit in-flight bios for device mapper in the 
> past, but there were not integrated into upstream.

Care to revive them? I am not an expert in dm but unbounded amount of
inflight IO doesn't really sound good.

[...]
-- 
Michal Hocko
SUSE Labs

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web