From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Coquelin Subject: Re: [PATCH] vhost: avoid memory barriers when no descriptors dequeued Date: Mon, 22 Oct 2018 10:31:44 +0200 Message-ID: <2132245c-4a16-b858-ab7f-fd2e7f94134b@redhat.com> References: <20181019140058.4981-1-maxime.coquelin@redhat.com> <20181022071510.GA30145@debian> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, zhihong.wang@intel.com, jfreimann@redhat.com, stable@dpdk.org To: Tiwei Bie Return-path: In-Reply-To: <20181022071510.GA30145@debian> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 >> --- >> 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 >>