Re: [Qemu-devel] [PATCH] msix: Drop tracking of used vectors

[Michael S. Tsirkin]

On Thu, Jul 05, 2012 at 04:27:33PM +0200, Jan Kiszka wrote:
> On 2012-07-05 15:17, Michael S. Tsirkin wrote:
> > On Thu, Jul 05, 2012 at 11:42:14AM +0200, Jan Kiszka wrote:
> >> This optimization was once used in qemu-kvm to keep KVM route usage low.
> >> But now we solved that problem via lazy updates.
> > 
> > What if we are using vhost which AFAIK can't use the lazy path?
> 
> It uses static vIRQs with irqfd, just as before.

Exactly. And the API is helpful in that case.

> > 
> >> It also tried to handle
> >> the case of vectors shared between different sources of the same device.
> >> However, this never really worked
> > 
> > Interesting. Guests actually have support and it seems to work for me.
> > What's the issue?
> 
> The state of shared vectors is not tracked. Consider a device has two
> sources for the same vector and one source which raised the "line"
> before is deregistered, then we will leave the vector set.

In virtio vectors are registered/deregistered during setup
(before driver-ok), so this does not happen.
Further, even in theory this might not be an issue since anyone sharing
interrupts needs to deal with spurious events anyway.

So is or is there not a bug in virtio when it shares vectors?

> What we rather need is to track the sharing at device level and report
> the resulting state to the MSI-X layer. That's what the reduced API will
> allow. I don't think that vector sharing belongs to the generic layer.
> > 
> >> and will have to be addressed
> >> differently anyway.
> > 
> > what's the plan to address this?
> 
> As said above: Extend virtio to track their queue states, let it
> calculate the vector state and report that to the generic layer.
> 
> If we really once identify another device model - besides virtio - that
> does vector sharing, we can still try to model an optional extension to
> the MSI-X layer for such devices. But the majority of device models
> should not be bothered with such special cases.
> 
> > 
> >> So drop this obsolete interface.
> >>
> >> We still need interfaces to clear pending vectors though. Provide
> >> msix_clear_vector and msix_clear_all_vectors for this.
> >>
> >> Signed-off-by: Jan Kiszka <address@hidden>
> > 
> > Looks like if guest tries to use too many vectors it will have to switch
> > to userspace virtio where previously we would report failure to guest
> > and it would configure less vectors, which seems better.
> 
> There is nothing in the current approach that tracks the availability of
> vector. That's because it depends on the device if it considers a vector
> used when a single event source grabbed it or if it is able to share it
> internally.

So I'd like to see some code that shows how we avoid the regression in the
above scenario, before we drop the API.

> > 
> > I'd like to understand whether this patch is a pre-requisite for
> > anything or just a cleanup?
> 
> It avoids spreading of redundant msix_vector_use/unuse to more devices
> (next ones will be VFIO and kvm device assignment).
> 
> Jan

Another less intrusive way is to add a flag that say
"device always uses all vectors".

> -- 
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux