USB: usbmon: Remove timeval usage for timestamp

PATCH Superseded Tina Ruchandani 4 participants 6 messages May 5, 2015 USB

Share & tools

Tina Ruchandani

author

2015-05-05 06:14 UTC #0 lore raw

PATCH 1/1 USB: usbmon: Remove timeval usage for timestamp

'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 <[email protected]>

---
 drivers/usb/mon/mon_text.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

drivers/usb/mon/mon_text.c +5 -5

index ad40825..f5ea7c2 100644
@@ -8,7 +8,7 @@
 #include <linux/list.h>
 #include <linux/usb.h>
 #include <linux/slab.h>
-#include <linux/time.h>
+#include <linux/time64.h>
 #include <linux/export.h>
 #include <linux/mutex.h>
 #include <linux/debugfs.h>

@@ -176,12 +176,12 @@ static inline char mon_text_get_data(struct mon_event_text *ep, struct urb *urb,
 
 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;
 }

Signature · 1 line

2.2.0.rc0.207.ga3a616c

Arnd Bergmann

2015-05-05 09:24 UTC #1 lore raw

Quoted 9 lines · 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 <[email protected]>

Good description!

Quoted 4 lines

static inline unsigned int mon_get_timestamp(void) {
drivers/usb/mon/mon_text.c +1 -1
-	struct timeval tval;
+	struct timespec64 now;

drivers/usb/mon/mon_text.c static inline char mon_text_get_data(struct mon_event_text *ep, struct urb *urb, · line 186

Quoted 10 lines

drivers/usb/mon/mon_text.c +3 -3

 	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

Alan Stern

2015-05-05 16:08 UTC #2 lore raw

Quoted 8 lines · Arnd Bergmann

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)

Quoted 8 lines

{
	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 don't know of any programs that use the timestamp value, but if some do exist then the way overflow works should not be changed.

In my experience, the timestamps are used by humans reading the usbmon output. Overflow is rare, but when it does occur, a human finds it much easier to wrap from 4095.999999 seconds to 0.000000 than to wrap from 4294.967295 to 0.000000.

(Also, in the rare cases where usbmon timestamps have to be matched up with printk timestamps, it's easier to figure the relative offset when overflow affects only the seconds, not the fractions of a second.)

Quoted 4 lines

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.

That seems reasonable to me. The absolute values of the timestamps are practically meaningless; only the differences are important.

Alan Stern

Arnd Bergmann

2015-05-05 16:10 UTC #3 lore raw

Quoted 22 lines · On Tuesday 05 May 2015 10:59:32 Alan Stern wrote:

Quoted 8 lines · Arnd Bergmann
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)
Quoted 8 lines
{
	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 don't know of any programs that use the timestamp value, but if some do exist then the way overflow works should not be changed.

Quoted 8 lines

In my experience, the timestamps are used by humans reading the usbmon output. Overflow is rare, but when it does occur, a human finds it much easier to wrap from 4095.999999 seconds to 0.000000 than to wrap from 4294.967295 to 0.000000.
(Also, in the rare cases where usbmon timestamps have to be matched up with printk timestamps, it's easier to figure the relative offset when overflow affects only the seconds, not the fractions of a second.)

Ok, got it.

Quoted 7 lines

Quoted 4 lines
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.
That seems reasonable to me. The absolute values of the timestamps are practically meaningless; only the differences are important.

Right, although it would also mean you could no longer match up the microseconds with the printk timestamps.

Arnd

Pete Zaitcev

2015-05-05 16:09 UTC #4 lore raw

In reply to Arnd Bergmann

Quoted 8 lines · On Tue, 05 May 2015 11:24:16 +0200 Arnd Bergmann <[email protected]> wrote:

Quoted 6 lines · On Tuesday 05 May 2015 11:44:33 Tina Ruchandani wrote:
static inline unsigned int mon_get_timestamp(void) {
drivers/usb/mon/mon_text.c +1 -1
-	struct timeval tval;
+	struct timespec64 now;
 	unsigned int stamp;

Quoted 13 lines

drivers/usb/mon/mon_text.c static inline char mon_text_get_data(struct mon_event_text *ep, struct urb *urb, · line 186
Quoted 8 lines
drivers/usb/mon/mon_text.c +3 -3
-	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.

The intent was to create a rolling timestamp that is not too large. Remember that the text format is intended to be eyeballed. The wrap point could be anything. No consideration was given to what's intuitive.

Quoted 5 lines

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?
[...]
I also wonder if we should make the output use monotonic time instead

The only guarantee we give is that the time corresponds to microseconds. This is sometimes necessary to debug things in microntrollers in USB devices.

A monotonic time is fine.

One thing though, I object to Tina's new comment. It does not matter what now.tv_sec is. The comment is about the destination, that is the stamp. So, please don't change it, unless you change the type of the ep->tstamp.

Quoted 6 lines

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

Now you made the truncation explicit, so if anyhing it's less obvious. The code is fine, but at least add a comment to that effect, if you don't want to tack %4096000000 or %4000000000.

-- Pete

Arnd Bergmann

2015-05-05 16:10 UTC #5 lore raw

Quoted 25 lines · On Tuesday 05 May 2015 09:19:37 Pete Zaitcev wrote:

Quoted 8 lines · On Tue, 05 May 2015 11:24:16 +0200 Arnd Bergmann <[email protected]> wrote:
Quoted 6 lines · On Tuesday 05 May 2015 11:44:33 Tina Ruchandani wrote:
static inline unsigned int mon_get_timestamp(void) {
drivers/usb/mon/mon_text.c +1 -1
-	struct timeval tval;
+	struct timespec64 now;
 	unsigned int stamp;
Quoted 13 lines
drivers/usb/mon/mon_text.c static inline char mon_text_get_data(struct mon_event_text *ep, struct urb *urb, · line 186
Quoted 8 lines
drivers/usb/mon/mon_text.c +3 -3
-	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.

Quoted 3 lines

The intent was to create a rolling timestamp that is not too large. Remember that the text format is intended to be eyeballed. The wrap point could be anything. No consideration was given to what's intuitive.

Quoted 10 lines

Quoted 5 lines
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?
[...]
I also wonder if we should make the output use monotonic time instead
The only guarantee we give is that the time corresponds to microseconds. This is sometimes necessary to debug things in microntrollers in USB devices.

Quoted 5 lines

A monotonic time is fine.
One thing though, I object to Tina's new comment. It does not matter what now.tv_sec is. The comment is about the destination, that is the stamp. So, please don't change it, unless you change the type of the ep->tstamp.

The point of the patch that I thought she had made clear enough is to eliminate the use of 'struct timeval' from one of the 236 files that currently use it, to make it clearer what the remaining problems are.

Quoted 10 lines

Quoted 6 lines
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
Now you made the truncation explicit, so if anyhing it's less obvious. The code is fine, but at least add a comment to that effect, if you don't want to tack %4096000000 or %4000000000.

As Alan Stern commented, we should probably leave the wrap point alone, and adding "% 4096000000" (actually do_div() or ktime_divns()) on a 64-bit value is more expensive than the current code, so it's probably best if Tina leaves the existing logic and just changes it to using ktime_get_ts64() for the monotonic time, as well as trying to be more explicit in the changelog about the intention.

Arnd