[ovs-dev] [PATCH] datapath: Don't query time for every packet.

[Ben Pfaff]

Thanks, Jesse.  A 3-4% CPU improvement is great.

On Wed, Jul 21, 2010 at 07:36:15PM -0700, Jesse Gross wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index eb260e3..0c39871 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -905,13 +905,28 @@ error:
>  
>  static void get_stats(struct sw_flow *flow, struct odp_flow_stats *stats)
>  {
> -	if (flow->used.tv_sec) {
> -		stats->used_sec = flow->used.tv_sec;
> -		stats->used_nsec = flow->used.tv_nsec;
> +	if (flow->used) {
> +		struct timespec now, delta, used;
> +
> +		ktime_get_ts(&now);
> +
> +		/* Deal with jiffie roll over.  Jiffies is always greater than
> +		 * flow->used so if it isn't that means that it has rolled
> +		 * over but flow->used hasn't. */
> +		if (jiffies > flow->used)
> +			jiffies_to_timespec(jiffies - flow->used, &delta);
> +		else
> +			jiffies_to_timespec(ULONG_MAX - flow->used + jiffies, &delta);

The jiffies > flow->used case is obviously correct.

I think that the jiffies <= flow->used case is off by 1.  Suppose that
jiffies == flow->used, for example: then this expression results in
ULONG_MAX, not 0.

I actually think that plain "jiffies - flow->used" is correct for all
cases.  I tried a couple of examples on paper and it looks correct
there.  Also, it's well known that (head - tail) % queue_size is always
the correct distance between the head and the tail in a circular queue,
for power-of-2 queue_size, and that's what we have here except that
queue_size is ULONG_MAX + 1 and therefore taking that as modulo is a
no-op.

> +		used = timespec_sub(now, delta);

'now' is an absolute time.
I think that 'delta' is an absolute time.
'used' is supposed to be an absolute time (on the monotonic clock),
too.
But I think that this subtraction will yield a duration relative to the
current time.  I don't think that's correct.

> +		stats->used_sec = used.tv_sec;
> +		stats->used_nsec = used.tv_nsec;
>  	} else {
>  		stats->used_sec = 0;
>  		stats->used_nsec = 0;
>  	}
> +
>  	stats->n_packets = flow->packet_count;
>  	stats->n_bytes = flow->byte_count;
>  	stats->ip_tos = flow->ip_tos;