All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] psi: two division fixes
@ 2019-12-03 18:35 Johannes Weiner
  2019-12-03 18:35 ` [PATCH 1/2] psi: fix sampling error and rare div0 crashes with cgroups and high uptime Johannes Weiner
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Johannes Weiner @ 2019-12-03 18:35 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Jingfeng Xie, Suren Baghdasaryan, linux-kernel

Hi Peter,

here is a patch that should fix the rare div0 crashes reported by
Jingfeng, and a second patch for a div32-with-64-bit-divisor issue
spotted during code review.

Can you please take a look and route them through the scheduler tree?

Thanks!

 kernel/sched/psi.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)



^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH 1/2] psi: fix sampling error and rare div0 crashes with cgroups and high uptime
  2019-12-03 18:35 [PATCH 0/2] psi: two division fixes Johannes Weiner
@ 2019-12-03 18:35 ` Johannes Weiner
  2019-12-17 12:39   ` [tip: sched/urgent] sched/psi: Fix " tip-bot2 for Johannes Weiner
  2019-12-03 18:35 ` [PATCH 2/2] psi: fix a division error in psi poll() Johannes Weiner
  2019-12-11 15:43 ` [PATCH 0/2] psi: two division fixes Peter Zijlstra
  2 siblings, 1 reply; 6+ messages in thread
From: Johannes Weiner @ 2019-12-03 18:35 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Jingfeng Xie, Suren Baghdasaryan, linux-kernel

Jingfeng reports rare div0 crashes in psi on systems with some uptime:

[58914.066423] divide error: 0000 [#1] SMP
[58914.070416] Modules linked in: ipmi_poweroff ipmi_watchdog toa overlay fuse tcp_diag inet_diag binfmt_misc aisqos(O) aisqos_hotfixes(O)
[58914.083158] CPU: 94 PID: 140364 Comm: kworker/94:2 Tainted: G W OE K 4.9.151-015.ali3000.alios7.x86_64 #1
[58914.093722] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.23.34 02/14/2019
[58914.102728] Workqueue: events psi_update_work
[58914.107258] task: ffff8879da83c280 task.stack: ffffc90059dcc000
[58914.113336] RIP: 0010:[] [] psi_update_stats+0x1c1/0x330
[58914.122183] RSP: 0018:ffffc90059dcfd60 EFLAGS: 00010246
[58914.127650] RAX: 0000000000000000 RBX: ffff8858fe98be50 RCX: 000000007744d640
[58914.134947] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00003594f700648e
[58914.142243] RBP: ffffc90059dcfdf8 R08: 0000359500000000 R09: 0000000000000000
[58914.149538] R10: 0000000000000000 R11: 0000000000000000 R12: 0000359500000000
[58914.156837] R13: 0000000000000000 R14: 0000000000000000 R15: ffff8858fe98bd78
[58914.164136] FS: 0000000000000000(0000) GS:ffff887f7f380000(0000) knlGS:0000000000000000
[58914.172529] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[58914.178467] CR2: 00007f2240452090 CR3: 0000005d5d258000 CR4: 00000000007606f0
[58914.185765] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[58914.193061] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[58914.200360] PKRU: 55555554
[58914.203221] Stack:
[58914.205383] ffff8858fe98bd48 00000000000002f0 0000002e81036d09 ffffc90059dcfde8
[58914.213168] ffff8858fe98bec8 0000000000000000 0000000000000000 0000000000000000
[58914.220951] 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[58914.228734] Call Trace:
[58914.231337] [] psi_update_work+0x22/0x60
[58914.237067] [] process_one_work+0x189/0x420
[58914.243063] [] worker_thread+0x4e/0x4b0
[58914.248701] [] ? process_one_work+0x420/0x420
[58914.254869] [] kthread+0xe6/0x100
[58914.259994] [] ? kthread_park+0x60/0x60
[58914.265640] [] ret_from_fork+0x39/0x50
[58914.271193] Code: 41 29 c3 4d 39 dc 4d 0f 42 dc <49> f7 f1 48 8b 13 48 89 c7 48 c1
[58914.279691] RIP [] psi_update_stats+0x1c1/0x330

The crashing instruction is trying to divide the observed stall time
by the sampling period. The period, stored in R8, is not 0, but we are
dividing by the lower 32 bits only, which are all 0 in this instance.

We could switch to a 64-bit division, but the period shouldn't be that
big in the first place. It's the time between the last update and the
next scheduled one, and so should always be around 2s and comfortably
fit into 32 bits.

The bug is in the initialization of new cgroups: we schedule the first
sampling event in a cgroup as an offset of sched_clock(), but fail to
initialize the last_update timestamp, and it defaults to 0. That
results in a bogusly large sampling period the first time we run the
sampling code, and consequently we underreport pressure for the first
2s of a cgroup's life. But worse, if sched_clock() is sufficiently
advanced on the system, and the user gets unlucky, the period's lower
32 bits can all be 0 and the sampling division will crash.

Fix this by initializing the last update timestamp to the creation
time of the cgroup, thus correctly marking the start of the first
pressure sampling period in a new cgroup.

Reported-by: Jingfeng Xie <xiejingfeng@linux.alibaba.com>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 kernel/sched/psi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 517e3719027e..970db4686dd4 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -185,7 +185,8 @@ static void group_init(struct psi_group *group)
 
 	for_each_possible_cpu(cpu)
 		seqcount_init(&per_cpu_ptr(group->pcpu, cpu)->seq);
-	group->avg_next_update = sched_clock() + psi_period;
+	group->avg_last_update = sched_clock();
+	group->avg_next_update = group->avg_last_update + psi_period;
 	INIT_DELAYED_WORK(&group->avgs_work, psi_avgs_work);
 	mutex_init(&group->avgs_lock);
 	/* Init trigger-related members */
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH 2/2] psi: fix a division error in psi poll()
  2019-12-03 18:35 [PATCH 0/2] psi: two division fixes Johannes Weiner
  2019-12-03 18:35 ` [PATCH 1/2] psi: fix sampling error and rare div0 crashes with cgroups and high uptime Johannes Weiner
