'struct frame' uses two variables to store the sent timestamp - 'struct timeval' and jiffies. jiffies is used to avoid discrepancies caused by updates to system time. 'struct timeval' uses 32-bit representation for seconds which will overflow in year 2038.
This patch does the following: - Replace the use of 'struct timeval' and jiffies with struct timespec64, which provides a 64-bit seconds timestamp and is year 2038 safe. - timespec64 provides both long range (like jiffies) and high resolution (like timeval). Using monotonic time (ktime_get_ts64) instead of wall-clock time prevents any discrepancies caused by updates to system time.
The previous attempt at this patch and related discussion is here: https://lists.linaro.org/pipermail/y2038/2015-May/000245.html As noted previously, the existing code is 2038-safe, as it only uses a delta instead of absolute timestamps. However, this patch is part of a larger attempt to remove _all_ instances of 32-bit timekeeping from the kernel (time_t, struct timeval and struct timespec) since we do not know which of the many instances of their usage are buggy.
It is worth noting that there may be a slight performance penalty, due to timespec64 using nanoseconds instead of microseconds. The tsince_hr function contains a 32-bit multiply just like the existing code, but now it also has a 32-bit divide.
Suggested-by: Arnd Bergmann arnd@arndb.de Signed-off-by: Tina Ruchandani ruchandani.tina@gmail.com
-- Changes in v2: - Change use of ktime_t to timespec64 to avoid expensive 64-bit divide in ktime_us_delta --- drivers/block/aoe/aoe.h | 3 +-- drivers/block/aoe/aoecmd.c | 38 +++++++++----------------------------- 2 files changed, 10 insertions(+), 31 deletions(-)
diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h index 9220f8e..d587806 100644 --- a/drivers/block/aoe/aoe.h +++ b/drivers/block/aoe/aoe.h @@ -112,8 +112,7 @@ enum frame_flags { struct frame { struct list_head head; u32 tag; - struct timeval sent; /* high-res time packet was sent */ - u32 sent_jiffs; /* low-res jiffies-based sent time */ + struct timespec64 sent; ulong waited; ulong waited_total; struct aoetgt *t; /* parent target I belong to */ diff --git a/drivers/block/aoe/aoecmd.c b/drivers/block/aoe/aoecmd.c index d597e43..4df542f 100644 --- a/drivers/block/aoe/aoecmd.c +++ b/drivers/block/aoe/aoecmd.c @@ -398,8 +398,7 @@ aoecmd_ata_rw(struct aoedev *d)
skb = skb_clone(f->skb, GFP_ATOMIC); if (skb) { - do_gettimeofday(&f->sent); - f->sent_jiffs = (u32) jiffies; + ktime_get_ts64(&f->sent); __skb_queue_head_init(&queue); __skb_queue_tail(&queue, skb); aoenet_xmit(&queue); @@ -489,8 +488,7 @@ resend(struct aoedev *d, struct frame *f) skb = skb_clone(skb, GFP_ATOMIC); if (skb == NULL) return; - do_gettimeofday(&f->sent); - f->sent_jiffs = (u32) jiffies; + ktime_get_ts64(&f->sent); __skb_queue_head_init(&queue); __skb_queue_tail(&queue, skb); aoenet_xmit(&queue); @@ -499,32 +497,17 @@ resend(struct aoedev *d, struct frame *f) static int tsince_hr(struct frame *f) { - struct timeval now; + struct timespec64 now, delta; int n;
- do_gettimeofday(&now); - n = now.tv_usec - f->sent.tv_usec; - n += (now.tv_sec - f->sent.tv_sec) * USEC_PER_SEC; + ktime_get_ts64(&now); + delta = timespec64_sub(now, f->sent); + n = ((int32_t) (delta.tv_sec)) * USEC_PER_SEC + + delta.tv_nsec / NSEC_PER_USEC;
if (n < 0) n = -n;
- /* For relatively long periods, use jiffies to avoid - * discrepancies caused by updates to the system time. - * - * On system with HZ of 1000, 32-bits is over 49 days - * worth of jiffies, or over 71 minutes worth of usecs. - * - * Jiffies overflow is handled by subtraction of unsigned ints: - * (gdb) print (unsigned) 2 - (unsigned) 0xfffffffe - * $3 = 4 - * (gdb) - */ - if (n > USEC_PER_SEC / 4) { - n = ((u32) jiffies) - f->sent_jiffs; - n *= USEC_PER_SEC / HZ; - } - return n; }
@@ -589,7 +572,6 @@ reassign_frame(struct frame *f) nf->waited = 0; nf->waited_total = f->waited_total; nf->sent = f->sent; - nf->sent_jiffs = f->sent_jiffs; f->skb = skb;
return nf; @@ -633,8 +615,7 @@ probe(struct aoetgt *t)
skb = skb_clone(f->skb, GFP_ATOMIC); if (skb) { - do_gettimeofday(&f->sent); - f->sent_jiffs = (u32) jiffies; + ktime_get_ts64(&f->sent); __skb_queue_head_init(&queue); __skb_queue_tail(&queue, skb); aoenet_xmit(&queue); @@ -1474,8 +1455,7 @@ aoecmd_ata_id(struct aoedev *d)
skb = skb_clone(skb, GFP_ATOMIC); if (skb) { - do_gettimeofday(&f->sent); - f->sent_jiffs = (u32) jiffies; + ktime_get_ts64(&f->sent); }
return skb; -- 2.8.0.rc3.226.g39d4020
On 05/30/2016 04:01 AM, Tina Ruchandani wrote:
'struct frame' uses two variables to store the sent timestamp - 'struct timeval' and jiffies. jiffies is used to avoid discrepancies caused by updates to system time. 'struct timeval' uses 32-bit representation for seconds which will overflow in year 2038.
This patch does the following:
- Replace the use of 'struct timeval' and jiffies with struct timespec64,
which provides a 64-bit seconds timestamp and is year 2038 safe.
- timespec64 provides both long range (like jiffies) and high resolution
(like timeval). Using monotonic time (ktime_get_ts64) instead of wall-clock time prevents any discrepancies caused by updates to system time.
The previous attempt at this patch and related discussion is here: https://lists.linaro.org/pipermail/y2038/2015-May/000245.html As noted previously, the existing code is 2038-safe, as it only uses a delta instead of absolute timestamps. However, this patch is part of a larger attempt to remove _all_ instances of 32-bit timekeeping from the kernel (time_t, struct timeval and struct timespec) since we do not know which of the many instances of their usage are buggy.
It is worth noting that there may be a slight performance penalty, due to timespec64 using nanoseconds instead of microseconds. The tsince_hr function contains a 32-bit multiply just like the existing code, but now it also has a 32-bit divide.
Thank you.
I don't think it's a good idea to introduce a performance regression, since there's no safety concern for aoe.
If the concern is that we don't know which users of the 32-bit timestamp are buggy, but we *do* know that this use is OK, then a comment in the source code seems more appropriate.
On 05/30/2016 06:02 PM, Ed Cashin wrote:
On 05/30/2016 04:01 AM, Tina Ruchandani wrote:
'struct frame' uses two variables to store the sent timestamp - 'struct timeval' and jiffies. jiffies is used to avoid discrepancies caused by updates to system time. 'struct timeval' uses 32-bit representation for seconds which will overflow in year 2038.
...
I don't think it's a good idea to introduce a performance regression, since there's no safety concern for aoe.
If the concern is that we don't know which users of the 32-bit timestamp are buggy, but we *do* know that this use is OK, then a comment in the source code seems more appropriate.
... But after reviewing the previous discussion I think I should try to find people with 32-bit systems who can tell me whether they see a performance regression.
I'll do that and try to get an answer soon.
... But after reviewing the previous discussion I think I should try to find people with 32-bit systems who can tell me whether they see a performance regression.
I'll do that and try to get an answer soon.
Please consider this patch as a first-cut draft attempt to get the ball rolling on this again. It would be nice to take this to completion, given all the effort you and Arnd put into the discussion last here. I didn't get the chance to discuss this with Arnd before sending it out, and its likely that what I implemented is different from and sub-par compared to what he had mind. The discussion last year didn't seem to mention the need for a 32-bit divide, I ended up needing it here. Also, Arnd has some efficient interfaces in mind - introducing light-weight ktime_get_us() perhaps. I don't know how that would be done, and hence I tried the other approach he had suggested.
On Monday, May 30, 2016 6:15:50 PM CEST Tina Ruchandani wrote:
... But after reviewing the previous discussion I think I should try to find people with 32-bit systems who can tell me whether they see a performance regression.
I'll do that and try to get an answer soon.
Please consider this patch as a first-cut draft attempt to get the ball rolling on this again. It would be nice to take this to completion, given all the effort you and Arnd put into the discussion last here. I didn't get the chance to discuss this with Arnd before sending it out, and its likely that what I implemented is different from and sub-par compared to what he had mind. The discussion last year didn't seem to mention the need for a 32-bit divide, I ended up needing it here. Also, Arnd has some efficient interfaces in mind - introducing light-weight ktime_get_us() perhaps. I don't know how that would be done, and hence I tried the other approach he had suggested.
I'm looking at this again now:
- do_gettimeofday(&now); - n = now.tv_usec - f->sent.tv_usec; - n += (now.tv_sec - f->sent.tv_sec) * USEC_PER_SEC; + ktime_get_ts64(&now); + delta = timespec64_sub(now, f->sent); + n = ((int32_t) (delta.tv_sec)) * USEC_PER_SEC + + delta.tv_nsec / NSEC_PER_USEC;
and find them to be almost exactly equivalent in terms of operations: The difference between do_gettimeofday and ktime_get_ts64() is that the former has an extra constant 32-bit division by NSEC_PER_USEC, which is the same thing you are addingn in the new version here.
Also, the use of a 64-bit tv_secs variable is not visible in the end, because do_gettimeofday puts a local copy of that type on the stack, and you end up converting the seconds to 32-bit either way.
I don't think there is any difference in speed between reading realtime and monotonic time either, so your version just helps avoid the fallback to jiffies and the overflow without any downsides.
Arnd
On 06/03/2016 08:43 AM, Arnd Bergmann wrote: ...
The difference between do_gettimeofday and ktime_get_ts64() is that the former has an extra constant 32-bit division by NSEC_PER_USEC, which is the same thing you are addingn in the new version here.
Also, the use of a 64-bit tv_secs variable is not visible in the end, because do_gettimeofday puts a local copy of that type on the stack, and you end up converting the seconds to 32-bit either way.
I don't think there is any difference in speed between reading realtime and monotonic time either, so your version just helps avoid the fallback to jiffies and the overflow without any downsides.
All that sounds very good, thanks.
What's the target for this patch, Tina? Is there a FAQ or something I can read to see how the y2038 project generally operates?
What's the target for this patch, Tina? Is there a FAQ or something I can read to see how the y2038 project generally operates?
-- Ed
Hi Ed,
A dedicated page on this project is available on Linux Kernel Newbies where you can find updated info on drivers,filesystems,systemcalls and other portions of kernel affected by y2038. (http://kernelnewbies.org/y2038)
-
Arnd Bergmann -
Ed Cashin -
Tina Ruchandani