linux-kernel - Re: [PATCH] virtio_ring: Suppress tx interrupt when napi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 24 Mar 2023 02:08:52 -0400
From:   "Michael S. Tsirkin" <mst@...hat.com>
To:     Albert Huang <huangjie.albert@...edance.com>
Cc:     Jason Wang <jasowang@...hat.com>,
        "open list:VIRTIO CORE AND NET DRIVERS" 
        <virtualization@...ts.linux-foundation.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [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@...edance.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@...edance.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