[PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats
@ 2016-04-18  6:31 Davidlohr Bueso
  2016-04-18  6:31 ` [PATCH -tip 2/3] locking/pvqspinlock: Avoid double resetting of stats Davidlohr Bueso
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Davidlohr Bueso @ 2016-04-18  6:31 UTC (permalink / raw)
  To: mingo, peterz; +Cc: waiman.long, dave, linux-kernel, Davidlohr Bueso

While playing with such statistics I ran into the following
splat on a VM when opening pv_hash_hops:

[   25.267962] divide error: 0000 [#1] SMP
...
[   25.268807] CPU: 17 PID: 1018 Comm: cat Not tainted 4.6.0-rc3-debug+ #2
[   25.268853] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.1-0-g4adadbd-20151208_145348-sheep05 04/01/2014
[   25.268930] task: ffff88029a10c040 ti: ffff8800b1e1c000 task.ti: ffff8800b1e1c000
[   25.268979] RIP: 0010:[<ffffffff810b61fe>]  [<ffffffff810b61fe>] qstat_read+0x12e/0x1e0
[   25.269043] RSP: 0018:ffff8800b1e1fde8  EFLAGS: 00010246
[   25.269081] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
[   25.269128] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff81911640
[   25.269175] RBP: 0000000000000000 R08: 0000000000000007 R09: 0000000000000000
[   25.269223] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000df00
[   25.269270] R13: 0000000000000000 R14: 0000000000000000 R15: ffff8800b1e1ff28
[   25.269319] FS:  00007f3bd2f88700(0000) GS:ffff8802b5240000(0000) knlGS:0000000000000000
[   25.269372] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   25.269411] CR2: 00007f3bd2ddc008 CR3: 000000002c5f0000 CR4: 00000000000406e0
[   25.269467] Stack:
[   25.269487]  00007f3bd2ddd000 0000000000020000 ffffffff811cad7c ffffffff8119750c
[   25.269549]  ffff88002d08e000 ffffea0000b07140 ffffea0000b07140 ffff88002d08e000
[   25.269609]  ffffffff8118d3b9 ffffffff811937a9 00000000102a46cb ffff8802992beb00
[   25.269670] Call Trace:
[   25.269698]  [<ffffffff811cad7c>] ? mem_cgroup_commit_charge+0x6c/0xd0
[   25.269745]  [<ffffffff8119750c>] ? page_add_new_anon_rmap+0x8c/0xd0
[   25.269791]  [<ffffffff8118d3b9>] ? handle_mm_fault+0x1439/0x1b40
[   25.269834]  [<ffffffff811937a9>] ? do_mmap+0x449/0x550
[   25.269872]  [<ffffffff811d3de3>] ? __vfs_read+0x23/0xd0
[   25.270506]  [<ffffffff811d4ab2>] ? rw_verify_area+0x52/0xd0
[   25.271093]  [<ffffffff811d4bb1>] ? vfs_read+0x81/0x120
[   25.271677]  [<ffffffff811d5f12>] ? SyS_read+0x42/0xa0
[   25.272294]  [<ffffffff815720f6>] ? entry_SYSCALL_64_fastpath+0x1e/0xa8
[   25.272904] Code: 00 48 8b 74 24 50 65 48 33 34 25 28 00 00 00 0f 85 b7 00 00 00 48 83 c4 58 5b 5d 41 5c 41 5d 41 5e 41 5f c3 89 ee 4c 89 f0 31 d2 <48> f7 f6 48 d1 ed 48 8d 5c 24 10 48 8d 3c 92 48 89 c1 31 d2 48
[   25.274347] RIP  [<ffffffff810b61fe>] qstat_read+0x12e/0x1e0
[   25.274885]  RSP <ffff8800b1e1fde8>
[   25.275457] ---[ end trace 8558569eb04480ab ]---

Fix this by verifying that qstat_pv_kick_unlock is in fact non-zero,
similarly to what the qstat_pv_latency_wake case does, as if nothing
else, this can come from resetting the statistics, thus having 0 kicks
should be quite valid in this context.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 kernel/locking/qspinlock_stat.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
index eb2a2c9bc3fc..d734b7502001 100644
--- a/kernel/locking/qspinlock_stat.h
+++ b/kernel/locking/qspinlock_stat.h
@@ -136,10 +136,12 @@ static ssize_t qstat_read(struct file *file, char __user *user_buf,
 	}
 
 	if (counter == qstat_pv_hash_hops) {
-		u64 frac;
+		u64 frac = 0;
 
-		frac = 100ULL * do_div(stat, kicks);
-		frac = DIV_ROUND_CLOSEST_ULL(frac, kicks);
+		if (kicks) {
+			frac = 100ULL * do_div(stat, kicks);
+			frac = DIV_ROUND_CLOSEST_ULL(frac, kicks);
+		}
 
 		/*
 		 * Return a X.XX decimal number
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH -tip 2/3] locking/pvqspinlock: Avoid double resetting of stats
  2016-04-18  6:31 [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats Davidlohr Bueso
@ 2016-04-18  6:31 ` Davidlohr Bueso
  2016-04-18 19:39   ` Waiman Long
  2016-05-05  9:43   ` [tip:locking/core] " tip-bot for Davidlohr Bueso
  2016-04-18  6:31 ` [PATCH -tip 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat() Davidlohr Bueso
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 14+ messages in thread
From: Davidlohr Bueso @ 2016-04-18  6:31 UTC (permalink / raw)
  To: mingo, peterz; +Cc: waiman.long, dave, linux-kernel, Davidlohr Bueso

... remove the redundant second iteration, this is most
likely a copy/past buglet.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 kernel/locking/qspinlock_stat.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
index d734b7502001..72722334237a 100644
--- a/kernel/locking/qspinlock_stat.h
+++ b/kernel/locking/qspinlock_stat.h
@@ -191,8 +191,6 @@ static ssize_t qstat_write(struct file *file, const char __user *user_buf,
 
 		for (i = 0 ; i < qstat_num; i++)
 			WRITE_ONCE(ptr[i], 0);
-		for (i = 0 ; i < qstat_num; i++)
-			WRITE_ONCE(ptr[i], 0);
 	}
 	return count;
 }
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH -tip 2/3] locking/pvqspinlock: Avoid double resetting of stats
  2016-04-18  6:31 ` [PATCH -tip 2/3] locking/pvqspinlock: Avoid double resetting of stats Davidlohr Bueso
@ 2016-04-18 19:39   ` Waiman Long
  2016-05-05  9:43   ` [tip:locking/core] " tip-bot for Davidlohr Bueso
  1 sibling, 0 replies; 14+ messages in thread
From: Waiman Long @ 2016-04-18 19:39 UTC (permalink / raw)
  To: Davidlohr Bueso; +Cc: mingo, peterz, linux-kernel, Davidlohr Bueso

On 04/18/2016 02:31 AM, Davidlohr Bueso wrote:
> ... remove the redundant second iteration, this is most
> likely a copy/past buglet.
>
> Signed-off-by: Davidlohr Bueso<dbueso@suse.de>
> ---
>   kernel/locking/qspinlock_stat.h | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
> index d734b7502001..72722334237a 100644
> --- a/kernel/locking/qspinlock_stat.h
> +++ b/kernel/locking/qspinlock_stat.h
> @@ -191,8 +191,6 @@ static ssize_t qstat_write(struct file *file, const char __user *user_buf,
>
>   		for (i = 0 ; i<  qstat_num; i++)
>   			WRITE_ONCE(ptr[i], 0);
> -		for (i = 0 ; i<  qstat_num; i++)
> -			WRITE_ONCE(ptr[i], 0);
>   	}
>   	return count;
>   }

The double write is done on purpose. As the statistics count update 
isn't atomic, there is a very small chance (p) that clearing the count 
may happen in the middle of read-modify-write bus transaction. Doing a 
double write will reduce the chance further to p^2. This isn't failsafe, 
but I think is good enough.

However, I don't mind eliminate the double write either as we can always 
view the statistics count after a reset to make sure that they are 
properly cleared.

Cheers,
Longman

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [tip:locking/core] locking/pvqspinlock: Avoid double resetting of stats
  2016-04-18  6:31 ` [PATCH -tip 2/3] locking/pvqspinlock: Avoid double resetting of stats Davidlohr Bueso
  2016-04-18 19:39   ` Waiman Long
@ 2016-05-05  9:43   ` tip-bot for Davidlohr Bueso
  1 sibling, 0 replies; 14+ messages in thread
From: tip-bot for Davidlohr Bueso @ 2016-05-05  9:43 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: linux-kernel, dbueso, peterz, mingo, tglx, dave, hpa, paulmck,
	torvalds, akpm

Commit-ID:  dc209a3fd73ec96d4491bcc128c3b50b0a8e8017
Gitweb:     http://git.kernel.org/tip/dc209a3fd73ec96d4491bcc128c3b50b0a8e8017
Author:     Davidlohr Bueso <dave@stgolabs.net>
AuthorDate: Sun, 17 Apr 2016 23:31:42 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 5 May 2016 09:58:49 +0200

locking/pvqspinlock: Avoid double resetting of stats

... remove the redundant second iteration, this is most
likely a copy/past buglet.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dave@stgolabs.net
Cc: waiman.long@hpe.com
Link: http://lkml.kernel.org/r/1460961103-24953-2-git-send-email-dave@stgolabs.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/qspinlock_stat.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
index d734b75..7272233 100644
--- a/kernel/locking/qspinlock_stat.h
+++ b/kernel/locking/qspinlock_stat.h
@@ -191,8 +191,6 @@ static ssize_t qstat_write(struct file *file, const char __user *user_buf,
 
 		for (i = 0 ; i < qstat_num; i++)
 			WRITE_ONCE(ptr[i], 0);
-		for (i = 0 ; i < qstat_num; i++)
-			WRITE_ONCE(ptr[i], 0);
 	}
 	return count;
 }

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* [PATCH -tip 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat()
  2016-04-18  6:31 [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats Davidlohr Bueso
  2016-04-18  6:31 ` [PATCH -tip 2/3] locking/pvqspinlock: Avoid double resetting of stats Davidlohr Bueso
