virtio_ring: Suppress tx interrupt when napi_tx disable

  From: "huangjie.albert" <huangjie.albert@bytedance.com>

fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")

if we disable the napi_tx. when we triger a tx interrupt, the
vq->event_triggered will be set to true. It will no longer be
set to false. Unless we explicitly call virtqueue_enable_cb_delayed
or virtqueue_enable_cb_prepare

if we disable the napi_tx, It will only be called when the tx ring
buffer is relatively small:
virtio_net->start_xmit:
	if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
		netif_stop_subqueue(dev, qnum);
		if (!use_napi &&
		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
			/* More just got used, free them then recheck. */
			free_old_xmit_skbs(sq, false);
			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
				netif_start_subqueue(dev, qnum);
				virtqueue_disable_cb(sq->vq);
			}
		}
	}
Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
every time we call virtqueue_get_buf_ctx.This will bring more interruptions.

if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
or vq->packed.vring.driver->off_wrap

Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
---
 drivers/virtio/virtio_ring.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
  

On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
<huangjie.albert@bytedance.com> wrote:
>
> From: "huangjie.albert" <huangjie.albert@bytedance.com>
>
> fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
>
> if we disable the napi_tx. when we triger a tx interrupt, the

typo should be "trigger"

> vq->event_triggered will be set to true. It will no longer be
> set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> or virtqueue_enable_cb_prepare
>
> if we disable the napi_tx, It will only be called when the tx ring
> buffer is relatively small:
> virtio_net->start_xmit:
>         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
>                 netif_stop_subqueue(dev, qnum);
>                 if (!use_napi &&
>                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
>                         /* More just got used, free them then recheck. */
>                         free_old_xmit_skbs(sq, false);
>                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
>                                 netif_start_subqueue(dev, qnum);
>                                 virtqueue_disable_cb(sq->vq);
>                         }

The code example here is out of date, make sure your tree has this:

commit d71ebe8114b4bf622804b810f5e274069060a174
Author: Jason Wang <jasowang@redhat.com>
Date:   Tue Jan 17 11:47:07 2023 +0800

    virtio-net: correctly enable callback during start_xmit

>                 }
>         }
> Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> every time we call virtqueue_get_buf_ctx.This will bring more interruptions.

Can you please post how to test with the performance numbers?

>
> if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> or vq->packed.vring.driver->off_wrap
>
> Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> ---
>  drivers/virtio/virtio_ring.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 307e139cb11d..f486cccadbeb 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
>         /* If we expect an interrupt for the next entry, tell host
>          * by writing event index and flush out the write before
>          * the read in the next get_buf call. */
> -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> +                       && (vq->event_triggered == false))

I'm not sure this can work, when event_triggered is true it means
we've got an interrupt, in this case if we want another interrupt for
the next entry, we should update used_event otherwise we will lose
that interrupt?

Thanks

>                 virtio_store_mb(vq->weak_barriers,
>                                 &vring_used_event(&vq->split.vring),
>                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
>          * by writing event index and flush out the write before
>          * the read in the next get_buf call.
>          */
> -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> +                       && (vq->event_triggered == false))
>                 virtio_store_mb(vq->weak_barriers,
>                                 &vq->packed.vring.driver->off_wrap,
>                                 cpu_to_le16(vq->last_used_idx));
> --
> 2.31.1
>
  

Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道：
>
> On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
> <huangjie.albert@bytedance.com> wrote:
> >
> > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> >
> > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> >
> > if we disable the napi_tx. when we triger a tx interrupt, the
>
> typo should be "trigger"
>

OK, thanks for this. I will correct it in the next version

> > vq->event_triggered will be set to true. It will no longer be
> > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > or virtqueue_enable_cb_prepare
> >
> > if we disable the napi_tx, It will only be called when the tx ring
> > buffer is relatively small:
> > virtio_net->start_xmit:
> >         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> >                 netif_stop_subqueue(dev, qnum);
> >                 if (!use_napi &&
> >                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> >                         /* More just got used, free them then recheck. */
> >                         free_old_xmit_skbs(sq, false);
> >                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> >                                 netif_start_subqueue(dev, qnum);
> >                                 virtqueue_disable_cb(sq->vq);
> >                         }
>
> The code example here is out of date, make sure your tree has this:

also, I will correct it in the next version，this is from kernel 5.15.

>
> commit d71ebe8114b4bf622804b810f5e274069060a174
> Author: Jason Wang <jasowang@redhat.com>
> Date:   Tue Jan 17 11:47:07 2023 +0800
>
>     virtio-net: correctly enable callback during start_xmit
>
> >                 }
> >         }
> > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
>
> Can you please post how to test with the performance numbers?
>

iperf3 tcp stream:
vm1 -----------------> vm2
vm2 just receive tcp data stream from vm1, and send the ack to vm1,
there are so
many tx interruptions  in vm2.

but without event_triggered there are just a few tx interruptions.

> >
> > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > or vq->packed.vring.driver->off_wrap
> >
> > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > ---
> >  drivers/virtio/virtio_ring.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 307e139cb11d..f486cccadbeb 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> >         /* If we expect an interrupt for the next entry, tell host
> >          * by writing event index and flush out the write before
> >          * the read in the next get_buf call. */
> > -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > +                       && (vq->event_triggered == false))
>
> I'm not sure this can work, when event_triggered is true it means
> we've got an interrupt, in this case if we want another interrupt for
> the next entry, we should update used_event otherwise we will lose
> that interrupt?
>
> Thanks

Normally, if we receive an interrupt, we should disable the interrupt
in the interrupt callback handler.
But because of the introduction of event_triggered, here,
virtqueue_get_buf_ctx_split  cannot be recognized
that the interrupt has been turned off.

if we want  another interrupt for the next entry, We should probably
call virtqueue_enable_cb？

Thanks

>
> >                 virtio_store_mb(vq->weak_barriers,
> >                                 &vring_used_event(&vq->split.vring),
> >                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> >          * by writing event index and flush out the write before
> >          * the read in the next get_buf call.
> >          */
> > -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > +                       && (vq->event_triggered == false))
> >                 virtio_store_mb(vq->weak_barriers,
> >                                 &vq->packed.vring.driver->off_wrap,
> >                                 cpu_to_le16(vq->last_used_idx));
> > --
> > 2.31.1
> >
>
  

On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote:
>
> Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道：
> >
> > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
> > <huangjie.albert@bytedance.com> wrote:
> > >
> > > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> > >
> > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > >
> > > if we disable the napi_tx. when we triger a tx interrupt, the
> >
> > typo should be "trigger"
> >
>
> OK, thanks for this. I will correct it in the next version
>
> > > vq->event_triggered will be set to true. It will no longer be
> > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > > or virtqueue_enable_cb_prepare
> > >
> > > if we disable the napi_tx, It will only be called when the tx ring
> > > buffer is relatively small:
> > > virtio_net->start_xmit:
> > >         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> > >                 netif_stop_subqueue(dev, qnum);
> > >                 if (!use_napi &&
> > >                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> > >                         /* More just got used, free them then recheck. */
> > >                         free_old_xmit_skbs(sq, false);
> > >                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> > >                                 netif_start_subqueue(dev, qnum);
> > >                                 virtqueue_disable_cb(sq->vq);
> > >                         }
> >
> > The code example here is out of date, make sure your tree has this:
>
> also, I will correct it in the next version，this is from kernel 5.15.
>
> >
> > commit d71ebe8114b4bf622804b810f5e274069060a174
> > Author: Jason Wang <jasowang@redhat.com>
> > Date:   Tue Jan 17 11:47:07 2023 +0800
> >
> >     virtio-net: correctly enable callback during start_xmit
> >
> > >                 }
> > >         }
> > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> >
> > Can you please post how to test with the performance numbers?
> >
>
> iperf3 tcp stream:
> vm1 -----------------> vm2
> vm2 just receive tcp data stream from vm1, and send the ack to vm1,
> there are so
> many tx interruptions  in vm2.
>
> but without event_triggered there are just a few tx interruptions.
>
> > >
> > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > > or vq->packed.vring.driver->off_wrap
> > >
> > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > > ---
> > >  drivers/virtio/virtio_ring.c | 6 ++++--
> > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > index 307e139cb11d..f486cccadbeb 100644
> > > --- a/drivers/virtio/virtio_ring.c
> > > +++ b/drivers/virtio/virtio_ring.c
> > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> > >         /* If we expect an interrupt for the next entry, tell host
> > >          * by writing event index and flush out the write before
> > >          * the read in the next get_buf call. */
> > > -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > > +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > > +                       && (vq->event_triggered == false))
> >
> > I'm not sure this can work, when event_triggered is true it means
> > we've got an interrupt, in this case if we want another interrupt for
> > the next entry, we should update used_event otherwise we will lose
> > that interrupt?
> >
> > Thanks
>
> Normally, if we receive an interrupt, we should disable the interrupt
> in the interrupt callback handler.

