[Qemu-devel] Re: [PATCH 14/22] kvm: Fix race between timer signals and vcpu entry under !IOTHREAD

[Marcelo Tosatti]

On Tue, Feb 01, 2011 at 02:32:38PM +0100, Jan Kiszka wrote:
> On 2011-02-01 13:47, Marcelo Tosatti wrote:
> > On Thu, Jan 27, 2011 at 02:09:58PM +0100, Jan Kiszka wrote:
> >> Found by Stefan Hajnoczi: There is a race in kvm_cpu_exec between
> >> checking for exit_request on vcpu entry and timer signals arriving
> >> before KVM starts to catch them. Plug it by blocking both timer related
> >> signals also on !CONFIG_IOTHREAD and process those via signalfd.
> >>
> >> Signed-off-by: Jan Kiszka <address@hidden>
> >> CC: Stefan Hajnoczi <address@hidden>
> >> ---
> >>  cpus.c |   18 ++++++++++++++++++
> >>  1 files changed, 18 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/cpus.c b/cpus.c
> >> index fc3f222..29b1070 100644
> >> --- a/cpus.c
> >> +++ b/cpus.c
> >> @@ -254,6 +254,10 @@ static void qemu_kvm_init_cpu_signals(CPUState *env)
> >>      pthread_sigmask(SIG_BLOCK, NULL, &set);
> >>      sigdelset(&set, SIG_IPI);
> >>      sigdelset(&set, SIGBUS);
> >> +#ifndef CONFIG_IOTHREAD
> >> +    sigdelset(&set, SIGIO);
> >> +    sigdelset(&set, SIGALRM);
> >> +#endif
> > 
> > I'd prefer separate qemu_kvm_init_cpu_signals in the !IOTHREAD
> > section.
> 
> You mean to duplicate qemu_kvm_init_cpu_signals for both configurations?

Yes, so to avoid #ifdefs spread.

> >> +
> >> +#ifndef CONFIG_IOTHREAD
> >> +    if (sigismember(&chkset, SIGIO) || sigismember(&chkset, SIGALRM)) {
> >> +        qemu_notify_event();
> >> +    }
> >> +#endif
> > 
> > Why is this necessary?
> > 
> > You should break out of cpu_exec_all if there's a pending alarm (see
> > qemu_alarm_pending()).
> 
> qemu_alarm_pending() is not true until the signal is actually taken. The
> alarm handler sets the required flags.

Right. What i mean is you need to execute the signal handler inside
cpu_exec_all loop (so that alarm pending is set).

So, if there is a SIGALRM pending, qemu_run_timers has highest
priority, not vcpu execution.

> >>  
> >>  #else /* _WIN32 */
> >> @@ -398,6 +408,14 @@ int qemu_init_main_loop(void)
> >>      int ret;
> >>  
> >>      sigemptyset(&blocked_signals);
> >> +    if (kvm_enabled()) {
> >> +        /*
> >> +         * We need to process timer signals synchronously to avoid a race
> >> +         * between exit_request check and KVM vcpu entry.
> >> +         */
> >> +        sigaddset(&blocked_signals, SIGIO);
> >> +        sigaddset(&blocked_signals, SIGALRM);
> >> +    }
> > 
> > A block_io_signals() function for !IOTHREAD would be nicer.
> 
> Well, we aren't blocking all I/O signals, so I decided against causing
> confusion to people that try to compare the result against real
> block_io_signals. If you mean just pushing those lines that set up
> blocked_signals into a separate function, then I need to find a good
> name for it.

Yes, separate function, similar to CONFIG_IOTHREAD case (feel free to
rename function).