public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/9] in-kernel APIC v9 (kernel side)
@ 2007-05-31 18:08 Gregory Haskins
       [not found] ` <20070531180005.1810.23884.stgit-5CR4LY5GPkvLDviKLk5550HKjMygAv58XqFh9Ls21Oc@public.gmane.org>
  0 siblings, 1 reply; 19+ messages in thread
From: Gregory Haskins @ 2007-05-31 18:08 UTC (permalink / raw)
  To: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f

Incorporates v8 plus the following changes:

1) Fix for hang on AMD
2) Fixes issue where irq-windows are inaccurately reported to userspace
3) Fixed issue where irq-window-exiting requests can be ignored in some cases

Note that we no longer need the backlog.patch to handle a corner cases now.

As before, this has been tested on 32 bit XP w/ACPI and 64 bit windows.  It
offers a 17% performance improvement over git HEAD in my testing.  Note that I
am not able to fully verify that this works on AMD, as even git-head does not
work on my system.  I am able to verify that it no longer hangs the kernel
hard.  The guest hangs, but it hangs without my patches as well.  Perhaps
someone with a known good environment on AMD can verify for me?

I am being pulled off of my KVM work for a little while, so I will not be able
to contribute again until further notice. If there are any remaining issues
that need to be addressed and someone wants to carry the torch, feel free to
do so.  Otherwise, I will pick up the effort to get this merged in when I am
able to return to KVM.

Thanks all for the feedback/comments/suggestions through all of this.  It has
been very fun and quite a learning experience.

-Greg
 

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

^ permalink raw reply	[flat|nested] 19+ messages in thread
* Re: [PATCH 9/9] KVM: Adds support for halting in the kernel
@ 2007-06-19 19:02 Gregory Haskins
  0 siblings, 0 replies; 19+ messages in thread
From: Gregory Haskins @ 2007-06-19 19:02 UTC (permalink / raw)
  To: avi-atKUWr5tajBWk0Htik3J/w; +Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f

On Tue, 2007-06-19 at 17:48 +0300, Avi Kivity wrote:
> Gregory Haskins wrote:
> > Halting in userspace requires a relatively cumbersome mechanism to signal the
> > halted VCPU.  Implementing halt in kernel should be relatively straight
> > forward and it eliminates the need for the signaling
> >
> >   
> 
> Merging this one in, found some nits:


I believe you will find that this implementation works correctly and
possibly even optimally for the situation.  I crafted it after carefully
balancing the design requirements with observed behavior through many
rounds of testing.  I do have a bit of experience in this area of linux,
though I am not an out-and-out expert.  Therefore its possible there are
other/better ways to do this. Based on this, I will just detail why I
did things this way and you guys can poke holes in it ;)

> 
> > +/*
> >   * This function is invoked whenever we want to interrupt a vcpu that is
> >   * currently executing in guest-mode.  It currently is a no-op because
> >   * the simple delivery of the IPI to execute this function accomplishes our
> > @@ -2481,6 +2556,16 @@ static void kvm_vcpu_intr(struct kvm_irqsink *this,
> >  			BUG_ON(direct_ipi == smp_processor_id());
> >  			++vcpu->stat.guest_preempt;
> >  		}
> > +
> > +		/*
> > +		 * If the CPU is halted it will be waiting for a wake-up
> > +		 */
> > +		if (waitqueue_active(&vcpu->irq.wq)) {
> >   
> 
> Why do the check?  The only reason I can see is to keep the stats 
> correct.  Otherwise we can do the body of the if unconditionally.

I agree the waitqueue_active() check is superfluous if you are only
waking up a wait_queue and nothing else.  However, one reason to have it
this way is the stat update (as you pointed out).  More importantly,
another reason is that we cannot use a simple wake_up here (I believe)
due to constraints of the different contexts that may invoke this
function.  Therefore we have to use wake_up_sync with a deferred
reschedule.   We could do these operations unconditionally, but having
an unnecessary reschedule seems like it would be wasteful at best.

> 
> > +			wake_up_interruptible_sync(&vcpu->irq.wq);
> > +			set_tsk_need_resched(current);
> >   
> 
> This is unneeded?  I'd expect wake_up_interruptible_sync() to take care 
> of any rescheduling needed.

IIUC, a regular wake_up() does two primary things:  1) it moves an item
from a wait_queue to the run_queue, and 2) it calls schedule.

wake_up_sync() only does (1) and expects that the caller will call
schedule() later on its own.  In our case, we cannot safely call
schedule() directly anywhere in the interrupt code because there are
multiple paths that can cause the apic to inject something, some of
which are not conducive to sleeping (for example hrtimer callback driven
LVTT interrupts).   Therefore, we mark the task as needing rescheduling
to defer scheduling before we return from this function, but to also
induce scheduling ASAP afterwards.

IIRC, if you remove the NEED_RESCHED, sometimes the LVTT will fail to
kick the vcpu out of halt until the next natural schedule point happens
orthogonal to this codepath.  This results in unexpected behavior in the
guest, and is what drove me to add it.


Regards,
-Greg


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

^ permalink raw reply	[flat|nested] 19+ messages in thread
* [PATCH 0/9] in-kernel APIC v8 (kernel side)
@ 2007-05-24 13:22 Gregory Haskins
       [not found] ` <20070524131917.11321.17746.stgit-sLgBBP33vUGnsjUZhwzVf9HuzzzSOjJt@public.gmane.org>
  0 siblings, 1 reply; 19+ messages in thread