@ 2019-12-03 18:35 ` Johannes Weiner
  2019-12-17 12:39   ` [tip: sched/urgent] psi: Fix " tip-bot2 for Johannes Weiner
  2019-12-11 15:43 ` [PATCH 0/2] psi: two division fixes Peter Zijlstra
  2 siblings, 1 reply; 6+ messages in thread
From: Johannes Weiner @ 2019-12-03 18:35 UTC (permalink / raw)
  To: Peter Zijlstra; +Cc: Jingfeng Xie, Suren Baghdasaryan, linux-kernel

The psi window size is a u64 an can be up to 10 seconds right now,
which exceeds the lower 32 bits of the variable. We currently use
div_u64 for it, which is meant only for 32-bit divisors. The result is
garbage pressure sampling values and even potential div0 crashes.

Use div64_u64.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
---
 kernel/sched/psi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 970db4686dd4..ce8f6748678a 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -482,7 +482,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 		u32 remaining;
 
 		remaining = win->size - elapsed;
-		growth += div_u64(win->prev_growth * remaining, win->size);
+		growth += div64_u64(win->prev_growth * remaining, win->size);
 	}
 
 	return growth;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH 0/2] psi: two division fixes
  2019-12-03 18:35 [PATCH 0/2] psi: two division fixes Johannes Weiner
  2019-12-03 18:35 ` [PATCH 1/2] psi: fix sampling error and rare div0 crashes with cgroups and high uptime Johannes Weiner
  2019-12-03 18:35 ` [PATCH 2/2] psi: fix a division error in psi poll() Johannes Weiner
@ 2019-12-11 15:43 ` Peter Zijlstra
  2 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2019-12-11 15:43 UTC (permalink / raw)
  To: Johannes Weiner; +Cc: Jingfeng Xie, Suren Baghdasaryan, linux-kernel

On Tue, Dec 03, 2019 at 01:35:22PM -0500, Johannes Weiner wrote:
> Hi Peter,
> 
> here is a patch that should fix the rare div0 crashes reported by
> Jingfeng, and a second patch for a div32-with-64-bit-divisor issue
> spotted during code review.
> 
> Can you please take a look and route them through the scheduler tree?

