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

[Jan Kiszka]

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 <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.

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