Re: [PATCH v3] vhost: add IRQ suppression

[Eelco Chaudron <echaudro@redhat.com>]

On 29 Sep 2023, at 12:38, Maxime Coquelin wrote:

> Guest notifications offloading, which has been introduced
> in v23.07, aims at offloading syscalls out of the datapath.
>
> This patch optimizes the offloading by not offloading the
> guest notification for a given virtqueue if one is already
> being offloaded by the application.
>
> With a single VDUSE device, we can already see few
> notifications being suppressed when doing throughput
> testing with Iperf3. We can expect to see much more being
> suppressed when the offloading thread is under pressure.
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks for adding this Maxime. I did some tests with OVS and my old determinism patchset, and it works perfectly.

I have two small nits, but this change looks good to me.

Acked-by: Eelco Chaudron <echaudro@redhat.com>

> ---
>
> v3: s/0/false/ (David)
>
>  lib/vhost/vhost.c |  4 ++++
>  lib/vhost/vhost.h | 27 +++++++++++++++++++++------
>  2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index c03bb9c6eb..7fde412ef3 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -49,6 +49,8 @@ static const struct vhost_vq_stats_name_off vhost_vq_stat_strings[] = {
>  		stats.guest_notifications_offloaded)},
>  	{"guest_notifications_error", offsetof(struct vhost_virtqueue,
>  		stats.guest_notifications_error)},
> +	{"guest_notifications_suppressed", offsetof(struct vhost_virtqueue,
> +		stats.guest_notifications_suppressed)},
>  	{"iotlb_hits",             offsetof(struct vhost_virtqueue, stats.iotlb_hits)},
>  	{"iotlb_misses",           offsetof(struct vhost_virtqueue, stats.iotlb_misses)},
>  	{"inflight_submitted",     offsetof(struct vhost_virtqueue, stats.inflight_submitted)},
> @@ -1517,6 +1519,8 @@ rte_vhost_notify_guest(int vid, uint16_t queue_id)
>
>  	rte_rwlock_read_lock(&vq->access_lock);
>
> +	__atomic_store_n(&vq->irq_pending, false, __ATOMIC_RELEASE);
> +
>  	if (dev->backend_ops->inject_irq(dev, vq)) {
>  		if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
>  			__atomic_fetch_add(&vq->stats.guest_notifications_error,
> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
> index 9723429b1c..5fc9035a1f 100644
> --- a/lib/vhost/vhost.h
> +++ b/lib/vhost/vhost.h
> @@ -156,6 +156,7 @@ struct virtqueue_stats {
>  	uint64_t iotlb_misses;
>  	uint64_t inflight_submitted;
>  	uint64_t inflight_completed;
> +	uint64_t guest_notifications_suppressed;
>  	/* Counters below are atomic, and should be incremented as such. */
>  	uint64_t guest_notifications;
>  	uint64_t guest_notifications_offloaded;
> @@ -346,6 +347,8 @@ struct vhost_virtqueue {
>
>  	struct vhost_vring_addr ring_addrs;
>  	struct virtqueue_stats	stats;
> +
> +	bool irq_pending;

nit: Other elements in this structure have the names aligned, not sure if this should be done for this item also.

>  } __rte_cache_aligned;
>
>  /* Virtio device status as per Virtio specification */
> @@ -908,12 +911,24 @@ vhost_need_event(uint16_t event_idx, uint16_t new_idx, uint16_t old)
>  static __rte_always_inline void
>  vhost_vring_inject_irq(struct virtio_net *dev, struct vhost_virtqueue *vq)
>  {
> -	if (dev->notify_ops->guest_notify &&
> -	    dev->notify_ops->guest_notify(dev->vid, vq->index)) {
> -		if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
> -			__atomic_fetch_add(&vq->stats.guest_notifications_offloaded,
> -				1, __ATOMIC_RELAXED);
> -		return;
> +	bool expected = false;
> +
> +	if (dev->notify_ops->guest_notify) {
> +		if (__atomic_compare_exchange_n(&vq->irq_pending, &expected, true, 0,
> +				  __ATOMIC_RELEASE, __ATOMIC_RELAXED)) {
> +			if (dev->notify_ops->guest_notify(dev->vid, vq->index)) {
> +				if (dev->flags & VIRTIO_DEV_STATS_ENABLED)
> +					__atomic_fetch_add(&vq->stats.guest_notifications_offloaded,
> +						1, __ATOMIC_RELAXED);
> +				return;
> +			}
> +
> +			/* Offloading failed, fallback to direct IRQ injection */

nit: Some comments end with a dot and some do not, not sure what is the preference in DPDK.

> +			__atomic_store_n(&vq->irq_pending, false, __ATOMIC_RELEASE);
> +		} else {
> +			vq->stats.guest_notifications_suppressed++;
> +			return;
> +		}
>  	}
>
>  	if (dev->backend_ops->inject_irq(dev, vq)) {
> --
> 2.41.0