So the problem is:

1) event_triggered was set to true in vring_interrupt()
2) after this nothing will happen for virtqueue_disable_cb() so
VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow
3) virtqueue_get_buf_ctx_split() will still think the cb is enabled
then it tries to publish new event

This makes me think about whether or not we really need
event_triggered. The assumption in the virtqueue_disable_cb() seems
wrong:

/* If device triggered an event already it won't trigger one again:
 * no need to disable.
 */
if (vq->event_triggered)
                return;

This is wrong if there's no event index support. And the
event_triggered is somehow duplicated with the
VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix
might be:

1) remove event_triggered
2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in
vring_interrrupt if event index is supported

?

Thanks


> But because of the introduction of event_triggered, here,
> virtqueue_get_buf_ctx_split  cannot be recognized
> that the interrupt has been turned off.
>
> if we want  another interrupt for the next entry, We should probably
> call virtqueue_enable_cb？
>
> Thanks
>
> >
> > >                 virtio_store_mb(vq->weak_barriers,
> > >                                 &vring_used_event(&vq->split.vring),
> > >                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > >          * by writing event index and flush out the write before
> > >          * the read in the next get_buf call.
> > >          */
> > > -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > > +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > > +                       && (vq->event_triggered == false))
> > >                 virtio_store_mb(vq->weak_barriers,
> > >                                 &vq->packed.vring.driver->off_wrap,
> > >                                 cpu_to_le16(vq->last_used_idx));
> > > --
> > > 2.31.1
> > >
> >
>
  

On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote:
> On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote:
> >
> > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道：
> > >
> > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
> > > <huangjie.albert@bytedance.com> wrote:
> > > >
> > > > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> > > >
> > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > >
> > > > if we disable the napi_tx. when we triger a tx interrupt, the
> > >
> > > typo should be "trigger"
> > >
> >
> > OK, thanks for this. I will correct it in the next version
> >
> > > > vq->event_triggered will be set to true. It will no longer be
> > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > > > or virtqueue_enable_cb_prepare
> > > >
> > > > if we disable the napi_tx, It will only be called when the tx ring
> > > > buffer is relatively small:
> > > > virtio_net->start_xmit:
> > > >         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> > > >                 netif_stop_subqueue(dev, qnum);
> > > >                 if (!use_napi &&
> > > >                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> > > >                         /* More just got used, free them then recheck. */
> > > >                         free_old_xmit_skbs(sq, false);
> > > >                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> > > >                                 netif_start_subqueue(dev, qnum);
> > > >                                 virtqueue_disable_cb(sq->vq);
> > > >                         }
> > >
> > > The code example here is out of date, make sure your tree has this:
> >
> > also, I will correct it in the next version，this is from kernel 5.15.
> >
> > >
> > > commit d71ebe8114b4bf622804b810f5e274069060a174
> > > Author: Jason Wang <jasowang@redhat.com>
> > > Date:   Tue Jan 17 11:47:07 2023 +0800
> > >
> > >     virtio-net: correctly enable callback during start_xmit
> > >
> > > >                 }
> > > >         }
> > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> > >
> > > Can you please post how to test with the performance numbers?
> > >
> >
> > iperf3 tcp stream:
> > vm1 -----------------> vm2
> > vm2 just receive tcp data stream from vm1, and send the ack to vm1,
> > there are so
> > many tx interruptions  in vm2.
> >
> > but without event_triggered there are just a few tx interruptions.
> >
> > > >
> > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > > > or vq->packed.vring.driver->off_wrap
> > > >
> > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > > > ---
> > > >  drivers/virtio/virtio_ring.c | 6 ++++--
> > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 307e139cb11d..f486cccadbeb 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> > > >         /* If we expect an interrupt for the next entry, tell host
> > > >          * by writing event index and flush out the write before
> > > >          * the read in the next get_buf call. */
> > > > -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > > > +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > > > +                       && (vq->event_triggered == false))
> > >
> > > I'm not sure this can work, when event_triggered is true it means
> > > we've got an interrupt, in this case if we want another interrupt for
> > > the next entry, we should update used_event otherwise we will lose
> > > that interrupt?
> > >
> > > Thanks
> >
> > Normally, if we receive an interrupt, we should disable the interrupt
> > in the interrupt callback handler.
> 
> So the problem is:
> 
> 1) event_triggered was set to true in vring_interrupt()
>
> 2) after this nothing will happen for virtqueue_disable_cb() so
> VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow
> 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled
> then it tries to publish new event

Oh. Good point! I think when I wrote up
8d622d21d248 ("virtio: fix up virtio_disable_cb")
I missed this corner case.



> This makes me think about whether or not we really need
> event_triggered. The assumption in the virtqueue_disable_cb() seems
> wrong:
> 
> /* If device triggered an event already it won't trigger one again:
>  * no need to disable.
>  */
> if (vq->event_triggered)
>                 return;
> 
> This is wrong if there's no event index support.


I don't get it.  how does this get triggered?

You are talking about device without event index?
Here's code from vring_interrupt():

        /* Just a hint for performance: so it's ok that this can be racy! */
        if (vq->event)
                vq->event_triggered = true;




> And the
> event_triggered is somehow duplicated with the
> VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix
> might be:
> 
> 1) remove event_triggered
> 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in
> vring_interrrupt if event index is supported
> 
> ?
> 
> Thanks

I am not sure all this is right and I'd rather we focused
performance/correctness and cleanups separately.




> 
> > But because of the introduction of event_triggered, here,
> > virtqueue_get_buf_ctx_split  cannot be recognized
> > that the interrupt has been turned off.
> >
> > if we want  another interrupt for the next entry, We should probably
> > call virtqueue_enable_cb？
> >
> > Thanks
> >
> > >
> > > >                 virtio_store_mb(vq->weak_barriers,
> > > >                                 &vring_used_event(&vq->split.vring),
> > > >                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > > >          * by writing event index and flush out the write before
> > > >          * the read in the next get_buf call.
> > > >          */
> > > > -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > > > +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > > > +                       && (vq->event_triggered == false))
> > > >                 virtio_store_mb(vq->weak_barriers,
> > > >                                 &vq->packed.vring.driver->off_wrap,
> > > >                                 cpu_to_le16(vq->last_used_idx));
> > > > --
> > > > 2.31.1
> > > >
> > >
> >
  

Thanks for the patch!
I picked it up.
I made small changes, please look at it in my branch,
both to see what I changed for your next submission,
and to test that it still addresses the problem for you.
Waiting for your confirmation to send upstream.
Thanks!


