[dpdk-dev] [RFC PATCH] avail idx update optimizations

[Xie, Huawei]

On 4/24/2016 5:15 PM, Michael S. Tsirkin wrote:
> On Sun, Apr 24, 2016 at 02:45:22AM +0000, Xie, Huawei wrote:
>> Forget to cc the mailing list.
>>
>> On 4/22/2016 9:53 PM, Xie, Huawei wrote:
>>> Hi:
>>>
>>> This is a series of virtio/vhost idx/ring update optimizations for cache
>>> to cache transfer. Actually I don't expect many of them as virtio/vhost
>>> has already done quite right.
> Hmm - is it a series or a single patch?

Others in my mind is caching a copy of avail index in vhost. Use the
cached copy if there are enough slots and then sync with the index in
the ring.
Haven't evaluated that idea.

>>> For this patch, in a VM2VM test, i observed ~6% performance increase.
> Interesting. In that case, it seems likely that new ring layout
> would give you an even bigger performance gain.
> Could you take a look at tools/virtio/ringtest/ring.c
> in latest Linux and tell me what do you think?
> In particular, I know you looked at using vectored instructions
> to do ring updates - would the layout in tools/virtio/ringtest/ring.c
> interfere with that?

Thanks. Would check. You know i have ever tried fixing avail ring in the
virtio driver. In purely vhost->virtio test, it could have 2~3 times
performance boost, but it isn't that obvious if with the physical nic
involved, investigating that issue.
I had planned to sync with you whether it is generic enough that we
could have a negotiated feature, either for in order descriptor
processing or like fixed avail ring. Would check your new ring layout.


>
>>> In VM1, run testpmd with txonly mode
>>> In VM2, run testpmd with rxonly mode
>>> In host, run testpmd(with two vhostpmds) with io forward
>>>
>>> Michael:
>>> We have talked about this method when i tried the fixed ring.
>>>
>>>
>>> On 4/22/2016 5:12 PM, Xie, Huawei wrote:
>>>> eliminate unnecessary cache to cache transfer between virtio and vhost
>>>> core
> Yes I remember proposing this, but you probably should include the
> explanation about why this works in he commit log:
>
> - pre-format avail ring with expected descriptor index values
> - as long as entries are consumed in-order, there's no
>   need to modify the avail ring
> - as long as avail ring is not modified, it can be
>   valid in caches of both consumer and producer

Yes, would add the explanation in the formal patch.


>
>>>> ---
>>>>  drivers/net/virtio/virtqueue.h | 3 ++-
>>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>>>> index 4e9239e..8c46a83 100644
>>>> --- a/drivers/net/virtio/virtqueue.h
>>>> +++ b/drivers/net/virtio/virtqueue.h
>>>> @@ -302,7 +302,8 @@ vq_update_avail_ring(struct virtqueue *vq, uint16_t desc_idx)
>>>>  	 * descriptor.
>>>>  	 */
>>>>  	avail_idx = (uint16_t)(vq->vq_avail_idx & (vq->vq_nentries - 1));
>>>> -	vq->vq_ring.avail->ring[avail_idx] = desc_idx;
>>>> +	if (unlikely(vq->vq_ring.avail->ring[avail_idx] != desc_idx))
>>>> +		vq->vq_ring.avail->ring[avail_idx] = desc_idx;
>>>>  	vq->vq_avail_idx++;
>>>>  }
>>>>