Re: [Qemu-block] [PATCH] Replacing (and removing) get_ticks_per_sec() function with NANOSECONDS_PER_SECOND Signed-off-by: Rutuja Shah <address@hidden>

[Paolo Bonzini]

On 11/03/2016 12:44, Christian Borntraeger wrote:
>> -    s->tick_offset_vmstate = s->tick_offset + delta / get_ticks_per_sec();
>> > +    s->tick_offset_vmstate = s->tick_offset + delta / 
>> > NANOSECONDS_PER_SECOND;
> [...]
> 
> While technically correct, I do not like these changes. The interfaces expect 
> "ticks",
> and the fact that this happens to be a nanosecond does not help regarding 
> readability.

Actually, I think usage of "tick" in this file is just for historical
reasons.

A long time ago QEMU had a timer that ticked every millisecond and the
timers were not in nanosecond precision; rather they used
cpu_get_ticks() which was tied to the TSC.  This was changed in 2006,
and since then the number of ticks per second was changed to a constant
10^9.

Usage of "tick" to represent nanoseconds is definitely the exception.
It's much more common to have code like:

            now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
            ...
                ticks = muldiv64(now, 32768, get_ticks_per_sec());

(hw/arm/omap1.c) that converts nanoseconds to 32768 Hz "ticks", and
using get_ticks_per_sec() is very confusing in the latter example.  You
would think that get_ticks_per_sec() is in the numerator (second
argument of muldiv64), not in the denominator!

Most of the time, get_ticks_per_sec()'s result end up being massaged in
some formula and passed to timer_mod which expects nanoseconds, such as

    timer_mod(fdctrl->result_timer,
              qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
              (get_ticks_per_sec() / 50));

where you have nanoseconds on the left of the plus sign and "ticks" on
the right.  NANOSECONDS_PER_SECOND makes it obvious that the timer will
expire in 1/50th of a second.

> If - for some reason - we want to replace a function with a MACRO, then
> please introduce TICKS_PER_SECOND which just feels better when reading the 
> code.

This would be wrong, for the reason mentioned above.

Paolo