On Tue, Mar 21, 2023 at 04:59:53PM +0800, Albert Huang wrote:
> From: "huangjie.albert" <huangjie.albert@bytedance.com>
> 
> fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> 
> if we disable the napi_tx. when we triger a tx interrupt, the
> vq->event_triggered will be set to true. It will no longer be
> set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> or virtqueue_enable_cb_prepare
> 
> if we disable the napi_tx, It will only be called when the tx ring
> buffer is relatively small:
> virtio_net->start_xmit:
> 	if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> 		netif_stop_subqueue(dev, qnum);
> 		if (!use_napi &&
> 		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> 			/* More just got used, free them then recheck. */
> 			free_old_xmit_skbs(sq, false);
> 			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> 				netif_start_subqueue(dev, qnum);
> 				virtqueue_disable_cb(sq->vq);
> 			}
> 		}
> 	}
> Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> 
> if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> or vq->packed.vring.driver->off_wrap
> 
> Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> ---
>  drivers/virtio/virtio_ring.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> index 307e139cb11d..f486cccadbeb 100644
> --- a/drivers/virtio/virtio_ring.c
> +++ b/drivers/virtio/virtio_ring.c
> @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
>  	/* If we expect an interrupt for the next entry, tell host
>  	 * by writing event index and flush out the write before
>  	 * the read in the next get_buf call. */
> -	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> +	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> +			&& (vq->event_triggered == false))
>  		virtio_store_mb(vq->weak_barriers,
>  				&vring_used_event(&vq->split.vring),
>  				cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
>  	 * by writing event index and flush out the write before
>  	 * the read in the next get_buf call.
>  	 */
> -	if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> +	if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> +			&& (vq->event_triggered == false))
>  		virtio_store_mb(vq->weak_barriers,
>  				&vq->packed.vring.driver->off_wrap,
>  				cpu_to_le16(vq->last_used_idx));
> -- 
> 2.31.1
  

On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote:
> > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote:
> > >
> > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道：
> > > >
> > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
> > > > <huangjie.albert@bytedance.com> wrote:
> > > > >
> > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> > > > >
> > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > > >
> > > > > if we disable the napi_tx. when we triger a tx interrupt, the
> > > >
> > > > typo should be "trigger"
> > > >
> > >
> > > OK, thanks for this. I will correct it in the next version
> > >
> > > > > vq->event_triggered will be set to true. It will no longer be
> > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > > > > or virtqueue_enable_cb_prepare
> > > > >
> > > > > if we disable the napi_tx, It will only be called when the tx ring
> > > > > buffer is relatively small:
> > > > > virtio_net->start_xmit:
> > > > >         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> > > > >                 netif_stop_subqueue(dev, qnum);
> > > > >                 if (!use_napi &&
> > > > >                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> > > > >                         /* More just got used, free them then recheck. */
> > > > >                         free_old_xmit_skbs(sq, false);
> > > > >                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> > > > >                                 netif_start_subqueue(dev, qnum);
> > > > >                                 virtqueue_disable_cb(sq->vq);
> > > > >                         }
> > > >
> > > > The code example here is out of date, make sure your tree has this:
> > >
> > > also, I will correct it in the next version，this is from kernel 5.15.
> > >
> > > >
> > > > commit d71ebe8114b4bf622804b810f5e274069060a174
> > > > Author: Jason Wang <jasowang@redhat.com>
> > > > Date:   Tue Jan 17 11:47:07 2023 +0800
> > > >
> > > >     virtio-net: correctly enable callback during start_xmit
> > > >
> > > > >                 }
> > > > >         }
> > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> > > >
> > > > Can you please post how to test with the performance numbers?
> > > >
> > >
> > > iperf3 tcp stream:
> > > vm1 -----------------> vm2
> > > vm2 just receive tcp data stream from vm1, and send the ack to vm1,
> > > there are so
> > > many tx interruptions  in vm2.
> > >
> > > but without event_triggered there are just a few tx interruptions.
> > >
> > > > >
> > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > > > > or vq->packed.vring.driver->off_wrap
> > > > >
> > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > > > > ---
> > > > >  drivers/virtio/virtio_ring.c | 6 ++++--
> > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 307e139cb11d..f486cccadbeb 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> > > > >         /* If we expect an interrupt for the next entry, tell host
> > > > >          * by writing event index and flush out the write before
> > > > >          * the read in the next get_buf call. */
> > > > > -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > > > > +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > > > > +                       && (vq->event_triggered == false))
> > > >
> > > > I'm not sure this can work, when event_triggered is true it means
> > > > we've got an interrupt, in this case if we want another interrupt for
> > > > the next entry, we should update used_event otherwise we will lose
> > > > that interrupt?
> > > >
> > > > Thanks
> > >
> > > Normally, if we receive an interrupt, we should disable the interrupt
> > > in the interrupt callback handler.
> >
> > So the problem is:
> >
> > 1) event_triggered was set to true in vring_interrupt()
> >
> > 2) after this nothing will happen for virtqueue_disable_cb() so
> > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow
> > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled
> > then it tries to publish new event
>
> Oh. Good point! I think when I wrote up
> 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> I missed this corner case.
>
>
>
> > This makes me think about whether or not we really need
> > event_triggered. The assumption in the virtqueue_disable_cb() seems
> > wrong:
> >
> > /* If device triggered an event already it won't trigger one again:
> >  * no need to disable.
> >  */
> > if (vq->event_triggered)
> >                 return;
> >
> > This is wrong if there's no event index support.
>
>
> I don't get it.  how does this get triggered?
>
> You are talking about device without event index?
> Here's code from vring_interrupt():
>
>         /* Just a hint for performance: so it's ok that this can be racy! */
>         if (vq->event)
>                 vq->event_triggered = true;

But we have the following in virtqueue_disable_cb():

        /* If device triggered an event already it won't trigger one again:
         * no need to disable.
         */
        if (vq->event_triggered)
                return;

        if (vq->packed_ring)
                virtqueue_disable_cb_packed(_vq);
        else
                virtqueue_disable_cb_split(_vq);

This means, without an event index, we don't set avail flags. So the
interrupt is not disabled actually in this case.

Thanks

>
>
>
>
> > And the
> > event_triggered is somehow duplicated with the
> > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix
> > might be:
> >
> > 1) remove event_triggered
> > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in
> > vring_interrrupt if event index is supported
> >
> > ?
> >
> > Thanks
>
> I am not sure all this is right and I'd rather we focused
> performance/correctness and cleanups separately.
>
>
>
>
> >
> > > But because of the introduction of event_triggered, here,
> > > virtqueue_get_buf_ctx_split  cannot be recognized
> > > that the interrupt has been turned off.
> > >
> > > if we want  another interrupt for the next entry, We should probably
> > > call virtqueue_enable_cb？
> > >
> > > Thanks
> > >
> > > >
> > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > >                                 &vring_used_event(&vq->split.vring),
> > > > >                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > > > >          * by writing event index and flush out the write before
> > > > >          * the read in the next get_buf call.
> > > > >          */
> > > > > -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > > > > +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > > > > +                       && (vq->event_triggered == false))
> > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > >                                 &vq->packed.vring.driver->off_wrap,
> > > > >                                 cpu_to_le16(vq->last_used_idx));
> > > > > --
> > > > > 2.31.1
> > > > >
> > > >
> > >
>
  

Hmm I sent a bit too fast, and my testing rig is down now.
So please do send a new version, I sent comments on what to fix
in this one.

