Re: [Qemu-devel] [PATCH 11/19] use a bottom half to run timers

[Paolo Bonzini]

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.

> 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.

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.

> Nice patchset!

Thanks. :-)

Paolo