Michael S. Tsirkin
2023-Mar-24 06:08 UTC
[PATCH] virtio_ring: Suppress tx interrupt when napi_tx disable
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 at 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 at 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
Michael S. Tsirkin
2023-Mar-24 06:37 UTC
[PATCH] virtio_ring: Suppress tx interrupt when napi_tx disable
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 at 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 at 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