Message-ID: <571536C5.70907@hpe.com>
Date: Mon, 18 Apr 2016 15:34:29 -0400
From: Waiman Long <waiman.long@hpe.com>
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:10.0.12) Gecko/20130109 Thunderbird/10.0.12
MIME-Version: 1.0
To: Davidlohr Bueso <dave@stgolabs.net>
CC: <mingo@kernel.org>, <peterz@infradead.org>, <linux-kernel@vger.kernel.org>,
        Davidlohr Bueso <dbueso@suse.de>
Subject: Re: [PATCH -tip 1/3] locking/pvqspinlock: Fix div by 0 in qstats
References: <1460961103-24953-1-git-send-email-dave@stgolabs.net>
In-Reply-To: <1460961103-24953-1-git-send-email-dave@stgolabs.net>
Content-Type: text/plain; charset="ISO-8859-1"; format=flowed
Content-Transfer-Encoding: 7bit
SpamDiagnosticOutput: 1:23
SpamDiagnosticMetadata: NSPM
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Apr 2016 19:34:40.0444 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CS1PR84MB0312
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 3733
Lines: 73

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