@ 2016-04-18  6:31 ` Davidlohr Bueso
  2016-04-18 16:16   ` Davidlohr Bueso
  2016-04-18 19:40   ` [PATCH -tip 3/3] " Waiman Long
  2016-04-18 19:34 ` [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats Waiman Long
  2016-04-19  9:34 ` [tip:locking/urgent] locking/pvqspinlock: Fix division by zero in qstat_read() tip-bot for Davidlohr Bueso
  3 siblings, 2 replies; 14+ messages in thread
From: Davidlohr Bueso @ 2016-04-18  6:31 UTC (permalink / raw)
  To: mingo, peterz; +Cc: waiman.long, dave, linux-kernel, Davidlohr Bueso

Specifically around the debugfs file creation calls,
I have no idea if they could ever possibly fail, but
this is core code (debug aside) so lets at least
check the return value and inform anything fishy.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 kernel/locking/qspinlock_stat.h | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
index 72722334237a..ddcd653c942c 100644
--- a/kernel/locking/qspinlock_stat.h
+++ b/kernel/locking/qspinlock_stat.h
@@ -225,12 +225,18 @@ static int __init init_qspinlock_stat(void)
 	 * performance.
 	 */
 	for (i = 0; i < qstat_num; i++)
-		debugfs_create_file(qstat_names[i], 0400, d_qstat,
-				   (void *)(long)i, &fops_qstat);
+		if (!debugfs_create_file(qstat_names[i], 0400, d_qstat,
+					 (void *)(long)i, &fops_qstat))
+			goto fail;
+
+	if (!debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
+				 (void *)(long)qstat_reset_cnts, &fops_qstat))
+		goto fail;
 
-	debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
-			   (void *)(long)qstat_reset_cnts, &fops_qstat);
 	return 0;
+fail:
+	debugfs_remove_recursive(d_qstat);
+	return 1;
 }
 fs_initcall(init_qspinlock_stat);
 
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH -tip 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat()
  2016-04-18  6:31 ` [PATCH -tip 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat() Davidlohr Bueso
@ 2016-04-18 16:16   ` Davidlohr Bueso
       [not found]     ` <CS1PR84MB0312315E60265F5E8972CACBF16C0@CS1PR84MB0312.NAMPRD84.PROD.OUTLOOK.COM>
  2016-04-18 19:40   ` [PATCH -tip 3/3] " Waiman Long
  1 sibling, 1 reply; 14+ messages in thread
From: Davidlohr Bueso @ 2016-04-18 16:16 UTC (permalink / raw)
  To: mingo, peterz; +Cc: waiman.long, linux-kernel, Davidlohr Bueso

On Sun, 17 Apr 2016, Davidlohr Bueso wrote:

>diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
>index 72722334237a..ddcd653c942c 100644
>--- a/kernel/locking/qspinlock_stat.h
>+++ b/kernel/locking/qspinlock_stat.h
>@@ -225,12 +225,18 @@ static int __init init_qspinlock_stat(void)
> 	 * performance.
> 	 */
> 	for (i = 0; i < qstat_num; i++)
>-		debugfs_create_file(qstat_names[i], 0400, d_qstat,
>-				   (void *)(long)i, &fops_qstat);
>+		if (!debugfs_create_file(qstat_names[i], 0400, d_qstat,
>+					 (void *)(long)i, &fops_qstat))
>+			goto fail;
>+
>+	if (!debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
>+				 (void *)(long)qstat_reset_cnts, &fops_qstat))
>+		goto fail;
>
>-	debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
>-			   (void *)(long)qstat_reset_cnts, &fops_qstat);
> 	return 0;
>+fail:
>+	debugfs_remove_recursive(d_qstat);
>+	return 1;

Hmm actually if debugfs_create_dir() fails firstly, we still return 0, Waiman, did
you mean 1 there, no?

Thanks,
Davidlohr

^ permalink raw reply	[flat|nested] 14+ messages in thread

[parent not found: <CS1PR84MB0312315E60265F5E8972CACBF16C0@CS1PR84MB0312.NAMPRD84.PROD.OUTLOOK.COM>]

* [PATCH -tip v2] locking/pvqspinlock: Robustify init_qspinlock_stat()
       [not found]     ` <CS1PR84MB0312315E60265F5E8972CACBF16C0@CS1PR84MB0312.NAMPRD84.PROD.OUTLOOK.COM>
@ 2016-04-20  4:10       ` Davidlohr Bueso
  2016-04-20  4:13         ` Davidlohr Bueso
  2016-04-20  4:17       ` [PATCH -tip v3 3/3] " Davidlohr Bueso
  1 sibling, 1 reply; 14+ messages in thread
From: Davidlohr Bueso @ 2016-04-20  4:10 UTC (permalink / raw)
  To: Long, Wai Man
  Cc: mingo@kernel.org, peterz@infradead.org,
	linux-kernel@vger.kernel.org, Davidlohr Bueso

Specifically around the debugfs file creation calls,
I have no idea if they could ever possibly fail, but
this is core code (debug aside) so lets at least
check the return value and inform anything fishy.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 kernel/locking/qspinlock_stat.h | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
index 72722334237a..fa0863423a04 100644
--- a/kernel/locking/qspinlock_stat.h
+++ b/kernel/locking/qspinlock_stat.h
@@ -212,10 +212,8 @@ static int __init init_qspinlock_stat(void)
	struct dentry *d_qstat = debugfs_create_dir("qlockstat", NULL);
	int i;

-	if (!d_qstat) {
-		pr_warn("Could not create 'qlockstat' debugfs directory\n");
-		return 0;
-	}
+	if (!d_qstat)
+		goto out;

	/*
	 * Create the debugfs files
@@ -225,12 +223,20 @@ static int __init init_qspinlock_stat(void)
	 * performance.
	 */
	for (i = 0; i < qstat_num; i++)
-		debugfs_create_file(qstat_names[i], 0400, d_qstat,
-				   (void *)(long)i, &fops_qstat);
+		if (!debugfs_create_file(qstat_names[i], 0400, d_qstat,
+					 (void *)(long)i, &fops_qstat))
+			goto fail_undo;
+
+	if (!debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
+				 (void *)(long)qstat_reset_cnts, &fops_qstat))
+		goto fail_undo;

-	debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
-			   (void *)(long)qstat_reset_cnts, &fops_qstat);
	return 0;
+fail_undo:
+	debugfs_remove_recursive(d_qstat);
+out:
+	pr_info("Could not create 'qlockstat' debugfs entries\n");
+	return -ENOMEM;
 }
 fs_initcall(init_qspinlock_stat);

--
2.8.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH -tip v2] locking/pvqspinlock: Robustify init_qspinlock_stat()
  2016-04-20  4:10       ` [PATCH -tip v2] " Davidlohr Bueso
@ 2016-04-20  4:13         ` Davidlohr Bueso
  0 siblings, 0 replies; 14+ messages in thread
From: Davidlohr Bueso @ 2016-04-20  4:13 UTC (permalink / raw)
  To: Long, Wai Man
  Cc: mingo@kernel.org, peterz@infradead.org,
	linux-kernel@vger.kernel.org, Davidlohr Bueso

>+	pr_info("Could not create 'qlockstat' debugfs entries\n");

Please ignore this, I was not meaning to change pr_warn level, this was
simply a stale version. Sending v3 *sigh*.

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH -tip v3 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat()
       [not found]     ` <CS1PR84MB0312315E60265F5E8972CACBF16C0@CS1PR84MB0312.NAMPRD84.PROD.OUTLOOK.COM>
  2016-04-20  4:10       ` [PATCH -tip v2] " Davidlohr Bueso
@ 2016-04-20  4:17       ` Davidlohr Bueso
  2016-04-20 20:06         ` Waiman Long
  2016-05-05  9:44         ` [tip:locking/core] " tip-bot for Davidlohr Bueso
  1 sibling, 2 replies; 14+ messages in thread
From: Davidlohr Bueso @ 2016-04-20  4:17 UTC (permalink / raw)
  To: Long, Wai Man
  Cc: mingo@kernel.org, peterz@infradead.org,
	linux-kernel@vger.kernel.org, Davidlohr Bueso

locking/pvqspinlock: Robustify init_qspinlock_stat()

Specifically around the debugfs file creation calls,
I have no idea if they could ever possibly fail, but
this is core code (debug aside) so lets at least
check the return value and inform anything fishy.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
---
 kernel/locking/qspinlock_stat.h | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
index 72722334237a..22e025309845 100644
--- a/kernel/locking/qspinlock_stat.h
+++ b/kernel/locking/qspinlock_stat.h
@@ -212,10 +212,8 @@ static int __init init_qspinlock_stat(void)
 	struct dentry *d_qstat = debugfs_create_dir("qlockstat", NULL);
 	int i;
 
-	if (!d_qstat) {
-		pr_warn("Could not create 'qlockstat' debugfs directory\n");
-		return 0;
-	}
+	if (!d_qstat)
+		goto out;
 
 	/*
 	 * Create the debugfs files
@@ -225,12 +223,20 @@ static int __init init_qspinlock_stat(void)
 	 * performance.
 	 */
 	for (i = 0; i < qstat_num; i++)
-		debugfs_create_file(qstat_names[i], 0400, d_qstat,
-				   (void *)(long)i, &fops_qstat);
+		if (!debugfs_create_file(qstat_names[i], 0400, d_qstat,
+					 (void *)(long)i, &fops_qstat))
+			goto fail_undo;
+
+	if (!debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
+				 (void *)(long)qstat_reset_cnts, &fops_qstat))
+		goto fail_undo;
 
-	debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
-			   (void *)(long)qstat_reset_cnts, &fops_qstat);
 	return 0;
