Linux Virtualization - Jul 2020 - [PATCH V4 4/6] vhost_vdpa: implement IRQ offloading in vhost_vdpa

On 2020/7/28 ??5:18, Zhu, Lingshan wrote:
>>>
>>> ?????? * status to 0.
>>> @@ -167,6 +220,15 @@ static long vhost_vdpa_set_status(struct 
>>> vhost_vdpa *v, u8 __user *statusp)
>>> ????? if (status != 0 && (ops->get_status(vdpa) &
~status) != 0)
>>> ????????? return -EINVAL;
>>> ? +??? /* vq irq is not expected to be changed once DRIVER_OK is
set */
>>
>>
>> So this basically limit the usage of get_vq_irq() in the context 
>> vhost_vdpa_set_status() and other vDPA bus drivers' set_status().
If
>> this is true, there's even no need to introduce any new config ops 
>> but just let set_status() to return the irqs used for the device. Or 
>> if we want this to be more generic, we need vpda's own irq manager 
>> (which should be similar to irq bypass manager). That is:
> I think there is no need for a driver to free / re-request its irqs after
DRIVER_OK though it can do so. If a driver changed its irq of a vq after
DRIVER_OK, the vq is still operational but will lose irq offloading that is
reasonable.
> If we want set_status() return irqs, we need to record the irqs somewhere
in vdpa_device,

Why, we can simply pass an array to the driver I think?

void (*set_status)(struct vdpa_device *vdev, u8 status, int *irqs);

> as we discussed in a previous thread, this may need initialize and cleanup
works, so a new ops
> with proper comments (don't say they could not change irq, but
highlight if irq changes, irq offloading will not work till next DRIVER_OK)
could be more elegant.
> However if we really need to change irq after DRIVER_OK, I think maybe we
still need vDPA vq irq allocate / free helpers, then the helpers can not be used
in probe() as we discussed before, it is a step back to V3 series.

Still, it's not about whether driver may change irq after DRIVER_OK but 
implication of the assumption. If one bus ops must be called in another 
ops, it's better to just implement them in one ops.

>>
>> - bus driver can register itself as consumer
>> - vDPA device driver can register itself as producer
>> - matching via queue index
> IMHO, is it too heavy for this feature,

Do you mean LOCs? We can:

1) refactor irq bypass manager
2) invent it our own (a much simplified version compared to bypass manager)
3) enforcing them via vDPA bus

Each of the above should be not a lot of coding. I think method 3 is 
partially done in your previous series but in an implicit manner:

- bus driver that has alloc_irq/free_irq implemented could be implicitly 
treated as consumer registering
- every vDPA device driver could be treated as producer
- vdpa_devm_alloc_irq() could be treated as producer registering
- alloc_irq/free_irq is the add_producer/del_procuer

We probably just lack some synchronization with driver probe/remove.

> and how can they match if two individual adapters both have vq idx = 1.

The matching is per vDPA device.

Thanks

> Thanks!
>> - deal with registering/unregistering of consumer/producer
>>
>> So there's no need to care when or where the vDPA device driver to 
>> allocate the irq, and we don't need to care at which context the
vDPA
>> bus driver can use the irq. Either side may get notified when the 
>> other side is gone (though we only care about the gone of producer 
>> probably).
>>
>> Any thought on this?
>>
>> Thanks
>>
>>