* [dpdk-dev] [PATCH] vhost: avoid memory barriers when no descriptors dequeued
@ 2018-10-19 14:00 Maxime Coquelin
2018-10-22 7:15 ` Tiwei Bie
0 siblings, 1 reply; 3+ messages in thread
From: Maxime Coquelin @ 2018-10-19 14:00 UTC (permalink / raw)
To: dev, tiwei.bie, zhihong.wang, jfreimann; +Cc: stable, Maxime Coquelin
In both split and packed dequeue paths, flush_shadow_used_ring
and vhost_ring_call variants gets called even if not packets
have been dequeued, and so no descriptors updates happened.
It has an impact on CPU pipeline, as memory barriers are used
in these functions.
This patch don't call these functions if no descriptors have
been dequeued. The performance gain with split ring when
dequeue zero-copy is disabled should be null, but should be
noticeable with packed ring or dequeue zero-copy enabled.
Fixes: ae999ce49dcb ("vhost: add Tx support for packed ring")
Fixes: 915cf9404225 ("vhost: use shadow used ring in dequeue path")
Cc: stable@dpdk.org
Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
lib/librte_vhost/virtio_net.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c
index f8794ee19..48228f16b 100644
--- a/lib/librte_vhost/virtio_net.c
+++ b/lib/librte_vhost/virtio_net.c
@@ -1359,8 +1359,10 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
}
}
- flush_shadow_used_ring_split(dev, vq);
- vhost_vring_call_split(dev, vq);
+ if (likely(vq->shadow_used_idx)) {
+ flush_shadow_used_ring_split(dev, vq);
+ vhost_vring_call_split(dev, vq);
+ }
}
rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]);
@@ -1435,7 +1437,7 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
}
vq->last_avail_idx += i;
- if (likely(dev->dequeue_zero_copy == 0)) {
+ if (likely(dev->dequeue_zero_copy == 0 && i != 0)) {
do_data_copy_dequeue(vq);
if (unlikely(i < count))
vq->shadow_used_idx = i;
@@ -1475,8 +1477,10 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
}
}
- flush_shadow_used_ring_packed(dev, vq);
- vhost_vring_call_packed(dev, vq);
+ if (likely(vq->shadow_used_idx)) {
+ flush_shadow_used_ring_packed(dev, vq);
+ vhost_vring_call_packed(dev, vq);
+ }
}
VHOST_LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__);
@@ -1550,7 +1554,7 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
}
}
- if (likely(dev->dequeue_zero_copy == 0)) {
+ if (likely(dev->dequeue_zero_copy == 0 && i != 0)) {
do_data_copy_dequeue(vq);
if (unlikely(i < count))
vq->shadow_used_idx = i;
--
2.17.1
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-dev] [PATCH] vhost: avoid memory barriers when no descriptors dequeued
2018-10-19 14:00 [dpdk-dev] [PATCH] vhost: avoid memory barriers when no descriptors dequeued Maxime Coquelin
@ 2018-10-22 7:15 ` Tiwei Bie
2018-10-22 8:31 ` Maxime Coquelin
0 siblings, 1 reply; 3+ messages in thread
From: Tiwei Bie @ 2018-10-22 7:15 UTC (permalink / raw)
To: Maxime Coquelin; +Cc: dev, zhihong.wang, jfreimann, stable
On Fri, Oct 19, 2018 at 04:00:58PM +0200, Maxime Coquelin wrote:
> In both split and packed dequeue paths, flush_shadow_used_ring
> and vhost_ring_call variants gets called even if not packets
> have been dequeued, and so no descriptors updates happened.
>
> It has an impact on CPU pipeline, as memory barriers are used
> in these functions.
>
> This patch don't call these functions if no descriptors have
> been dequeued. The performance gain with split ring when
> dequeue zero-copy is disabled should be null, but should be
> noticeable with packed ring or dequeue zero-copy enabled.
>
> Fixes: ae999ce49dcb ("vhost: add Tx support for packed ring")
> Fixes: 915cf9404225 ("vhost: use shadow used ring in dequeue path")
> Cc: stable@dpdk.org
>
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
> lib/librte_vhost/virtio_net.c | 16 ++++++++++------
> 1 file changed, 10 insertions(+), 6 deletions(-)
[...]
>
> - if (likely(dev->dequeue_zero_copy == 0)) {
> + if (likely(dev->dequeue_zero_copy == 0 && i != 0)) {
> do_data_copy_dequeue(vq);
> if (unlikely(i < count))
> vq->shadow_used_idx = i;
When i is 0, we may need to update vq->shadow_used_idx to 0,
e.g. when error happens after update_shadow_used_ring_split()
in the first iteration of the loop.
> @@ -1475,8 +1477,10 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
> }
> }
>
> - flush_shadow_used_ring_packed(dev, vq);
> - vhost_vring_call_packed(dev, vq);
> + if (likely(vq->shadow_used_idx)) {
> + flush_shadow_used_ring_packed(dev, vq);
> + vhost_vring_call_packed(dev, vq);
> + }
> }
>
> VHOST_LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__);
> @@ -1550,7 +1554,7 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
> }
> }
>
> - if (likely(dev->dequeue_zero_copy == 0)) {
> + if (likely(dev->dequeue_zero_copy == 0 && i != 0)) {
Ditto
> do_data_copy_dequeue(vq);
> if (unlikely(i < count))
> vq->shadow_used_idx = i;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [dpdk-dev] [PATCH] vhost: avoid memory barriers when no descriptors dequeued
2018-10-22 7:15 ` Tiwei Bie
@ 2018-10-22 8:31 ` Maxime Coquelin
0 siblings, 0 replies; 3+ messages in thread
From: Maxime Coquelin @ 2018-10-22 8:31 UTC (permalink / raw)
To: Tiwei Bie; +Cc: dev, zhihong.wang, jfreimann, stable
On 10/22/2018 09:15 AM, Tiwei Bie wrote:
> On Fri, Oct 19, 2018 at 04:00:58PM +0200, Maxime Coquelin wrote:
>> In both split and packed dequeue paths, flush_shadow_used_ring
>> and vhost_ring_call variants gets called even if not packets
>> have been dequeued, and so no descriptors updates happened.
>>
>> It has an impact on CPU pipeline, as memory barriers are used
>> in these functions.
>>
>> This patch don't call these functions if no descriptors have
>> been dequeued. The performance gain with split ring when
>> dequeue zero-copy is disabled should be null, but should be
>> noticeable with packed ring or dequeue zero-copy enabled.
>>
>> Fixes: ae999ce49dcb ("vhost: add Tx support for packed ring")
>> Fixes: 915cf9404225 ("vhost: use shadow used ring in dequeue path")
>> Cc: stable@dpdk.org
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>> lib/librte_vhost/virtio_net.c | 16 ++++++++++------
>> 1 file changed, 10 insertions(+), 6 deletions(-)
> [...]
>>
>> - if (likely(dev->dequeue_zero_copy == 0)) {
>> + if (likely(dev->dequeue_zero_copy == 0 && i != 0)) {
>> do_data_copy_dequeue(vq);
>> if (unlikely(i < count))
>> vq->shadow_used_idx = i;
>
> When i is 0, we may need to update vq->shadow_used_idx to 0,
> e.g. when error happens after update_shadow_used_ring_split()
> in the first iteration of the loop.
I totally agree, it is broken when error happens.
I will fix that in next revision.
Thanks,
Maxime
>
>> @@ -1475,8 +1477,10 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> }
>> }
>>
>> - flush_shadow_used_ring_packed(dev, vq);
>> - vhost_vring_call_packed(dev, vq);
>> + if (likely(vq->shadow_used_idx)) {
>> + flush_shadow_used_ring_packed(dev, vq);
>> + vhost_vring_call_packed(dev, vq);
>> + }
>> }
>>
>> VHOST_LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__);
>> @@ -1550,7 +1554,7 @@ virtio_dev_tx_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
>> }
>> }
>>
>> - if (likely(dev->dequeue_zero_copy == 0)) {
>> + if (likely(dev->dequeue_zero_copy == 0 && i != 0)) {
>
> Ditto
>
>> do_data_copy_dequeue(vq);
>> if (unlikely(i < count))
>> vq->shadow_used_idx = i;
>> --
>> 2.17.1
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-10-22 8:31 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-19 14:00 [dpdk-dev] [PATCH] vhost: avoid memory barriers when no descriptors dequeued Maxime Coquelin
2018-10-22 7:15 ` Tiwei Bie
2018-10-22 8:31 ` Maxime Coquelin
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).