[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 5/8] timers: prepare the code for future races i
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp |
Date: |
Tue, 08 Oct 2013 19:10:40 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130923 Thunderbird/17.0.9 |
Il 08/10/2013 19:08, Alex Bligh ha scritto:
>
> On 8 Oct 2013, at 17:56, Paolo Bonzini wrote:
>
>>> Arguably the patch could document why removing the check for deadline >
>>> INT32_MAX
>>> (the bug for bug compatibility) is safe, as I couldn't entirely convince
>>> myself it
>>> was, mostly because I couldn't see why it was doing it in the first place.
>>
>> I couldn't convince myself that it is _not_ safe :) and it made the code
>> more complicated. As soon as a deadline appears, qemu_clock_warp() will
>> be called again and update the icount_warp_timer.
>>
>> Ok to move that to a separate patch?
>
> To be honest I put it in out of an abundance of caution. I am very
> tempted to take it out and see what breaks. As far as I can see all
> the time arithmetic is not int64_t; perhaps this was not always the
> case. I was more checking you hadn't removed it by accident. Perhaps
> just add "special casing deadlines > INT32_MAX removed as all
> arithmetic now 64 bit".
>
> There is another offender in tcg_cpu_exec.
>
> deadline = qemu_clock_deadline_ns_all(QEMU_CLOCK_VIRTUAL);
>
> /* Maintain prior (possibly buggy) behaviour where if no deadline
> * was set (as there is no QEMU_CLOCK_VIRTUAL 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;
> }
>
> count = qemu_icount_round(deadline);
> qemu_icount += count;
> decr = (count > 0xffff) ? 0xffff : count;
> count -= decr;
> env->icount_decr.u16.low = decr;
> env->icount_extra = count;
>
> This implies that qemu_icount_round() cannot take a 64 bit int.
>
> static int64_t qemu_icount_round(int64_t count)
> {
> return (count + (1 << icount_time_shift) - 1) >> icount_time_shift;
> }
>
> I would have thought it better if qemu_icount_round just
> dealt sensibly with negative parameters.
>
I'll clean that up separately.
Thanks,
Paolo
- Re: [Qemu-devel] [PATCH 2/8] timers: add timer_mod_anticipate and timer_mod_anticipate_ns, (continued)
[Qemu-devel] [PATCH 3/8] timers: use cpu_get_icount() directly, Paolo Bonzini, 2013/10/08
[Qemu-devel] [PATCH 4/8] timers: reorganize icount_warp_rt, Paolo Bonzini, 2013/10/08
[Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp, Paolo Bonzini, 2013/10/08
[Qemu-devel] [PATCH 6/8] timers: introduce cpu_get_clock_locked, Paolo Bonzini, 2013/10/08
[Qemu-devel] [PATCH 7/8] timers: document (future) locking rules for icount, Paolo Bonzini, 2013/10/08
[Qemu-devel] [PATCH 8/8] timers: make icount thread-safe, Paolo Bonzini, 2013/10/08
Re: [Qemu-devel] [PATCH 0/8] Make icount thread-safe, Andreas Färber, 2013/10/08