Re: [Qemu-devel] [RFC] [PATCHv10 19/31] aio / timers: Use all timerlists in icount warp calculations

[Alex Bligh]

On 15 Aug 2013, at 13:30, Stefan Hajnoczi wrote:

> On Sun, Aug 11, 2013 at 05:43:13PM +0100, Alex Bligh wrote:
>> @@ -314,7 +314,18 @@ void qemu_clock_warp(QEMUClock *clock)
>>     }
>> 
>>     vm_clock_warp_start = qemu_get_clock_ns(rt_clock);
>> -    deadline = qemu_clock_deadline(vm_clock);
>> +    /* We want to use the earliest deadline from ALL vm_clocks */
>> +    deadline = qemu_clock_deadline_ns_all(vm_clock);
>> +
>> +    /* Maintain prior (possibly buggy) behaviour where if no deadline
>> +     * was set (as there is no vm_clock timer) or it is more than
>> +     * INT32_MAX nanoseconds ahead, we still use INT32_MAX
>> +     * nanoseconds.
>> +     */
>> +    if ((deadline < 0) || (deadline > INT32_MAX)) {
>> +        deadline = INT32_MAX;
>> +    }
>> +
> 
> I see no value in a spurious wakeup if no deadline was set, but it's
> harmless.
> 
> As for overflow, I don't really understand how INT32_MAX prevents
> overflow.  If the base timer value we're adding to is already huge then
> INT32_MAX could still overflow it.

This is my understanding. I don't think we need to worry about overflowing
int64_t.

>>     if (deadline > 0) {
>>         /*
>>          * Ensure the vm_clock proceeds even when the virtual CPU goes to
>> @@ -333,8 +344,8 @@ void qemu_clock_warp(QEMUClock *clock)
>>          * packets continuously instead of every 100ms.
>>          */
>>         qemu_mod_timer(icount_warp_timer, vm_clock_warp_start + deadline);
>> -    } else {
>> -        qemu_notify_event();
>> +    } else if (deadline == 0) {
>> +        qemu_clock_notify(vm_clock);
> 
> It seems this change is just for clarity: deadline can either be >0 or
> 0?  I think a plain else statement is clearer and doesn't leave you
> wondering in what case this qemu_clock_notify() isn't taken.

The change from qemu_notify_event() to qemu_clock_notify(vm_clock)
is needed anyway.

The change to "... else if (deadline == 0)" is in case the spurious
wakeup stuff above gets excised (which I think it ought to be),
in which case deadline could be smaller than zero and no wakeup
is necessary.

-- 
Alex Bligh