Message-ID: <a017e479-2ea5-459f-a016-011d53e09ced@redhat.com>
Date:   Tue, 28 Jul 2020 18:29:52 +0800
From:   Jason Wang <jasowang@...hat.com>
To:     "Zhu, Lingshan" <lingshan.zhu@...el.com>,
        alex.williamson@...hat.com, mst@...hat.com, pbonzini@...hat.com,
        sean.j.christopherson@...el.com, wanpengli@...cent.com
Cc:     virtualization@...ts.linux-foundation.org, netdev@...r.kernel.org,
        kvm@...r.kernel.org, eli@...lanox.com, shahafs@...lanox.com,
        parav@...lanox.com
Subject: Re: [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
>>
>>

Powered by blists - more mailing lists