Re: [Qemu-devel] [PATCH] vhost: force vhost off for non-MSI guests

[Michael S. Tsirkin]

On Fri, Jan 21, 2011 at 06:19:13AM -0700, Alex Williamson wrote:
> On Fri, 2011-01-21 at 11:55 +0200, Michael S. Tsirkin wrote:
> > On Thu, Jan 20, 2011 at 06:35:46PM -0700, Alex Williamson wrote:
> > > On Thu, 2011-01-20 at 18:23 -0600, Anthony Liguori wrote:
> > > > On 01/20/2011 10:07 AM, Michael S. Tsirkin wrote:
> > > > > On Thu, Jan 20, 2011 at 09:43:57AM -0600, Anthony Liguori wrote:
> > > > >    
> > > > >> On 01/20/2011 09:35 AM, Michael S. Tsirkin wrote:
> > > > >>      
> > > > >>> When MSI is off, each interrupt needs to be bounced through the io
> > > > >>> thread when it's set/cleared, so vhost-net causes more context 
> > > > >>> switches and
> > > > >>> higher CPU utilization than userspace virtio which handles 
> > > > >>> networking in
> > > > >>> the same thread.
> > > > >>>
> > > > >>> We'll need to fix this by adding level irq support in kvm irqfd,
> > > > >>> for now disable vhost-net in these configurations.
> > > > >>>
> > > > >>> Signed-off-by: Michael S. Tsirkin<address@hidden>
> > > > >>>        
> > > > >> I actually think this should be a terminal error.  The user asks for
> > > > >> vhost-net, if we cannot enable it, we should exit.
> > > > >>
> > > > >> Or we should warn the user that they should expect bad performance.
> > > > >> Silently doing something that the user has explicitly asked us not
> > > > >> to do is not a good behavior.
> > > > >>
> > > > >> Regards,
> > > > >>
> > > > >> Anthony Liguori
> > > > >>      
> > > > > The issue is that user has no control of the guest, and can not know
> > > > > whether the guest enables MSI. So what you ask for will just make
> > > > > some guests fail, and others fail sometimes.
> > > > > The user also has no way to know that version X of kvm does not 
> > > > > expose a
> > > > > way to inject level interrupts with irqfd.
> > > > >
> > > > > We could have *another* flag that says "use vhost where it helps" but
> > > > > then I think this is what everyone wants to do, anyway, and libvirt
> > > > > already sets vhost=on so I prefer redefining the meaning of an 
> > > > > existing
> > > > > flag.
> > > > >    
> > > > 
> > > > In the very least, there needs to be a vhost=force.
> > > > 
> > > > Having some sort of friendly default policy is fine but we need to 
> > > > provide a mechanism for a user to have the final say.  If you want to 
> > > > redefine vhost=on to really mean, use the friendly default, that's fine 
> > > > by me, but only if the vhost=force option exists.
> > > > 
> > > > I actually would think libvirt would want to use vhost=force.  
> > > > Debugging 
> > > > with vhost=on is going to be a royal pain in the ass if a user reports 
> > > > bad performance.  Given the libvirt XML, you can't actually tell from 
> > > > the guest and the XML whether or not vhost was actually in use or not.
> > > 
> > > If we add a force option, let's please distinguish hotplug from VM
> > > creation time.  The latter can abort.  Hotplug should print an error and
> > > fail the initfn.
> > 
> > It can't abort at init - MSI is disabled at init, it needs to be enabled
> > by the guest later. And aborting the guest in the middle of the run
> > is a very bad idea.
> 
> Yeah, I was thinking about the ordering of device being added vs guest
> enabling MSI this morning.  Waiting until the guest decides to try to
> start using the device to NAK it with an abort is very undesirable.
> What if when we have vhost=on,force, the device doesn't advertise an
> INTx (PCI_INTERRUPT_PIN = 0)?
> 
> Alex

Then we break backward compatibility with old guests.
I don't see what the issue is really:
It is trivial to check that the guest uses MSIX.

-- 
MST