On Fri, Mar 24, 2023 at 02:08:55AM -0400, Michael S. Tsirkin wrote:
> Thanks for the patch!
> I picked it up.
> I made small changes, please look at it in my branch,
> both to see what I changed for your next submission,
> and to test that it still addresses the problem for you.
> Waiting for your confirmation to send upstream.
> Thanks!
> 
> 
> On Tue, Mar 21, 2023 at 04:59:53PM +0800, Albert Huang wrote:
> > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> > 
> > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > 
> > if we disable the napi_tx. when we triger a tx interrupt, the
> > vq->event_triggered will be set to true. It will no longer be
> > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > or virtqueue_enable_cb_prepare
> > 
> > if we disable the napi_tx, It will only be called when the tx ring
> > buffer is relatively small:
> > virtio_net->start_xmit:
> > 	if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> > 		netif_stop_subqueue(dev, qnum);
> > 		if (!use_napi &&
> > 		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> > 			/* More just got used, free them then recheck. */
> > 			free_old_xmit_skbs(sq, false);
> > 			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> > 				netif_start_subqueue(dev, qnum);
> > 				virtqueue_disable_cb(sq->vq);
> > 			}
> > 		}
> > 	}
> > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> > 
> > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > or vq->packed.vring.driver->off_wrap
> > 
> > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > ---
> >  drivers/virtio/virtio_ring.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > index 307e139cb11d..f486cccadbeb 100644
> > --- a/drivers/virtio/virtio_ring.c
> > +++ b/drivers/virtio/virtio_ring.c
> > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> >  	/* If we expect an interrupt for the next entry, tell host
> >  	 * by writing event index and flush out the write before
> >  	 * the read in the next get_buf call. */
> > -	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > +	if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > +			&& (vq->event_triggered == false))
> >  		virtio_store_mb(vq->weak_barriers,
> >  				&vring_used_event(&vq->split.vring),
> >  				cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> >  	 * by writing event index and flush out the write before
> >  	 * the read in the next get_buf call.
> >  	 */
> > -	if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > +	if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > +			&& (vq->event_triggered == false))
> >  		virtio_store_mb(vq->weak_barriers,
> >  				&vq->packed.vring.driver->off_wrap,
> >  				cpu_to_le16(vq->last_used_idx));
> > -- 
> > 2.31.1
  

On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote:
> On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote:
> > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote:
> > > >
> > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道：
> > > > >
> > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
> > > > > <huangjie.albert@bytedance.com> wrote:
> > > > > >
> > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> > > > > >
> > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > > > >
> > > > > > if we disable the napi_tx. when we triger a tx interrupt, the
> > > > >
> > > > > typo should be "trigger"
> > > > >
> > > >
> > > > OK, thanks for this. I will correct it in the next version
> > > >
> > > > > > vq->event_triggered will be set to true. It will no longer be
> > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > > > > > or virtqueue_enable_cb_prepare
> > > > > >
> > > > > > if we disable the napi_tx, It will only be called when the tx ring
> > > > > > buffer is relatively small:
> > > > > > virtio_net->start_xmit:
> > > > > >         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> > > > > >                 netif_stop_subqueue(dev, qnum);
> > > > > >                 if (!use_napi &&
> > > > > >                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> > > > > >                         /* More just got used, free them then recheck. */
> > > > > >                         free_old_xmit_skbs(sq, false);
> > > > > >                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> > > > > >                                 netif_start_subqueue(dev, qnum);
> > > > > >                                 virtqueue_disable_cb(sq->vq);
> > > > > >                         }
> > > > >
> > > > > The code example here is out of date, make sure your tree has this:
> > > >
> > > > also, I will correct it in the next version，this is from kernel 5.15.
> > > >
> > > > >
> > > > > commit d71ebe8114b4bf622804b810f5e274069060a174
> > > > > Author: Jason Wang <jasowang@redhat.com>
> > > > > Date:   Tue Jan 17 11:47:07 2023 +0800
> > > > >
> > > > >     virtio-net: correctly enable callback during start_xmit
> > > > >
> > > > > >                 }
> > > > > >         }
> > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> > > > >
> > > > > Can you please post how to test with the performance numbers?
> > > > >
> > > >
> > > > iperf3 tcp stream:
> > > > vm1 -----------------> vm2
> > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1,
> > > > there are so
> > > > many tx interruptions  in vm2.
> > > >
> > > > but without event_triggered there are just a few tx interruptions.
> > > >
> > > > > >
> > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > > > > > or vq->packed.vring.driver->off_wrap
> > > > > >
> > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > > > > > ---
> > > > > >  drivers/virtio/virtio_ring.c | 6 ++++--
> > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > index 307e139cb11d..f486cccadbeb 100644
> > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> > > > > >         /* If we expect an interrupt for the next entry, tell host
> > > > > >          * by writing event index and flush out the write before
> > > > > >          * the read in the next get_buf call. */
> > > > > > -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > > > > > +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > > > > > +                       && (vq->event_triggered == false))
> > > > >
> > > > > I'm not sure this can work, when event_triggered is true it means
> > > > > we've got an interrupt, in this case if we want another interrupt for
> > > > > the next entry, we should update used_event otherwise we will lose
> > > > > that interrupt?
> > > > >
> > > > > Thanks
> > > >
> > > > Normally, if we receive an interrupt, we should disable the interrupt
> > > > in the interrupt callback handler.
> > >
> > > So the problem is:
> > >
> > > 1) event_triggered was set to true in vring_interrupt()
> > >
> > > 2) after this nothing will happen for virtqueue_disable_cb() so
> > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow
> > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled
> > > then it tries to publish new event
> >
> > Oh. Good point! I think when I wrote up
> > 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > I missed this corner case.
> >
> >
> >
> > > This makes me think about whether or not we really need
> > > event_triggered. The assumption in the virtqueue_disable_cb() seems
> > > wrong:
> > >
> > > /* If device triggered an event already it won't trigger one again:
> > >  * no need to disable.
> > >  */
> > > if (vq->event_triggered)
> > >                 return;
> > >
> > > This is wrong if there's no event index support.
> >
> >
> > I don't get it.  how does this get triggered?
> >
> > You are talking about device without event index?
> > Here's code from vring_interrupt():
> >
> >         /* Just a hint for performance: so it's ok that this can be racy! */
> >         if (vq->event)
> >                 vq->event_triggered = true;
> 
> But we have the following in virtqueue_disable_cb():
> 
>         /* If device triggered an event already it won't trigger one again:
>          * no need to disable.
>          */
>         if (vq->event_triggered)
>                 return;
> 
>         if (vq->packed_ring)
>                 virtqueue_disable_cb_packed(_vq);
>         else
>                 virtqueue_disable_cb_split(_vq);
> 
> This means, without an event index, we don't set avail flags. So the
> interrupt is not disabled actually in this case.
> 
> Thanks

Only if event_triggered is true, which without event index it never is.

> >
> >
> >
> >
> > > And the
> > > event_triggered is somehow duplicated with the
> > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix
> > > might be:
> > >
> > > 1) remove event_triggered
> > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in
> > > vring_interrrupt if event index is supported
> > >
> > > ?
> > >
> > > Thanks
> >
> > I am not sure all this is right and I'd rather we focused
> > performance/correctness and cleanups separately.
> >
> >
> >
> >
> > >
> > > > But because of the introduction of event_triggered, here,
> > > > virtqueue_get_buf_ctx_split  cannot be recognized
> > > > that the interrupt has been turned off.
> > > >
> > > > if we want  another interrupt for the next entry, We should probably
> > > > call virtqueue_enable_cb？
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > >                                 &vring_used_event(&vq->split.vring),
> > > > > >                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > > > > >          * by writing event index and flush out the write before
> > > > > >          * the read in the next get_buf call.
> > > > > >          */
> > > > > > -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > > > > > +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > > > > > +                       && (vq->event_triggered == false))
> > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > >                                 &vq->packed.vring.driver->off_wrap,
> > > > > >                                 cpu_to_le16(vq->last_used_idx));
> > > > > > --
> > > > > > 2.31.1
> > > > > >
> > > > >
> > > >
> >
  

On Fri, Mar 24, 2023 at 2:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote:
> > On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote:
> > > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote:
> > > > >
> > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道：
> > > > > >
> > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
> > > > > > <huangjie.albert@bytedance.com> wrote:
> > > > > > >
> > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> > > > > > >
> > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > > > > >
> > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the
> > > > > >
> > > > > > typo should be "trigger"
> > > > > >
> > > > >
> > > > > OK, thanks for this. I will correct it in the next version
> > > > >
> > > > > > > vq->event_triggered will be set to true. It will no longer be
> > > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > > > > > > or virtqueue_enable_cb_prepare
> > > > > > >
> > > > > > > if we disable the napi_tx, It will only be called when the tx ring
> > > > > > > buffer is relatively small:
> > > > > > > virtio_net->start_xmit:
> > > > > > >         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> > > > > > >                 netif_stop_subqueue(dev, qnum);
> > > > > > >                 if (!use_napi &&
> > > > > > >                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> > > > > > >                         /* More just got used, free them then recheck. */
> > > > > > >                         free_old_xmit_skbs(sq, false);
> > > > > > >                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> > > > > > >                                 netif_start_subqueue(dev, qnum);
> > > > > > >                                 virtqueue_disable_cb(sq->vq);
> > > > > > >                         }
> > > > > >
> > > > > > The code example here is out of date, make sure your tree has this:
> > > > >
> > > > > also, I will correct it in the next version，this is from kernel 5.15.
> > > > >
> > > > > >
> > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174
> > > > > > Author: Jason Wang <jasowang@redhat.com>
> > > > > > Date:   Tue Jan 17 11:47:07 2023 +0800
> > > > > >
> > > > > >     virtio-net: correctly enable callback during start_xmit
> > > > > >
> > > > > > >                 }
> > > > > > >         }
> > > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> > > > > >
> > > > > > Can you please post how to test with the performance numbers?
> > > > > >
> > > > >
> > > > > iperf3 tcp stream:
> > > > > vm1 -----------------> vm2
> > > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1,
> > > > > there are so
> > > > > many tx interruptions  in vm2.
> > > > >
> > > > > but without event_triggered there are just a few tx interruptions.
> > > > >
> > > > > > >
> > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > > > > > > or vq->packed.vring.driver->off_wrap
> > > > > > >
> > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > > > > > > ---
> > > > > > >  drivers/virtio/virtio_ring.c | 6 ++++--
> > > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > > index 307e139cb11d..f486cccadbeb 100644
> > > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> > > > > > >         /* If we expect an interrupt for the next entry, tell host
> > > > > > >          * by writing event index and flush out the write before
> > > > > > >          * the read in the next get_buf call. */
> > > > > > > -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > > > > > > +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > > > > > > +                       && (vq->event_triggered == false))
> > > > > >
> > > > > > I'm not sure this can work, when event_triggered is true it means
> > > > > > we've got an interrupt, in this case if we want another interrupt for
> > > > > > the next entry, we should update used_event otherwise we will lose
> > > > > > that interrupt?
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > Normally, if we receive an interrupt, we should disable the interrupt
> > > > > in the interrupt callback handler.
> > > >
> > > > So the problem is:
> > > >
> > > > 1) event_triggered was set to true in vring_interrupt()
> > > >
> > > > 2) after this nothing will happen for virtqueue_disable_cb() so
> > > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow
> > > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled
> > > > then it tries to publish new event
> > >
> > > Oh. Good point! I think when I wrote up
> > > 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > I missed this corner case.
> > >
> > >
> > >
> > > > This makes me think about whether or not we really need
> > > > event_triggered. The assumption in the virtqueue_disable_cb() seems
> > > > wrong:
> > > >
> > > > /* If device triggered an event already it won't trigger one again:
> > > >  * no need to disable.
> > > >  */
> > > > if (vq->event_triggered)
> > > >                 return;
> > > >
> > > > This is wrong if there's no event index support.
> > >
> > >
> > > I don't get it.  how does this get triggered?
> > >
> > > You are talking about device without event index?
> > > Here's code from vring_interrupt():
> > >
> > >         /* Just a hint for performance: so it's ok that this can be racy! */
> > >         if (vq->event)
> > >                 vq->event_triggered = true;
> >
> > But we have the following in virtqueue_disable_cb():
> >
> >         /* If device triggered an event already it won't trigger one again:
> >          * no need to disable.
> >          */
> >         if (vq->event_triggered)
> >                 return;
> >
> >         if (vq->packed_ring)
> >                 virtqueue_disable_cb_packed(_vq);
> >         else
> >                 virtqueue_disable_cb_split(_vq);
> >
> > This means, without an event index, we don't set avail flags. So the
> > interrupt is not disabled actually in this case.
> >
> > Thanks
>
> Only if event_triggered is true, which without event index it never is.

I'm not sure I will get here. I meant for example the commit
suppresses the effort of skb_xmit_done():

static void skb_xmit_done(struct virtqueue *vq)
{
        struct virtnet_info *vi = vq->vdev->priv;
        struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi;

        /* Suppress further interrupts. */
        virtqueue_disable_cb(vq);

The virtqueue_disable_cb() doesn't disable further interrupts when the
event index is not there.

Thanks

>
> > >
> > >
> > >
> > >
> > > > And the
> > > > event_triggered is somehow duplicated with the
> > > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix
> > > > might be:
> > > >
> > > > 1) remove event_triggered
> > > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in
> > > > vring_interrrupt if event index is supported
> > > >
> > > > ?
> > > >
> > > > Thanks
> > >
> > > I am not sure all this is right and I'd rather we focused
> > > performance/correctness and cleanups separately.
> > >
> > >
> > >
> > >
> > > >
> > > > > But because of the introduction of event_triggered, here,
> > > > > virtqueue_get_buf_ctx_split  cannot be recognized
> > > > > that the interrupt has been turned off.
> > > > >
> > > > > if we want  another interrupt for the next entry, We should probably
> > > > > call virtqueue_enable_cb？
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > > >                                 &vring_used_event(&vq->split.vring),
> > > > > > >                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > > > > > >          * by writing event index and flush out the write before
> > > > > > >          * the read in the next get_buf call.
> > > > > > >          */
> > > > > > > -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > > > > > > +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > > > > > > +                       && (vq->event_triggered == false))
> > > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > > >                                 &vq->packed.vring.driver->off_wrap,
> > > > > > >                                 cpu_to_le16(vq->last_used_idx));
> > > > > > > --
> > > > > > > 2.31.1
> > > > > > >
> > > > > >
> > > > >
> > >
>
  

