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
