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

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

[Huawei Xie]

eliminate unnecessary cache to cache transfer between virtio and vhost
core

---
 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++;
 }
 
-- 
2.4.3

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

[Xie, Huawei]

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.
>
> For this patch, in a VM2VM test, i observed ~6% performance increase.
> 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
>>
>> ---
>>  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++;
>>  }
>>  
>

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

[Michael S. Tsirkin]

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?

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

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


> >>
> >> ---
> >>  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++;
> >>  }
> >>  
> >
> 

Re: [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++;
>>>>  }
>>>>  

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

[Xie, Huawei]

On 4/24/2016 9:24 PM, Xie, Huawei wrote:
> 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.

Tried cached vhost idx which gives a slight better perf, but will hold
this patch, as i guess we couldn't blindly set this cached avail idx to
0, which might cause issue in live migration.

>
>>>> 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++;
>>>>>  }
>>>>>  
>

[dpdk-dev] [PATCH] virtio: avoid avail ring entry index update if equal

[Huawei Xie]

Avail ring is updated by the frontend and consumed by the backend.
There are frequent core to core cache transfers for the avail ring.

This optmization avoids avail ring entry index update if the entry
already holds the same value.
As DPDK virtio PMD implements FIFO free descriptor list (also for
performance reason of CACHE), in which descriptors are allocated
from the head and freed to the tail, with this patch in most cases
avail ring will remain the same, then it would be valid in both caches
of frontend and backend.

Signed-off-by: Huawei Xie <huawei.xie@intel.com>
Suggested-by: ms >> Michael S. Tsirkin <mst@redhat.com>
---
 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++;
 }
 
-- 
2.4.3

Re: [dpdk-dev] [PATCH] virtio: avoid avail ring entry index update if equal

[Yuanhan Liu]

On Wed, Apr 27, 2016 at 04:53:58AM -0400, Huawei Xie wrote:
> Avail ring is updated by the frontend and consumed by the backend.
> There are frequent core to core cache transfers for the avail ring.
> 
> This optmization avoids avail ring entry index update if the entry
> already holds the same value.
> As DPDK virtio PMD implements FIFO free descriptor list (also for
> performance reason of CACHE), in which descriptors are allocated
> from the head and freed to the tail, with this patch in most cases
> avail ring will remain the same, then it would be valid in both caches
> of frontend and backend.

Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

> 
> Signed-off-by: Huawei Xie <huawei.xie@intel.com>
> Suggested-by: ms >> Michael S. Tsirkin <mst@redhat.com>

And applied to dpdk-next-virtio, with few tiny changes:

- we normally put suggested/reported-by above the SoB.

- removed "ms >>"

Thanks.

	--yliu

Re: [dpdk-dev] [PATCH] virtio: avoid avail ring entry index update if equal

[Thomas Monjalon]

2016-04-27 23:19, Yuanhan Liu:
> And applied to dpdk-next-virtio, with few tiny changes:
> 
> - we normally put suggested/reported-by above the SoB.

Yes we must keep the chronological order in these lines.

Re: [dpdk-dev] [PATCH] virtio: avoid avail ring entry index update if equal

[Xie, Huawei]

On 4/28/2016 4:14 PM, Thomas Monjalon wrote:
> 2016-04-27 23:19, Yuanhan Liu:
>> And applied to dpdk-next-virtio, with few tiny changes:
>>
>> - we normally put suggested/reported-by above the SoB.
> Yes we must keep the chronological order in these lines.
>

Thanks for reminder