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