On Fri, Mar 24, 2023 at 02:47:02PM +0800, Jason Wang wrote:
> On Fri, Mar 24, 2023 at 2:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote:
> > > On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote:
> > > > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote:
> > > > > >
> > > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道：
> > > > > > >
> > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
> > > > > > > <huangjie.albert@bytedance.com> wrote:
> > > > > > > >
> > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> > > > > > > >
> > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > > > > > >
> > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the
> > > > > > >
> > > > > > > typo should be "trigger"
> > > > > > >
> > > > > >
> > > > > > OK, thanks for this. I will correct it in the next version
> > > > > >
> > > > > > > > vq->event_triggered will be set to true. It will no longer be
> > > > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > > > > > > > or virtqueue_enable_cb_prepare
> > > > > > > >
> > > > > > > > if we disable the napi_tx, It will only be called when the tx ring
> > > > > > > > buffer is relatively small:
> > > > > > > > virtio_net->start_xmit:
> > > > > > > >         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> > > > > > > >                 netif_stop_subqueue(dev, qnum);
> > > > > > > >                 if (!use_napi &&
> > > > > > > >                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> > > > > > > >                         /* More just got used, free them then recheck. */
> > > > > > > >                         free_old_xmit_skbs(sq, false);
> > > > > > > >                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> > > > > > > >                                 netif_start_subqueue(dev, qnum);
> > > > > > > >                                 virtqueue_disable_cb(sq->vq);
> > > > > > > >                         }
> > > > > > >
> > > > > > > The code example here is out of date, make sure your tree has this:
> > > > > >
> > > > > > also, I will correct it in the next version，this is from kernel 5.15.
> > > > > >
> > > > > > >
> > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174
> > > > > > > Author: Jason Wang <jasowang@redhat.com>
> > > > > > > Date:   Tue Jan 17 11:47:07 2023 +0800
> > > > > > >
> > > > > > >     virtio-net: correctly enable callback during start_xmit
> > > > > > >
> > > > > > > >                 }
> > > > > > > >         }
> > > > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > > > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > > > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > > > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> > > > > > >
> > > > > > > Can you please post how to test with the performance numbers?
> > > > > > >
> > > > > >
> > > > > > iperf3 tcp stream:
> > > > > > vm1 -----------------> vm2
> > > > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1,
> > > > > > there are so
> > > > > > many tx interruptions  in vm2.
> > > > > >
> > > > > > but without event_triggered there are just a few tx interruptions.
> > > > > >
> > > > > > > >
> > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > > > > > > > or vq->packed.vring.driver->off_wrap
> > > > > > > >
> > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > > > > > > > ---
> > > > > > > >  drivers/virtio/virtio_ring.c | 6 ++++--
> > > > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > > > index 307e139cb11d..f486cccadbeb 100644
> > > > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> > > > > > > >         /* If we expect an interrupt for the next entry, tell host
> > > > > > > >          * by writing event index and flush out the write before
> > > > > > > >          * the read in the next get_buf call. */
> > > > > > > > -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > > > > > > > +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > > > > > > > +                       && (vq->event_triggered == false))
> > > > > > >
> > > > > > > I'm not sure this can work, when event_triggered is true it means
> > > > > > > we've got an interrupt, in this case if we want another interrupt for
> > > > > > > the next entry, we should update used_event otherwise we will lose
> > > > > > > that interrupt?
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > > Normally, if we receive an interrupt, we should disable the interrupt
> > > > > > in the interrupt callback handler.
> > > > >
> > > > > So the problem is:
> > > > >
> > > > > 1) event_triggered was set to true in vring_interrupt()
> > > > >
> > > > > 2) after this nothing will happen for virtqueue_disable_cb() so
> > > > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow
> > > > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled
> > > > > then it tries to publish new event
> > > >
> > > > Oh. Good point! I think when I wrote up
> > > > 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > > I missed this corner case.
> > > >
> > > >
> > > >
> > > > > This makes me think about whether or not we really need
> > > > > event_triggered. The assumption in the virtqueue_disable_cb() seems
> > > > > wrong:
> > > > >
> > > > > /* If device triggered an event already it won't trigger one again:
> > > > >  * no need to disable.
> > > > >  */
> > > > > if (vq->event_triggered)
> > > > >                 return;
> > > > >
> > > > > This is wrong if there's no event index support.
> > > >
> > > >
> > > > I don't get it.  how does this get triggered?
> > > >
> > > > You are talking about device without event index?
> > > > Here's code from vring_interrupt():
> > > >
> > > >         /* Just a hint for performance: so it's ok that this can be racy! */
> > > >         if (vq->event)
> > > >                 vq->event_triggered = true;
> > >
> > > But we have the following in virtqueue_disable_cb():
> > >
> > >         /* If device triggered an event already it won't trigger one again:
> > >          * no need to disable.
> > >          */
> > >         if (vq->event_triggered)
> > >                 return;
> > >
> > >         if (vq->packed_ring)
> > >                 virtqueue_disable_cb_packed(_vq);
> > >         else
> > >                 virtqueue_disable_cb_split(_vq);
> > >
> > > This means, without an event index, we don't set avail flags. So the
> > > interrupt is not disabled actually in this case.
> > >
> > > Thanks
> >
> > Only if event_triggered is true, which without event index it never is.
> 
> I'm not sure I will get here. I meant for example the commit
> suppresses the effort of skb_xmit_done():
> 
> static void skb_xmit_done(struct virtqueue *vq)
> {
>         struct virtnet_info *vi = vq->vdev->priv;
>         struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi;
> 
>         /* Suppress further interrupts. */
>         virtqueue_disable_cb(vq);
> 
> The virtqueue_disable_cb() doesn't disable further interrupts when the
> event index is not there.
> 
> Thanks

Check what can set event_triggered, you will see.



> >
> > > >
> > > >
> > > >
> > > >
> > > > > And the
> > > > > event_triggered is somehow duplicated with the
> > > > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix
> > > > > might be:
> > > > >
> > > > > 1) remove event_triggered
> > > > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in
> > > > > vring_interrrupt if event index is supported
> > > > >
> > > > > ?
> > > > >
> > > > > Thanks
> > > >
> > > > I am not sure all this is right and I'd rather we focused
> > > > performance/correctness and cleanups separately.
> > > >
> > > >
> > > >
> > > >
> > > > >
> > > > > > But because of the introduction of event_triggered, here,
> > > > > > virtqueue_get_buf_ctx_split  cannot be recognized
> > > > > > that the interrupt has been turned off.
> > > > > >
> > > > > > if we want  another interrupt for the next entry, We should probably
> > > > > > call virtqueue_enable_cb？
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > >
> > > > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > > > >                                 &vring_used_event(&vq->split.vring),
> > > > > > > >                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > > > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > > > > > > >          * by writing event index and flush out the write before
> > > > > > > >          * the read in the next get_buf call.
> > > > > > > >          */
> > > > > > > > -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > > > > > > > +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > > > > > > > +                       && (vq->event_triggered == false))
> > > > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > > > >                                 &vq->packed.vring.driver->off_wrap,
> > > > > > > >                                 cpu_to_le16(vq->last_used_idx));
> > > > > > > > --
> > > > > > > > 2.31.1
> > > > > > > >
> > > > > > >
> > > > > >
> > > >
> >
  

