Jason Wang
2020-Jul-28 10:29 UTC
[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 >> >>