Re: [RFC] QEMU/KVM: dedicated IO thread

[Marcelo Tosatti <marcelo@kvack.org>]

On Tue, Mar 25, 2008 at 04:58:06PM +0200, Avi Kivity wrote:
> Marcelo Tosatti wrote:
> >Avi was concerned that this would cause problems with migration. I
> >haven't specifically tested it yet, but it seems there will be no
> >problems introduced by this change: the IO thread will stop all vcpu's
> >in the same way the vcpu0 thread did before.
> >  
> 
> I believe this is broken for smp_cpus > 1, and will with this change 
> will be broken even for non smp.  The pause/resume logic is rotten.

Ok, I'll look into it in more detail.

> >QEMU/KVM: separate thread for IO handling
> >
> >Move IO processing from vcpu0 to a dedicated thread.
> >
> >This removes load on vcpu0 by allowing better cache locality and also
> >improves latency.
> >
> >We can now block signal handling for IO events, so sigtimedwait won't
> >race with handlers:
> >
> >- Currently the SIGALRM handler fails to set CPU_INTERRUPT_EXIT because
> >the "next_cpu" variable is not initialized in the KVM path, meaning that
> >processing of timer expiration might be delayed until the next vcpu0 exit.
> >  
> 
> I think we call main_loop_wait() is called unconditionally after every 
> signal.

We exit the kvm_run() loop if CPU_INTERRUPT_EXIT is detected by pre_kvm_run().

But the SIGALRM handler won't set it:

static void host_alarm_handler(int host_signum)
{
...
        CPUState *env = next_cpu;

        alarm_timer->flags |= ALARM_FLAG_EXPIRED;

        if (env) {
            /* stop the currently executing cpu because a timer occured */
            cpu_interrupt(env, CPU_INTERRUPT_EXIT);
#ifdef USE_KQEMU
            if (env->kqemu_enabled) {
                kqemu_cpu_interrupt(env);
            }
#endif

Because the KVM path does not initialize "next_cpu":

static int main_loop(void)
{
    int ret, timeout;
#ifdef CONFIG_PROFILER
    int64_t ti;
#endif
    CPUState *env;


    if (kvm_enabled()) {
        kvm_main_loop();
        cpu_disable_ticks();
        return 0;
    }

    cur_cpu = first_cpu;
    next_cpu = cur_cpu->next_cpu ?: first_cpu;


> 
> >- Processing of IO events will not be unnecessarily interrupted.
> >
> >
> >Index: kvm-userspace.io/libkvm/libkvm.c
> >===================================================================
> >--- kvm-userspace.io.orig/libkvm/libkvm.c
> >+++ kvm-userspace.io/libkvm/libkvm.c
> >@@ -388,9 +388,6 @@ int kvm_create(kvm_context_t kvm, unsign
> > 	if (r < 0)
> > 	        return r;
> > 	kvm_create_irqchip(kvm);
> >-	r = kvm_create_vcpu(kvm, 0);
> >-	if (r < 0)
> >-		return r;
> > 
> > 	return 0;
> > }
> >  
> 
> Please put this and the corresponding qemu change in a separate patch.
> 
> [...lots more...]
> 
> Looks good.

Will do.

-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace