From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932493AbcKPQ67 (ORCPT ); Wed, 16 Nov 2016 11:58:59 -0500 Received: from mout.kundenserver.de ([212.227.126.130]:56350 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753444AbcKPQ64 (ORCPT ); Wed, 16 Nov 2016 11:58:56 -0500 From: Arnd Bergmann To: Johannes Thumshirn Cc: Bart Van Assche , "jejb@linux.vnet.ibm.com" , "hch@infradead.org" , "linux-kernel@vger.kernel.org" , "hare@suse.de" , "martin.petersen@oracle.com" , "linux-scsi@vger.kernel.org" Subject: Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation Date: Wed, 16 Nov 2016 17:58:14 +0100 Message-ID: <4107905.14NieVZ4Uo@wuerfel> User-Agent: KMail/5.1.3 (Linux/4.4.0-34-generic; KDE/5.18.0; x86_64; ; ) In-Reply-To: <20161115150531.73jvxfclzzbtpfal@linux-x5ow.site> References: <1478594694-98847-1-git-send-email-jthumshirn@suse.de> <1479221415.3426.3.camel@sandisk.com> <20161115150531.73jvxfclzzbtpfal@linux-x5ow.site> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:TT9fZLG+kLJFKuN7792jnAh4LEM6cSZ2UGkRFFj6EbGOazFstgU L1kh6DcrKNOS6gZbLpB7y6Pp3205+K1nykg/Wi07AUWTLJQXLd5nV2+4qKKnvOXqIvyfIPa UiXZ+Aetgq9fQI8X5YuaZ5klqnwq7D8LWIgPYNZ24mJ3FFKS3AJ1HL9xTELaFbjYatp75vZ YE8P/Nv+ecta5Ic/tqVsA== X-UI-Out-Filterresults: notjunk:1;V01:K0:y3rTWSSKbOc=:oUKOYP/pt1h+2Qm4X0GXB0 xBN+zTbRQ7Sg6CfEj5DxC5xe5QPOyB9KSbMsS/nQgXDA6xgVgrY+jNvMYdX+bHCuY2X1m1z4N YcLNKqf0kx9a1KBMKzijD8NlHvwQCnvJqlrKzxV9MyPZJWsGBJQQgrE7VY4n6H5hK1HxP0L8i m4Bf4folRl6Ss+KIQgdiB2ZViU4fFK4F0Xcg3eTB2/cdL/puRpKBnJjF9KsSB+cTnaWsIQlC9 c4NfvANCPVo8KhMXOUGWXybkC/FlL7tHJJjZGojbtTDWBQFhAtfoOGw3WfEDjNmE3i1QEWexV Gk1w7qs+MOfBhSGvdMjx70E8C0KW3rL5P/yN3miP1QWW8gUHuyLt1nEPHnLP9BF+AwvzWbR1Q WvxdJOeeCfMEVmwO9C3oK8MRBtEYsEcz+hKXrZ5PzKZPRZoDqu7ww8aAJKaMLUUSulFFrQ1Ro pUBqM7wmJU3Wq4JYcqHtUbxkyYwUYGjKjH8jXwYLULhDj3Y4kWy5c3qr5Z5Miig/0WHSilLA/ 1ALpkQwQR/Hk/upm2/ba3TSgk/BncfAURGcqwAeavQTc31QUXw78ZCBrBMliCXnGc6a948c4O bbtOSfPh5U+XBaBLTNcOag4nuux3GQpb+/x1oWTEu3chrPmp2DT+4euEfxIKrTFNmSHPctdGw ibBQGQ6OFhtzOg2dGufCSIrka0XlZKxjV91RaPVsuKdtkXjsSLPte30ZdhMULYy5QyXsa5/nX odfDt4Ixq2wd+brM Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tuesday, November 15, 2016 4:05:31 PM CET Johannes Thumshirn wrote: > On Tue, Nov 15, 2016 at 02:50:17PM +0000, Bart Van Assche wrote: > > On Tue, 2016-11-15 at 10:18 +0100, Johannes Thumshirn wrote: > > > On Tue, Nov 08, 2016 at 03:04:43PM +0000, Bart Van Assche wrote: > > > > I think the above code will miscalculate seconds_since_last_reset > > > > if > > > > 'jiffies' wraps around after an lport has been created and before > > > > seconds_since_last_reset is computed. Shouldn't > > > > seconds_since_last_reset > > > > be computed as follows? > > > > > > > > fc_stats->seconds_since_last_reset = (jiffies - boot_time) / > > > > HZ; > > > > > > But what happens when jiffies - boot_time becomes negative? Then we > > > reintroduce the bug again and have 'fcoeadm -s' show weird values. > > > > Hello Johannes, > > > > If your concern is about 'jiffies' wrapping around on 32-bit systems > > then you should use get_jiffies_64(). get_jiffies_64() - boot_time > > can't become negative. It namely takes several million years before a > > 64-bit HZ counter wraps around. > > You're right. I'll respin using get_jiffies_64() and resent once it is tested. Sorry for the bug I introduced and for not noticing this thread earlier. Looking at this again now, I think it's clear that the bug was simply mixing up the left and right side of the subtraction, the simple fix would be diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c index 2d3133f62463..fe643f2195f0 100644 --- a/drivers/scsi/libfc/fc_lport.c +++ b/drivers/scsi/libfc/fc_lport.c @@ -311,7 +311,7 @@ struct fc_host_statistics *fc_get_host_stats(struct Scsi_Host *shost) fc_stats = &lport->host_stats; memset(fc_stats, 0, sizeof(struct fc_host_statistics)); - fc_stats->seconds_since_last_reset = (lport->boot_time - jiffies) / HZ; + fc_stats->seconds_since_last_reset = (jiffies - lport->boot_time) / HZ; for_each_possible_cpu(cpu) { struct fc_stats *stats; This works correctly across jiffies overflows, as long as there is at least one reset for every jiffies overflow (49 days or more). If we can have longer times between resets, then we could either use get_jiffies_64() or ktime_get_seconds(). The latter would only need a 32-bit variable (overflow is after 136 years). Arnd