Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1711598 > unrolled thread
| Started by | Tim Chen <tim.c.chen@linux.intel.com> |
|---|---|
| First post | 2017-08-15 03:20 +0200 |
| Last post | 2017-08-18 15:10 +0200 |
| Articles | 20 on this page of 63 — 9 participants |
Back to article view | Back to linux.kernel
[PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-15 03:20 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 03:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-15 04:30 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 05:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-15 05:20 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 05:30 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-15 21:10 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 21:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-15 21:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Davidlohr Bueso <dave@stgolabs.net> - 2017-08-16 00:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 01:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 02:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk ebiederm@xmission.com (Eric W. Biederman) - 2017-08-17 01:30 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-16 01:00 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-17 18:20 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 18:30 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-17 22:20 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-17 22:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-18 14:30 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-18 16:30 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-18 16:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-18 18:40 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-18 18:50 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-18 19:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 19:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-18 21:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 21:20 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-18 22:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 22:20 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-21 20:40 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-21 21:00 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-22 19:30 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 20:20 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 20:30 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Peter Zijlstra <peterz@infradead.org> - 2017-08-22 21:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 21:20 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Peter Zijlstra <peterz@infradead.org> - 2017-08-22 21:10 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-22 21:40 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Christopher Lameter <cl@linux.com> - 2017-08-22 23:10 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-22 23:30 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-23 01:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-23 01:20 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-23 17:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 21:40 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-22 22:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 22:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-22 23:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Peter Zijlstra <peterz@infradead.org> - 2017-08-22 23:00 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-23 16:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-23 18:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-23 20:20 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-23 23:00 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 01:40 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Tim Chen <tim.c.chen@linux.intel.com> - 2017-08-24 19:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-24 20:20 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-24 22:50 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Mel Gorman <mgorman@techsingularity.net> - 2017-08-23 18:10 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Andi Kleen <ak@linux.intel.com> - 2017-08-18 22:10 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 22:40 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-18 22:30 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 22:40 +0200
Re: [PATCH 1/2] sched/wait: Break up long wake list walk Linus Torvalds <torvalds@linux-foundation.org> - 2017-08-18 19:00 +0200
RE: [PATCH 1/2] sched/wait: Break up long wake list walk "Liang, Kan" <kan.liang@intel.com> - 2017-08-18 15:10 +0200
Page 1 of 4 [1] 2 3 4 Next page →
| From | Tim Chen <tim.c.chen@linux.intel.com> |
|---|---|
| Date | 2017-08-15 03:20 +0200 |
| Subject | [PATCH 1/2] sched/wait: Break up long wake list walk |
| Message-ID | <ueyGu-35K-5@gated-at.bofh.it> |
We encountered workloads that have very long wake up list on large
systems. A waker takes a long time to traverse the entire wake list and
execute all the wake functions.
We saw page wait list that are up to 3700+ entries long in tests of large
4 and 8 socket systems. It took 0.8 sec to traverse such list during
wake up. Any other CPU that contends for the list spin lock will spin
for a long time. As page wait list is shared by many pages so it could
get very long on systems with large memory.
Multiple CPUs waking are queued up behind the lock, and the last one queued
has to wait until all CPUs did all the wakeups.
The page wait list is traversed with interrupt disabled, which caused
various problems. This was the original cause that triggered the NMI
watch dog timer in: https://patchwork.kernel.org/patch/9800303/ . Only
extending the NMI watch dog timer there helped.
This patch bookmarks the waker's scan position in wake list and break
the wake up walk, to allow access to the list before the waker resume
its walk down the rest of the wait list. It lowers the interrupt and
rescheduling latency.
This patch also provides a performance boost when combined with the next
patch to break up page wakeup list walk. We saw 22% improvement in the
will-it-scale file pread2 test on a Xeon Phi system running 256 threads.
Thanks.
Tim
Reported-by: Kan Liang <kan.liang@intel.com>
Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
include/linux/wait.h | 9 +++++++
kernel/sched/wait.c | 76 ++++++++++++++++++++++++++++++++++++++++++----------
2 files changed, 71 insertions(+), 14 deletions(-)
diff --git a/include/linux/wait.h b/include/linux/wait.h
index 5b74e36..588a5d2 100644
--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -18,6 +18,14 @@ int default_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, int
/* wait_queue_entry::flags */
#define WQ_FLAG_EXCLUSIVE 0x01
#define WQ_FLAG_WOKEN 0x02
+#define WQ_FLAG_BOOKMARK 0x04
+
+/*
+ * Scan threshold to break wait queue walk.
+ * This allows a waker to take a break from holding the
+ * wait queue lock during the wait queue walk.
+ */
+#define WAITQUEUE_WALK_BREAK_CNT 64
/*
* A single wait-queue entry structure:
@@ -947,6 +955,7 @@ void finish_wait(struct wait_queue_head *wq_head, struct wait_queue_entry *wq_en
long wait_woken(struct wait_queue_entry *wq_entry, unsigned mode, long timeout);
int woken_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, int sync, void *key);
int autoremove_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, int sync, void *key);
+int bookmark_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, int sync, void *key);
#define DEFINE_WAIT_FUNC(name, function) \
struct wait_queue_entry name = { \
diff --git a/kernel/sched/wait.c b/kernel/sched/wait.c
index 17f11c6..d02e6c6 100644
--- a/kernel/sched/wait.c
+++ b/kernel/sched/wait.c
@@ -63,17 +63,64 @@ EXPORT_SYMBOL(remove_wait_queue);
* started to run but is not in state TASK_RUNNING. try_to_wake_up() returns
* zero in this (rare) case, and we handle it by continuing to scan the queue.
*/
-static void __wake_up_common(struct wait_queue_head *wq_head, unsigned int mode,
- int nr_exclusive, int wake_flags, void *key)
+static int __wake_up_common(struct wait_queue_head *wq_head, unsigned int mode,
+ int nr_exclusive, int wake_flags, void *key,
+ wait_queue_entry_t *bookmark)
{
wait_queue_entry_t *curr, *next;
+ int cnt = 0;
+
+ if (bookmark && (bookmark->flags & WQ_FLAG_BOOKMARK)) {
+ curr = list_next_entry(bookmark, entry);
+
+ list_del(&bookmark->entry);
+ bookmark->flags = 0;
+ } else
+ curr = list_first_entry(&wq_head->head, wait_queue_entry_t, entry);
+
+ if (&curr->entry == &wq_head->head)
+ return nr_exclusive;
- list_for_each_entry_safe(curr, next, &wq_head->head, entry) {
+ list_for_each_entry_safe_from(curr, next, &wq_head->head, entry) {
unsigned flags = curr->flags;
+ if (curr->flags & WQ_FLAG_BOOKMARK)
+ continue;
+
if (curr->func(curr, mode, wake_flags, key) &&
(flags & WQ_FLAG_EXCLUSIVE) && !--nr_exclusive)
break;
+
+ if (bookmark && (++cnt > WAITQUEUE_WALK_BREAK_CNT) &&
+ (&next->entry != &wq_head->head)) {
+ bookmark->flags = WQ_FLAG_BOOKMARK;
+ list_add_tail(&bookmark->entry, &next->entry);
+ break;
+ }
+ }
+ return nr_exclusive;
+}
+
+static void __wake_up_common_lock(struct wait_queue_head *wq_head, unsigned int mode,
+ int nr_exclusive, int wake_flags, void *key)
+{
+ unsigned long flags;
+ wait_queue_entry_t bookmark;
+
+ bookmark.flags = 0;
+ bookmark.private = NULL;
+ bookmark.func = bookmark_wake_function;
+ INIT_LIST_HEAD(&bookmark.entry);
+
+ spin_lock_irqsave(&wq_head->lock, flags);
+ nr_exclusive = __wake_up_common(wq_head, mode, nr_exclusive, wake_flags, key, &bookmark);
+ spin_unlock_irqrestore(&wq_head->lock, flags);
+
+ while (bookmark.flags & WQ_FLAG_BOOKMARK) {
+ spin_lock_irqsave(&wq_head->lock, flags);
+ nr_exclusive = __wake_up_common(wq_head, mode, nr_exclusive,
+ wake_flags, key, &bookmark);
+ spin_unlock_irqrestore(&wq_head->lock, flags);
}
}
@@ -90,11 +137,7 @@ static void __wake_up_common(struct wait_queue_head *wq_head, unsigned int mode,
void __wake_up(struct wait_queue_head *wq_head, unsigned int mode,
int nr_exclusive, void *key)
{
- unsigned long flags;
-
- spin_lock_irqsave(&wq_head->lock, flags);
- __wake_up_common(wq_head, mode, nr_exclusive, 0, key);
- spin_unlock_irqrestore(&wq_head->lock, flags);
+ __wake_up_common_lock(wq_head, mode, nr_exclusive, 0, key);
}
EXPORT_SYMBOL(__wake_up);
@@ -103,13 +146,13 @@ EXPORT_SYMBOL(__wake_up);
*/
void __wake_up_locked(struct wait_queue_head *wq_head, unsigned int mode, int nr)
{
- __wake_up_common(wq_head, mode, nr, 0, NULL);
+ __wake_up_common(wq_head, mode, nr, 0, NULL, NULL);
}
EXPORT_SYMBOL_GPL(__wake_up_locked);
void __wake_up_locked_key(struct wait_queue_head *wq_head, unsigned int mode, void *key)
{
- __wake_up_common(wq_head, mode, 1, 0, key);
+ __wake_up_common(wq_head, mode, 1, 0, key, NULL);
}
EXPORT_SYMBOL_GPL(__wake_up_locked_key);
@@ -133,7 +176,6 @@ EXPORT_SYMBOL_GPL(__wake_up_locked_key);
void __wake_up_sync_key(struct wait_queue_head *wq_head, unsigned int mode,
int nr_exclusive, void *key)
{
- unsigned long flags;
int wake_flags = 1; /* XXX WF_SYNC */
if (unlikely(!wq_head))
@@ -142,9 +184,7 @@ void __wake_up_sync_key(struct wait_queue_head *wq_head, unsigned int mode,
if (unlikely(nr_exclusive != 1))
wake_flags = 0;
- spin_lock_irqsave(&wq_head->lock, flags);
- __wake_up_common(wq_head, mode, nr_exclusive, wake_flags, key);
- spin_unlock_irqrestore(&wq_head->lock, flags);
+ __wake_up_common_lock(wq_head, mode, nr_exclusive, wake_flags, key);
}
EXPORT_SYMBOL_GPL(__wake_up_sync_key);
@@ -326,6 +366,14 @@ int autoremove_wake_function(struct wait_queue_entry *wq_entry, unsigned mode, i
}
EXPORT_SYMBOL(autoremove_wake_function);
+int bookmark_wake_function(wait_queue_entry_t *wait, unsigned mode, int sync, void *key)
+{
+ /* bookmark only, no real wake up */
+ BUG();
+ return 0;
+}
+EXPORT_SYMBOL(bookmark_wake_function);
+
static inline bool is_kthread_should_stop(void)
{
return (current->flags & PF_KTHREAD) && kthread_should_stop();
--
2.9.4
[toc] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-15 03:50 +0200 |
| Message-ID | <uez9v-3gj-7@gated-at.bofh.it> |
| In reply to | #1711598 |
On Mon, Aug 14, 2017 at 5:52 PM, Tim Chen <tim.c.chen@linux.intel.com> wrote:
> We encountered workloads that have very long wake up list on large
> systems. A waker takes a long time to traverse the entire wake list and
> execute all the wake functions.
>
> We saw page wait list that are up to 3700+ entries long in tests of large
> 4 and 8 socket systems. It took 0.8 sec to traverse such list during
> wake up. Any other CPU that contends for the list spin lock will spin
> for a long time. As page wait list is shared by many pages so it could
> get very long on systems with large memory.
I really dislike this patch.
The patch seems a band-aid for really horrible kernel behavior, rather
than fixing the underlying problem itself.
Now, it may well be that we do end up needing this band-aid in the
end, so this isn't a NAK of the patch per se. But I'd *really* like to
see if we can fix the underlying cause for what you see somehow..
In particular, if this is about the page wait table, maybe we can just
make the wait table bigger. IOW, are people actually waiting on the
*same* page, or are they mainly waiting on totally different pages,
just hashing to the same wait queue?
Because right now that page wait table is a small fixed size, and the
only reason it's a small fixed size is that nobody reported any issues
with it - particularly since we now avoid the wait table entirely for
the common cases by having that "contention" bit.
But it really is a *small* table. We literally have
#define PAGE_WAIT_TABLE_BITS 8
so it's just 256 entries. We could easily it much bigger, if we are
actually seeing a lot of collissions.
We *used* to have a very complex per-zone thing for bit-waitiqueues,
but that was because we got lots and lots of contention issues, and
everybody *always* touched the wait-queues whether they waited or not
(so being per-zone was a big deal)
We got rid of all that per-zone complexity when the normal case didn't
hit in the page wait queues at all, but we may have over-done the
simplification a bit since nobody showed any issue.
In particular, we used to size the per-zone thing by amount of memory.
We could easily re-introduce that for the new simpler page queues.
The page_waitiqueue() is a simple helper function inside mm/filemap.c,
and thanks to the per-page "do we have actual waiters" bit that we
have now, we can actually afford to make it bigger and more complex
now if we want to.
What happens to your load if you just make that table bigger? You can
literally test by just changing the constant from 8 to 16 or
something, making us use twice as many bits for hashing. A "real"
patch would size it by amount of memory, but just for testing the
contention on your load, you can do the hacky one-liner.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Andi Kleen <ak@linux.intel.com> |
|---|---|
| Date | 2017-08-15 04:30 +0200 |
| Message-ID | <uezMd-3MW-5@gated-at.bofh.it> |
| In reply to | #1711688 |
On Mon, Aug 14, 2017 at 06:48:06PM -0700, Linus Torvalds wrote: > On Mon, Aug 14, 2017 at 5:52 PM, Tim Chen <tim.c.chen@linux.intel.com> wrote: > > We encountered workloads that have very long wake up list on large > > systems. A waker takes a long time to traverse the entire wake list and > > execute all the wake functions. > > > > We saw page wait list that are up to 3700+ entries long in tests of large > > 4 and 8 socket systems. It took 0.8 sec to traverse such list during > > wake up. Any other CPU that contends for the list spin lock will spin > > for a long time. As page wait list is shared by many pages so it could > > get very long on systems with large memory. > > I really dislike this patch. > > The patch seems a band-aid for really horrible kernel behavior, rather > than fixing the underlying problem itself. > > Now, it may well be that we do end up needing this band-aid in the > end, so this isn't a NAK of the patch per se. But I'd *really* like to > see if we can fix the underlying cause for what you see somehow.. We could try it and it may even help in this case and it may be a good idea in any case on such a system, but: - Even with a large hash table it might be that by chance all CPUs will be queued up on the same page - There are a lot of other wait queues in the kernel and they all could run into a similar problem - I suspect it's even possible to construct it from user space as a kind of DoS attack Given all that I don't see any alternative to fixing wait queues somehow. It's just that systems are so big that now that they're starting to stretch the tried old primitives. Now in one case (on a smaller system) we debugged we had - 4S system with 208 logical threads - during the test the wait queue length was 3700 entries. - the last CPUs queued had to wait roughly 0.8s This gives a budget of roughly 1us per wake up. It could be that we could find some way to do "bulk wakeups" in the scheduler that are much cheaper, and switch to them if there are a lot of entries in the wait queues. With that it may be possible to do a wake up in less than 1us. But even with that it will be difficult to beat the scaling curve. If systems get bigger again (and they will be) it could easily break again, as the budget gets smaller and smaller. Also disabling interrupts for that long is just nasty. Given all that I still think a lock breaker of some form is needed. -Andi
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-15 05:00 +0200 |
| Message-ID | <ueAff-3Y4-1@gated-at.bofh.it> |
| In reply to | #1711775 |
On Mon, Aug 14, 2017 at 7:27 PM, Andi Kleen <ak@linux.intel.com> wrote:
>
> We could try it and it may even help in this case and it may
> be a good idea in any case on such a system, but:
>
> - Even with a large hash table it might be that by chance all CPUs
> will be queued up on the same page
> - There are a lot of other wait queues in the kernel and they all
> could run into a similar problem
> - I suspect it's even possible to construct it from user space
> as a kind of DoS attack
Maybe. Which is why I didn't NAK the patch outright.
But I don't think it's the solution for the scalability issue you guys
found. It's just a workaround, and it's likely a bad one at that.
> Now in one case (on a smaller system) we debugged we had
>
> - 4S system with 208 logical threads
> - during the test the wait queue length was 3700 entries.
> - the last CPUs queued had to wait roughly 0.8s
>
> This gives a budget of roughly 1us per wake up.
I'm not at all convinced that follows.
When bad scaling happens, you often end up hitting quadratic (or
worse) behavior. So if you are able to fix the scaling by some fixed
amount, it's possible that almost _all_ the problems just go away.
The real issue is that "3700 entries" part. What was it that actually
triggered them? In particular, if it's just a hashing issue, and we
can trivially just make the hash table be bigger (256 entries is
*tiny*) then the whole thing goes away.
Which is why I really want to hear what happens if you just change
PAGE_WAIT_TABLE_BITS to 16. The right fix would be to just make it
scale by memory, but before we even do that, let's just look at what
happens when you increase the size the stupid way.
Maybe those 3700 entries will just shrink down to 14 entries because
the hash just works fine and 256 entries was just much much too small
when you have hundreds of thousands of threads or whatever
But it is *also* possible that it's actually all waiting on the exact
same page, and there's some way to do a thundering herd on the page
lock bit, for example. But then it would be really good to hear what
it is that triggers that.
The thing is, the reason we perform well on many loads in the kernel
is that I have *always* pushed back against bad workarounds.
We do *not* do lock back-off in our locks, for example, because I told
people that lock contention gets fixed by not contending, not by
trying to act better when things have already become bad.
This is the same issue. We don't "fix" things by papering over some
symptom. We try to fix the _actual_ underlying problem. Maybe there is
some caller that can simply be rewritten. Maybe we can do other tricks
than just make the wait tables bigger. But we should not say "3700
entries is ok, let's just make that sh*t be interruptible".
That is what the patch does now, and that is why I dislike the patch.
So I _am_ NAK'ing the patch if nobody is willing to even try alternatives.
Because a band-aid is ok for "some theoretical worst-case behavior".
But a band-aid is *not* ok for "we can't even be bothered to try to
figure out the right thing, so we're just adding this hack and leaving
it".
Linus
[toc] | [prev] | [next] | [standalone]
| From | Andi Kleen <ak@linux.intel.com> |
|---|---|
| Date | 2017-08-15 05:20 +0200 |
| Message-ID | <ueAyC-4lc-3@gated-at.bofh.it> |
| In reply to | #1711785 |
> That is what the patch does now, and that is why I dislike the patch. > > So I _am_ NAK'ing the patch if nobody is willing to even try alternatives. Ok, perhaps larger hash table is the right solution for this one. But what should we do when some other (non page) wait queue runs into the same problem? -Andi
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-15 05:30 +0200 |
| Message-ID | <ueAIi-4oo-5@gated-at.bofh.it> |
| In reply to | #1711802 |
On Mon, Aug 14, 2017 at 8:15 PM, Andi Kleen <ak@linux.intel.com> wrote:
> But what should we do when some other (non page) wait queue runs into the
> same problem?
Hopefully the same: root-cause it.
Once you have a test-case, it should generally be fairly simple to do
with profiles, just seeing who the caller is when ttwu() (or whatever
it is that ends up being the most noticeable part of the wakeup chain)
shows up very heavily.
And I think that ends up being true whether the "break up long chains"
patch goes in or not. Even if we end up allowing interrupts in the
middle, a long wait-queue is a problem.
I think the "break up long chains" thing may be the right thing
against actual malicious attacks, but not for any actual real
benchmark or load.
I don't think we normally have cases of long wait-queues, though. At
least not the kinds that cause problems. The real (and valid)
thundering herd cases should already be using exclusive waiters that
only wake up one process at a time.
The page bit-waiting is hopefully special. As mentioned, we used to
have some _really_ special code for it for other reasons, and I
suspect you see this problem with them because we over-simplified it
from being a per-zone dynamically sized one (where the per-zone thing
caused both performance problems and actual bugs) to being that
"static small array".
So I think/hope that just re-introducing some dynamic sizing will help
sufficiently, and that this really is an odd and unusual case.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Tim Chen <tim.c.chen@linux.intel.com> |
|---|---|
| Date | 2017-08-15 21:10 +0200 |
| Message-ID | <uePnY-5ik-27@gated-at.bofh.it> |
| In reply to | #1711808 |
On 08/14/2017 08:28 PM, Linus Torvalds wrote: > On Mon, Aug 14, 2017 at 8:15 PM, Andi Kleen <ak@linux.intel.com> wrote: >> But what should we do when some other (non page) wait queue runs into the >> same problem? > > Hopefully the same: root-cause it. > > Once you have a test-case, it should generally be fairly simple to do > with profiles, just seeing who the caller is when ttwu() (or whatever > it is that ends up being the most noticeable part of the wakeup chain) > shows up very heavily. We have a test case but it is a customer workload. We'll try to get a bit more info. > > And I think that ends up being true whether the "break up long chains" > patch goes in or not. Even if we end up allowing interrupts in the > middle, a long wait-queue is a problem. > > I think the "break up long chains" thing may be the right thing > against actual malicious attacks, but not for any actual real > benchmark or load. This is a concern from our customer as we could trigger the watchdog timer by running user space workloads. > > I don't think we normally have cases of long wait-queues, though. At > least not the kinds that cause problems. The real (and valid) > thundering herd cases should already be using exclusive waiters that > only wake up one process at a time. > > The page bit-waiting is hopefully special. As mentioned, we used to > have some _really_ special code for it for other reasons, and I > suspect you see this problem with them because we over-simplified it > from being a per-zone dynamically sized one (where the per-zone thing > caused both performance problems and actual bugs) to being that > "static small array". > > So I think/hope that just re-introducing some dynamic sizing will help > sufficiently, and that this really is an odd and unusual case. I agree that dynamic sizing makes a lot of sense. We'll check to see if additional size to the hash table helps, assuming that the waiters are distributed among different pages for our test case. Thanks. Tim
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-15 21:50 +0200 |
| Message-ID | <ueQ0F-5vJ-1@gated-at.bofh.it> |
| In reply to | #1712430 |
On Tue, Aug 15, 2017 at 12:41 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So if we have unnecessarily collisions because we have waiters looking
> at different bits of the same page, we could just hash in the bit
> number that we're waiting for too.
Oh, nope, we can't do that, because we only have one "PageWaters" bit
per page, and it is shared across all bits we're waiting for on that
page.
So collisions between different bits on the same page are inevitable,
and we just need to make sure the hash table is big enough that we
don't get unnecessary collisions between different pages.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-15 21:50 +0200 |
| Message-ID | <ueQ0F-5vJ-3@gated-at.bofh.it> |
| In reply to | #1712430 |
On Tue, Aug 15, 2017 at 12:05 PM, Tim Chen <tim.c.chen@linux.intel.com> wrote:
>
> We have a test case but it is a customer workload. We'll try to get
> a bit more info.
Ok. Being a customer workload is lovely in the sense that it is
actually a real load, not just a microbecnhmark.
But yeah, it makes it harder to describe and show what's going on.
But you do have access to that workload internally at Intel, and can
at least test things out that way, I assume?
> I agree that dynamic sizing makes a lot of sense. We'll check to
> see if additional size to the hash table helps, assuming that the
> waiters are distributed among different pages for our test case.
One more thing: it turns out that there are two very different kinds
of users of the page waitqueue.
There's the "wait_on_page_bit*()" users - people waiting for a page to
unlock or stop being under writeback etc.
Those *should* generally be limited to just one wait-queue per waiting
thread, I think.
Then there is the "cachefiles" use, which ends up adding a lot of
waitqueues to a lot of paghes to monitor their state.
Honestly, I think that second use a horrible hack. It basically adds a
waitqueue to each page in order to get a callback when it is ready,
and then copies it.
And it does this for things like cachefiles_read_backing_file(), so
you might have a huge list of pages for copying a large file, and it
adds a callback for every single one of those all at once.
The fix for the cachefiles behavior might be very different from the
fix to the "normal" operations. But making the wait queue hash tables
bigger _should_ help both cases.
We might also want to hash based on the actual bit we're waiting for.
Right now we just do a
wait_queue_head_t *q = page_waitqueue(page);
but I think the actual bit is always explicit (well, the cachefiles
interface doesn't have that, but looking at the callback for that, it
really only cares about PG_locked, so it *should* make the bit it is
waiting for explicit).
So if we have unnecessarily collisions because we have waiters looking
at different bits of the same page, we could just hash in the bit
number that we're waiting for too.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Davidlohr Bueso <dave@stgolabs.net> |
|---|---|
| Date | 2017-08-16 00:50 +0200 |
| Message-ID | <ueSOR-7jm-7@gated-at.bofh.it> |
| In reply to | #1711808 |
On Mon, 14 Aug 2017, Linus Torvalds wrote: >On Mon, Aug 14, 2017 at 8:15 PM, Andi Kleen <ak@linux.intel.com> wrote: >> But what should we do when some other (non page) wait queue runs into the >> same problem? > >Hopefully the same: root-cause it. Or you can always use wake_qs; which exists _exactly_ for the issues you are running into. Note that Linus does not want them in general wait: https://lkml.org/lkml/2017/7/7/605 ... but you can always use them on your own if you really need to (ie locks, ipc, etc). Thanks, Davidlohr
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-16 01:00 +0200 |
| Message-ID | <ueSYx-7mo-13@gated-at.bofh.it> |
| In reply to | #1712506 |
On Tue, Aug 15, 2017 at 3:56 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Except they really don't actually work for this case, exactly because
> they also simplify away "minor" details like exclusive vs
> non-exclusive etc.
>
> The page wait-queue very much has a mix of "wake all" and "wake one" semantics.
Oh, and the page wait-queue really needs that key argument too, which
is another thing that swait queue code got rid of in the name of
simplicity.
So no. The swait code is absolutely _entirely_ the wrong thing to use.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-16 02:00 +0200 |
| Message-ID | <ueTUB-7Wz-1@gated-at.bofh.it> |
| In reply to | #1712511 |
On Tue, Aug 15, 2017 at 3:57 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Oh, and the page wait-queue really needs that key argument too, which
> is another thing that swait queue code got rid of in the name of
> simplicity.
Actually, it gets worse.
Because the page wait queues are hashed, it's not an all-or-nothing
thing even for the non-exclusive cases, and it's not a "wake up first
entry" for the exclusive case. Both have to be conditional on the wait
entry actually matching the page and bit in question.
So no way to use swait, or any of the lockless queuing code in general
(so we can't do some clever private wait-list using llist.h either).
End result: it looks like you fairly fundamentally do need to use a
lock over the whole list traversal (like the standard wait-queues),
and then add a cursor entry like Tim's patch if dropping the lock in
the middle.
Anyway, looking at the old code, we *used* to limit the page wait hash
table to 4k entries, and we used to have one hash table per memory
zone.
The per-zone thing didn't work at all for the generic bit-waitqueues,
because of how people used them on virtual addresses on the stack.
But it *could* work for the page waitqueues, which are now a totally
separate entity, and is obviously always physically addressed (since
the indexing is by "struct page" pointer), and doesn't have that
issue.
So I guess we could re-introduce the notion of per-zone page waitqueue
hash tables. It was disgusting to allocate and free though (and hooked
into the memory hotplug code).
So I'd still hope that we can instead just have one larger hash table,
and that is sufficient for the problem.
Linus
[toc] | [prev] | [next] | [standalone]
| From | ebiederm@xmission.com (Eric W. Biederman) |
|---|---|
| Date | 2017-08-17 01:30 +0200 |
| Message-ID | <uffV7-52i-3@gated-at.bofh.it> |
| In reply to | #1712522 |
Linus Torvalds <torvalds@linux-foundation.org> writes: > On Tue, Aug 15, 2017 at 3:57 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> Oh, and the page wait-queue really needs that key argument too, which >> is another thing that swait queue code got rid of in the name of >> simplicity. > > Actually, it gets worse. > > Because the page wait queues are hashed, it's not an all-or-nothing > thing even for the non-exclusive cases, and it's not a "wake up first > entry" for the exclusive case. Both have to be conditional on the wait > entry actually matching the page and bit in question. > > So no way to use swait, or any of the lockless queuing code in general > (so we can't do some clever private wait-list using llist.h either). > > End result: it looks like you fairly fundamentally do need to use a > lock over the whole list traversal (like the standard wait-queues), > and then add a cursor entry like Tim's patch if dropping the lock in > the middle. > > Anyway, looking at the old code, we *used* to limit the page wait hash > table to 4k entries, and we used to have one hash table per memory > zone. > > The per-zone thing didn't work at all for the generic bit-waitqueues, > because of how people used them on virtual addresses on the stack. > > But it *could* work for the page waitqueues, which are now a totally > separate entity, and is obviously always physically addressed (since > the indexing is by "struct page" pointer), and doesn't have that > issue. > > So I guess we could re-introduce the notion of per-zone page waitqueue > hash tables. It was disgusting to allocate and free though (and hooked > into the memory hotplug code). > > So I'd still hope that we can instead just have one larger hash table, > and that is sufficient for the problem. If increasing the hash table size fixes the problem I am wondering if rhash tables might be the proper solution to this problem. They start out small and then grow as needed. Eric
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-16 01:00 +0200 |
| Message-ID | <ueSYx-7mo-15@gated-at.bofh.it> |
| In reply to | #1712506 |
On Tue, Aug 15, 2017 at 3:47 PM, Davidlohr Bueso <dave@stgolabs.net> wrote:
>
> Or you can always use wake_qs; which exists _exactly_ for the issues you
> are running into
Except they really don't actually work for this case, exactly because
they also simplify away "minor" details like exclusive vs
non-exclusive etc.
The page wait-queue very much has a mix of "wake all" and "wake one" semantics.
But I guess we could have two queues per page hash - one that is
wake-once, and one that is wake-all.
Which might solve the technical problem.
And if somebody then rewrote the swait code to not use the
unbelievably broken and misleading naming, it might even be
acceptable.
But as is, that swait code is broken shit, and absolutely does *not*
need new users. We got rid of one user, and the KVM people already
admitted that one of the remaining users is broken and doesn't
actually want swait at all and should use "wake_up_process()" instead
since there is no actual queuing going on.
In the meantime, stop peddling crap. That thing really is broken.
Linus
[toc] | [prev] | [next] | [standalone]
| From | "Liang, Kan" <kan.liang@intel.com> |
|---|---|
| Date | 2017-08-17 18:20 +0200 |
| Message-ID | <ufvGy-7iQ-9@gated-at.bofh.it> |
| In reply to | #1711688 |
> On Mon, Aug 14, 2017 at 5:52 PM, Tim Chen <tim.c.chen@linux.intel.com>
> wrote:
> > We encountered workloads that have very long wake up list on large
> > systems. A waker takes a long time to traverse the entire wake list
> > and execute all the wake functions.
> >
> > We saw page wait list that are up to 3700+ entries long in tests of
> > large
> > 4 and 8 socket systems. It took 0.8 sec to traverse such list during
> > wake up. Any other CPU that contends for the list spin lock will spin
> > for a long time. As page wait list is shared by many pages so it
> > could get very long on systems with large memory.
>
> I really dislike this patch.
>
> The patch seems a band-aid for really horrible kernel behavior, rather than
> fixing the underlying problem itself.
>
> Now, it may well be that we do end up needing this band-aid in the end, so
> this isn't a NAK of the patch per se. But I'd *really* like to see if we can fix the
> underlying cause for what you see somehow..
>
> In particular, if this is about the page wait table, maybe we can just make the
> wait table bigger. IOW, are people actually waiting on the
> *same* page, or are they mainly waiting on totally different pages, just
> hashing to the same wait queue?
>
> Because right now that page wait table is a small fixed size, and the only
> reason it's a small fixed size is that nobody reported any issues with it -
> particularly since we now avoid the wait table entirely for the common cases
> by having that "contention" bit.
>
> But it really is a *small* table. We literally have
>
> #define PAGE_WAIT_TABLE_BITS 8
>
> so it's just 256 entries. We could easily it much bigger, if we are actually
> seeing a lot of collissions.
>
> We *used* to have a very complex per-zone thing for bit-waitiqueues, but
> that was because we got lots and lots of contention issues, and everybody
> *always* touched the wait-queues whether they waited or not (so being per-
> zone was a big deal)
>
> We got rid of all that per-zone complexity when the normal case didn't hit in
> the page wait queues at all, but we may have over-done the simplification a
> bit since nobody showed any issue.
>
> In particular, we used to size the per-zone thing by amount of memory.
> We could easily re-introduce that for the new simpler page queues.
>
> The page_waitiqueue() is a simple helper function inside mm/filemap.c, and
> thanks to the per-page "do we have actual waiters" bit that we have now, we
> can actually afford to make it bigger and more complex now if we want to.
>
> What happens to your load if you just make that table bigger? You can
> literally test by just changing the constant from 8 to 16 or something, making
> us use twice as many bits for hashing. A "real"
> patch would size it by amount of memory, but just for testing the contention
> on your load, you can do the hacky one-liner.
Hi Linus,
We tried both 12 and 16 bit table and that didn't make a difference.
The long wake ups are mostly on the same page when we do instrumentation
Here is the wake_up_page_bit call stack when the workaround is running, which
is collected by perf record -g -a -e probe:wake_up_page_bit -- sleep 10
# To display the perf.data header info, please use --header/--header-only options.
#
#
# Total Lost Samples: 0
#
# Samples: 374 of event 'probe:wake_up_page_bit'
# Event count (approx.): 374
#
# Overhead Trace output
# ........ ..................
#
100.00% (ffffffffae1ad000)
|
---wake_up_page_bit
|
|--49.73%--migrate_misplaced_transhuge_page
| do_huge_pmd_numa_page
| __handle_mm_fault
| handle_mm_fault
| __do_page_fault
| do_page_fault
| page_fault
| |
| |--28.07%--0x2b7b7
| | |
| | |--13.64%--0x127a2
| | | 0x7fb5247eddc5
| | |
| | |--13.37%--0x127d8
| | | 0x7fb5247eddc5
| | |
| | |--0.53%--0x1280e
| | | 0x7fb5247eddc5
| | |
| | --0.27%--0x12844
| | 0x7fb5247eddc5
| |
| |--18.18%--0x2b788
| | |
| | |--14.97%--0x127a2
| | | 0x7fb5247eddc5
| | |
| | |--1.34%--0x1287a
| | | 0x7fb5247eddc5
| | |
| | |--0.53%--0x128b0
| | | 0x7fb5247eddc5
| | |
| | |--0.53%--0x1280e
| | | 0x7fb5247eddc5
| | |
| | |--0.53%--0x127d8
| | | 0x7fb5247eddc5
| | |
| | --0.27%--0x12844
| | 0x7fb5247eddc5
| |
| |--1.07%--0x2b823
| | |
| | |--0.53%--0x127a2
| | | 0x7fb5247eddc5
| | |
| | |--0.27%--0x1287a
| | | 0x7fb5247eddc5
| | |
| | --0.27%--0x127d8
| | 0x7fb5247eddc5
| |
| |--0.80%--0x2b88f
| | |
| | --0.53%--0x127d8
| | 0x7fb5247eddc5
| |
| |--0.80%--0x2b7f4
| | |
| | |--0.53%--0x127d8
| | | 0x7fb5247eddc5
| | |
| | --0.27%--0x127a2
| | 0x7fb5247eddc5
| |
| |--0.53%--0x2b8fb
| | 0x127a2
| | 0x7fb5247eddc5
| |
| --0.27%--0x2b8e9
| 0x127a2
| 0x7fb5247eddc5
|
|--44.12%--__handle_mm_fault
| handle_mm_fault
| __do_page_fault
| do_page_fault
| page_fault
| |
| |--30.75%--_dl_relocate_object
| | dl_main
| | _dl_sysdep_start
| | 0x40
| |
| --13.37%--memset
| _dl_map_object
| |
| |--2.94%--_etext
| |
| |--0.80%--0x7f34ea294b08
| | 0
| |
| |--0.80%--0x7f1d5fa64b08
| | 0
| |
| |--0.53%--0x7fd4c83dbb08
| | 0
| |
| |--0.53%--0x7efe3724cb08
| | 0
| |
| |--0.27%--0x7ff2cf0b69c0
| | 0
| |
| |--0.27%--0x7fc9bc22cb08
| | 0
| |
| |--0.27%--0x7fc432971058
| | 0
| |
| |--0.27%--0x7faf21ec2b08
| | 0
| |
| |--0.27%--0x7faf21ec2640
| | 0
| |
| |--0.27%--0x7f940f08e058
| | 0
| |
| |--0.27%--0x7f4b84122640
| | 0
| |
| |--0.27%--0x7f42c8fd7fd8
| | 0
| |
| |--0.27%--0x7f3f15778fd8
| | 0
| |
| |--0.27%--0x7f3f15776058
| | 0
| |
| |--0.27%--0x7f34ea27dfd8
| | 0
| |
| |--0.27%--0x7f34ea27b058
| | 0
| |
| |--0.27%--0x7f2a0409bb08
| | 0
| |
| |--0.27%--0x7f2a04084fd8
| | 0
| |
| |--0.27%--0x7f2a04082058
| | 0
| |
| |--0.27%--0x7f1949633b08
| | 0
| |
| |--0.27%--0x7f194961cfd8
| | 0
| |
| |--0.27%--0x7f1629f87b08
| | 0
| |
| |--0.27%--0x7f1629f70fd8
| | 0
| |
| |--0.27%--0x7f1629f6e058
| | 0
| |
| |--0.27%--0x7f060696eb08
| | 0
| |
| |--0.27%--0x7f04ac14c9c0
| | 0
| |
| |--0.27%--0x7efe8b4bbb08
| | 0
| |
| |--0.27%--0x7efe8b4a59c0
| | 0
| |
| |--0.27%--0x7efe8b4a4fd8
| | 0
| |
| |--0.27%--0x7efe8b4a2058
| | 0
| |
| |--0.27%--0x7efcd0c70b08
| | 0
| |
| |--0.27%--0x207ad8
| | 0
| |
| --0.27%--0x206b30
| 0
|
|--2.14%--filemap_map_pages
| __handle_mm_fault
| handle_mm_fault
| __do_page_fault
| do_page_fault
| page_fault
| |
| |--0.53%--_IO_vfscanf
| | |
| | |--0.27%--0x6563697665442055
| | |
| | --0.27%--_IO_vsscanf
| | 0x6563697665442055
| |
| |--0.53%--_dl_map_object_from_fd
| | _dl_map_object
| | |
| | |--0.27%--0x7faf21ec2640
| | | 0
| | |
| | --0.27%--_etext
| |
| |--0.27%--__libc_enable_asynccancel
| | __fopen_internal
| | 0x6d6f6f2f30373635
| |
| |--0.27%--vfprintf
| | _IO_vsprintf
| | 0x4
| |
| |--0.27%--0x1fb40
| | 0x41d589495541f689
| |
| --0.27%--memset@plt
|
|--1.87%--do_huge_pmd_numa_page
| __handle_mm_fault
| handle_mm_fault
| __do_page_fault
| do_page_fault
| page_fault
| |
| |--0.80%--0x2b7b7
| | 0x127d8
| | 0x7fb5247eddc5
| |
| |--0.80%--0x2b788
| | 0x127a2
| | 0x7fb5247eddc5
| |
| --0.27%--0x2b918
| 0x127d8
| 0x7fb5247eddc5
|
|--1.87%--migrate_pages
| migrate_misplaced_page
| __handle_mm_fault
| handle_mm_fault
| __do_page_fault
| do_page_fault
| page_fault
| |
| |--1.07%--0x2b7b7
| | |
| | |--0.80%--0x127d8
| | | 0x7fb5247eddc5
| | |
| | --0.27%--0x1287a
| | 0x7fb5247eddc5
| |
| --0.80%--0x2b788
| 0x127a2
| 0x7fb5247eddc5
|
--0.27%--do_wp_page
__handle_mm_fault
handle_mm_fault
__do_page_fault
do_page_fault
page_fault
_IO_link_in
Thanks,
Kan
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-17 18:30 +0200 |
| Message-ID | <ufvQd-7mK-7@gated-at.bofh.it> |
| In reply to | #1714179 |
On Thu, Aug 17, 2017 at 9:17 AM, Liang, Kan <kan.liang@intel.com> wrote:
>
> We tried both 12 and 16 bit table and that didn't make a difference.
> The long wake ups are mostly on the same page when we do instrumentation
Ok.
> Here is the wake_up_page_bit call stack when the workaround is running, which
> is collected by perf record -g -a -e probe:wake_up_page_bit -- sleep 10
It's actually not really wake_up_page_bit() that is all that
interesting, it would be more interesting to see which path it is that
*adds* the entries.
So it's mainly wait_on_page_bit_common(), but also add_page_wait_queue().
Can you get that call stack instead (or in addition to)?
Linus
[toc] | [prev] | [next] | [standalone]
| From | "Liang, Kan" <kan.liang@intel.com> |
|---|---|
| Date | 2017-08-17 22:20 +0200 |
| Message-ID | <ufzqO-1nj-23@gated-at.bofh.it> |
| In reply to | #1714182 |
> > Here is the wake_up_page_bit call stack when the workaround is running,
> which
> > is collected by perf record -g -a -e probe:wake_up_page_bit -- sleep 10
>
> It's actually not really wake_up_page_bit() that is all that
> interesting, it would be more interesting to see which path it is that
> *adds* the entries.
>
> So it's mainly wait_on_page_bit_common(), but also
> add_page_wait_queue().
>
> Can you get that call stack instead (or in addition to)?
>
Here is the call stack of wait_on_page_bit_common
when the queue is long (entries >1000).
# Overhead Trace output
# ........ ..................
#
100.00% (ffffffff931aefca)
|
---wait_on_page_bit
__migration_entry_wait
migration_entry_wait
do_swap_page
__handle_mm_fault
handle_mm_fault
__do_page_fault
do_page_fault
page_fault
|
|--21.89%--0x123a2
| start_thread
|
|--21.64%--0x12352
| start_thread
|
|--20.90%--_int_free
| |
| --20.44%--0
|
|--7.34%--0x127a9
| start_thread
|
|--6.84%--0x127df
| start_thread
|
|--6.65%--0x12205
| 0x1206d
| 0x11f85
| 0x11a05
| 0x10302
| |
| --6.62%--0xa8ee
| |
| --5.22%--0x3af5
| __libc_start_main
|
|--5.40%--0x1284b
| start_thread
|
|--3.14%--0x12881
| start_thread
|
|--3.02%--0x12773
| start_thread
|
--2.97%--0x12815
start_thread
Thanks,
Kan
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2017-08-17 22:50 +0200 |
| Message-ID | <ufzTP-1Ak-1@gated-at.bofh.it> |
| In reply to | #1714358 |
On Thu, Aug 17, 2017 at 1:18 PM, Liang, Kan <kan.liang@intel.com> wrote:
>
> Here is the call stack of wait_on_page_bit_common
> when the queue is long (entries >1000).
>
> # Overhead Trace output
> # ........ ..................
> #
> 100.00% (ffffffff931aefca)
> |
> ---wait_on_page_bit
> __migration_entry_wait
> migration_entry_wait
> do_swap_page
> __handle_mm_fault
> handle_mm_fault
> __do_page_fault
> do_page_fault
> page_fault
Hmm. Ok, so it does seem to very much be related to migration. Your
wake_up_page_bit() profile made me suspect that, but this one seems to
pretty much confirm it.
So it looks like that wait_on_page_locked() thing in
__migration_entry_wait(), and what probably happens is that your load
ends up triggering a lot of migration (or just migration of a very hot
page), and then *every* thread ends up waiting for whatever page that
ended up getting migrated.
And so the wait queue for that page grows hugely long.
Looking at the other profile, the thing that is locking the page (that
everybody then ends up waiting on) would seem to be
migrate_misplaced_transhuge_page(), so this is _presumably_ due to
NUMA balancing.
Does the problem go away if you disable the NUMA balancing code?
Adding Mel and Kirill to the participants, just to make them aware of
the issue, and just because their names show up when I look at blame.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Mel Gorman <mgorman@techsingularity.net> |
|---|---|
| Date | 2017-08-18 14:30 +0200 |
| Message-ID | <ufOzw-3Ek-5@gated-at.bofh.it> |
| In reply to | #1714369 |
On Thu, Aug 17, 2017 at 01:44:40PM -0700, Linus Torvalds wrote: > On Thu, Aug 17, 2017 at 1:18 PM, Liang, Kan <kan.liang@intel.com> wrote: > > > > Here is the call stack of wait_on_page_bit_common > > when the queue is long (entries >1000). > > > > # Overhead Trace output > > # ........ .................. > > # > > 100.00% (ffffffff931aefca) > > | > > ---wait_on_page_bit > > __migration_entry_wait > > migration_entry_wait > > do_swap_page > > __handle_mm_fault > > handle_mm_fault > > __do_page_fault > > do_page_fault > > page_fault > > Hmm. Ok, so it does seem to very much be related to migration. Your > wake_up_page_bit() profile made me suspect that, but this one seems to > pretty much confirm it. > > So it looks like that wait_on_page_locked() thing in > __migration_entry_wait(), and what probably happens is that your load > ends up triggering a lot of migration (or just migration of a very hot > page), and then *every* thread ends up waiting for whatever page that > ended up getting migrated. > Agreed. > And so the wait queue for that page grows hugely long. > It's basically only bounded by the maximum number of threads that can exist. > Looking at the other profile, the thing that is locking the page (that > everybody then ends up waiting on) would seem to be > migrate_misplaced_transhuge_page(), so this is _presumably_ due to > NUMA balancing. > Yes, migrate_misplaced_transhuge_page requires NUMA balancing to be part of the picture. > Does the problem go away if you disable the NUMA balancing code? > > Adding Mel and Kirill to the participants, just to make them aware of > the issue, and just because their names show up when I look at blame. > I'm not imagining a way of dealing with this that would reliably detect when there are a large number of waiters without adding a mess. We could adjust the scanning rate to reduce the problem but it would be difficult to target properly and wouldn't prevent the problem occurring with the added hassle that it would now be intermittent. Assuming the problem goes away by disabling NUMA then it would be nice if it could be determined that the page lock holder is trying to allocate a page when the queue is huge. That is part of the operation that potentially takes a long time and may be why so many callers are stacking up. If so, I would suggest clearing __GFP_DIRECT_RECLAIM from the GFP flags in migrate_misplaced_transhuge_page and assume that a remote hit is always going to be cheaper than compacting memory to successfully allocate a THP. That may be worth doing unconditionally because we'd have to save a *lot* of remote misses to offset compaction cost. Nothing fancy other than needing a comment if it works. diff --git a/mm/migrate.c b/mm/migrate.c index 627671551873..87b0275ddcdb 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -1926,7 +1926,7 @@ int migrate_misplaced_transhuge_page(struct mm_struct *mm, goto out_dropref; new_page = alloc_pages_node(node, - (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE), + (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE) & ~__GFP_DIRECT_RECLAIM, HPAGE_PMD_ORDER); if (!new_page) goto out_fail; -- Mel Gorman SUSE Labs
[toc] | [prev] | [next] | [standalone]
| From | "Liang, Kan" <kan.liang@intel.com> |
|---|---|
| Date | 2017-08-18 16:30 +0200 |
| Message-ID | <ufQrF-4YI-35@gated-at.bofh.it> |
| In reply to | #1714866 |
> On Thu, Aug 17, 2017 at 01:44:40PM -0700, Linus Torvalds wrote: > > On Thu, Aug 17, 2017 at 1:18 PM, Liang, Kan <kan.liang@intel.com> wrote: > > > > > > Here is the call stack of wait_on_page_bit_common when the queue is > > > long (entries >1000). > > > > > > # Overhead Trace output > > > # ........ .................. > > > # > > > 100.00% (ffffffff931aefca) > > > | > > > ---wait_on_page_bit > > > __migration_entry_wait > > > migration_entry_wait > > > do_swap_page > > > __handle_mm_fault > > > handle_mm_fault > > > __do_page_fault > > > do_page_fault > > > page_fault > > > > Hmm. Ok, so it does seem to very much be related to migration. Your > > wake_up_page_bit() profile made me suspect that, but this one seems to > > pretty much confirm it. > > > > So it looks like that wait_on_page_locked() thing in > > __migration_entry_wait(), and what probably happens is that your load > > ends up triggering a lot of migration (or just migration of a very hot > > page), and then *every* thread ends up waiting for whatever page that > > ended up getting migrated. > > > > Agreed. > > > And so the wait queue for that page grows hugely long. > > > > It's basically only bounded by the maximum number of threads that can exist. > > > Looking at the other profile, the thing that is locking the page (that > > everybody then ends up waiting on) would seem to be > > migrate_misplaced_transhuge_page(), so this is _presumably_ due to > > NUMA balancing. > > > > Yes, migrate_misplaced_transhuge_page requires NUMA balancing to be > part of the picture. > > > Does the problem go away if you disable the NUMA balancing code? > > > > Adding Mel and Kirill to the participants, just to make them aware of > > the issue, and just because their names show up when I look at blame. > > > > I'm not imagining a way of dealing with this that would reliably detect when > there are a large number of waiters without adding a mess. We could adjust > the scanning rate to reduce the problem but it would be difficult to target > properly and wouldn't prevent the problem occurring with the added hassle > that it would now be intermittent. > > Assuming the problem goes away by disabling NUMA then it would be nice if > it could be determined that the page lock holder is trying to allocate a page > when the queue is huge. That is part of the operation that potentially takes a > long time and may be why so many callers are stacking up. If so, I would > suggest clearing __GFP_DIRECT_RECLAIM from the GFP flags in > migrate_misplaced_transhuge_page and assume that a remote hit is always > going to be cheaper than compacting memory to successfully allocate a THP. > That may be worth doing unconditionally because we'd have to save a > *lot* of remote misses to offset compaction cost. > > Nothing fancy other than needing a comment if it works. > No, the patch doesn't work. Thanks, Kan > diff --git a/mm/migrate.c b/mm/migrate.c index > 627671551873..87b0275ddcdb 100644 > --- a/mm/migrate.c > +++ b/mm/migrate.c > @@ -1926,7 +1926,7 @@ int migrate_misplaced_transhuge_page(struct > mm_struct *mm, > goto out_dropref; > > new_page = alloc_pages_node(node, > - (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE), > + (GFP_TRANSHUGE_LIGHT | __GFP_THISNODE) & > ~__GFP_DIRECT_RECLAIM, > HPAGE_PMD_ORDER); > if (!new_page) > goto out_fail; > > -- > Mel Gorman > SUSE Labs
[toc] | [prev] | [next] | [standalone]
Page 1 of 4 [1] 2 3 4 Next page →
Back to top | Article view | linux.kernel
csiph-web