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