Re: [PATCH] aoe: Use 64-bit timestamp in frame

[Ed Cashin <ed.cashin@acm.org>]

First, thanks for the patch.  I do appreciate the attempt to simplify
this part of the driver, but I don't think that this patch is good to merge.

I'll make some comments inline below.

On 05/10/2015 10:35 PM, 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.

The comment in the deleted lines below mentions the fact that the
overflow does not matter for calculating rough-grained deltas in time.
So there is no problem in 2038 or on systems with the clock set to 2038
accidentally.

> This patch does the following:
> - Replace the use of 'struct timeval' and jiffies with ktime_t, which
> is a 64-bit timestamp and is year 2038 safe.
> - ktime_t provides both long range (like jiffies) and high resolution
> (like timeval). Using ktime_get (monotonic time) instead of wall-clock
> time prevents any discprepancies caused by updates to system time.

But the patch only changes the struct frame data.  The aoe driver
only has the struct frame for an incoming AoE response when that
response is "expected".  If the response comes in a bit late, the frame
may have already been used for a new command.

You can see that in aoecmd_ata_rsp when getframe_deferred returns
NULL and tsince is called instead of tsince_hr.

In that case, there is still information about the timing embedded in
the AoE tag.  The send time in jiffies is a rough-grained record of the
send time, and it's extracted from the tag.  For these "unexpected"
responses, this timing information can improve performance significantly
without introducing extra overhead or risk.

I don't think the patch considers this aspect of the way the round trip
time is calculated, and I don't think the primary motivation is justified
(if that's 2038 safety, which we have already).

Simplifying it would be nice, but it would be difficult to thoroughly test
all of the performance implications.  There are still people using 32-bit
systems, for example.

>
> Signed-off-by: Tina Ruchandani <ruchandani.tina@gmail.com>
> ---
>   drivers/block/aoe/aoe.h    |  3 +--
>   drivers/block/aoe/aoecmd.c | 36 +++++++-----------------------------
>   2 files changed, 8 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/block/aoe/aoe.h b/drivers/block/aoe/aoe.h
> index 9220f8e..4582b3c 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 */
> +	ktime_t 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 422b7d8..7f78780 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;
> +		f->sent = ktime_get();
>   		__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;
> +	f->sent = ktime_get();
>   	__skb_queue_head_init(&queue);
>   	__skb_queue_tail(&queue, skb);
>   	aoenet_xmit(&queue);
> @@ -499,32 +497,15 @@ resend(struct aoedev *d, struct frame *f)
>   static int
>   tsince_hr(struct frame *f)
>   {
> -	struct timeval now;
> +	ktime_t now;
>   	int n;
>   
> -	do_gettimeofday(&now);
> -	n = now.tv_usec - f->sent.tv_usec;
> -	n += (now.tv_sec - f->sent.tv_sec) * USEC_PER_SEC;
> +	now = ktime_get();
> +	n = ktime_to_us(ktime_sub(now, f->sent));
>   
>   	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 +570,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 +613,7 @@ probe(struct aoetgt *t)
>   
>   	skb = skb_clone(f->skb, GFP_ATOMIC);
>   	if (skb) {
> -		do_gettimeofday(&f->sent);
> -		f->sent_jiffs = (u32) jiffies;
> +		f->sent = ktime_get();
>   		__skb_queue_head_init(&queue);
>   		__skb_queue_tail(&queue, skb);
>   		aoenet_xmit(&queue);
> @@ -1474,8 +1453,7 @@ aoecmd_ata_id(struct aoedev *d)
>   
>   	skb = skb_clone(skb, GFP_ATOMIC);
>   	if (skb) {
> -		do_gettimeofday(&f->sent);
> -		f->sent_jiffs = (u32) jiffies;
> +		f->sent = ktime_get();
>   	}
>   
>   	return skb;