+fail_undo:
+	debugfs_remove_recursive(d_qstat);
+out:
+	pr_warn("Could not create 'qlockstat' debugfs entries\n");
+	return -ENOMEM;
 }
 fs_initcall(init_qspinlock_stat);
 
-- 
2.8.1

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH -tip v3 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat()
  2016-04-20  4:17       ` [PATCH -tip v3 3/3] " Davidlohr Bueso
@ 2016-04-20 20:06         ` Waiman Long
  2016-05-05  9:44         ` [tip:locking/core] " tip-bot for Davidlohr Bueso
  1 sibling, 0 replies; 14+ messages in thread
From: Waiman Long @ 2016-04-20 20:06 UTC (permalink / raw)
  To: Davidlohr Bueso
  Cc: mingo@kernel.org, peterz@infradead.org,
	linux-kernel@vger.kernel.org, Davidlohr Bueso

On 04/20/2016 12:17 AM, Davidlohr Bueso wrote:
> locking/pvqspinlock: Robustify init_qspinlock_stat()
>
> Specifically around the debugfs file creation calls,
> I have no idea if they could ever possibly fail, but
> this is core code (debug aside) so lets at least
> check the return value and inform anything fishy.
>
> Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
> ---
> kernel/locking/qspinlock_stat.h | 22 ++++++++++++++--------
> 1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/kernel/locking/qspinlock_stat.h 
> b/kernel/locking/qspinlock_stat.h
> index 72722334237a..22e025309845 100644
> --- a/kernel/locking/qspinlock_stat.h
> +++ b/kernel/locking/qspinlock_stat.h
> @@ -212,10 +212,8 @@ static int __init init_qspinlock_stat(void)
>     struct dentry *d_qstat = debugfs_create_dir("qlockstat", NULL);
>     int i;
>
> -    if (!d_qstat) {
> -        pr_warn("Could not create 'qlockstat' debugfs directory\n");
> -        return 0;
> -    }
> +    if (!d_qstat)
> +        goto out;
>
>     /*
>      * Create the debugfs files
> @@ -225,12 +223,20 @@ static int __init init_qspinlock_stat(void)
>      * performance.
>      */
>     for (i = 0; i < qstat_num; i++)
> -        debugfs_create_file(qstat_names[i], 0400, d_qstat,
> -                   (void *)(long)i, &fops_qstat);
> +        if (!debugfs_create_file(qstat_names[i], 0400, d_qstat,
> +                     (void *)(long)i, &fops_qstat))
> +            goto fail_undo;
> +
> +    if (!debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, 
> d_qstat,
> +                 (void *)(long)qstat_reset_cnts, &fops_qstat))
> +        goto fail_undo;
>
> -    debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
> -               (void *)(long)qstat_reset_cnts, &fops_qstat);
>     return 0;
> +fail_undo:
> +    debugfs_remove_recursive(d_qstat);
> +out:
> +    pr_warn("Could not create 'qlockstat' debugfs entries\n");
> +    return -ENOMEM;
> }
> fs_initcall(init_qspinlock_stat);
>

Reviewed-by: Waiman Long <Waiman.Long@hpe.com>

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [tip:locking/core] locking/pvqspinlock: Robustify init_qspinlock_stat()
  2016-04-20  4:17       ` [PATCH -tip v3 3/3] " Davidlohr Bueso
  2016-04-20 20:06         ` Waiman Long
@ 2016-05-05  9:44         ` tip-bot for Davidlohr Bueso
  1 sibling, 0 replies; 14+ messages in thread