On Fri, Mar 24, 2023 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Fri, Mar 24, 2023 at 02:47:02PM +0800, Jason Wang wrote:
> > On Fri, Mar 24, 2023 at 2:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote:
> > > > On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote:
> > > > > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote:
> > > > > > >
> > > > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道：
> > > > > > > >
> > > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
> > > > > > > > <huangjie.albert@bytedance.com> wrote:
> > > > > > > > >
> > > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> > > > > > > > >
> > > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > > > > > > >
> > > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the
> > > > > > > >
> > > > > > > > typo should be "trigger"
> > > > > > > >
> > > > > > >
> > > > > > > OK, thanks for this. I will correct it in the next version
> > > > > > >
> > > > > > > > > vq->event_triggered will be set to true. It will no longer be
> > > > > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > > > > > > > > or virtqueue_enable_cb_prepare
> > > > > > > > >
> > > > > > > > > if we disable the napi_tx, It will only be called when the tx ring
> > > > > > > > > buffer is relatively small:
> > > > > > > > > virtio_net->start_xmit:
> > > > > > > > >         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> > > > > > > > >                 netif_stop_subqueue(dev, qnum);
> > > > > > > > >                 if (!use_napi &&
> > > > > > > > >                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> > > > > > > > >                         /* More just got used, free them then recheck. */
> > > > > > > > >                         free_old_xmit_skbs(sq, false);
> > > > > > > > >                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> > > > > > > > >                                 netif_start_subqueue(dev, qnum);
> > > > > > > > >                                 virtqueue_disable_cb(sq->vq);
> > > > > > > > >                         }
> > > > > > > >
> > > > > > > > The code example here is out of date, make sure your tree has this:
> > > > > > >
> > > > > > > also, I will correct it in the next version，this is from kernel 5.15.
> > > > > > >
> > > > > > > >
> > > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174
> > > > > > > > Author: Jason Wang <jasowang@redhat.com>
> > > > > > > > Date:   Tue Jan 17 11:47:07 2023 +0800
> > > > > > > >
> > > > > > > >     virtio-net: correctly enable callback during start_xmit
> > > > > > > >
> > > > > > > > >                 }
> > > > > > > > >         }
> > > > > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > > > > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > > > > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > > > > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> > > > > > > >
> > > > > > > > Can you please post how to test with the performance numbers?
> > > > > > > >
> > > > > > >
> > > > > > > iperf3 tcp stream:
> > > > > > > vm1 -----------------> vm2
> > > > > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1,
> > > > > > > there are so
> > > > > > > many tx interruptions  in vm2.
> > > > > > >
> > > > > > > but without event_triggered there are just a few tx interruptions.
> > > > > > >
> > > > > > > > >
> > > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > > > > > > > > or vq->packed.vring.driver->off_wrap
> > > > > > > > >
> > > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/virtio/virtio_ring.c | 6 ++++--
> > > > > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > > > > index 307e139cb11d..f486cccadbeb 100644
> > > > > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> > > > > > > > >         /* If we expect an interrupt for the next entry, tell host
> > > > > > > > >          * by writing event index and flush out the write before
> > > > > > > > >          * the read in the next get_buf call. */
> > > > > > > > > -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > > > > > > > > +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > > > > > > > > +                       && (vq->event_triggered == false))
> > > > > > > >
> > > > > > > > I'm not sure this can work, when event_triggered is true it means
> > > > > > > > we've got an interrupt, in this case if we want another interrupt for
> > > > > > > > the next entry, we should update used_event otherwise we will lose
> > > > > > > > that interrupt?
> > > > > > > >
> > > > > > > > Thanks
> > > > > > >
> > > > > > > Normally, if we receive an interrupt, we should disable the interrupt
> > > > > > > in the interrupt callback handler.
> > > > > >
> > > > > > So the problem is:
> > > > > >
> > > > > > 1) event_triggered was set to true in vring_interrupt()
> > > > > >
> > > > > > 2) after this nothing will happen for virtqueue_disable_cb() so
> > > > > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow
> > > > > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled
> > > > > > then it tries to publish new event
> > > > >
> > > > > Oh. Good point! I think when I wrote up
> > > > > 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > > > I missed this corner case.
> > > > >
> > > > >
> > > > >
> > > > > > This makes me think about whether or not we really need
> > > > > > event_triggered. The assumption in the virtqueue_disable_cb() seems
> > > > > > wrong:
> > > > > >
> > > > > > /* If device triggered an event already it won't trigger one again:
> > > > > >  * no need to disable.
> > > > > >  */
> > > > > > if (vq->event_triggered)
> > > > > >                 return;
> > > > > >
> > > > > > This is wrong if there's no event index support.
> > > > >
> > > > >
> > > > > I don't get it.  how does this get triggered?
> > > > >
> > > > > You are talking about device without event index?
> > > > > Here's code from vring_interrupt():
> > > > >
> > > > >         /* Just a hint for performance: so it's ok that this can be racy! */
> > > > >         if (vq->event)
> > > > >                 vq->event_triggered = true;
> > > >
> > > > But we have the following in virtqueue_disable_cb():
> > > >
> > > >         /* If device triggered an event already it won't trigger one again:
> > > >          * no need to disable.
> > > >          */
> > > >         if (vq->event_triggered)
> > > >                 return;
> > > >
> > > >         if (vq->packed_ring)
> > > >                 virtqueue_disable_cb_packed(_vq);
> > > >         else
> > > >                 virtqueue_disable_cb_split(_vq);
> > > >
> > > > This means, without an event index, we don't set avail flags. So the
> > > > interrupt is not disabled actually in this case.
> > > >
> > > > Thanks
> > >
> > > Only if event_triggered is true, which without event index it never is.
> >
> > I'm not sure I will get here. I meant for example the commit
> > suppresses the effort of skb_xmit_done():
> >
> > static void skb_xmit_done(struct virtqueue *vq)
> > {
> >         struct virtnet_info *vi = vq->vdev->priv;
> >         struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi;
> >
> >         /* Suppress further interrupts. */
> >         virtqueue_disable_cb(vq);
> >
> > The virtqueue_disable_cb() doesn't disable further interrupts when the
> > event index is not there.
> >
> > Thanks
>
> Check what can set event_triggered, you will see.

Set to truth by vring_interrupt()
Set to false by virtqueue_init(), virtqueue_enable_cb_prepare(),
virtqueue_enable_cb_delayed()

Assuming NAPI TX is enabled and the device doesn't support event index.

1) driver sends packets 1-10
2) the start_xmit() for the last packet will call
virtqueue_enable_cb_delayed() which set event_triggered = false
3) 1st packet were sent, vring_interrupt set event_triggered = true
4) skb_xmit_done() won't disable virtqueue_disable_cb() in this case
5) so we will get the interrupts for 2nd to 10th packet

Anything I missed here?

Note the comment said it's used for event index:

        /* Hint for event idx: already triggered no need to disable. */
        bool event_triggered;

I guess what you meant is that if we don't publish a new event, we
will get at most 1 interrupt for everything $queue_size used buffers.
But this is not the case without event index. Btw, it may supress the
effort of:

vring_used_event(&vq->split.vring) = 0x0;

Thanks

>
>
>
> > >
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > > And the
> > > > > > event_triggered is somehow duplicated with the
> > > > > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix
> > > > > > might be:
> > > > > >
> > > > > > 1) remove event_triggered
> > > > > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in
> > > > > > vring_interrrupt if event index is supported
> > > > > >
> > > > > > ?
> > > > > >
> > > > > > Thanks
> > > > >
> > > > > I am not sure all this is right and I'd rather we focused
> > > > > performance/correctness and cleanups separately.
> > > > >
> > > > >
> > > > >
> > > > >
> > > > > >
> > > > > > > But because of the introduction of event_triggered, here,
> > > > > > > virtqueue_get_buf_ctx_split  cannot be recognized
> > > > > > > that the interrupt has been turned off.
> > > > > > >
> > > > > > > if we want  another interrupt for the next entry, We should probably
> > > > > > > call virtqueue_enable_cb？
> > > > > > >
> > > > > > > Thanks
> > > > > > >
> > > > > > > >
> > > > > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > > > > >                                 &vring_used_event(&vq->split.vring),
> > > > > > > > >                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > > > > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > > > > > > > >          * by writing event index and flush out the write before
> > > > > > > > >          * the read in the next get_buf call.
> > > > > > > > >          */
> > > > > > > > > -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > > > > > > > > +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > > > > > > > > +                       && (vq->event_triggered == false))
> > > > > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > > > > >                                 &vq->packed.vring.driver->off_wrap,
> > > > > > > > >                                 cpu_to_le16(vq->last_used_idx));
> > > > > > > > > --
> > > > > > > > > 2.31.1
> > > > > > > > >
> > > > > > > >
> > > > > > >
> > > > >
> > >
>
  

