On 2012-07-05 22:47, Michael S. Tsirkin wrote: > 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.
Look at the code: this API serves no practical purpose anymore, not even for the irqfd scenario. I'm not touching the irqfd code while removing msix_entry_used. > >>> >>>> 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. From that point of view, the current final msix_clr_pending in msix_vector_unuse is also useless. Then what is the remaining purpose of msix_entry_used? Also, we are not discussing a virtio API here but a generic MSI-X feature that affects every MSI-X using device. > Further, even in theory this might not be an issue since anyone sharing > interrupts needs to deal with spurious events anyway. This is not about spurious interrupts, it's about the proper state of the PBA, something that should not contain any spurious states. > > So is or is there not a bug in virtio when it shares vectors? There are deficits in virtio about how it handles potential vector sharing internally. But the use/unuse API will not be involved in making this more spec conforming. Nor are there other users behind the horizon. That's my point. > >> 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 <jan.kis...@siemens.com> >>> >>> 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. Sorry, what regression? This patch does not change the behavior of virtio. Please review it and point out the contrary. > >>> >>> 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". The existing API is the intrusive one, that's why I'm trying to remove it for a while now. Can you sketch a scenario where it helps to track vector usage via a central counter like it's done now? I simply don't see this. Therefore I see no point in keeping this infrastructure around - or even adding a flag to prolong its life. Jan
signature.asc
Description: OpenPGP digital signature
