Re: [PATCH] Replacing (and removing) get_ticks_per_sec() function with NANOSECONDS_PER_SECOND Signed-off-by: Rutuja Shah <rutu.shah.26@gmail.com>

[Paolo Bonzini <pbonzini@redhat.com>]

On 11/03/2016 13:12, Christian Borntraeger wrote:
> On 03/11/2016 01:07 PM, Paolo Bonzini wrote:
>>
>>
>> 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.
> 
> So in essence the patch is ok and we should try to get rid of the "tick" word in future
> patches?

Not necessarily.  The patch stops overloading the word "tick", so that
"tick" means "whatever the timer device counts".

In fact, you and I were both confused by the appearance of the word
"tick" in get_ticks_per_sec().  In the above line of code, the unit of
measure of delta is nanoseconds, because it is computed like this on the
line before:

    int64_t delta = qemu_clock_get_ns(rtc_clock)
			- qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);

therefore s->tick_offset_vmstate is *seconds*

       delta   /  get_ticks_per_sec()
       ^^^^^      ^^^^^^^^^^^^^^^^^^^
    nanosecond / (nanosecond / second)  = second

The code after Rutu's patch is easier to understand because "tick" now
means uniformly "whatever a PL031 counts" -- which is seconds.  Before,
"tick" on the left of the plus meant seconds and tick on the right of
the plus meant nanoseconds.  And I got confused *despite being the
author of that line* (commit b0f2663, "pl031: switch clock base to
rtc_clock", 2012-03-30).


Paolo