On Tue, Feb 18, 2025 at 10:39:08AM +0800, Jason Wang wrote:
> There are several issues existed in start_xmit():
>
> - Transmitted packets need to be freed before sending a packet, this
> introduces delay and increases the average packets transmit
> time. This also increase the time that spent in holding the TX lock.
> - Notification is enabled after free_old_xmit_skbs() which will
> introduce unnecessary interrupts if TX notification happens on the
> same CPU that is doing the transmission now (actually, virtio-net
> driver are optimized for this case).
>
> So this patch tries to avoid those issues by not cleaning transmitted
> packets in start_xmit() when TX NAPI is enabled and disable
> notifications even more aggressively. Notification will be since the
> beginning of the start_xmit(). But we can't enable delayed
> notification after TX is stopped as we will lose the
> notifications. Instead, the delayed notification needs is enabled
> after the virtqueue is kicked for best performance.
>
> Performance numbers:
>
> 1) single queue 2 vcpus guest with pktgen_sample03_burst_single_flow.sh
> (burst 256) + testpmd (rxonly) on the host:
>
> - When pinning TX IRQ to pktgen VCPU: split virtqueue PPS were
> increased 55% from 6.89 Mpps to 10.7 Mpps and 32% TX interrupts were
> eliminated. Packed virtqueue PPS were increased 50% from 7.09 Mpps to
> 10.7 Mpps, 99% TX interrupts were eliminated.
>
> - When pinning TX IRQ to VCPU other than pktgen: split virtqueue PPS
> were increased 96% from 5.29 Mpps to 10.4 Mpps and 45% TX interrupts
> were eliminated; Packed virtqueue PPS were increased 78% from 6.12
> Mpps to 10.9 Mpps and 99% TX interrupts were eliminated.
>
> 2) single queue 1 vcpu guest + vhost-net/TAP on the host: single
> session netperf from guest to host shows 82% improvement from
> 31Gb/s to 58Gb/s, %stddev were reduced from 34.5% to 1.9% and 88%
> of TX interrupts were eliminated.
>
> Signed-off-by: Jason Wang <jasowang@xxxxxxxxxx>
okay
Acked-by: Michael S. Tsirkin <mst@xxxxxxxxxx>
but tell me something, would it be even better to schedule
napi once, and have that deal with enabling notifications?
> ---
> drivers/net/virtio_net.c | 45 ++++++++++++++++++++++++++++------------
> 1 file changed, 32 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7646ddd9bef7..ac26a6201c44 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1088,11 +1088,10 @@ static bool is_xdp_raw_buffer_queue(struct virtnet_info *vi, int q)
> return false;
> }
>
> -static void check_sq_full_and_disable(struct virtnet_info *vi,
> - struct net_device *dev,
> - struct send_queue *sq)
> +static bool tx_may_stop(struct virtnet_info *vi,
> + struct net_device *dev,
> + struct send_queue *sq)
> {
> - bool use_napi = sq->napi.weight;
> int qnum;
>
> qnum = sq - vi->sq;
> @@ -1114,6 +1113,25 @@ static void check_sq_full_and_disable(struct virtnet_info *vi,
> u64_stats_update_begin(&sq->stats.syncp);
> u64_stats_inc(&sq->stats.stop);
> u64_stats_update_end(&sq->stats.syncp);
> +
> + return true;
> + }
> +
> + return false;
> +}
> +
> +static void check_sq_full_and_disable(struct virtnet_info *vi,
> + struct net_device *dev,
> + struct send_queue *sq)
> +{
> + bool use_napi = sq->napi.weight;
> + int qnum;
> +
> + qnum = sq - vi->sq;
> +
> + if (tx_may_stop(vi, dev, sq)) {
> + struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
> +
> if (use_napi) {
> if (unlikely(!virtqueue_enable_cb_delayed(sq->vq)))
> virtqueue_napi_schedule(&sq->napi, sq->vq);
> @@ -3253,15 +3271,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> bool use_napi = sq->napi.weight;
> bool kick;
>
> - /* Free up any pending old buffers before queueing new ones. */
> - do {
> - if (use_napi)
> - virtqueue_disable_cb(sq->vq);
> -
> + if (!use_napi)
> free_old_xmit(sq, txq, false);
> -
> - } while (use_napi && !xmit_more &&
> - unlikely(!virtqueue_enable_cb_delayed(sq->vq)));
> + else
> + virtqueue_disable_cb(sq->vq);
>
> /* timestamp packet in software */
> skb_tx_timestamp(skb);
> @@ -3287,7 +3300,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> nf_reset_ct(skb);
> }
>
> - check_sq_full_and_disable(vi, dev, sq);
> + if (use_napi)
> + tx_may_stop(vi, dev, sq);
> + else
> + check_sq_full_and_disable(vi, dev,sq);
>
> kick = use_napi ? __netdev_tx_sent_queue(txq, skb->len, xmit_more) :
> !xmit_more || netif_xmit_stopped(txq);
> @@ -3299,6 +3315,9 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> }
> }
>
> + if (use_napi && kick && unlikely(!virtqueue_enable_cb_delayed(sq->vq)))
> + virtqueue_napi_schedule(&sq->napi, sq->vq);
> +
> return NETDEV_TX_OK;
> }
>
> --
> 2.34.1
