[PATCH] libfc: fix seconds_since_last_reset miscalculation

[PATCH] libfc: fix seconds_since_last_reset miscalculation

[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

Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation

[Bart Van Assche]

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.

Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation

[Johannes Thumshirn]

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

Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation

[Bart Van Assche]

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.

Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation

[Johannes Thumshirn]

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

Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation

[Arnd Bergmann]

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

Re: [PATCH] libfc: fix seconds_since_last_reset miscalculation

[Johannes Thumshirn]

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