[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 11/19] use a bottom half to run timers
From: |
Marcelo Tosatti |
Subject: |
Re: [Qemu-devel] [PATCH 11/19] use a bottom half to run timers |
Date: |
Thu, 24 Dec 2009 09:25:18 -0200 |
User-agent: |
Mutt/1.5.19 (2009-01-05) |
On Thu, Dec 24, 2009 at 11:27:06AM +0100, Paolo Bonzini wrote:
> On 12/23/2009 07:37 PM, Marcelo Tosatti wrote:
>> You should probably make sure the bh handling is signal safe. Perhaps
>> use atomic test-and-set for bh->schedule on qemu_bh_poll, etc...
>
> The worst thing that can happen is that qemu_bh_poll misses the alarm
> bottom half, and tcg_cpu_exec exits immediately because
> qemu_alarm_pending() returns true. This is the same that would happen
> without my patches, if the signal was raised during qemu_bh_poll which
> is after the timers were processed.
OK.
>> Also there's a similar problem with the expired flag
>>
>>> > + /* rearm timer, if not periodic */
>>> > + if (t->expired) {
>>> > + t->expired = 0;
>>> > + qemu_rearm_alarm_timer(t);
>>> > + }
>> (it was there before your patch).
>
> If t->expired is true, the alarm timer is dynticks and host_alarm_timer
> is one-shot, so it is impossible that host_alarm_timer is called before
> qemu_rearm_alarm_timer finishes. (Except for the Windows bug fixed
> earlier in the series).
>
>> BTW, if host_alarm_handler runs after is t->expired is cleared,
>> but before qemu_run_timers->callback->qemu_mod_timer, subsequent
>> qemu_mod_timer callbacks fail to rearm the alarm timer, in case
>> timer in question expires earlier?
>
> bh->scheduled is set to 0 before executing the bottom half, so
> qemu_mod_timer sees qemu_alarm_pending() == false and does rearm the
> alarm timer.
static void qemu_timer_bh(void *opaque)
{
struct qemu_alarm_timer *t = opaque;
/* rearm timer, if not periodic */
if (t->expired) {
t->expired = 0;
qemu_rearm_alarm_timer(t);
}
-> host_alarm_handler fires, sets bh->scheduled = 1
-> qemu_mod_timer sees qemu_alarm_pending() == true and does
not rearm the alarm timer.
/* vm time timers */
if (vm_running) {
if (!cur_cpu || likely(!(cur_cpu->singlestep_enabled & STEP_NOTIMER)))
qemu_run_timers(&active_timers[QEMU_CLOCK_VIRTUAL],
qemu_get_clock(vm_clock));
}
/* real time timers */
qemu_run_timers(&active_timers[QEMU_CLOCK_REALTIME],
qemu_get_clock(rt_clock));
qemu_run_timers(&active_timers[QEMU_CLOCK_HOST],
qemu_get_clock(host_clock));
}
>
> Cases like this are why it is important to get t->expired vs.
> qemu_alarm_pending right; I had thought of some similar races while
> reviewing the submission, but I admit I hadn't thought about any of
> these particular issues, so thanks for the review and for making me do
> my homework.
The purpose of t->expired is somewhat unclear to me (and similarly
in the current code). Why not simply leave the rearm decision
to qemu_mod_timer? (and kill the explicit qemu_rearm_alarm_timer from
the timer bh handler). Maybe for a future patch...
- [Qemu-devel] [PATCH 02/19] add qemu_icount_round, (continued)
- [Qemu-devel] [PATCH 02/19] add qemu_icount_round, Paolo Bonzini, 2009/12/21
- [Qemu-devel] [PATCH 03/19] avoid dubiously clever code in win32_start_timer, Paolo Bonzini, 2009/12/21
- [Qemu-devel] [PATCH 05/19] only one flag is needed for alarm_timer, Paolo Bonzini, 2009/12/21
- [Qemu-devel] [PATCH 04/19] fix error in win32_rearm_timer, Paolo Bonzini, 2009/12/21
- [Qemu-devel] [PATCH 07/19] add qemu_get_clock_ns, Paolo Bonzini, 2009/12/21
- [Qemu-devel] [PATCH 09/19] remove qemu_rearm_alarm_timer from main loop, Paolo Bonzini, 2009/12/21
- [Qemu-devel] [PATCH 08/19] move kbd/mouse events to event.c, Paolo Bonzini, 2009/12/21
- [Qemu-devel] [PATCH 11/19] use a bottom half to run timers, Paolo Bonzini, 2009/12/21
[Qemu-devel] [PATCH 10/19] add qemu_bh_scheduled, Paolo Bonzini, 2009/12/21
[Qemu-devel] [PATCH 06/19] more alarm timer cleanup, Paolo Bonzini, 2009/12/21
[Qemu-devel] [PATCH 16/19] tweak qemu_notify_event, Paolo Bonzini, 2009/12/21
[Qemu-devel] [PATCH 13/19] move tcg_has_work to cpu-exec.c and rename it, Paolo Bonzini, 2009/12/21
[Qemu-devel] [PATCH 12/19] new function qemu_icount_delta, Paolo Bonzini, 2009/12/21
[Qemu-devel] [PATCH 15/19] do not provide qemu_event_increment if iothread not used, Paolo Bonzini, 2009/12/21
[Qemu-devel] [PATCH 14/19] disentangle tcg and deadline calculation, Paolo Bonzini, 2009/12/21