[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: |
Alex Bligh |
Subject: |
Re: [Qemu-devel] [PATCH 5/8] timers: prepare the code for future races in calling qemu_clock_warp |
Date: |
Tue, 8 Oct 2013 18:08:29 +0100 |
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.
--
Alex Bligh
- 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