From: tip-bot for Davidlohr Bueso @ 2016-05-05  9:44 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: akpm, Waiman.Long, paulmck, mingo, dave, peterz, dbueso, hpa,
	linux-kernel, tglx, torvalds

Commit-ID:  b96bbdde19cc56f288372d25fd5ea7af04fc1271
Gitweb:     http://git.kernel.org/tip/b96bbdde19cc56f288372d25fd5ea7af04fc1271
Author:     Davidlohr Bueso <dave@stgolabs.net>
AuthorDate: Tue, 19 Apr 2016 21:17:25 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 5 May 2016 09:58:51 +0200

locking/pvqspinlock: Robustify init_qspinlock_stat()

Specifically around the debugfs file creation calls,
I have no idea if they could ever possibly fail, but
this is core code (debug aside) so lets at least
check the return value and inform anything fishy.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Reviewed-by: Waiman Long <Waiman.Long@hpe.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Link: http://lkml.kernel.org/r/20160420041725.GC3472@linux-uzut.site
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/qspinlock_stat.h | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
index 7272233..22e0253 100644
--- a/kernel/locking/qspinlock_stat.h
+++ b/kernel/locking/qspinlock_stat.h
@@ -212,10 +212,8 @@ static int __init init_qspinlock_stat(void)
 	struct dentry *d_qstat = debugfs_create_dir("qlockstat", NULL);
 	int i;
 