From: Gregory Haskins @ 2007-05-24 13:22 UTC (permalink / raw)
  To: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f

Changelog: v7 plus the following:

1) Rebased on git-head

2) Removed level-senstive patch.  I re-read the spec more carefully and we had
it right the first time.

3) Got rid of sigprocmask from vcpu_halt per comments

Passes the usual tests, and offers an 11% speedup.  There is another
enhancement not included here (coming shortly) which exposes another 12%
speedup over this as well.  Stay tuned

Regards
-Greg

-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2007-06-19 19:02 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2007-05-31 18:08 [PATCH 0/9] in-kernel APIC v9 (kernel side) Gregory Haskins
     [not found] ` <20070531180005.1810.23884.stgit-5CR4LY5GPkvLDviKLk5550HKjMygAv58XqFh9Ls21Oc@public.gmane.org>
2007-05-31 18:08   ` [PATCH 1/9] KVM: Adds support for in-kernel mmio handlers Gregory Haskins
2007-05-31 18:08   ` [PATCH 2/9] KVM: VMX - fix interrupt checking on light-exit Gregory Haskins
2007-05-31 18:09   ` [PATCH 3/9] KVM: Add irqdevice object Gregory Haskins
     [not found]     ` <20070531180903.1810.87474.stgit-5CR4LY5GPkvLDviKLk5550HKjMygAv58XqFh9Ls21Oc@public.gmane.org>
2007-06-04  7:13       ` Dong, Eddie
2007-05-31 18:09   ` [PATCH 4/9] KVM: Adds ability to preempt an executing VCPU Gregory Haskins
2007-05-31 18:09   ` [PATCH 5/9] KVM: Add support for in-kernel LAPIC model Gregory Haskins
2007-05-31 18:09   ` [PATCH 6/9] KVM: Adds support for real NMI injection on VMX processors Gregory Haskins
     [not found]     ` <20070531180919.1810.30009.stgit-5CR4LY5GPkvLDviKLk5550HKjMygAv58XqFh9Ls21Oc@public.gmane.org>
2007-06-01  2:43       ` [PATCH 6/9] KVM: Adds support for real NMI injection onVMX processors Li, Xin B
2007-05-31 18:09   ` [PATCH 7/9] KVM: Adds basic plumbing to support TPR shadow features Gregory Haskins
2007-05-31 18:09   ` [PATCH 8/9] KVM: Add statistics from interrupt subsystem Gregory Haskins
2007-05-31 18:09   ` [PATCH 9/9] KVM: Adds support for halting in the kernel Gregory Haskins
     [not found]     ` <20070531180934.1810.45024.stgit-5CR4LY5GPkvLDviKLk5550HKjMygAv58XqFh9Ls21Oc@public.gmane.org>
2007-06-19 14:48       ` Avi Kivity
2007-06-02 22:04   ` [PATCH 0/9] in-kernel APIC v9 (kernel side) Dor Laor
2007-06-03  9:28   ` Avi Kivity
     [not found]     ` <466289A4.9000201-atKUWr5tajBWk0Htik3J/w@public.gmane.org>
2007-06-03 15:39       ` Avi Kivity
2007-06-07  8:20   ` Dong, Eddie
  -- strict thread matches above, loose matches on Subject: below --
2007-06-19 19:02 [PATCH 9/9] KVM: Adds support for halting in the kernel Gregory Haskins
2007-05-24 13:22 [PATCH 0/9] in-kernel APIC v8 (kernel side) Gregory Haskins
     [not found] ` <20070524131917.11321.17746.stgit-sLgBBP33vUGnsjUZhwzVf9HuzzzSOjJt@public.gmane.org>
2007-05-24 13:23   ` [PATCH 9/9] KVM: Adds support for halting in the kernel Gregory Haskins

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox