diff mbox series

[RFC,v4,10/11] vduse: Introduce a workqueue for irq injection

Message ID 20210223115048.435-11-xieyongji@bytedance.com
State New
Headers show
Series Introduce VDUSE - vDPA Device in Userspace | expand

Commit Message

Yongji Xie Feb. 23, 2021, 11:50 a.m. UTC
This patch introduces a workqueue to support injecting
virtqueue's interrupt asynchronously. This is mainly
for performance considerations which makes sure the push()
and pop() for used vring can be asynchronous.

Signed-off-by: Xie Yongji <xieyongji@bytedance.com>
---
 drivers/vdpa/vdpa_user/vduse_dev.c | 29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)

Comments

Jason Wang March 4, 2021, 6:59 a.m. UTC | #1
On 2021/2/23 7:50 下午, Xie Yongji wrote:
> This patch introduces a workqueue to support injecting

> virtqueue's interrupt asynchronously. This is mainly

> for performance considerations which makes sure the push()

> and pop() for used vring can be asynchronous.



Do you have pref numbers for this patch?

Thanks


>

> Signed-off-by: Xie Yongji <xieyongji@bytedance.com>

> ---

>   drivers/vdpa/vdpa_user/vduse_dev.c | 29 +++++++++++++++++++++++------

>   1 file changed, 23 insertions(+), 6 deletions(-)

>

> diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c

> index 8042d3fa57f1..f5adeb9ee027 100644

> --- a/drivers/vdpa/vdpa_user/vduse_dev.c

> +++ b/drivers/vdpa/vdpa_user/vduse_dev.c

> @@ -42,6 +42,7 @@ struct vduse_virtqueue {

>   	spinlock_t irq_lock;

>   	struct eventfd_ctx *kickfd;

>   	struct vdpa_callback cb;

> +	struct work_struct inject;

>   };

>   

>   struct vduse_dev;

> @@ -99,6 +100,7 @@ static DEFINE_IDA(vduse_ida);

>   

>   static dev_t vduse_major;

>   static struct class *vduse_class;

> +static struct workqueue_struct *vduse_irq_wq;

>   

>   static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)

>   {

> @@ -852,6 +854,17 @@ static int vduse_kickfd_setup(struct vduse_dev *dev,

>   	return 0;

>   }

>   

> +static void vduse_vq_irq_inject(struct work_struct *work)

> +{

> +	struct vduse_virtqueue *vq = container_of(work,

> +					struct vduse_virtqueue, inject);

> +

> +	spin_lock_irq(&vq->irq_lock);

> +	if (vq->ready && vq->cb.callback)

> +		vq->cb.callback(vq->cb.private);

> +	spin_unlock_irq(&vq->irq_lock);

> +}

> +

>   static long vduse_dev_ioctl(struct file *file, unsigned int cmd,

>   			unsigned long arg)

>   {

> @@ -917,12 +930,7 @@ static long vduse_dev_ioctl(struct file *file, unsigned int cmd,

>   			break;

>   

>   		vq = &dev->vqs[arg];

> -		spin_lock_irq(&vq->irq_lock);

> -		if (vq->ready && vq->cb.callback) {

> -			vq->cb.callback(vq->cb.private);

> -			ret = 0;

> -		}

> -		spin_unlock_irq(&vq->irq_lock);

> +		queue_work(vduse_irq_wq, &vq->inject);

>   		break;

>   	}

>   	case VDUSE_INJECT_CONFIG_IRQ:

> @@ -1109,6 +1117,7 @@ static int vduse_create_dev(struct vduse_dev_config *config)

>   

>   	for (i = 0; i < dev->vq_num; i++) {

>   		dev->vqs[i].index = i;

> +		INIT_WORK(&dev->vqs[i].inject, vduse_vq_irq_inject);

>   		spin_lock_init(&dev->vqs[i].kick_lock);

>   		spin_lock_init(&dev->vqs[i].irq_lock);

>   	}

> @@ -1333,6 +1342,11 @@ static int vduse_init(void)

>   	if (ret)

>   		goto err_chardev;

>   

> +	vduse_irq_wq = alloc_workqueue("vduse-irq",

> +				WQ_HIGHPRI | WQ_SYSFS | WQ_UNBOUND, 0);

> +	if (!vduse_irq_wq)

> +		goto err_wq;

> +

>   	ret = vduse_domain_init();

>   	if (ret)

>   		goto err_domain;

> @@ -1344,6 +1358,8 @@ static int vduse_init(void)

>   	return 0;

>   err_mgmtdev:

>   	vduse_domain_exit();

> +err_wq:

> +	destroy_workqueue(vduse_irq_wq);

>   err_domain:

>   	unregister_chrdev_region(vduse_major, VDUSE_DEV_MAX);

>   err_chardev:

> @@ -1359,6 +1375,7 @@ static void vduse_exit(void)

>   	misc_deregister(&vduse_misc);

>   	class_destroy(vduse_class);

>   	unregister_chrdev_region(vduse_major, VDUSE_DEV_MAX);

> +	destroy_workqueue(vduse_irq_wq);

>   	vduse_domain_exit();

>   	vduse_mgmtdev_exit();

>   }
Yongji Xie March 4, 2021, 8:58 a.m. UTC | #2
On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:
>

>

> On 2021/2/23 7:50 下午, Xie Yongji wrote:

> > This patch introduces a workqueue to support injecting

> > virtqueue's interrupt asynchronously. This is mainly

> > for performance considerations which makes sure the push()

> > and pop() for used vring can be asynchronous.

>

>

> Do you have pref numbers for this patch?

>


No, I can do some tests for it if needed.

Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless
if we call irq callback in ioctl context. Something like:

virtqueue_push();
virtio_notify();
    ioctl()
-------------------------------------------------
        irq_cb()
            virtqueue_get_buf()

The used vring is always empty each time we call virtqueue_push() in
userspace. Not sure if it is what we expected.

Thanks,
Yongji
Jason Wang March 5, 2021, 3:04 a.m. UTC | #3
On 2021/3/4 4:58 下午, Yongji Xie wrote:
> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

>>

>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

>>> This patch introduces a workqueue to support injecting

>>> virtqueue's interrupt asynchronously. This is mainly

>>> for performance considerations which makes sure the push()

>>> and pop() for used vring can be asynchronous.

>>

>> Do you have pref numbers for this patch?

>>

> No, I can do some tests for it if needed.

>

> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

> if we call irq callback in ioctl context. Something like:

>

> virtqueue_push();

> virtio_notify();

>      ioctl()

> -------------------------------------------------

>          irq_cb()

>              virtqueue_get_buf()

>

> The used vring is always empty each time we call virtqueue_push() in

> userspace. Not sure if it is what we expected.



I'm not sure I get the issue.

THe used ring should be filled by virtqueue_push() which is done by 
userspace before?

Thanks


>

> Thanks,

> Yongji

>
Yongji Xie March 5, 2021, 3:30 a.m. UTC | #4
On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:
>

>

> On 2021/3/4 4:58 下午, Yongji Xie wrote:

> > On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

> >>

> >> On 2021/2/23 7:50 下午, Xie Yongji wrote:

> >>> This patch introduces a workqueue to support injecting

> >>> virtqueue's interrupt asynchronously. This is mainly

> >>> for performance considerations which makes sure the push()

> >>> and pop() for used vring can be asynchronous.

> >>

> >> Do you have pref numbers for this patch?

> >>

> > No, I can do some tests for it if needed.

> >

> > Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

> > if we call irq callback in ioctl context. Something like:

> >

> > virtqueue_push();

> > virtio_notify();

> >      ioctl()

> > -------------------------------------------------

> >          irq_cb()

> >              virtqueue_get_buf()

> >

> > The used vring is always empty each time we call virtqueue_push() in

> > userspace. Not sure if it is what we expected.

>

>

> I'm not sure I get the issue.

>

> THe used ring should be filled by virtqueue_push() which is done by

> userspace before?

>


After userspace call virtqueue_push(), it always call virtio_notify()
immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()
will inject an irq to VM and return, then vcpu thread will call
interrupt handler. But in container (virtio-vdpa) cases,
virtio_notify() will call interrupt handler directly. So it looks like
we have to optimize the virtio-vdpa cases. But one problem is we don't
know whether we are in the VM user case or container user case.

Thanks,
Yongji
Jason Wang March 5, 2021, 3:42 a.m. UTC | #5
On 2021/3/5 11:30 上午, Yongji Xie wrote:
> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

>>

>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

>>>>> This patch introduces a workqueue to support injecting

>>>>> virtqueue's interrupt asynchronously. This is mainly

>>>>> for performance considerations which makes sure the push()

>>>>> and pop() for used vring can be asynchronous.

>>>> Do you have pref numbers for this patch?

>>>>

>>> No, I can do some tests for it if needed.

>>>

>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

>>> if we call irq callback in ioctl context. Something like:

>>>

>>> virtqueue_push();

>>> virtio_notify();

>>>       ioctl()

>>> -------------------------------------------------

>>>           irq_cb()

>>>               virtqueue_get_buf()

>>>

>>> The used vring is always empty each time we call virtqueue_push() in

>>> userspace. Not sure if it is what we expected.

>>

>> I'm not sure I get the issue.

>>

>> THe used ring should be filled by virtqueue_push() which is done by

>> userspace before?

>>

> After userspace call virtqueue_push(), it always call virtio_notify()

> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

> will inject an irq to VM and return, then vcpu thread will call

> interrupt handler. But in container (virtio-vdpa) cases,

> virtio_notify() will call interrupt handler directly. So it looks like

> we have to optimize the virtio-vdpa cases. But one problem is we don't

> know whether we are in the VM user case or container user case.



Yes, but I still don't get why used ring is empty after the ioctl()? 
Used ring does not use bounce page so it should be visible to the kernel 
driver. What did I miss :) ?

Thanks



>

> Thanks,

> Yongji

>
Yongji Xie March 5, 2021, 6:36 a.m. UTC | #6
On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:
>

>

> On 2021/3/5 11:30 上午, Yongji Xie wrote:

> > On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

> >>

> >> On 2021/3/4 4:58 下午, Yongji Xie wrote:

> >>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

> >>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

> >>>>> This patch introduces a workqueue to support injecting

> >>>>> virtqueue's interrupt asynchronously. This is mainly

> >>>>> for performance considerations which makes sure the push()

> >>>>> and pop() for used vring can be asynchronous.

> >>>> Do you have pref numbers for this patch?

> >>>>

> >>> No, I can do some tests for it if needed.

> >>>

> >>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

> >>> if we call irq callback in ioctl context. Something like:

> >>>

> >>> virtqueue_push();

> >>> virtio_notify();

> >>>       ioctl()

> >>> -------------------------------------------------

> >>>           irq_cb()

> >>>               virtqueue_get_buf()

> >>>

> >>> The used vring is always empty each time we call virtqueue_push() in

> >>> userspace. Not sure if it is what we expected.

> >>

> >> I'm not sure I get the issue.

> >>

> >> THe used ring should be filled by virtqueue_push() which is done by

> >> userspace before?

> >>

> > After userspace call virtqueue_push(), it always call virtio_notify()

> > immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

> > will inject an irq to VM and return, then vcpu thread will call

> > interrupt handler. But in container (virtio-vdpa) cases,

> > virtio_notify() will call interrupt handler directly. So it looks like

> > we have to optimize the virtio-vdpa cases. But one problem is we don't

> > know whether we are in the VM user case or container user case.

>

>

> Yes, but I still don't get why used ring is empty after the ioctl()?

> Used ring does not use bounce page so it should be visible to the kernel

> driver. What did I miss :) ?

>


Sorry, I'm not saying the kernel can't see the correct used vring. I
mean the kernel will consume the used vring in the ioctl context
directly in the virtio-vdpa case. In userspace's view, that means
virtqueue_push() is used vring's producer and virtio_notify() is used
vring's consumer. They will be called one by one in one thread rather
than different threads, which looks odd and has a bad effect on
performance.

Thanks,
Yongji
Jason Wang March 5, 2021, 7:01 a.m. UTC | #7
On 2021/3/5 2:36 下午, Yongji Xie wrote:
> On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:

>>

>> On 2021/3/5 11:30 上午, Yongji Xie wrote:

>>> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

>>>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

>>>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

>>>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

>>>>>>> This patch introduces a workqueue to support injecting

>>>>>>> virtqueue's interrupt asynchronously. This is mainly

>>>>>>> for performance considerations which makes sure the push()

>>>>>>> and pop() for used vring can be asynchronous.

>>>>>> Do you have pref numbers for this patch?

>>>>>>

>>>>> No, I can do some tests for it if needed.

>>>>>

>>>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

>>>>> if we call irq callback in ioctl context. Something like:

>>>>>

>>>>> virtqueue_push();

>>>>> virtio_notify();

