Re: [dpdk-dev] [PATCH] vhost: avoid memory barriers when no descriptors dequeued

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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