On Fri, Mar 24, 2023 at 03:37:04PM +0800, Jason Wang wrote:
> On Fri, Mar 24, 2023 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Mar 24, 2023 at 02:47:02PM +0800, Jason Wang wrote:
> > > On Fri, Mar 24, 2023 at 2:42 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote:
> > > > > On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > > >
> > > > > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote:
> > > > > > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote:
> > > > > > > >
> > > > > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道：
> > > > > > > > >
> > > > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang
> > > > > > > > > <huangjie.albert@bytedance.com> wrote:
> > > > > > > > > >
> > > > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com>
> > > > > > > > > >
> > > > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > > > > > > > >
> > > > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the
> > > > > > > > >
> > > > > > > > > typo should be "trigger"
> > > > > > > > >
> > > > > > > >
> > > > > > > > OK, thanks for this. I will correct it in the next version
> > > > > > > >
> > > > > > > > > > vq->event_triggered will be set to true. It will no longer be
> > > > > > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed
> > > > > > > > > > or virtqueue_enable_cb_prepare
> > > > > > > > > >
> > > > > > > > > > if we disable the napi_tx, It will only be called when the tx ring
> > > > > > > > > > buffer is relatively small:
> > > > > > > > > > virtio_net->start_xmit:
> > > > > > > > > >         if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
> > > > > > > > > >                 netif_stop_subqueue(dev, qnum);
> > > > > > > > > >                 if (!use_napi &&
> > > > > > > > > >                     unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
> > > > > > > > > >                         /* More just got used, free them then recheck. */
> > > > > > > > > >                         free_old_xmit_skbs(sq, false);
> > > > > > > > > >                         if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
> > > > > > > > > >                                 netif_start_subqueue(dev, qnum);
> > > > > > > > > >                                 virtqueue_disable_cb(sq->vq);
> > > > > > > > > >                         }
> > > > > > > > >
> > > > > > > > > The code example here is out of date, make sure your tree has this:
> > > > > > > >
> > > > > > > > also, I will correct it in the next version，this is from kernel 5.15.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174
> > > > > > > > > Author: Jason Wang <jasowang@redhat.com>
> > > > > > > > > Date:   Tue Jan 17 11:47:07 2023 +0800
> > > > > > > > >
> > > > > > > > >     virtio-net: correctly enable callback during start_xmit
> > > > > > > > >
> > > > > > > > > >                 }
> > > > > > > > > >         }
> > > > > > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or
> > > > > > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update
> > > > > > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap
> > > > > > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions.
> > > > > > > > >
> > > > > > > > > Can you please post how to test with the performance numbers?
> > > > > > > > >
> > > > > > > >
> > > > > > > > iperf3 tcp stream:
> > > > > > > > vm1 -----------------> vm2
> > > > > > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1,
> > > > > > > > there are so
> > > > > > > > many tx interruptions  in vm2.
> > > > > > > >
> > > > > > > > but without event_triggered there are just a few tx interruptions.
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring)
> > > > > > > > > > or vq->packed.vring.driver->off_wrap
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/virtio/virtio_ring.c | 6 ++++--
> > > > > > > > > >  1 file changed, 4 insertions(+), 2 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > > > > > > index 307e139cb11d..f486cccadbeb 100644
> > > > > > > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > > > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq,
> > > > > > > > > >         /* If we expect an interrupt for the next entry, tell host
> > > > > > > > > >          * by writing event index and flush out the write before
> > > > > > > > > >          * the read in the next get_buf call. */
> > > > > > > > > > -       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT))
> > > > > > > > > > +       if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)
> > > > > > > > > > +                       && (vq->event_triggered == false))
> > > > > > > > >
> > > > > > > > > I'm not sure this can work, when event_triggered is true it means
> > > > > > > > > we've got an interrupt, in this case if we want another interrupt for
> > > > > > > > > the next entry, we should update used_event otherwise we will lose
> > > > > > > > > that interrupt?
> > > > > > > > >
> > > > > > > > > Thanks
> > > > > > > >
> > > > > > > > Normally, if we receive an interrupt, we should disable the interrupt
> > > > > > > > in the interrupt callback handler.
> > > > > > >
> > > > > > > So the problem is:
> > > > > > >
> > > > > > > 1) event_triggered was set to true in vring_interrupt()
> > > > > > >
> > > > > > > 2) after this nothing will happen for virtqueue_disable_cb() so
> > > > > > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow
> > > > > > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled
> > > > > > > then it tries to publish new event
> > > > > >
> > > > > > Oh. Good point! I think when I wrote up
> > > > > > 8d622d21d248 ("virtio: fix up virtio_disable_cb")
> > > > > > I missed this corner case.
> > > > > >
> > > > > >
> > > > > >
> > > > > > > This makes me think about whether or not we really need
> > > > > > > event_triggered. The assumption in the virtqueue_disable_cb() seems
> > > > > > > wrong:
> > > > > > >
> > > > > > > /* If device triggered an event already it won't trigger one again:
> > > > > > >  * no need to disable.
> > > > > > >  */
> > > > > > > if (vq->event_triggered)
> > > > > > >                 return;
> > > > > > >
> > > > > > > This is wrong if there's no event index support.
> > > > > >
> > > > > >
> > > > > > I don't get it.  how does this get triggered?
> > > > > >
> > > > > > You are talking about device without event index?
> > > > > > Here's code from vring_interrupt():
> > > > > >
> > > > > >         /* Just a hint for performance: so it's ok that this can be racy! */
> > > > > >         if (vq->event)
> > > > > >                 vq->event_triggered = true;
> > > > >
> > > > > But we have the following in virtqueue_disable_cb():
> > > > >
> > > > >         /* If device triggered an event already it won't trigger one again:
> > > > >          * no need to disable.
> > > > >          */
> > > > >         if (vq->event_triggered)
> > > > >                 return;
> > > > >
> > > > >         if (vq->packed_ring)
> > > > >                 virtqueue_disable_cb_packed(_vq);
> > > > >         else
> > > > >                 virtqueue_disable_cb_split(_vq);
> > > > >
> > > > > This means, without an event index, we don't set avail flags. So the
> > > > > interrupt is not disabled actually in this case.
> > > > >
> > > > > Thanks
> > > >
> > > > Only if event_triggered is true, which without event index it never is.
> > >
> > > I'm not sure I will get here. I meant for example the commit
> > > suppresses the effort of skb_xmit_done():
> > >
> > > static void skb_xmit_done(struct virtqueue *vq)
> > > {
> > >         struct virtnet_info *vi = vq->vdev->priv;
> > >         struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi;
> > >
> > >         /* Suppress further interrupts. */
> > >         virtqueue_disable_cb(vq);
> > >
> > > The virtqueue_disable_cb() doesn't disable further interrupts when the
> > > event index is not there.
> > >
> > > Thanks
> >
> > Check what can set event_triggered, you will see.
> 
> Set to truth by vring_interrupt()

vring_interrupt only sets it to true if vq->event is true


> Set to false by virtqueue_init(), virtqueue_enable_cb_prepare(),
> virtqueue_enable_cb_delayed()
> 
> Assuming NAPI TX is enabled and the device doesn't support event index.
> 
> 1) driver sends packets 1-10
> 2) the start_xmit() for the last packet will call
> virtqueue_enable_cb_delayed() which set event_triggered = false
> 3) 1st packet were sent, vring_interrupt set event_triggered = true
> 4) skb_xmit_done() won't disable virtqueue_disable_cb() in this case
> 5) so we will get the interrupts for 2nd to 10th packet
> 
> Anything I missed here?

3 does not happen if event index is off.

> Note the comment said it's used for event index:
> 
>         /* Hint for event idx: already triggered no need to disable. */
>         bool event_triggered;
> 
> I guess what you meant is that if we don't publish a new event, we
> will get at most 1 interrupt for everything $queue_size used buffers.
> But this is not the case without event index. Btw, it may supress the
> effort of:
> 
> vring_used_event(&vq->split.vring) = 0x0;
> 
> Thanks


Because it's not necessary then.

> >
> >
> >
> > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > > And the
> > > > > > > event_triggered is somehow duplicated with the
> > > > > > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix
> > > > > > > might be:
> > > > > > >
> > > > > > > 1) remove event_triggered
> > > > > > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in
> > > > > > > vring_interrrupt if event index is supported
> > > > > > >
> > > > > > > ?
> > > > > > >
> > > > > > > Thanks
> > > > > >
> > > > > > I am not sure all this is right and I'd rather we focused
> > > > > > performance/correctness and cleanups separately.
> > > > > >
> > > > > >
> > > > > >
> > > > > >
> > > > > > >
> > > > > > > > But because of the introduction of event_triggered, here,
> > > > > > > > virtqueue_get_buf_ctx_split  cannot be recognized
> > > > > > > > that the interrupt has been turned off.
> > > > > > > >
> > > > > > > > if we want  another interrupt for the next entry, We should probably
> > > > > > > > call virtqueue_enable_cb？
> > > > > > > >
> > > > > > > > Thanks
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > > > > > >                                 &vring_used_event(&vq->split.vring),
> > > > > > > > > >                                 cpu_to_virtio16(_vq->vdev, vq->last_used_idx));
> > > > > > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq,
> > > > > > > > > >          * by writing event index and flush out the write before
> > > > > > > > > >          * the read in the next get_buf call.
> > > > > > > > > >          */
> > > > > > > > > > -       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC)
> > > > > > > > > > +       if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC
> > > > > > > > > > +                       && (vq->event_triggered == false))
> > > > > > > > > >                 virtio_store_mb(vq->weak_barriers,
> > > > > > > > > >                                 &vq->packed.vring.driver->off_wrap,
> > > > > > > > > >                                 cpu_to_le16(vq->last_used_idx));
> > > > > > > > > > --
> > > > > > > > > > 2.31.1
> > > > > > > > > >
> > > > > > > > >
> > > > > > > >
> > > > > >
> > > >
> >