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

[Marcelo Tosatti]

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