Will do, Thanks!

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [tip: sched/urgent] psi: Fix a division error in psi poll()
  2019-12-03 18:35 ` [PATCH 2/2] psi: fix a division error in psi poll() Johannes Weiner
@ 2019-12-17 12:39   ` tip-bot2 for Johannes Weiner
  0 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Johannes Weiner @ 2019-12-17 12:39 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Johannes Weiner, Peter Zijlstra (Intel),
	Suren Baghdasaryan, Jingfeng Xie, x86, LKML

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     c3466952ca1514158d7c16c9cfc48c27d5c5dc0f
Gitweb:        https://git.kernel.org/tip/c3466952ca1514158d7c16c9cfc48c27d5c5dc0f
Author:        Johannes Weiner <hannes@cmpxchg.org>
AuthorDate:    Tue, 03 Dec 2019 13:35:24 -05:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 17 Dec 2019 13:32:48 +01:00

psi: Fix a division error in psi poll()

The psi window size is a u64 an can be up to 10 seconds right now,
which exceeds the lower 32 bits of the variable. We currently use
div_u64 for it, which is meant only for 32-bit divisors. The result is
garbage pressure sampling values and even potential div0 crashes.

Use div64_u64.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Suren Baghdasaryan <surenb@google.com>
Cc: Jingfeng Xie <xiejingfeng@linux.alibaba.com>
Link: https://lkml.kernel.org/r/20191203183524.41378-3-hannes@cmpxchg.org
---
 kernel/sched/psi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 970db46..ce8f674 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -482,7 +482,7 @@ static u64 window_update(struct psi_window *win, u64 now, u64 value)
 		u32 remaining;
 
 		remaining = win->size - elapsed;
-		growth += div_u64(win->prev_growth * remaining, win->size);
+		growth += div64_u64(win->prev_growth * remaining, win->size);
 	}
 
 	return growth;

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [tip: sched/urgent] sched/psi: Fix sampling error and rare div0 crashes with cgroups and high uptime
  2019-12-03 18:35 ` [PATCH 1/2] psi: fix sampling error and rare div0 crashes with cgroups and high uptime Johannes Weiner
@ 2019-12-17 12:39   ` tip-bot2 for Johannes Weiner
  0 siblings, 0 replies; 6+ messages in thread
From: tip-bot2 for Johannes Weiner @ 2019-12-17 12:39 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: Jingfeng Xie, Johannes Weiner, Peter Zijlstra (Intel),
	Suren Baghdasaryan, x86, LKML

The following commit has been merged into the sched/urgent branch of tip:

Commit-ID:     3dfbe25c27eab7c90c8a7e97b4c354a9d24dd985
Gitweb:        https://git.kernel.org/tip/3dfbe25c27eab7c90c8a7e97b4c354a9d24dd985
Author:        Johannes Weiner <hannes@cmpxchg.org>
AuthorDate:    Tue, 03 Dec 2019 13:35:23 -05:00
Committer:     Peter Zijlstra <peterz@infradead.org>
CommitterDate: Tue, 17 Dec 2019 13:32:47 +01:00

sched/psi: Fix sampling error and rare div0 crashes with cgroups and high uptime

Jingfeng reports rare div0 crashes in psi on systems with some uptime:

