[PATCH v3] vhost: add IRQ suppression

[Maxime Coquelin]

Hi Eelco,

On 10/3/23 14:36, Eelco Chaudron wrote:
> 
> 
> 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 at 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 at 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.

Ha yes, you're right.
I'll fix it while applying.

> 
>>   } __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.

I'm not sure either! I'm personally fine either way.

>> +			__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
> 

Thanks,
Maxime