Re: [PATCH v9 2/2] [PATCH 2/2] Implement VirtIO Network Notification Coalescing (VIRTIO_NET_F_NOTF_COAL). This allows guests to reduce interrupt overhead by configuring coalescing parameters via ethtool -C for both RX and TX paths.

["Michael S. Tsirkin" <mst@redhat.com>]

Thanks for the patch! Yet something to improve:

On Thu, Jul 02, 2026 at 06:46:32PM +0530, Koushik Dutta wrote:
> The feature supports two coalescing modes:
> - Time-based: delay notifications up to N microseconds
> - Count-based: delay until N packets are processed
> 
> Implementation details:
> - Added VIRTIO_NET_CTRL_NOTF_COAL class handling in control virtqueue
> - RX path: batches notifications based on packet count or timeout
> - TX path: leverages the unified dispatcher to dynamically enable
>   timer-based coalescing when guest configures it via ethtool
> - Coalescing parameters persist across live migration
> 
> Note: When tx=timer is configured at VM launch, the coalescing feature
> is automatically disabled because tx=timer already introduces a fixed
> 150µs delay for packet batching, making notification coalescing redundant.
> 
> Signed-off-by: Koushik Dutta <kdutta@redhat.com>

What is the benefit of the feature for the users?
E.g. do you see a perf gain sometimes?

> ---
>  hw/core/machine.c              |   1 +
>  hw/net/virtio-net.c            | 176 +++++++++++++++++++++++++++++++--
>  include/hw/virtio/virtio-net.h |   7 ++
>  net/passt.c                    |   1 +
>  net/tap.c                      |   1 +
>  net/vhost-user.c               |   1 +
>  net/vhost-vdpa.c               |   1 +
>  7 files changed, 182 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index 4d8b15d99e..55ee886f41 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -43,6 +43,7 @@ GlobalProperty hw_compat_11_0[] = {
>      { "chardev-vc", "encoding", "cp437" },
>      { "tpm-crb", "cap-chunk", "off" },
>      { "tpm-crb", "x-allow-chunk-migration", "off" },
> +    { TYPE_VIRTIO_NET, "vq_notf_coal", "off" },
>  };
>  const size_t hw_compat_11_0_len = G_N_ELEMENTS(hw_compat_11_0);

it is off by default, right? why do we need compat?

> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 319842cf28..4de6aa3b23 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -157,6 +157,16 @@ static void flush_or_purge_queued_packets(NetClientState *nc)
>   * - we could suppress RX interrupt if we were so inclined.
>   */
>  
> +static void virtio_net_rx_notify(void *opaque)
> +{
> +    VirtIONetQueue *q = opaque;
> +    VirtIONet *n = q->n;
> +    VirtIODevice *vdev = VIRTIO_DEVICE(n);
> +
> +    q->rx_pkt_cnt = 0;
> +    virtio_notify(vdev, q->rx_vq);
> +}
> +
>  static void virtio_net_get_config(VirtIODevice *vdev, uint8_t *config)
>  {
>      VirtIONet *n = VIRTIO_NET(vdev);
> @@ -1004,6 +1014,56 @@ static void virtio_net_set_features(VirtIODevice *vdev,
>  
>  static void virtio_net_tx_timer(void *opaque);
>  
> +static int virtio_net_handle_coal(VirtIONet *n, uint8_t cmd,
> +                                  struct iovec *iov, unsigned int iov_cnt)
> +{
> +    struct virtio_net_ctrl_coal coal;
> +    VirtIONetQueue *q;
> +    size_t s;
> +    int i;
> +
> +    s = iov_to_buf(iov, iov_cnt, 0, &coal, sizeof(coal));
> +    if (s != sizeof(coal)) {
> +        return VIRTIO_NET_ERR;
> +    }
> +
> +    if (cmd == VIRTIO_NET_CTRL_NOTF_COAL_RX_SET) {
> +        for (i = 0; i < n->max_queue_pairs; i++) {
> +            q = &n->vqs[i];
> +            q->rx_coal_usecs = le32_to_cpu(coal.max_usecs);
> +            q->rx_coal_packets = le32_to_cpu(coal.max_packets);
> +            if (q->rx_coal_usecs > 0) {
> +                if (!q->rx_timer) {
> +                    q->rx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                               virtio_net_rx_notify,
> +                                               q);
> +                }
> +            } else {
> +                g_clear_pointer(&q->rx_timer, timer_free);
> +            }
> +        }
> +    } else if (cmd == VIRTIO_NET_CTRL_NOTF_COAL_TX_SET) {
> +        for (i = 0; i < n->max_queue_pairs; i++) {
> +            q = &n->vqs[i];
> +            q->tx_coal_usecs = le32_to_cpu(coal.max_usecs);
> +            q->tx_coal_packets = le32_to_cpu(coal.max_packets);
> +            /* Converted us to ns */
> +            n->tx_timeout = q->tx_coal_usecs * 1000;
> +            if (q->tx_coal_usecs > 0) {
> +                if (!q->tx_timer) {
> +                    q->tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                               virtio_net_tx_timer,
> +                                               q);
> +                }
> +            } else {
> +                g_clear_pointer(&q->tx_timer, timer_free);
> +            }
> +        }
> +    }
> +
> +    return VIRTIO_NET_OK;
> +}
> +
>  static int virtio_net_handle_rx_mode(VirtIONet *n, uint8_t cmd,
>                                       struct iovec *iov, unsigned int iov_cnt)
>  {
> @@ -1583,6 +1643,8 @@ size_t virtio_net_handle_ctrl_iov(VirtIODevice *vdev,
>          status = virtio_net_handle_mq(n, ctrl.cmd, iov, out_num);
>      } else if (ctrl.class == VIRTIO_NET_CTRL_GUEST_OFFLOADS) {
>          status = virtio_net_handle_offloads(n, ctrl.cmd, iov, out_num);
> +    } else if (ctrl.class == VIRTIO_NET_CTRL_NOTF_COAL) {
> +        status = virtio_net_handle_coal(n, ctrl.cmd, iov, out_num);
>      }
>  
>      s = iov_from_buf(in_sg, in_num, 0, &status, sizeof(status));
> @@ -2042,7 +2104,23 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
>      }
>  
>      virtqueue_flush(q->rx_vq, i);
> -    virtio_notify(vdev, q->rx_vq);
> +
> +    /* rx coalescing */
> +    q->rx_pkt_cnt += i;
> +    if (q->rx_coal_usecs == 0 || q->rx_pkt_cnt >= q->rx_coal_packets) {
> +        if (q->rx_timer) {
> +            timer_del(q->rx_timer);
> +        }
> +        virtio_net_rx_notify(q);
> +    } else {
> +        if (q->rx_timer) {
> +            if (!timer_pending(q->rx_timer)) {
> +                timer_mod(q->rx_timer,
> +                          qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) +
> +                          q->rx_coal_usecs * 1000);
> +            }
> +        }
> +    }
>  
>      return size;
>  
> @@ -2900,6 +2978,12 @@ static void virtio_net_tx_timer(void *opaque)
>      if (ret == -EBUSY || ret == -EINVAL) {
>          return;
>      }
> +    if (q->tx_pkt_cnt < ret) {
> +        q->tx_pkt_cnt = 0;
> +    } else {
> +        q->tx_pkt_cnt -= ret;
> +    }
> +

q->tx_pkt_cnt -= MIN(q->tx_pkt_cnt, ret);

?