[58914.066423] divide error: 0000 [#1] SMP
[58914.070416] Modules linked in: ipmi_poweroff ipmi_watchdog toa overlay fuse tcp_diag inet_diag binfmt_misc aisqos(O) aisqos_hotfixes(O)
[58914.083158] CPU: 94 PID: 140364 Comm: kworker/94:2 Tainted: G W OE K 4.9.151-015.ali3000.alios7.x86_64 #1
[58914.093722] Hardware name: Alibaba Alibaba Cloud ECS/Alibaba Cloud ECS, BIOS 3.23.34 02/14/2019
[58914.102728] Workqueue: events psi_update_work
[58914.107258] task: ffff8879da83c280 task.stack: ffffc90059dcc000
[58914.113336] RIP: 0010:[] [] psi_update_stats+0x1c1/0x330
[58914.122183] RSP: 0018:ffffc90059dcfd60 EFLAGS: 00010246
[58914.127650] RAX: 0000000000000000 RBX: ffff8858fe98be50 RCX: 000000007744d640
[58914.134947] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 00003594f700648e
[58914.142243] RBP: ffffc90059dcfdf8 R08: 0000359500000000 R09: 0000000000000000
[58914.149538] R10: 0000000000000000 R11: 0000000000000000 R12: 0000359500000000
[58914.156837] R13: 0000000000000000 R14: 0000000000000000 R15: ffff8858fe98bd78
[58914.164136] FS: 0000000000000000(0000) GS:ffff887f7f380000(0000) knlGS:0000000000000000
[58914.172529] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[58914.178467] CR2: 00007f2240452090 CR3: 0000005d5d258000 CR4: 00000000007606f0
[58914.185765] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[58914.193061] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[58914.200360] PKRU: 55555554
[58914.203221] Stack:
[58914.205383] ffff8858fe98bd48 00000000000002f0 0000002e81036d09 ffffc90059dcfde8
[58914.213168] ffff8858fe98bec8 0000000000000000 0000000000000000 0000000000000000
[58914.220951] 0000000000000000 0000000000000000 0000000000000000 0000000000000000
[58914.228734] Call Trace:
[58914.231337] [] psi_update_work+0x22/0x60
[58914.237067] [] process_one_work+0x189/0x420
[58914.243063] [] worker_thread+0x4e/0x4b0
[58914.248701] [] ? process_one_work+0x420/0x420
[58914.254869] [] kthread+0xe6/0x100
[58914.259994] [] ? kthread_park+0x60/0x60
[58914.265640] [] ret_from_fork+0x39/0x50
[58914.271193] Code: 41 29 c3 4d 39 dc 4d 0f 42 dc <49> f7 f1 48 8b 13 48 89 c7 48 c1
[58914.279691] RIP [] psi_update_stats+0x1c1/0x330

The crashing instruction is trying to divide the observed stall time
by the sampling period. The period, stored in R8, is not 0, but we are
dividing by the lower 32 bits only, which are all 0 in this instance.

We could switch to a 64-bit division, but the period shouldn't be that
big in the first place. It's the time between the last update and the
next scheduled one, and so should always be around 2s and comfortably
fit into 32 bits.

The bug is in the initialization of new cgroups: we schedule the first
sampling event in a cgroup as an offset of sched_clock(), but fail to
initialize the last_update timestamp, and it defaults to 0. That
results in a bogusly large sampling period the first time we run the
sampling code, and consequently we underreport pressure for the first
2s of a cgroup's life. But worse, if sched_clock() is sufficiently
advanced on the system, and the user gets unlucky, the period's lower
32 bits can all be 0 and the sampling division will crash.

Fix this by initializing the last update timestamp to the creation
time of the cgroup, thus correctly marking the start of the first
pressure sampling period in a new cgroup.

Reported-by: Jingfeng Xie <xiejingfeng@linux.alibaba.com>
Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Suren Baghdasaryan <surenb@google.com>
Link: https://lkml.kernel.org/r/20191203183524.41378-2-hannes@cmpxchg.org
---
 kernel/sched/psi.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/kernel/sched/psi.c b/kernel/sched/psi.c
index 517e371..970db46 100644
--- a/kernel/sched/psi.c
+++ b/kernel/sched/psi.c
@@ -185,7 +185,8 @@ static void group_init(struct psi_group *group)
 
 	for_each_possible_cpu(cpu)
 		seqcount_init(&per_cpu_ptr(group->pcpu, cpu)->seq);
-	group->avg_next_update = sched_clock() + psi_period;
+	group->avg_last_update = sched_clock();
+	group->avg_next_update = group->avg_last_update + psi_period;
 	INIT_DELAYED_WORK(&group->avgs_work, psi_avgs_work);
 	mutex_init(&group->avgs_lock);
 	/* Init trigger-related members */

^ permalink raw reply related	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-12-17 12:40 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-03 18:35 [PATCH 0/2] psi: two division fixes Johannes Weiner
2019-12-03 18:35 ` [PATCH 1/2] psi: fix sampling error and rare div0 crashes with cgroups and high uptime Johannes Weiner
2019-12-17 12:39   ` [tip: sched/urgent] sched/psi: Fix " tip-bot2 for Johannes Weiner
2019-12-03 18:35 ` [PATCH 2/2] psi: fix a division error in psi poll() Johannes Weiner
2019-12-17 12:39   ` [tip: sched/urgent] psi: Fix " tip-bot2 for Johannes Weiner
2019-12-11 15:43 ` [PATCH 0/2] psi: two division fixes Peter Zijlstra

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.