From: Arnd Bergmann <arnd@arndb.de>
To: Tina Ruchandani <ruchandani.tina@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org, Pete Zaitcev <zaitcev@redhat.com>
Subject: Re: [PATCH] USB: usbmon: Remove timeval usage for timestamp
Date: Tue, 05 May 2015 11:24:16 +0200
Message-ID: <4936805.OkgPExQ11v@wuerfel>
User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; )
In-Reply-To: <20150505061433.GA4690@tinar>
References: <20150505061433.GA4690@tinar>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 2295
Lines: 56

On Tuesday 05 May 2015 11:44:33 Tina Ruchandani wrote:
> 'struct timeval' uses 32-bits for its seconds field and will overflow in
> the year 2038 and beyond. This patch replaces the usage of 'struct timeval'
> in mon_get_timestamp() with timespec64 which uses a 64-bit seconds field
> and is y2038-safe. mon_get_timestamp() truncates the timestamp at 4096 seconds,
> so the correctness of the code is not affected. This patch is part of a larger
> attempt to remove instances of struct timeval and other 32-bit timekeeping
> (time_t, struct timespec) from the kernel.
> 
> Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com>

Good description!

>  static inline unsigned int mon_get_timestamp(void)
>  {
> -	struct timeval tval;
> +	struct timespec64 now;
>  	unsigned int stamp;
>  
> -	do_gettimeofday(&tval);
> -	stamp = tval.tv_sec & 0xFFF;	/* 2^32 = 4294967296. Limit to 4096s. */
> -	stamp = stamp * 1000000 + tval.tv_usec;
> +	getnstimeofday64(&now);
> +	stamp = now.tv_sec & 0xFFF;  /* now.tv_sec is 64-bit. Limit to 4096s */
> +	stamp = stamp * USEC_PER_SEC + now.tv_nsec / NSEC_PER_USEC;
>  	return stamp;
>  }

Your conversion looks entirely correct, but the original code is a bit
odd here as it does not use the entire range of the 32-bit microsecond
value, and counts from 0 to 4096000000us instead of the more intuitive
0 to 4294967296 us range before wrapping around.

If we change the code to 

static inline unsigned int mon_get_timestamp(void)
{
	return ktime_to_us(ktime_get_real());
}

it might be more obvious what is going on, but it would slightly change
the output in the debugfs file to use the full range. Do we know what
behavior is expected by normal user space here? Pete Zaitcev submitted
a patch for this behavior in 2010, he might remember something about it.

I also wonder if we should make the output use monotonic time instead
of real time (so change it to ktime_get_ts64() or ktime_get()). The effect
of that would be to keep the time ticking monotonically across a concurrent
settimeofday() call.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/