-	if (!d_qstat) {
-		pr_warn("Could not create 'qlockstat' debugfs directory\n");
-		return 0;
-	}
+	if (!d_qstat)
+		goto out;
 
 	/*
 	 * Create the debugfs files
@@ -225,12 +223,20 @@ static int __init init_qspinlock_stat(void)
 	 * performance.
 	 */
 	for (i = 0; i < qstat_num; i++)
-		debugfs_create_file(qstat_names[i], 0400, d_qstat,
-				   (void *)(long)i, &fops_qstat);
+		if (!debugfs_create_file(qstat_names[i], 0400, d_qstat,
+					 (void *)(long)i, &fops_qstat))
+			goto fail_undo;
+
+	if (!debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
+				 (void *)(long)qstat_reset_cnts, &fops_qstat))
+		goto fail_undo;
 
-	debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
-			   (void *)(long)qstat_reset_cnts, &fops_qstat);
 	return 0;
+fail_undo:
+	debugfs_remove_recursive(d_qstat);
+out:
+	pr_warn("Could not create 'qlockstat' debugfs entries\n");
+	return -ENOMEM;
 }
 fs_initcall(init_qspinlock_stat);
 

^ permalink raw reply related	[flat|nested] 14+ messages in thread

* Re: [PATCH -tip 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat()
  2016-04-18  6:31 ` [PATCH -tip 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat() Davidlohr Bueso
  2016-04-18 16:16   ` Davidlohr Bueso
@ 2016-04-18 19:40   ` Waiman Long
  1 sibling, 0 replies; 14+ messages in thread
From: Waiman Long @ 2016-04-18 19:40 UTC (permalink / raw)
  To: Davidlohr Bueso; +Cc: mingo, peterz, linux-kernel, Davidlohr Bueso

On 04/18/2016 02:31 AM, Davidlohr Bueso wrote:
> Specifically around the debugfs file creation calls,
> I have no idea if they could ever possibly fail, but
> this is core code (debug aside) so lets at least
> check the return value and inform anything fishy.
>
> Signed-off-by: Davidlohr Bueso<dbueso@suse.de>
> ---
>   kernel/locking/qspinlock_stat.h | 14 ++++++++++----
>   1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
> index 72722334237a..ddcd653c942c 100644
> --- a/kernel/locking/qspinlock_stat.h
> +++ b/kernel/locking/qspinlock_stat.h
> @@ -225,12 +225,18 @@ static int __init init_qspinlock_stat(void)
>   	 * performance.
>   	 */
>   	for (i = 0; i<  qstat_num; i++)
> -		debugfs_create_file(qstat_names[i], 0400, d_qstat,
> -				   (void *)(long)i,&fops_qstat);
> +		if (!debugfs_create_file(qstat_names[i], 0400, d_qstat,
> +					 (void *)(long)i,&fops_qstat))
> +			goto fail;
> +
> +	if (!debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
> +				 (void *)(long)qstat_reset_cnts,&fops_qstat))
> +		goto fail;
>
> -	debugfs_create_file(qstat_names[qstat_reset_cnts], 0200, d_qstat,
> -			   (void *)(long)qstat_reset_cnts,&fops_qstat);
>   	return 0;
> +fail:
> +	debugfs_remove_recursive(d_qstat);
> +	return 1;
>   }
>   fs_initcall(init_qspinlock_stat);
>

Reviewed-by: Waiman Long <Waiman.Long@hpe.com>

Cheers,
Longman

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats
  2016-04-18  6:31 [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats Davidlohr Bueso
  2016-04-18  6:31 ` [PATCH -tip 2/3] locking/pvqspinlock: Avoid double resetting of stats Davidlohr Bueso
  2016-04-18  6:31 ` [PATCH -tip 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat() Davidlohr Bueso
@ 2016-04-18 19:34 ` Waiman Long
  2016-04-19  9:34 ` [tip:locking/urgent] locking/pvqspinlock: Fix division by zero in qstat_read() tip-bot for Davidlohr Bueso
  3 siblings, 0 replies; 14+ messages in thread
From: Waiman Long @ 2016-04-18 19:34 UTC (permalink / raw)
  To: Davidlohr Bueso; +Cc: mingo, peterz, linux-kernel, Davidlohr Bueso

On 04/18/2016 02:31 AM, Davidlohr Bueso wrote:
> While playing with such statistics I ran into the following
> splat on a VM when opening pv_hash_hops:
>
> [   25.267962] divide error: 0000 [#1] SMP
> ...
> [   25.268807] CPU: 17 PID: 1018 Comm: cat Not tainted 4.6.0-rc3-debug+ #2
> [   25.268853] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.8.1-0-g4adadbd-20151208_145348-sheep05 04/01/2014
> [   25.268930] task: ffff88029a10c040 ti: ffff8800b1e1c000 task.ti: ffff8800b1e1c000
> [   25.268979] RIP: 0010:[<ffffffff810b61fe>]  [<ffffffff810b61fe>] qstat_read+0x12e/0x1e0
> [   25.269043] RSP: 0018:ffff8800b1e1fde8  EFLAGS: 00010246
> [   25.269081] RAX: 0000000000000000 RBX: 0000000000000000 RCX: 0000000000000000
> [   25.269128] RDX: 0000000000000000 RSI: 0000000000000000 RDI: ffffffff81911640
> [   25.269175] RBP: 0000000000000000 R08: 0000000000000007 R09: 0000000000000000
> [   25.269223] R10: 0000000000000000 R11: 0000000000000246 R12: 000000000000df00
> [   25.269270] R13: 0000000000000000 R14: 0000000000000000 R15: ffff8800b1e1ff28
> [   25.269319] FS:  00007f3bd2f88700(0000) GS:ffff8802b5240000(0000) knlGS:0000000000000000
> [   25.269372] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   25.269411] CR2: 00007f3bd2ddc008 CR3: 000000002c5f0000 CR4: 00000000000406e0
> [   25.269467] Stack:
> [   25.269487]  00007f3bd2ddd000 0000000000020000 ffffffff811cad7c ffffffff8119750c
> [   25.269549]  ffff88002d08e000 ffffea0000b07140 ffffea0000b07140 ffff88002d08e000
> [   25.269609]  ffffffff8118d3b9 ffffffff811937a9 00000000102a46cb ffff8802992beb00
> [   25.269670] Call Trace:
> [   25.269698]  [<ffffffff811cad7c>] ? mem_cgroup_commit_charge+0x6c/0xd0
> [   25.269745]  [<ffffffff8119750c>] ? page_add_new_anon_rmap+0x8c/0xd0
> [   25.269791]  [<ffffffff8118d3b9>] ? handle_mm_fault+0x1439/0x1b40
> [   25.269834]  [<ffffffff811937a9>] ? do_mmap+0x449/0x550
> [   25.269872]  [<ffffffff811d3de3>] ? __vfs_read+0x23/0xd0
> [   25.270506]  [<ffffffff811d4ab2>] ? rw_verify_area+0x52/0xd0
> [   25.271093]  [<ffffffff811d4bb1>] ? vfs_read+0x81/0x120
> [   25.271677]  [<ffffffff811d5f12>] ? SyS_read+0x42/0xa0
> [   25.272294]  [<ffffffff815720f6>] ? entry_SYSCALL_64_fastpath+0x1e/0xa8
> [   25.272904] Code: 00 48 8b 74 24 50 65 48 33 34 25 28 00 00 00 0f 85 b7 00 00 00 48 83 c4 58 5b 5d 41 5c 41 5d 41 5e 41 5f c3 89 ee 4c 89 f0 31 d2<48>  f7 f6 48 d1 ed 48 8d 5c 24 10 48 8d 3c 92 48 89 c1 31 d2 48
> [   25.274347] RIP  [<ffffffff810b61fe>] qstat_read+0x12e/0x1e0
> [   25.274885]  RSP<ffff8800b1e1fde8>
> [   25.275457] ---[ end trace 8558569eb04480ab ]---
>
> Fix this by verifying that qstat_pv_kick_unlock is in fact non-zero,
> similarly to what the qstat_pv_latency_wake case does, as if nothing
> else, this can come from resetting the statistics, thus having 0 kicks
> should be quite valid in this context.
>
> Signed-off-by: Davidlohr Bueso<dbueso@suse.de>
> ---
>   kernel/locking/qspinlock_stat.h | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
> index eb2a2c9bc3fc..d734b7502001 100644
> --- a/kernel/locking/qspinlock_stat.h
> +++ b/kernel/locking/qspinlock_stat.h
> @@ -136,10 +136,12 @@ static ssize_t qstat_read(struct file *file, char __user *user_buf,
>   	}
>
>   	if (counter == qstat_pv_hash_hops) {
> -		u64 frac;
> +		u64 frac = 0;
>
> -		frac = 100ULL * do_div(stat, kicks);
> -		frac = DIV_ROUND_CLOSEST_ULL(frac, kicks);
> +		if (kicks) {
> +			frac = 100ULL * do_div(stat, kicks);
> +			frac = DIV_ROUND_CLOSEST_ULL(frac, kicks);
> +		}
>
>   		/*
>   		 * Return a X.XX decimal number

Reviewed-by: Waiman Long <Waiman.Long@hpe.com>

Cheers,
Longman

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [tip:locking/urgent] locking/pvqspinlock: Fix division by zero in qstat_read()
  2016-04-18  6:31 [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats Davidlohr Bueso
                   ` (2 preceding siblings ...)
  2016-04-18 19:34 ` [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats Waiman Long
@ 2016-04-19  9:34 ` tip-bot for Davidlohr Bueso
  3 siblings, 0 replies; 14+ messages in thread
From: tip-bot for Davidlohr Bueso @ 2016-04-19  9:34 UTC (permalink / raw)
  To: linux-tip-commits
  Cc: hpa, torvalds, linux-kernel, tglx, Waiman.Long, paulmck, dbueso,
	akpm, mingo, dave, peterz

Commit-ID:  6687659568e2ec5b3ac24b39c5d26ce8b9d90434
Gitweb:     http://git.kernel.org/tip/6687659568e2ec5b3ac24b39c5d26ce8b9d90434
Author:     Davidlohr Bueso <dave@stgolabs.net>
AuthorDate: Sun, 17 Apr 2016 23:31:41 -0700
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Tue, 19 Apr 2016 10:49:19 +0200

locking/pvqspinlock: Fix division by zero in qstat_read()

While playing with the qstat statistics (in <debugfs>/qlockstat/) I ran into
the following splat on a VM when opening pv_hash_hops:

  divide error: 0000 [#1] SMP
  ...
  RIP: 0010:[<ffffffff810b61fe>]  [<ffffffff810b61fe>] qstat_read+0x12e/0x1e0
  ...
  Call Trace:
    [<ffffffff811cad7c>] ? mem_cgroup_commit_charge+0x6c/0xd0
    [<ffffffff8119750c>] ? page_add_new_anon_rmap+0x8c/0xd0
    [<ffffffff8118d3b9>] ? handle_mm_fault+0x1439/0x1b40
    [<ffffffff811937a9>] ? do_mmap+0x449/0x550
    [<ffffffff811d3de3>] ? __vfs_read+0x23/0xd0
    [<ffffffff811d4ab2>] ? rw_verify_area+0x52/0xd0
    [<ffffffff811d4bb1>] ? vfs_read+0x81/0x120
    [<ffffffff811d5f12>] ? SyS_read+0x42/0xa0
    [<ffffffff815720f6>] ? entry_SYSCALL_64_fastpath+0x1e/0xa8

Fix this by verifying that qstat_pv_kick_unlock is in fact non-zero,
similarly to what the qstat_pv_latency_wake case does, as if nothing
else, this can come from resetting the statistics, thus having 0 kicks
should be quite valid in this context.

Signed-off-by: Davidlohr Bueso <dbueso@suse.de>
Reviewed-by: Waiman Long <Waiman.Long@hpe.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dave@stgolabs.net
Cc: waiman.long@hpe.com
Link: http://lkml.kernel.org/r/1460961103-24953-1-git-send-email-dave@stgolabs.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/locking/qspinlock_stat.h | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/locking/qspinlock_stat.h b/kernel/locking/qspinlock_stat.h
index eb2a2c9..d734b75 100644
--- a/kernel/locking/qspinlock_stat.h
+++ b/kernel/locking/qspinlock_stat.h
@@ -136,10 +136,12 @@ static ssize_t qstat_read(struct file *file, char __user *user_buf,
 	}
 
 	if (counter == qstat_pv_hash_hops) {
-		u64 frac;
+		u64 frac = 0;
 
-		frac = 100ULL * do_div(stat, kicks);
-		frac = DIV_ROUND_CLOSEST_ULL(frac, kicks);
+		if (kicks) {
+			frac = 100ULL * do_div(stat, kicks);
+			frac = DIV_ROUND_CLOSEST_ULL(frac, kicks);
+		}
 
 		/*
 		 * Return a X.XX decimal number

^ permalink raw reply related	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2016-05-05  9:44 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-04-18  6:31 [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats Davidlohr Bueso
2016-04-18  6:31 ` [PATCH -tip 2/3] locking/pvqspinlock: Avoid double resetting of stats Davidlohr Bueso
2016-04-18 19:39   ` Waiman Long
2016-05-05  9:43   ` [tip:locking/core] " tip-bot for Davidlohr Bueso
2016-04-18  6:31 ` [PATCH -tip 3/3] locking/pvqspinlock: Robustify init_qspinlock_stat() Davidlohr Bueso
2016-04-18 16:16   ` Davidlohr Bueso
     [not found]     ` <CS1PR84MB0312315E60265F5E8972CACBF16C0@CS1PR84MB0312.NAMPRD84.PROD.OUTLOOK.COM>
2016-04-20  4:10       ` [PATCH -tip v2] " Davidlohr Bueso
2016-04-20  4:13         ` Davidlohr Bueso
2016-04-20  4:17       ` [PATCH -tip v3 3/3] " Davidlohr Bueso
2016-04-20 20:06         ` Waiman Long
2016-05-05  9:44         ` [tip:locking/core] " tip-bot for Davidlohr Bueso
2016-04-18 19:40   ` [PATCH -tip 3/3] " Waiman Long
2016-04-18 19:34 ` [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats Waiman Long
2016-04-19  9:34 ` [tip:locking/urgent] locking/pvqspinlock: Fix division by zero in qstat_read() tip-bot for Davidlohr Bueso

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.