KVM Archive on lore.kernel.org
 help / color / Atom feed
From: Jason Wang <jasowang@redhat.com>
To: "Zhu, Lingshan" <lingshan.zhu@intel.com>,
	alex.williamson@redhat.com, mst@redhat.com, pbonzini@redhat.com,
	sean.j.christopherson@intel.com, wanpengli@tencent.com
Cc: virtualization@lists.linux-foundation.org,
	netdev@vger.kernel.org, kvm@vger.kernel.org, eli@mellanox.com,
	shahafs@mellanox.com, parav@mellanox.com
Subject: Re: [PATCH V4 4/6] vhost_vdpa: implement IRQ offloading in vhost_vdpa
Date: Tue, 28 Jul 2020 18:29:52 +0800
Message-ID: <a017e479-2ea5-459f-a016-011d53e09ced@redhat.com> (raw)
In-Reply-To: <f3e375da-3aa8-7a0c-237c-25943667a535@intel.com>

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

  parent reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-28  4:23 [PATCH V4 0/6] IRQ offloading for vDPA Zhu Lingshan
2020-07-28  4:24 ` [PATCH V4 1/6] vhost: introduce vhost_vring_call Zhu Lingshan
2020-07-28  4:24 ` [PATCH V4 2/6] kvm: detect assigned device via irqbypass manager Zhu Lingshan
2020-07-28  4:24 ` [PATCH V4 3/6] vDPA: add get_vq_irq() in vdpa_config_ops Zhu Lingshan
2020-07-28  7:42   ` Jason Wang
2020-07-28  4:24 ` [PATCH V4 4/6] vhost_vdpa: implement IRQ offloading in vhost_vdpa Zhu Lingshan
2020-07-28  7:53   ` Jason Wang
     [not found]     ` <f3e375da-3aa8-7a0c-237c-25943667a535@intel.com>
2020-07-28 10:29       ` Jason Wang [this message]
2020-07-28  9:04   ` Eli Cohen
2020-07-29  9:21     ` Jason Wang
2020-07-29  9:55       ` Eli Cohen
2020-07-29 10:19         ` Jason Wang
2020-07-29 11:13           ` Eli Cohen
2020-07-29 14:15           ` Eli Cohen
2020-07-31  3:11             ` Jason Wang
2020-07-28  4:24 ` [PATCH V4 5/6] ifcvf: implement vdpa_config_ops.get_vq_irq() Zhu Lingshan
2020-07-28  4:24 ` [PATCH V4 6/6] irqbypass: do not start cons/prod when failed connect Zhu Lingshan
2020-07-31  3:18 ` [PATCH V4 0/6] IRQ offloading for vDPA Jason Wang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a017e479-2ea5-459f-a016-011d53e09ced@redhat.com \
    --to=jasowang@redhat.com \
    --cc=alex.williamson@redhat.com \
    --cc=eli@mellanox.com \
    --cc=kvm@vger.kernel.org \
    --cc=lingshan.zhu@intel.com \
    --cc=mst@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=parav@mellanox.com \
    --cc=pbonzini@redhat.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=shahafs@mellanox.com \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=wanpengli@tencent.com \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git