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@xxxxxxx                                +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