>>>>>        ioctl()

>>>>> -------------------------------------------------

>>>>>            irq_cb()

>>>>>                virtqueue_get_buf()

>>>>>

>>>>> The used vring is always empty each time we call virtqueue_push() in

>>>>> userspace. Not sure if it is what we expected.

>>>> I'm not sure I get the issue.

>>>>

>>>> THe used ring should be filled by virtqueue_push() which is done by

>>>> userspace before?

>>>>

>>> After userspace call virtqueue_push(), it always call virtio_notify()

>>> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

>>> will inject an irq to VM and return, then vcpu thread will call

>>> interrupt handler. But in container (virtio-vdpa) cases,

>>> virtio_notify() will call interrupt handler directly. So it looks like

>>> we have to optimize the virtio-vdpa cases. But one problem is we don't

>>> know whether we are in the VM user case or container user case.

>>

>> Yes, but I still don't get why used ring is empty after the ioctl()?

>> Used ring does not use bounce page so it should be visible to the kernel

>> driver. What did I miss :) ?

>>

> Sorry, I'm not saying the kernel can't see the correct used vring. I

> mean the kernel will consume the used vring in the ioctl context

> directly in the virtio-vdpa case. In userspace's view, that means

> virtqueue_push() is used vring's producer and virtio_notify() is used

> vring's consumer. They will be called one by one in one thread rather

> than different threads, which looks odd and has a bad effect on

> performance.



Yes, that's why we need a workqueue (WQ_UNBOUND you used). Or do you 
want to squash this patch into patch 8?

So I think we can see obvious difference when virtio-vdpa is used.

Thanks


>

> Thanks,

> Yongji

>
Yongji Xie March 5, 2021, 7:27 a.m. UTC | #8
On Fri, Mar 5, 2021 at 3:01 PM Jason Wang <jasowang@redhat.com> wrote:
>

>

> On 2021/3/5 2:36 下午, Yongji Xie wrote:

> > On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:

> >>

> >> On 2021/3/5 11:30 上午, Yongji Xie wrote:

> >>> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

> >>>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

> >>>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

> >>>>>>> This patch introduces a workqueue to support injecting

> >>>>>>> virtqueue's interrupt asynchronously. This is mainly

> >>>>>>> for performance considerations which makes sure the push()

> >>>>>>> and pop() for used vring can be asynchronous.

> >>>>>> Do you have pref numbers for this patch?

> >>>>>>

> >>>>> No, I can do some tests for it if needed.

> >>>>>

> >>>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

> >>>>> if we call irq callback in ioctl context. Something like:

> >>>>>

> >>>>> virtqueue_push();

> >>>>> virtio_notify();

> >>>>>        ioctl()

> >>>>> -------------------------------------------------

> >>>>>            irq_cb()

> >>>>>                virtqueue_get_buf()

> >>>>>

> >>>>> The used vring is always empty each time we call virtqueue_push() in

> >>>>> userspace. Not sure if it is what we expected.

> >>>> I'm not sure I get the issue.

> >>>>

> >>>> THe used ring should be filled by virtqueue_push() which is done by

> >>>> userspace before?

> >>>>

> >>> After userspace call virtqueue_push(), it always call virtio_notify()

> >>> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

> >>> will inject an irq to VM and return, then vcpu thread will call

> >>> interrupt handler. But in container (virtio-vdpa) cases,

> >>> virtio_notify() will call interrupt handler directly. So it looks like

> >>> we have to optimize the virtio-vdpa cases. But one problem is we don't

> >>> know whether we are in the VM user case or container user case.

> >>

> >> Yes, but I still don't get why used ring is empty after the ioctl()?

> >> Used ring does not use bounce page so it should be visible to the kernel

> >> driver. What did I miss :) ?

> >>

> > Sorry, I'm not saying the kernel can't see the correct used vring. I

> > mean the kernel will consume the used vring in the ioctl context

> > directly in the virtio-vdpa case. In userspace's view, that means

> > virtqueue_push() is used vring's producer and virtio_notify() is used

> > vring's consumer. They will be called one by one in one thread rather

> > than different threads, which looks odd and has a bad effect on

> > performance.

>

>

> Yes, that's why we need a workqueue (WQ_UNBOUND you used). Or do you

> want to squash this patch into patch 8?

>

> So I think we can see obvious difference when virtio-vdpa is used.

>


But it looks like we don't need this workqueue in vhost-vdpa cases.
Any suggestions?

Thanks,
Yongji
Jason Wang March 5, 2021, 7:36 a.m. UTC | #9
On 2021/3/5 3:27 下午, Yongji Xie wrote:
> On Fri, Mar 5, 2021 at 3:01 PM Jason Wang <jasowang@redhat.com> wrote:

>>

>> On 2021/3/5 2:36 下午, Yongji Xie wrote:

>>> On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:

>>>> On 2021/3/5 11:30 上午, Yongji Xie wrote:

>>>>> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

>>>>>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

>>>>>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

>>>>>>>>> This patch introduces a workqueue to support injecting

>>>>>>>>> virtqueue's interrupt asynchronously. This is mainly

>>>>>>>>> for performance considerations which makes sure the push()

>>>>>>>>> and pop() for used vring can be asynchronous.

>>>>>>>> Do you have pref numbers for this patch?

>>>>>>>>

>>>>>>> No, I can do some tests for it if needed.

>>>>>>>

>>>>>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

>>>>>>> if we call irq callback in ioctl context. Something like:

>>>>>>>

>>>>>>> virtqueue_push();

>>>>>>> virtio_notify();

>>>>>>>         ioctl()

>>>>>>> -------------------------------------------------

>>>>>>>             irq_cb()

>>>>>>>                 virtqueue_get_buf()

>>>>>>>

>>>>>>> The used vring is always empty each time we call virtqueue_push() in

>>>>>>> userspace. Not sure if it is what we expected.

>>>>>> I'm not sure I get the issue.

>>>>>>

>>>>>> THe used ring should be filled by virtqueue_push() which is done by

>>>>>> userspace before?

>>>>>>

>>>>> After userspace call virtqueue_push(), it always call virtio_notify()

>>>>> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

>>>>> will inject an irq to VM and return, then vcpu thread will call

>>>>> interrupt handler. But in container (virtio-vdpa) cases,

>>>>> virtio_notify() will call interrupt handler directly. So it looks like

>>>>> we have to optimize the virtio-vdpa cases. But one problem is we don't

>>>>> know whether we are in the VM user case or container user case.

>>>> Yes, but I still don't get why used ring is empty after the ioctl()?

>>>> Used ring does not use bounce page so it should be visible to the kernel

>>>> driver. What did I miss :) ?

>>>>

>>> Sorry, I'm not saying the kernel can't see the correct used vring. I

>>> mean the kernel will consume the used vring in the ioctl context

>>> directly in the virtio-vdpa case. In userspace's view, that means

>>> virtqueue_push() is used vring's producer and virtio_notify() is used

>>> vring's consumer. They will be called one by one in one thread rather

>>> than different threads, which looks odd and has a bad effect on

>>> performance.

>>

>> Yes, that's why we need a workqueue (WQ_UNBOUND you used). Or do you

>> want to squash this patch into patch 8?

>>

>> So I think we can see obvious difference when virtio-vdpa is used.

>>

> But it looks like we don't need this workqueue in vhost-vdpa cases.

> Any suggestions?



I haven't had a deep thought. But I feel we can solve this by using the 
irq bypass manager (or something similar). Then we don't need it to be 
relayed via workqueue and vdpa. But I'm not sure how hard it will be.

Do you see any obvious performance regression by using the workqueue? Or 
we can optimize it in the future.

Thanks


>

> Thanks,

> Yongji

>
Yongji Xie March 5, 2021, 8:12 a.m. UTC | #10
On Fri, Mar 5, 2021 at 3:37 PM Jason Wang <jasowang@redhat.com> wrote:
>

>

> On 2021/3/5 3:27 下午, Yongji Xie wrote:

> > On Fri, Mar 5, 2021 at 3:01 PM Jason Wang <jasowang@redhat.com> wrote:

> >>

> >> On 2021/3/5 2:36 下午, Yongji Xie wrote:

> >>> On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:

> >>>> On 2021/3/5 11:30 上午, Yongji Xie wrote:

> >>>>> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

> >>>>>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

> >>>>>>>>> This patch introduces a workqueue to support injecting

> >>>>>>>>> virtqueue's interrupt asynchronously. This is mainly

> >>>>>>>>> for performance considerations which makes sure the push()

> >>>>>>>>> and pop() for used vring can be asynchronous.

> >>>>>>>> Do you have pref numbers for this patch?

> >>>>>>>>

> >>>>>>> No, I can do some tests for it if needed.

> >>>>>>>

> >>>>>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

> >>>>>>> if we call irq callback in ioctl context. Something like:

> >>>>>>>

> >>>>>>> virtqueue_push();

> >>>>>>> virtio_notify();

> >>>>>>>         ioctl()

> >>>>>>> -------------------------------------------------

> >>>>>>>             irq_cb()

> >>>>>>>                 virtqueue_get_buf()

> >>>>>>>

> >>>>>>> The used vring is always empty each time we call virtqueue_push() in

> >>>>>>> userspace. Not sure if it is what we expected.

> >>>>>> I'm not sure I get the issue.

> >>>>>>

> >>>>>> THe used ring should be filled by virtqueue_push() which is done by

> >>>>>> userspace before?

> >>>>>>

> >>>>> After userspace call virtqueue_push(), it always call virtio_notify()

> >>>>> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

> >>>>> will inject an irq to VM and return, then vcpu thread will call

> >>>>> interrupt handler. But in container (virtio-vdpa) cases,

> >>>>> virtio_notify() will call interrupt handler directly. So it looks like

> >>>>> we have to optimize the virtio-vdpa cases. But one problem is we don't

> >>>>> know whether we are in the VM user case or container user case.

> >>>> Yes, but I still don't get why used ring is empty after the ioctl()?

> >>>> Used ring does not use bounce page so it should be visible to the kernel

> >>>> driver. What did I miss :) ?

> >>>>

> >>> Sorry, I'm not saying the kernel can't see the correct used vring. I

> >>> mean the kernel will consume the used vring in the ioctl context

> >>> directly in the virtio-vdpa case. In userspace's view, that means

> >>> virtqueue_push() is used vring's producer and virtio_notify() is used

> >>> vring's consumer. They will be called one by one in one thread rather

> >>> than different threads, which looks odd and has a bad effect on

> >>> performance.

> >>

> >> Yes, that's why we need a workqueue (WQ_UNBOUND you used). Or do you

> >> want to squash this patch into patch 8?

> >>

> >> So I think we can see obvious difference when virtio-vdpa is used.

> >>

> > But it looks like we don't need this workqueue in vhost-vdpa cases.

> > Any suggestions?

>

>

> I haven't had a deep thought. But I feel we can solve this by using the

> irq bypass manager (or something similar). Then we don't need it to be

> relayed via workqueue and vdpa. But I'm not sure how hard it will be.

>


 Or let vdpa bus drivers give us some information?

> Do you see any obvious performance regression by using the workqueue? Or

> we can optimize it in the future.

>


Agree.

Thanks,
Yongji
Jason Wang March 8, 2021, 3:04 a.m. UTC | #11
On 2021/3/5 4:12 下午, Yongji Xie wrote:
> On Fri, Mar 5, 2021 at 3:37 PM Jason Wang <jasowang@redhat.com> wrote:

>>

>> On 2021/3/5 3:27 下午, Yongji Xie wrote:

>>> On Fri, Mar 5, 2021 at 3:01 PM Jason Wang <jasowang@redhat.com> wrote:

>>>> On 2021/3/5 2:36 下午, Yongji Xie wrote:

>>>>> On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:

>>>>>> On 2021/3/5 11:30 上午, Yongji Xie wrote:

>>>>>>> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

>>>>>>>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

>>>>>>>>>>> This patch introduces a workqueue to support injecting

>>>>>>>>>>> virtqueue's interrupt asynchronously. This is mainly

>>>>>>>>>>> for performance considerations which makes sure the push()

>>>>>>>>>>> and pop() for used vring can be asynchronous.

>>>>>>>>>> Do you have pref numbers for this patch?

>>>>>>>>>>

>>>>>>>>> No, I can do some tests for it if needed.

>>>>>>>>>

>>>>>>>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

>>>>>>>>> if we call irq callback in ioctl context. Something like:

>>>>>>>>>

>>>>>>>>> virtqueue_push();

>>>>>>>>> virtio_notify();

>>>>>>>>>          ioctl()

>>>>>>>>> -------------------------------------------------

>>>>>>>>>              irq_cb()

>>>>>>>>>                  virtqueue_get_buf()

>>>>>>>>>

>>>>>>>>> The used vring is always empty each time we call virtqueue_push() in

>>>>>>>>> userspace. Not sure if it is what we expected.

>>>>>>>> I'm not sure I get the issue.

>>>>>>>>

>>>>>>>> THe used ring should be filled by virtqueue_push() which is done by

>>>>>>>> userspace before?

>>>>>>>>

>>>>>>> After userspace call virtqueue_push(), it always call virtio_notify()

>>>>>>> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

>>>>>>> will inject an irq to VM and return, then vcpu thread will call

>>>>>>> interrupt handler. But in container (virtio-vdpa) cases,

>>>>>>> virtio_notify() will call interrupt handler directly. So it looks like

>>>>>>> we have to optimize the virtio-vdpa cases. But one problem is we don't

>>>>>>> know whether we are in the VM user case or container user case.

>>>>>> Yes, but I still don't get why used ring is empty after the ioctl()?

>>>>>> Used ring does not use bounce page so it should be visible to the kernel

>>>>>> driver. What did I miss :) ?

>>>>>>

>>>>> Sorry, I'm not saying the kernel can't see the correct used vring. I

>>>>> mean the kernel will consume the used vring in the ioctl context

>>>>> directly in the virtio-vdpa case. In userspace's view, that means

>>>>> virtqueue_push() is used vring's producer and virtio_notify() is used

>>>>> vring's consumer. They will be called one by one in one thread rather

>>>>> than different threads, which looks odd and has a bad effect on

>>>>> performance.

>>>> Yes, that's why we need a workqueue (WQ_UNBOUND you used). Or do you

>>>> want to squash this patch into patch 8?

>>>>

>>>> So I think we can see obvious difference when virtio-vdpa is used.

>>>>

>>> But it looks like we don't need this workqueue in vhost-vdpa cases.

>>> Any suggestions?

>>

>> I haven't had a deep thought. But I feel we can solve this by using the

>> irq bypass manager (or something similar). Then we don't need it to be

>> relayed via workqueue and vdpa. But I'm not sure how hard it will be.

>>

>   Or let vdpa bus drivers give us some information?



This kind of 'type' is proposed in the early RFC of vDPA series. One 
issue is that at device level, we should not differ virtio from vhost, 
so if we introduce that, it might encourge people to design a device 
that is dedicated to vhost or virtio which might not be good.

But we can re-visit this when necessary.

Thanks


>

>> Do you see any obvious performance regression by using the workqueue? Or

>> we can optimize it in the future.

>>

> Agree.

>

> Thanks,

> Yongji

>
Yongji Xie March 8, 2021, 4:50 a.m. UTC | #12
On Mon, Mar 8, 2021 at 11:04 AM Jason Wang <jasowang@redhat.com> wrote:
>

>

> On 2021/3/5 4:12 下午, Yongji Xie wrote:

> > On Fri, Mar 5, 2021 at 3:37 PM Jason Wang <jasowang@redhat.com> wrote:

> >>

> >> On 2021/3/5 3:27 下午, Yongji Xie wrote:

> >>> On Fri, Mar 5, 2021 at 3:01 PM Jason Wang <jasowang@redhat.com> wrote:

> >>>> On 2021/3/5 2:36 下午, Yongji Xie wrote:

> >>>>> On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>> On 2021/3/5 11:30 上午, Yongji Xie wrote:

> >>>>>>> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>>>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

> >>>>>>>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>>>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

> >>>>>>>>>>> This patch introduces a workqueue to support injecting

> >>>>>>>>>>> virtqueue's interrupt asynchronously. This is mainly

> >>>>>>>>>>> for performance considerations which makes sure the push()

> >>>>>>>>>>> and pop() for used vring can be asynchronous.

> >>>>>>>>>> Do you have pref numbers for this patch?

> >>>>>>>>>>

> >>>>>>>>> No, I can do some tests for it if needed.

> >>>>>>>>>

> >>>>>>>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

> >>>>>>>>> if we call irq callback in ioctl context. Something like:

> >>>>>>>>>

> >>>>>>>>> virtqueue_push();

> >>>>>>>>> virtio_notify();

> >>>>>>>>>          ioctl()

> >>>>>>>>> -------------------------------------------------

> >>>>>>>>>              irq_cb()

> >>>>>>>>>                  virtqueue_get_buf()

> >>>>>>>>>

> >>>>>>>>> The used vring is always empty each time we call virtqueue_push() in

> >>>>>>>>> userspace. Not sure if it is what we expected.

> >>>>>>>> I'm not sure I get the issue.

> >>>>>>>>

> >>>>>>>> THe used ring should be filled by virtqueue_push() which is done by

> >>>>>>>> userspace before?

> >>>>>>>>

> >>>>>>> After userspace call virtqueue_push(), it always call virtio_notify()

> >>>>>>> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

> >>>>>>> will inject an irq to VM and return, then vcpu thread will call

> >>>>>>> interrupt handler. But in container (virtio-vdpa) cases,

> >>>>>>> virtio_notify() will call interrupt handler directly. So it looks like

> >>>>>>> we have to optimize the virtio-vdpa cases. But one problem is we don't

> >>>>>>> know whether we are in the VM user case or container user case.

> >>>>>> Yes, but I still don't get why used ring is empty after the ioctl()?

> >>>>>> Used ring does not use bounce page so it should be visible to the kernel

> >>>>>> driver. What did I miss :) ?

> >>>>>>

> >>>>> Sorry, I'm not saying the kernel can't see the correct used vring. I

> >>>>> mean the kernel will consume the used vring in the ioctl context

> >>>>> directly in the virtio-vdpa case. In userspace's view, that means

> >>>>> virtqueue_push() is used vring's producer and virtio_notify() is used

> >>>>> vring's consumer. They will be called one by one in one thread rather

> >>>>> than different threads, which looks odd and has a bad effect on

> >>>>> performance.

> >>>> Yes, that's why we need a workqueue (WQ_UNBOUND you used). Or do you

> >>>> want to squash this patch into patch 8?

> >>>>

> >>>> So I think we can see obvious difference when virtio-vdpa is used.

> >>>>

> >>> But it looks like we don't need this workqueue in vhost-vdpa cases.

> >>> Any suggestions?

> >>

> >> I haven't had a deep thought. But I feel we can solve this by using the

> >> irq bypass manager (or something similar). Then we don't need it to be

> >> relayed via workqueue and vdpa. But I'm not sure how hard it will be.

> >>

> >   Or let vdpa bus drivers give us some information?

>

>

> This kind of 'type' is proposed in the early RFC of vDPA series. One

> issue is that at device level, we should not differ virtio from vhost,

> so if we introduce that, it might encourge people to design a device

> that is dedicated to vhost or virtio which might not be good.

>

> But we can re-visit this when necessary.

>


OK, I see. How about adding some information in ops.set_vq_cb()?

Thanks,
Yongji
Jason Wang March 8, 2021, 7:01 a.m. UTC | #13
On 2021/3/8 12:50 下午, Yongji Xie wrote:
> On Mon, Mar 8, 2021 at 11:04 AM Jason Wang <jasowang@redhat.com> wrote:

>>

>> On 2021/3/5 4:12 下午, Yongji Xie wrote:

>>> On Fri, Mar 5, 2021 at 3:37 PM Jason Wang <jasowang@redhat.com> wrote:

>>>> On 2021/3/5 3:27 下午, Yongji Xie wrote:

>>>>> On Fri, Mar 5, 2021 at 3:01 PM Jason Wang <jasowang@redhat.com> wrote:

>>>>>> On 2021/3/5 2:36 下午, Yongji Xie wrote:

>>>>>>> On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>> On 2021/3/5 11:30 上午, Yongji Xie wrote:

>>>>>>>>> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>>>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

>>>>>>>>>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>>>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

>>>>>>>>>>>>> This patch introduces a workqueue to support injecting

>>>>>>>>>>>>> virtqueue's interrupt asynchronously. This is mainly

>>>>>>>>>>>>> for performance considerations which makes sure the push()

>>>>>>>>>>>>> and pop() for used vring can be asynchronous.

>>>>>>>>>>>> Do you have pref numbers for this patch?

>>>>>>>>>>>>

>>>>>>>>>>> No, I can do some tests for it if needed.

>>>>>>>>>>>

>>>>>>>>>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

>>>>>>>>>>> if we call irq callback in ioctl context. Something like:

>>>>>>>>>>>

>>>>>>>>>>> virtqueue_push();

>>>>>>>>>>> virtio_notify();

>>>>>>>>>>>           ioctl()

>>>>>>>>>>> -------------------------------------------------

>>>>>>>>>>>               irq_cb()

>>>>>>>>>>>                   virtqueue_get_buf()

>>>>>>>>>>>

>>>>>>>>>>> The used vring is always empty each time we call virtqueue_push() in

>>>>>>>>>>> userspace. Not sure if it is what we expected.

>>>>>>>>>> I'm not sure I get the issue.

>>>>>>>>>>

>>>>>>>>>> THe used ring should be filled by virtqueue_push() which is done by

>>>>>>>>>> userspace before?

>>>>>>>>>>

>>>>>>>>> After userspace call virtqueue_push(), it always call virtio_notify()

>>>>>>>>> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

>>>>>>>>> will inject an irq to VM and return, then vcpu thread will call

>>>>>>>>> interrupt handler. But in container (virtio-vdpa) cases,

>>>>>>>>> virtio_notify() will call interrupt handler directly. So it looks like

>>>>>>>>> we have to optimize the virtio-vdpa cases. But one problem is we don't

>>>>>>>>> know whether we are in the VM user case or container user case.

>>>>>>>> Yes, but I still don't get why used ring is empty after the ioctl()?

>>>>>>>> Used ring does not use bounce page so it should be visible to the kernel

>>>>>>>> driver. What did I miss :) ?

>>>>>>>>

>>>>>>> Sorry, I'm not saying the kernel can't see the correct used vring. I

>>>>>>> mean the kernel will consume the used vring in the ioctl context

>>>>>>> directly in the virtio-vdpa case. In userspace's view, that means

>>>>>>> virtqueue_push() is used vring's producer and virtio_notify() is used

>>>>>>> vring's consumer. They will be called one by one in one thread rather

>>>>>>> than different threads, which looks odd and has a bad effect on

>>>>>>> performance.

>>>>>> Yes, that's why we need a workqueue (WQ_UNBOUND you used). Or do you

>>>>>> want to squash this patch into patch 8?

>>>>>>

>>>>>> So I think we can see obvious difference when virtio-vdpa is used.

>>>>>>

>>>>> But it looks like we don't need this workqueue in vhost-vdpa cases.

>>>>> Any suggestions?

>>>> I haven't had a deep thought. But I feel we can solve this by using the

>>>> irq bypass manager (or something similar). Then we don't need it to be

>>>> relayed via workqueue and vdpa. But I'm not sure how hard it will be.

>>>>

>>>    Or let vdpa bus drivers give us some information?

>>

>> This kind of 'type' is proposed in the early RFC of vDPA series. One

>> issue is that at device level, we should not differ virtio from vhost,

>> so if we introduce that, it might encourge people to design a device

>> that is dedicated to vhost or virtio which might not be good.

>>

>> But we can re-visit this when necessary.

>>

> OK, I see. How about adding some information in ops.set_vq_cb()?



I'm not sure I get this, maybe you can explain a little bit more?

Thanks


>

> Thanks,

> Yongji

>
Yongji Xie March 8, 2021, 7:16 a.m. UTC | #14
On Mon, Mar 8, 2021 at 3:02 PM Jason Wang <jasowang@redhat.com> wrote:
>

>

> On 2021/3/8 12:50 下午, Yongji Xie wrote:

> > On Mon, Mar 8, 2021 at 11:04 AM Jason Wang <jasowang@redhat.com> wrote:

> >>

> >> On 2021/3/5 4:12 下午, Yongji Xie wrote:

> >>> On Fri, Mar 5, 2021 at 3:37 PM Jason Wang <jasowang@redhat.com> wrote:

> >>>> On 2021/3/5 3:27 下午, Yongji Xie wrote:

> >>>>> On Fri, Mar 5, 2021 at 3:01 PM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>> On 2021/3/5 2:36 下午, Yongji Xie wrote:

> >>>>>>> On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>>>> On 2021/3/5 11:30 上午, Yongji Xie wrote:

> >>>>>>>>> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>>>>>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

> >>>>>>>>>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

> >>>>>>>>>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

> >>>>>>>>>>>>> This patch introduces a workqueue to support injecting

> >>>>>>>>>>>>> virtqueue's interrupt asynchronously. This is mainly

> >>>>>>>>>>>>> for performance considerations which makes sure the push()

> >>>>>>>>>>>>> and pop() for used vring can be asynchronous.

> >>>>>>>>>>>> Do you have pref numbers for this patch?

> >>>>>>>>>>>>

> >>>>>>>>>>> No, I can do some tests for it if needed.

> >>>>>>>>>>>

> >>>>>>>>>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

> >>>>>>>>>>> if we call irq callback in ioctl context. Something like:

> >>>>>>>>>>>

> >>>>>>>>>>> virtqueue_push();

> >>>>>>>>>>> virtio_notify();

> >>>>>>>>>>>           ioctl()

> >>>>>>>>>>> -------------------------------------------------

> >>>>>>>>>>>               irq_cb()

> >>>>>>>>>>>                   virtqueue_get_buf()

> >>>>>>>>>>>

> >>>>>>>>>>> The used vring is always empty each time we call virtqueue_push() in

> >>>>>>>>>>> userspace. Not sure if it is what we expected.

> >>>>>>>>>> I'm not sure I get the issue.

> >>>>>>>>>>

> >>>>>>>>>> THe used ring should be filled by virtqueue_push() which is done by

> >>>>>>>>>> userspace before?

> >>>>>>>>>>

> >>>>>>>>> After userspace call virtqueue_push(), it always call virtio_notify()

> >>>>>>>>> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

> >>>>>>>>> will inject an irq to VM and return, then vcpu thread will call

> >>>>>>>>> interrupt handler. But in container (virtio-vdpa) cases,

> >>>>>>>>> virtio_notify() will call interrupt handler directly. So it looks like

> >>>>>>>>> we have to optimize the virtio-vdpa cases. But one problem is we don't

> >>>>>>>>> know whether we are in the VM user case or container user case.

> >>>>>>>> Yes, but I still don't get why used ring is empty after the ioctl()?

> >>>>>>>> Used ring does not use bounce page so it should be visible to the kernel

> >>>>>>>> driver. What did I miss :) ?

> >>>>>>>>

> >>>>>>> Sorry, I'm not saying the kernel can't see the correct used vring. I

> >>>>>>> mean the kernel will consume the used vring in the ioctl context

> >>>>>>> directly in the virtio-vdpa case. In userspace's view, that means

> >>>>>>> virtqueue_push() is used vring's producer and virtio_notify() is used

> >>>>>>> vring's consumer. They will be called one by one in one thread rather

> >>>>>>> than different threads, which looks odd and has a bad effect on

> >>>>>>> performance.

> >>>>>> Yes, that's why we need a workqueue (WQ_UNBOUND you used). Or do you

> >>>>>> want to squash this patch into patch 8?

> >>>>>>

> >>>>>> So I think we can see obvious difference when virtio-vdpa is used.

> >>>>>>

> >>>>> But it looks like we don't need this workqueue in vhost-vdpa cases.

> >>>>> Any suggestions?

> >>>> I haven't had a deep thought. But I feel we can solve this by using the

> >>>> irq bypass manager (or something similar). Then we don't need it to be

> >>>> relayed via workqueue and vdpa. But I'm not sure how hard it will be.

> >>>>

> >>>    Or let vdpa bus drivers give us some information?

> >>

> >> This kind of 'type' is proposed in the early RFC of vDPA series. One

> >> issue is that at device level, we should not differ virtio from vhost,

> >> so if we introduce that, it might encourge people to design a device

> >> that is dedicated to vhost or virtio which might not be good.

> >>

> >> But we can re-visit this when necessary.

> >>

> > OK, I see. How about adding some information in ops.set_vq_cb()?

>

>

> I'm not sure I get this, maybe you can explain a little bit more?

>


For example, add an extra parameter for ops.set_vq_cb() to indicate
whether this callback will trigger the interrupt handler directly.

Thanks,
Yongji
Jason Wang March 8, 2021, 7:29 a.m. UTC | #15
On 2021/3/8 3:16 下午, Yongji Xie wrote:
> On Mon, Mar 8, 2021 at 3:02 PM Jason Wang <jasowang@redhat.com> wrote:

>>

>> On 2021/3/8 12:50 下午, Yongji Xie wrote:

>>> On Mon, Mar 8, 2021 at 11:04 AM Jason Wang <jasowang@redhat.com> wrote:

>>>> On 2021/3/5 4:12 下午, Yongji Xie wrote:

>>>>> On Fri, Mar 5, 2021 at 3:37 PM Jason Wang <jasowang@redhat.com> wrote:

>>>>>> On 2021/3/5 3:27 下午, Yongji Xie wrote:

>>>>>>> On Fri, Mar 5, 2021 at 3:01 PM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>> On 2021/3/5 2:36 下午, Yongji Xie wrote:

>>>>>>>>> On Fri, Mar 5, 2021 at 11:42 AM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>>>> On 2021/3/5 11:30 上午, Yongji Xie wrote:

>>>>>>>>>>> On Fri, Mar 5, 2021 at 11:05 AM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>>>>>> On 2021/3/4 4:58 下午, Yongji Xie wrote:

>>>>>>>>>>>>> On Thu, Mar 4, 2021 at 2:59 PM Jason Wang <jasowang@redhat.com> wrote:

>>>>>>>>>>>>>> On 2021/2/23 7:50 下午, Xie Yongji wrote:

>>>>>>>>>>>>>>> This patch introduces a workqueue to support injecting

>>>>>>>>>>>>>>> virtqueue's interrupt asynchronously. This is mainly

>>>>>>>>>>>>>>> for performance considerations which makes sure the push()

>>>>>>>>>>>>>>> and pop() for used vring can be asynchronous.

>>>>>>>>>>>>>> Do you have pref numbers for this patch?

>>>>>>>>>>>>>>

>>>>>>>>>>>>> No, I can do some tests for it if needed.

>>>>>>>>>>>>>

>>>>>>>>>>>>> Another problem is the VIRTIO_RING_F_EVENT_IDX feature will be useless

>>>>>>>>>>>>> if we call irq callback in ioctl context. Something like:

>>>>>>>>>>>>>

>>>>>>>>>>>>> virtqueue_push();

>>>>>>>>>>>>> virtio_notify();

>>>>>>>>>>>>>            ioctl()

>>>>>>>>>>>>> -------------------------------------------------

>>>>>>>>>>>>>                irq_cb()

>>>>>>>>>>>>>                    virtqueue_get_buf()

>>>>>>>>>>>>>

>>>>>>>>>>>>> The used vring is always empty each time we call virtqueue_push() in

>>>>>>>>>>>>> userspace. Not sure if it is what we expected.

>>>>>>>>>>>> I'm not sure I get the issue.

>>>>>>>>>>>>

>>>>>>>>>>>> THe used ring should be filled by virtqueue_push() which is done by

