Re: [PATCH] gettime: minimize integer division

[Jens Axboe <axboe@kernel.dk>]

On 2012-12-20 18:18, Sam Bradshaw wrote:
> On 12/20/2012 12:14 AM, Jens Axboe wrote:
> 
>> On 2012-12-20 01:52, Sam Bradshaw wrote:
>>>
>>> This patch generally converts a division to a subtraction in fio_gettime().
>>>
>>> Shows ~1% better iops with synthetic benchmarking at roughly the same cpu 
>>> time spent in fio_gettime().
>>>
>>> Signed-off-by: Sam Bradshaw <sbradshaw@micron.com>
>>>
>>> diff --git a/gettime.c b/gettime.c
>>> index 248f146..e2a6241 100644
>>> --- a/gettime.c
>>> +++ b/gettime.c
>>> @@ -163,17 +163,23 @@ void fio_gettime(struct timeval *tp, void fio_unused *caller)
>>>  		}
>>>  #ifdef ARCH_HAVE_CPU_CLOCK
>>>  	case CS_CPUCLOCK: {
>>> -		unsigned long long usecs, t;
>>> +		unsigned long long usecs, t, delta = 0;
>>>
>>>  		t = get_cpu_clock();
>>>  		if (tv && t < tv->last_cycles) {
>>>  			dprint(FD_TIME, "CPU clock going back in time\n");
>>>  			t = tv->last_cycles;
>>> -		} else if (tv)
>>> +		} else if (tv) {
>>> +			if (tv->last_tv_valid)
>>> +				delta = t - tv->last_cycles;
>>>  			tv->last_cycles = t;
>>> +		}
>>>
>>>  		usecs = t / cycles_per_usec;
>>> -		tp->tv_sec = usecs / 1000000;
>>> +		if (delta > 1000000)
>>> +			tp->tv_sec = tv->last_tv.tv_sec;
>>> +		else
>>> +			tp->tv_sec = usecs / 1000000;
>>
>> Shouldn't that be delta < 1000000? What am I missing? If the diff is
>> more than 1M usecs, then do the division. If not, we can reuse the
>> seconds from the last one.
>>
> 
> 
> 
> Yes, my bad.  Correct patch below.
> 
> diff --git a/gettime.c b/gettime.c
> index 035d275..89f3e27 100644
> --- a/gettime.c
> +++ b/gettime.c
> @@ -168,17 +168,23 @@ void fio_gettime(struct timeval *tp, void
> fio_unused *caller)
>  		}
>  #ifdef ARCH_HAVE_CPU_CLOCK
>  	case CS_CPUCLOCK: {
> -		unsigned long long usecs, t;
> +		unsigned long long usecs, t, delta = 0;
> 
>  		t = get_cpu_clock();
>  		if (tv && t < tv->last_cycles) {
>  			dprint(FD_TIME, "CPU clock going back in time\n");
>  			t = tv->last_cycles;
> -		} else if (tv)
> +		} else if (tv) {
> +			if (tv->last_tv_valid)
> +				delta = t - tv->last_cycles;
>  			tv->last_cycles = t;
> +		}
> 
>  		usecs = t / cycles_per_usec;
> -		tp->tv_sec = usecs / 1000000;
> +		if (delta && delta < 1000000)
> +			tp->tv_sec = tv->last_tv.tv_sec;
> +		else
> +			tp->tv_sec = usecs / 1000000;
>  		tp->tv_usec = usecs % 1000000;
>  		break;
>  		}

I was thinking about this... Is it actually guarenteed to work. If
tv->last_tv.tv_usec is eg 900,000, you'd only need a 100k usec diff to
need to wrap, not 1000k. And since this is about avoiding costly divs,
since we know the number of cycles last time, it might make more sense
to just do the single div to go from cycles to usecs, then add that to
the tv->last_tv.

-- 
Jens Axboe