>      /*
>       * If we flush a full burst of packets, assume there are
>       * more coming and immediately rearm
> @@ -2919,6 +3003,7 @@ static void virtio_net_tx_timer(void *opaque)
>      ret = virtio_net_flush_tx(q);
>      if (ret > 0) {
>          virtio_queue_set_notification(q->tx_vq, 0);
> +        q->tx_pkt_cnt -= ret;
>          q->tx_waiting = 1;
>          timer_mod(q->tx_timer,
>                    qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + n->tx_timeout);
> @@ -2985,6 +3070,15 @@ static void virtio_net_handle_tx_dispatch(VirtIODevice *vdev, VirtQueue *vq)
>                                         virtio_net_tx_timer, q);
>          }
>          virtio_net_handle_tx_timer(vdev, vq);
> +    } else if (q->tx_coal_usecs > 0 && q->tx_timer) {
> +        q->tx_pkt_cnt++;
> +        if (q->tx_pkt_cnt < q->tx_coal_packets) {
> +            virtio_net_handle_tx_timer(vdev, vq);
> +        } else {
> +            q->tx_pkt_cnt = 0;
> +            timer_del(q->tx_timer);
> +            virtio_net_handle_tx_bh(vdev, vq);
> +        }
>      } else {
>          virtio_net_handle_tx_bh(vdev, vq);
>      }
> @@ -3006,6 +3100,12 @@ static void virtio_net_add_queue(VirtIONet *n, int index)
>                              &DEVICE(vdev)->mem_reentrancy_guard);
>  
>      n->vqs[index].tx_waiting = 0;
> +    n->vqs[index].rx_pkt_cnt = 0;
> +    n->vqs[index].tx_pkt_cnt = 0;
> +    n->vqs[index].rx_coal_usecs = 0;
> +    n->vqs[index].tx_coal_usecs = 0;
> +    n->vqs[index].rx_coal_packets = 0;
> +    n->vqs[index].tx_coal_packets = 0;
>      n->vqs[index].n = n;
>  }
>  
> @@ -3018,10 +3118,12 @@ static void virtio_net_del_queue(VirtIONet *n, int index)
>      qemu_purge_queued_packets(nc);
>  
>      virtio_del_queue(vdev, index * 2);
> -    if (q->tx_timer) {
> -        timer_free(q->tx_timer);
> -        q->tx_timer = NULL;
> -    } else {
> +
> +    /* Cleanup coalescing timers */
> +    g_clear_pointer(&q->rx_timer, timer_free);
> +    g_clear_pointer(&q->tx_timer, timer_free);
> +
> +    if (q->tx_bh) {
>          qemu_bh_delete(q->tx_bh);
>          q->tx_bh = NULL;
>      }
> @@ -3098,6 +3200,13 @@ static void virtio_net_get_features(VirtIODevice *vdev, uint64_t *features,
>      virtio_features_or(features, features, n->host_features_ex);
>  
>      virtio_add_feature_ex(features, VIRTIO_NET_F_MAC);
> +    if (n->tx_timer_activate) {
> +        virtio_clear_feature_ex(features, VIRTIO_NET_F_NOTF_COAL);
> +    } else {
> +        if (!virtio_has_feature(*features, VIRTIO_NET_F_NOTF_COAL)) {
> +            *features &= ~(1ULL << VIRTIO_NET_F_NOTF_COAL);
> +        }
> +    }
>  
>      if (!peer_has_vnet_hdr(n)) {
>          virtio_clear_feature_ex(features, VIRTIO_NET_F_CSUM);
> @@ -3252,6 +3361,27 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>      }
>  
>      virtio_net_commit_rss_config(n);
> +
> +    for (i = 0; i < n->max_queue_pairs; i++) {
> +        VirtIONetQueue *q = &n->vqs[i];
> +        if (q->rx_coal_usecs > 0) {
> +            if (!q->rx_timer) {
> +                q->rx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                           virtio_net_rx_notify,
> +                                           q);
> +            }
> +        }
> +
> +        if (q->tx_coal_usecs > 0) {
> +            n->tx_timeout = q->tx_coal_usecs * 1000;
> +            if (!q->tx_timer) {
> +                q->tx_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                           virtio_net_tx_timer,
> +                                           q);
> +            }
> +        }
> +    }
> +
>      return 0;
>  }
>  
> @@ -3271,13 +3401,41 @@ static int virtio_net_post_load_virtio(VirtIODevice *vdev)
>      return 0;
>  }
>  
> +static bool virtio_net_queue_notf_coal_needed(void *opaque)
> +{
> +    VirtIONetQueue *q = opaque;
> +    VirtIONet *n = q->n;
> +
> +    return virtio_vdev_has_feature(VIRTIO_DEVICE(n), VIRTIO_NET_F_NOTF_COAL);
> +}
> +
> +static const VMStateDescription vmstate_virtio_net_queue_notf_coal = {
> +    .name = "virtio-net-queue-tx_waiting/notf-coal",
> +    .version_id = 1,
> +    .minimum_version_id = 1,
> +    .needed = virtio_net_queue_notf_coal_needed,
> +    .fields = (const VMStateField[]) {
> +        VMSTATE_UINT32(rx_coal_usecs, VirtIONetQueue),
> +        VMSTATE_UINT32(tx_coal_usecs, VirtIONetQueue),
> +        VMSTATE_UINT32(rx_coal_packets, VirtIONetQueue),
> +        VMSTATE_UINT32(tx_coal_packets, VirtIONetQueue),
> +        VMSTATE_UINT32(rx_pkt_cnt, VirtIONetQueue),
> +        VMSTATE_UINT32(tx_pkt_cnt, VirtIONetQueue),
> +        VMSTATE_END_OF_LIST()
> +    },
> +};
> +
>  /* tx_waiting field of a VirtIONetQueue */
>  static const VMStateDescription vmstate_virtio_net_queue_tx_waiting = {
>      .name = "virtio-net-queue-tx_waiting",
>      .fields = (const VMStateField[]) {
>          VMSTATE_UINT32(tx_waiting, VirtIONetQueue),
>          VMSTATE_END_OF_LIST()
> -   },
> +    },
> +    .subsections = (const VMStateDescription * const []) {
> +        &vmstate_virtio_net_queue_notf_coal,
> +        NULL
> +    }
>  };
>  
>  static bool max_queue_pairs_gt_1(void *opaque, int version_id)
> @@ -3982,6 +4140,10 @@ static void virtio_net_device_realize(DeviceState *dev, Error **errp)
>  
>      if (n->net_conf.tx && strcmp(n->net_conf.tx, "timer") == 0) {
>          n->tx_timer_activate = true;
> +        if (n->host_features & (1ULL << VIRTIO_NET_F_NOTF_COAL)) {
> +            warn_report("virtio-net: 'vq_notf_coal' is incompatible with 'tx=timer',"
> +                        "disabling notification coalescing");
> +        }
>      }
>  
>      n->net_conf.tx_queue_size = MIN(virtio_net_max_tx_queue_size(n),
> @@ -4272,6 +4434,8 @@ static const Property virtio_net_properties[] = {
>                        VIRTIO_NET_F_GUEST_USO6, true),
>      DEFINE_PROP_BIT64("host_uso", VirtIONet, host_features,
>                        VIRTIO_NET_F_HOST_USO, true),
> +    DEFINE_PROP_BIT64("vq_notf_coal", VirtIONet, host_features,
> +                      VIRTIO_NET_F_NOTF_COAL, false),

So when do you expect users to enable this?
If this is experimental, let's make it unstable (start with "x-").


>      DEFINE_PROP_ON_OFF_AUTO_BIT64("hash-ipv4", VirtIONet,
>                                    rss_data.specified_hash_types,
>                                    VIRTIO_NET_HASH_REPORT_IPv4 - 1,
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index a4eb3f407e..550aaea2be 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -159,12 +159,19 @@ typedef struct VirtIONetQueue {
>      VirtQueue *rx_vq;
>      VirtQueue *tx_vq;
>      QEMUTimer *tx_timer;
> +    QEMUTimer *rx_timer;
>      QEMUBH *tx_bh;
>      uint32_t tx_waiting;
>      struct {
>          VirtQueueElement *elem;
>      } async_tx;
>      struct VirtIONet *n;
> +    uint32_t rx_coal_usecs;   /* RX interrupt coalescing timeout (microseconds) */
> +    uint32_t rx_coal_packets; /* RX packet count threshold for coalescing */
> +    uint32_t rx_pkt_cnt;      /* Current RX packet count since last notification */
> +    uint32_t tx_coal_usecs;   /* TX interrupt coalescing timeout (microseconds) */
> +    uint32_t tx_coal_packets; /* TX packet count threshold for coalescing */
> +    uint32_t tx_pkt_cnt;      /* Current TX packet count since last notification */

Please organize this in some kind of structure.
This way no need to repeat "coalescing" each time.


>  } VirtIONetQueue;
>  
>  struct VirtIONet {
> diff --git a/net/passt.c b/net/passt.c
> index 45440c399b..43b36ed8c5 100644
> --- a/net/passt.c
> +++ b/net/passt.c
> @@ -52,6 +52,7 @@ static const int user_feature_bits[] = {
>      VIRTIO_NET_F_GUEST_USO4,
>      VIRTIO_NET_F_GUEST_USO6,
>      VIRTIO_NET_F_HOST_USO,
> +    VIRTIO_NET_F_NOTF_COAL,
>  
>      /* This bit implies RARP isn't sent by QEMU out of band */
>      VIRTIO_NET_F_GUEST_ANNOUNCE,
> diff --git a/net/tap.c b/net/tap.c
> index 57ffb09885..e6ddbc1eb1 100644
> --- a/net/tap.c
> +++ b/net/tap.c
> @@ -63,6 +63,7 @@ static const int kernel_feature_bits[] = {
>      VIRTIO_F_NOTIFICATION_DATA,
>      VIRTIO_NET_F_RSC_EXT,
>      VIRTIO_NET_F_HASH_REPORT,
> +    VIRTIO_NET_F_NOTF_COAL,
>      VIRTIO_NET_F_GUEST_UDP_TUNNEL_GSO,
>      VIRTIO_NET_F_HOST_UDP_TUNNEL_GSO,
>      VHOST_INVALID_FEATURE_BIT
> diff --git a/net/vhost-user.c b/net/vhost-user.c
> index 2d0fc49b4d..f1e9b7a038 100644
> --- a/net/vhost-user.c
> +++ b/net/vhost-user.c
> @@ -54,6 +54,7 @@ static const int user_feature_bits[] = {
>      VIRTIO_NET_F_GUEST_USO4,
>      VIRTIO_NET_F_GUEST_USO6,
>      VIRTIO_NET_F_HOST_USO,
> +    VIRTIO_NET_F_NOTF_COAL,
>  
>      /* This bit implies RARP isn't sent by QEMU out of band */
>      VIRTIO_NET_F_GUEST_ANNOUNCE,


So the point of the whitelists is not to make people blindly
copy-paste them everywhere, but instead to make a plan how this will work.

For example, we need to send the control to backend I presume?


Similar for other backends.


> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index f1523697e2..0dcd6fb9f1 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -70,6 +70,7 @@ static const int vdpa_feature_bits[] = {
>      VIRTIO_NET_F_CTRL_RX,
>      VIRTIO_NET_F_CTRL_RX_EXTRA,
>      VIRTIO_NET_F_CTRL_VLAN,
> +    VIRTIO_NET_F_NOTF_COAL,
>      VIRTIO_NET_F_CTRL_VQ,
>      VIRTIO_NET_F_GSO,
>      VIRTIO_NET_F_GUEST_CSUM,
> -- 
> 2.53.0