Re: [PATCH V4 4/6] vhost_vdpa: implement IRQ offloading in vhost_vdpa

On 2020/7/28 下午5:18, Zhu, Lingshan wrote:

quoted

quoted

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

quoted

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

quoted

- 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