* [PATCH] libfc: fix seconds_since_last_reset miscalculation @ 2016-11-08 8:44 Johannes Thumshirn 2016-11-08 15:04 ` Bart Van Assche 0 siblings, 1 reply; 7+ messages in thread From: Johannes Thumshirn @ 2016-11-08 8:44 UTC (permalink / raw) To: Martin K . Petersen, James Bottomley Cc: Hannes Reinecke, Christoph Hellwig, Linux SCSI Mailinglist, Linux Kernel Mailinglist, Arnd Bergmann, Bart Van Assche, Johannes Thumshirn Commit 540eb1eef 'scsi: libfc: fix seconds_since_last_reset calculation' removed the use of 'struct timespec' from fc_get_host_stats(). This broke the output of 'fcoeadm -s' after kernel 4.8-rc1 as lport->boot_time - jiffies could become negative as in this example: $ cat /sys/class/fc_host/host8/statistics/seconds_since_last_reset 0x10624dd2f1977b4 Take this into account so /sys/class/fc_host/hostX/statistics/seconds_since_last_reset is sane again. Fixes: 540eb1eef ('scsi: libfc: fix seconds_since_last_reset calculation') Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> Tested-by: Holger Schranz <holger@fam-schranz.de> --- drivers/scsi/libfc/fc_lport.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c index 04ce7cf..475c0a9 100644 --- a/drivers/scsi/libfc/fc_lport.c +++ b/drivers/scsi/libfc/fc_lport.c @@ -304,11 +304,15 @@ struct fc_host_statistics *fc_get_host_stats(struct Scsi_Host *shost) unsigned int cpu; u64 fcp_in_bytes = 0; u64 fcp_out_bytes = 0; + unsigned long boot_time = lport->boot_time; 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; + if (boot_time > jiffies) + fc_stats->seconds_since_last_reset = (boot_time - jiffies) / HZ; + else + fc_stats->seconds_since_last_reset = (jiffies - boot_time) / HZ; for_each_possible_cpu(cpu) { struct fc_stats *stats; -- 1.8.5.6 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation 2016-11-08 8:44 [PATCH] libfc: fix seconds_since_last_reset miscalculation Johannes Thumshirn @ 2016-11-08 15:04 ` Bart Van Assche 2016-11-15 9:18 ` Johannes Thumshirn 0 siblings, 1 reply; 7+ messages in thread From: Bart Van Assche @ 2016-11-08 15:04 UTC (permalink / raw) To: Johannes Thumshirn, Martin K . Petersen, James Bottomley Cc: Hannes Reinecke, Christoph Hellwig, Linux SCSI Mailinglist, Linux Kernel Mailinglist, Arnd Bergmann On 11/08/16 00:45, Johannes Thumshirn wrote: > Commit 540eb1eef 'scsi: libfc: fix seconds_since_last_reset calculation' > removed the use of 'struct timespec' from fc_get_host_stats(). This broke the > output of 'fcoeadm -s' after kernel 4.8-rc1 as lport->boot_time - jiffies > could become negative as in this example: > > $ cat /sys/class/fc_host/host8/statistics/seconds_since_last_reset > 0x10624dd2f1977b4 > > Take this into account so > /sys/class/fc_host/hostX/statistics/seconds_since_last_reset is sane again. > > Fixes: 540eb1eef ('scsi: libfc: fix seconds_since_last_reset calculation') > Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de> > Tested-by: Holger Schranz <holger@fam-schranz.de> > --- > drivers/scsi/libfc/fc_lport.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/libfc/fc_lport.c b/drivers/scsi/libfc/fc_lport.c > index 04ce7cf..475c0a9 100644 > --- a/drivers/scsi/libfc/fc_lport.c > +++ b/drivers/scsi/libfc/fc_lport.c > @@ -304,11 +304,15 @@ struct fc_host_statistics *fc_get_host_stats(struct Scsi_Host *shost) > unsigned int cpu; > u64 fcp_in_bytes = 0; > u64 fcp_out_bytes = 0; > + unsigned long boot_time = lport->boot_time; > > 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; > + if (boot_time > jiffies) > + fc_stats->seconds_since_last_reset = (boot_time - jiffies) / HZ; > + else > + fc_stats->seconds_since_last_reset = (jiffies - boot_time) / HZ; > > for_each_possible_cpu(cpu) { > struct fc_stats *stats; Hello Johannes, 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; Bart. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation 2016-11-08 15:04 ` Bart Van Assche @ 2016-11-15 9:18 ` Johannes Thumshirn 2016-11-15 14:50 ` Bart Van Assche 0 siblings, 1 reply; 7+ messages in thread From: Johannes Thumshirn @ 2016-11-15 9:18 UTC (permalink / raw) To: Bart Van Assche Cc: Martin K . Petersen, James Bottomley, Hannes Reinecke, Christoph Hellwig, Linux SCSI Mailinglist, Linux Kernel Mailinglist, Arnd Bergmann On Tue, Nov 08, 2016 at 03:04:43PM +0000, Bart Van Assche wrote: [...] > Hello Johannes, > > 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. Byte, Johannes -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation 2016-11-15 9:18 ` Johannes Thumshirn @ 2016-11-15 14:50 ` Bart Van Assche 2016-11-15 15:05 ` Johannes Thumshirn 0 siblings, 1 reply; 7+ messages in thread From: Bart Van Assche @ 2016-11-15 14:50 UTC (permalink / raw) To: jthumshirn Cc: jejb, hch, linux-kernel, hare, martin.petersen, linux-scsi, arnd 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. Bart. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation 2016-11-15 14:50 ` Bart Van Assche @ 2016-11-15 15:05 ` Johannes Thumshirn 2016-11-16 16:58 ` Arnd Bergmann 0 siblings, 1 reply; 7+ messages in thread From: Johannes Thumshirn @ 2016-11-15 15:05 UTC (permalink / raw) To: Bart Van Assche Cc: jejb, hch, linux-kernel, hare, martin.petersen, linux-scsi, arnd 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. Byte, Johannes -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation 2016-11-15 15:05 ` Johannes Thumshirn @ 2016-11-16 16:58 ` Arnd Bergmann 2016-11-17 8:12 ` Johannes Thumshirn 0 siblings, 1 reply; 7+ messages in thread From: Arnd Bergmann @ 2016-11-16 16:58 UTC (permalink / raw) To: Johannes Thumshirn Cc: Bart Van Assche, jejb, hch, linux-kernel, hare, martin.petersen, linux-scsi 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation 2016-11-16 16:58 ` Arnd Bergmann @ 2016-11-17 8:12 ` Johannes Thumshirn 0 siblings, 0 replies; 7+ messages in thread From: Johannes Thumshirn @ 2016-11-17 8:12 UTC (permalink / raw) To: Arnd Bergmann Cc: Bart Van Assche, jejb, hch, linux-kernel, hare, martin.petersen, linux-scsi On Wed, Nov 16, 2016 at 05:58:14PM +0100, Arnd Bergmann wrote: > 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(). Yes I was going to resend this today, but I'm trapped in the s390 pit... Johannes -- Johannes Thumshirn Storage jthumshirn@suse.de +49 911 74053 689 SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg) Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-11-17 8:12 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-11-08 8:44 [PATCH] libfc: fix seconds_since_last_reset miscalculation Johannes Thumshirn 2016-11-08 15:04 ` Bart Van Assche 2016-11-15 9:18 ` Johannes Thumshirn 2016-11-15 14:50 ` Bart Van Assche 2016-11-15 15:05 ` Johannes Thumshirn 2016-11-16 16:58 ` Arnd Bergmann 2016-11-17 8:12 ` Johannes Thumshirn
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).