>>>>>>>>>>>> userspace before?

>>>>>>>>>>>>

>>>>>>>>>>> After userspace call virtqueue_push(), it always call virtio_notify()

>>>>>>>>>>> immediately. In traditional VM (vhost-vdpa) cases, virtio_notify()

>>>>>>>>>>> will inject an irq to VM and return, then vcpu thread will call

>>>>>>>>>>> interrupt handler. But in container (virtio-vdpa) cases,

>>>>>>>>>>> virtio_notify() will call interrupt handler directly. So it looks like

>>>>>>>>>>> we have to optimize the virtio-vdpa cases. But one problem is we don't

>>>>>>>>>>> know whether we are in the VM user case or container user case.

>>>>>>>>>> Yes, but I still don't get why used ring is empty after the ioctl()?

>>>>>>>>>> Used ring does not use bounce page so it should be visible to the kernel

>>>>>>>>>> driver. What did I miss :) ?

>>>>>>>>>>

>>>>>>>>> Sorry, I'm not saying the kernel can't see the correct used vring. I

>>>>>>>>> mean the kernel will consume the used vring in the ioctl context

>>>>>>>>> directly in the virtio-vdpa case. In userspace's view, that means

>>>>>>>>> virtqueue_push() is used vring's producer and virtio_notify() is used

>>>>>>>>> vring's consumer. They will be called one by one in one thread rather

>>>>>>>>> than different threads, which looks odd and has a bad effect on

>>>>>>>>> performance.

>>>>>>>> Yes, that's why we need a workqueue (WQ_UNBOUND you used). Or do you

>>>>>>>> want to squash this patch into patch 8?

>>>>>>>>

>>>>>>>> So I think we can see obvious difference when virtio-vdpa is used.

>>>>>>>>

>>>>>>> But it looks like we don't need this workqueue in vhost-vdpa cases.

>>>>>>> Any suggestions?

>>>>>> I haven't had a deep thought. But I feel we can solve this by using the

>>>>>> irq bypass manager (or something similar). Then we don't need it to be

>>>>>> relayed via workqueue and vdpa. But I'm not sure how hard it will be.

>>>>>>

>>>>>     Or let vdpa bus drivers give us some information?

>>>> This kind of 'type' is proposed in the early RFC of vDPA series. One

>>>> issue is that at device level, we should not differ virtio from vhost,

>>>> so if we introduce that, it might encourge people to design a device

>>>> that is dedicated to vhost or virtio which might not be good.

>>>>

>>>> But we can re-visit this when necessary.

>>>>

>>> OK, I see. How about adding some information in ops.set_vq_cb()?

>>

>> I'm not sure I get this, maybe you can explain a little bit more?

>>

> For example, add an extra parameter for ops.set_vq_cb() to indicate

> whether this callback will trigger the interrupt handler directly.



Sounds intersting. I think it may work.

Thanks


>

> Thanks,

> Yongji

>
diff mbox series

Patch

diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c
index 8042d3fa57f1..f5adeb9ee027 100644
--- a/drivers/vdpa/vdpa_user/vduse_dev.c
+++ b/drivers/vdpa/vdpa_user/vduse_dev.c
@@ -42,6 +42,7 @@  struct vduse_virtqueue {
 	spinlock_t irq_lock;
 	struct eventfd_ctx *kickfd;
 	struct vdpa_callback cb;
+	struct work_struct inject;
 };
 
 struct vduse_dev;
@@ -99,6 +100,7 @@  static DEFINE_IDA(vduse_ida);
 
 static dev_t vduse_major;
 static struct class *vduse_class;
+static struct workqueue_struct *vduse_irq_wq;
 
 static inline struct vduse_dev *vdpa_to_vduse(struct vdpa_device *vdpa)
 {
@@ -852,6 +854,17 @@  static int vduse_kickfd_setup(struct vduse_dev *dev,
 	return 0;
 }
 
+static void vduse_vq_irq_inject(struct work_struct *work)
+{
+	struct vduse_virtqueue *vq = container_of(work,
+					struct vduse_virtqueue, inject);
+
+	spin_lock_irq(&vq->irq_lock);
+	if (vq->ready && vq->cb.callback)
+		vq->cb.callback(vq->cb.private);
+	spin_unlock_irq(&vq->irq_lock);
+}
+
 static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 			unsigned long arg)
 {
@@ -917,12 +930,7 @@  static long vduse_dev_ioctl(struct file *file, unsigned int cmd,
 			break;
 
 		vq = &dev->vqs[arg];
-		spin_lock_irq(&vq->irq_lock);
-		if (vq->ready && vq->cb.callback) {
-			vq->cb.callback(vq->cb.private);
-			ret = 0;
-		}
-		spin_unlock_irq(&vq->irq_lock);
+		queue_work(vduse_irq_wq, &vq->inject);
 		break;
 	}
 	case VDUSE_INJECT_CONFIG_IRQ:
@@ -1109,6 +1117,7 @@  static int vduse_create_dev(struct vduse_dev_config *config)
 
 	for (i = 0; i < dev->vq_num; i++) {
 		dev->vqs[i].index = i;
+		INIT_WORK(&dev->vqs[i].inject, vduse_vq_irq_inject);
 		spin_lock_init(&dev->vqs[i].kick_lock);
 		spin_lock_init(&dev->vqs[i].irq_lock);
 	}
@@ -1333,6 +1342,11 @@  static int vduse_init(void)
 	if (ret)
 		goto err_chardev;
 
+	vduse_irq_wq = alloc_workqueue("vduse-irq",
+				WQ_HIGHPRI | WQ_SYSFS | WQ_UNBOUND, 0);
+	if (!vduse_irq_wq)
+		goto err_wq;
+
 	ret = vduse_domain_init();
 	if (ret)
 		goto err_domain;
@@ -1344,6 +1358,8 @@  static int vduse_init(void)
 	return 0;
 err_mgmtdev:
 	vduse_domain_exit();
+err_wq:
+	destroy_workqueue(vduse_irq_wq);
 err_domain:
 	unregister_chrdev_region(vduse_major, VDUSE_DEV_MAX);
 err_chardev:
@@ -1359,6 +1375,7 @@  static void vduse_exit(void)
 	misc_deregister(&vduse_misc);
 	class_destroy(vduse_class);
 	unregister_chrdev_region(vduse_major, VDUSE_DEV_MAX);
+	destroy_workqueue(vduse_irq_wq);
 	vduse_domain_exit();
 	vduse_mgmtdev_exit();
 }