Re: [Qemu-block] [Qemu-devel] [PATCH v1 02/26] qemu-io-cmds: use clock_gettime for benchmarking

[Richard Henderson]

On 5/30/19 5:15 AM, Alex Bennée wrote:
> -static struct timeval tsub(struct timeval t1, struct timeval t2)
> +static struct timespec tsub(struct timespec t1, struct timespec t2)
>  {
> -    t1.tv_usec -= t2.tv_usec;
> -    if (t1.tv_usec < 0) {
> -        t1.tv_usec += 1000000;
> +    t1.tv_nsec -= t2.tv_nsec;
> +    if (t1.tv_nsec < 0) {
> +        t1.tv_nsec += 1000000000;

Rather than counting zeros, should we move or copy NANOSECONDS_PER_SECOND?

> +    double time = (double)tv.tv_sec + ((double)tv.tv_nsec / 1000000000.0);

On that same vein, I'll note this can also be spelled "1e9".
Also, the casts to double are superfluous, once we have one FP constant.

> +static void timestr(struct timespec *tv, char *ts, size_t size, int format)
>  {
> -    double usec = (double)tv->tv_usec / 1000000.0;
> +    double nsec = (double)tv->tv_nsec / 1000000000.0;

Similarly.

>  
>      if (format & TERSE_FIXED_TIME) {
>          if (!HOURS(tv->tv_sec)) {
>              snprintf(ts, size, "%u:%02u.%02u",
>                      (unsigned int) MINUTES(tv->tv_sec),
>                      (unsigned int) SECONDS(tv->tv_sec),
> -                    (unsigned int) (usec * 100));
> +                    (unsigned int) (nsec * 100000));

The multiplier here is wrong.

The existing formatting here is bonkers, which doesn't help.  Why should we
convert to double, divide into a fraction of a second, shift the decimal place,
and truncate conversion to integer?

The formatting should clearly be

  snprintf(ts, size, "%u:%05.2f",
           (unsigned int) MINUTES(tv->tv_sec),
           SECONDS(tv->tv_sec) + nsec);

so that the complete seconds plus fraction of a second is rounded to two digits
after the decimal point, and is left-padded with 0's so that the entire number
fits in 5 characters, not forgetting the decimal point itself (i.e. 00.00).

Likewise to the other two occurrences in this function.


r~