diff mbox series

[v3] vhost: add IRQ suppression

Message ID 20230929103809.765417-1-maxime.coquelin@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series [v3] vhost: add IRQ suppression |

Checks

Context Check Description
ci/loongarch-compilation success Compilation OK
ci/checkpatch warning coding style issues
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/intel-Functional success Functional PASS
ci/github-robot: build success github build: passed
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-amd64-testing success Testing PASS
ci/iol-sample-apps-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS

Commit Message

Maxime Coquelin Sept. 29, 2023, 10:38 a.m. UTC 
  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>
---

v3: s/0/false/ (David)

 lib/vhost/vhost.c |  4 ++++
 lib/vhost/vhost.h | 27 +++++++++++++++++++++------
 2 files changed, 25 insertions(+), 6 deletions(-)

--
2.41.0

Comments

Eelco Chaudron Oct. 3, 2023, 12:36 p.m. UTC | #1 
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
Maxime Coquelin Oct. 3, 2023, 12:49 p.m. UTC | #2 
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@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.

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
David Marchand Oct. 10, 2023, 7:50 a.m. UTC | #3 
On Fri, Sep 29, 2023 at 12:38 PM Maxime Coquelin
<maxime.coquelin@redhat.com> 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>

This looks like a good idea.
Reviewed-by: David Marchand <david.marchand@redhat.com>
Maxime Coquelin Oct. 12, 2023, 1:49 p.m. UTC | #4 
On 9/29/23 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>
> ---
> 
> v3: s/0/false/ (David)
> 
>   lib/vhost/vhost.c |  4 ++++
>   lib/vhost/vhost.h | 27 +++++++++++++++++++++------
>   2 files changed, 25 insertions(+), 6 deletions(-)
> 
Applied to nex-virtio/for-next-net.

Thanks,
Maxime
diff mbox series

Patch

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;
 } __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 */
+			__atomic_store_n(&vq->irq_pending, false, __ATOMIC_RELEASE);
+		} else {
+			vq->stats.guest_notifications_suppressed++;
+			return;
+		}
 	}

 	if (dev->backend_ops->inject_irq(dev, vq)) {