From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: Eelco Chaudron <echaudro@redhat.com>
Cc: dev@dpdk.org, david.marchand@redhat.com, chenbo.xia@outlook.com
Subject: Re: [PATCH v3] vhost: add IRQ suppression
Date: Tue, 3 Oct 2023 14:49:29 +0200 [thread overview]
Message-ID: <b3e81400-9768-eb41-6474-e66173ebb3a9@redhat.com> (raw)
In-Reply-To: <D4D075DF-072B-46EE-A24E-E720999C8829@redhat.com>
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
next prev parent reply other threads:[~2023-10-03 12:49 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-09-29 10:38 Maxime Coquelin
2023-10-03 12:36 ` Eelco Chaudron
2023-10-03 12:49 ` Maxime Coquelin [this message]
2023-10-10 7:50 ` David Marchand
2023-10-12 13:49 ` Maxime Coquelin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=b3e81400-9768-eb41-6474-e66173ebb3a9@redhat.com \
--to=maxime.coquelin@redhat.com \
--cc=chenbo.xia@outlook.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=echaudro@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).