[v3,1/2] virtio: one way barrier for packed vring desc avail flags
Checks
Commit Message
In case VIRTIO_F_ORDER_PLATFORM(36) is not negotiated, then the frontend
and backend are assumed to be implemented in software, that is they can
run on identical CPUs in an SMP configuration.
Thus a weak form of memory barriers like rte_smp_r/wmb, other than
rte_cio_r/wmb, is sufficient for this case(vq->hw->weak_barriers == 1)
and yields better performance.
For the above case, this patch helps yielding even better performance
by replacing the two-way barriers with C11 one-way barriers for avail
flags in packed ring.
Meanwhile, a read barrier is required to ensure ordering between
descriptor's flags and content reads[1]. With C11, load-acquire can
enforce the ordering instead of rmb barrier.
[1]https://patchwork.dpdk.org/patch/49109/
Signed-off-by: Joyce Kong <joyce.kong@arm.com>
Reviewed-by: Gavin Hu <gavin.hu@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
---
drivers/net/virtio/virtio_rxtx.c | 13 +++++++------
drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 +++++-
drivers/net/virtio/virtqueue.h | 11 +++++++++++
lib/librte_vhost/vhost.h | 2 +-
lib/librte_vhost/virtio_net.c | 11 +++++------
5 files changed, 29 insertions(+), 14 deletions(-)
Comments
On 9/9/19 11:14 AM, Joyce Kong wrote:
> In case VIRTIO_F_ORDER_PLATFORM(36) is not negotiated, then the frontend
> and backend are assumed to be implemented in software, that is they can
> run on identical CPUs in an SMP configuration.
> Thus a weak form of memory barriers like rte_smp_r/wmb, other than
> rte_cio_r/wmb, is sufficient for this case(vq->hw->weak_barriers == 1)
> and yields better performance.
> For the above case, this patch helps yielding even better performance
> by replacing the two-way barriers with C11 one-way barriers for avail
> flags in packed ring.
>
> Meanwhile, a read barrier is required to ensure ordering between
> descriptor's flags and content reads[1]. With C11, load-acquire can
> enforce the ordering instead of rmb barrier.
>
> [1]https://patchwork.dpdk.org/patch/49109/
>
> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> ---
> drivers/net/virtio/virtio_rxtx.c | 13 +++++++------
> drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 +++++-
> drivers/net/virtio/virtqueue.h | 11 +++++++++++
> lib/librte_vhost/vhost.h | 2 +-
> lib/librte_vhost/virtio_net.c | 11 +++++------
> 5 files changed, 29 insertions(+), 14 deletions(-)
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
Thanks,
Maxime
Hi Joyce,
I just test performance impact of your patch set with code base commit id: d03d8622db48918d14bfe805641b1766ecc40088, after applying your v3 patch set , seven paths of vhost/virtio pvp test shows performance drop as below:
PVP vhost/virtio 1c1q test before apply patch apply patch
test_perf_pvp_inorder_mergeable 7.603 7.474
test_perf_pvp_inorder_no_mergeable 7.642 7.525
test_perf_pvp_mergeable 7.556 7.431
test_perf_pvp_normal 7.554 7.478
test_perf_pvp_vector_rx 7.581 7.469
test_perf_pvp_virtio11_mergeable 7.068 6.905
test_perf_pvp_virtio11_normal 7.088 6.888
Thanks,
Yinan
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> Sent: 2019年9月9日 18:10
> To: Joyce Kong <joyce.kong@arm.com>; dev@dpdk.org
> Cc: nd@arm.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> jfreimann@redhat.com; honnappa.nagarahalli@arm.com; gavin.hu@arm.com
> Subject: Re: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed vring
> desc avail flags
>
>
>
> On 9/9/19 11:14 AM, Joyce Kong wrote:
> > In case VIRTIO_F_ORDER_PLATFORM(36) is not negotiated, then the
> > frontend and backend are assumed to be implemented in software, that
> > is they can run on identical CPUs in an SMP configuration.
> > Thus a weak form of memory barriers like rte_smp_r/wmb, other than
> > rte_cio_r/wmb, is sufficient for this case(vq->hw->weak_barriers == 1)
> > and yields better performance.
> > For the above case, this patch helps yielding even better performance
> > by replacing the two-way barriers with C11 one-way barriers for avail
> > flags in packed ring.
> >
> > Meanwhile, a read barrier is required to ensure ordering between
> > descriptor's flags and content reads[1]. With C11, load-acquire can
> > enforce the ordering instead of rmb barrier.
> >
> > [1]https://patchwork.dpdk.org/patch/49109/
> >
> > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > ---
> > drivers/net/virtio/virtio_rxtx.c | 13 +++++++------
> > drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 +++++-
> > drivers/net/virtio/virtqueue.h | 11 +++++++++++
> > lib/librte_vhost/vhost.h | 2 +-
> > lib/librte_vhost/virtio_net.c | 11 +++++------
> > 5 files changed, 29 insertions(+), 14 deletions(-)
>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>
> Thanks,
> Maxime
Hi Yinan,
We have done a comparative analysis and found with the old code the if(weak_barriers) and else branches were saved on x86 as rte_smp_wmb and rte_cio_wmb are identical.
http://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtqueue.h#n49
For the new code, with Joyce's patches applied, the branches were not saved, which requir additional cpu cycles, this caused slight degradation on x86.
The patches uplifted the performance on aarch64 about 9% as indicated in the cover letter. While I am thinking over a solution to the degradation on x86,could you help answer:
1. Is rte_cio_wmb is sufficient for the non weak-barrier case(HW offloading)?
I got this question because I see in Intel NIC PMDs, it is almost never used, it is rte_wmb that is more widely used to notify the NIC device, any difference between the virtio ring compatible smartNIC device(or vDPA?) and i40e like devices?
2. If the rte_cio_wmb is not sufficient for this case and replaced by stronger barriers, like sfence, then the branches will not be saved by the compiler, then the problem becomes with the correct use of barriers, other than the degradation.
Any comments are welcome!
Best Regards,
Gavin
> -----Original Message-----
> From: Wang, Yinan <yinan.wang@intel.com>
> Sent: Tuesday, September 10, 2019 11:54 AM
> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm
> Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> jfreimann@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> <Gavin.Hu@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed vring
> desc avail flags
>
>
> Hi Joyce,
>
> I just test performance impact of your patch set with code base commit id:
> d03d8622db48918d14bfe805641b1766ecc40088, after applying your v3 patch
> set , seven paths of vhost/virtio pvp test shows performance drop as below:
>
> PVP vhost/virtio 1c1q test before apply patch apply patch
> test_perf_pvp_inorder_mergeable 7.603 7.474
> test_perf_pvp_inorder_no_mergeable 7.642 7.525
> test_perf_pvp_mergeable 7.556 7.431
> test_perf_pvp_normal 7.554 7.478
> test_perf_pvp_vector_rx 7.581 7.469
> test_perf_pvp_virtio11_mergeable 7.068 6.905
> test_perf_pvp_virtio11_normal 7.088 6.888
>
> Thanks,
> Yinan
>
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> > Sent: 2019年9月9日 18:10
> > To: Joyce Kong <joyce.kong@arm.com>; dev@dpdk.org
> > Cc: nd@arm.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > jfreimann@redhat.com; honnappa.nagarahalli@arm.com;
> gavin.hu@arm.com
> > Subject: Re: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> vring
> > desc avail flags
> >
> >
> >
> > On 9/9/19 11:14 AM, Joyce Kong wrote:
> > > In case VIRTIO_F_ORDER_PLATFORM(36) is not negotiated, then the
> > > frontend and backend are assumed to be implemented in software, that
> > > is they can run on identical CPUs in an SMP configuration.
> > > Thus a weak form of memory barriers like rte_smp_r/wmb, other than
> > > rte_cio_r/wmb, is sufficient for this case(vq->hw->weak_barriers == 1)
> > > and yields better performance.
> > > For the above case, this patch helps yielding even better performance
> > > by replacing the two-way barriers with C11 one-way barriers for avail
> > > flags in packed ring.
> > >
> > > Meanwhile, a read barrier is required to ensure ordering between
> > > descriptor's flags and content reads[1]. With C11, load-acquire can
> > > enforce the ordering instead of rmb barrier.
> > >
> > > [1]https://patchwork.dpdk.org/patch/49109/
> > >
> > > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > > ---
> > > drivers/net/virtio/virtio_rxtx.c | 13 +++++++------
> > > drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 +++++-
> > > drivers/net/virtio/virtqueue.h | 11 +++++++++++
> > > lib/librte_vhost/vhost.h | 2 +-
> > > lib/librte_vhost/virtio_net.c | 11 +++++------
> > > 5 files changed, 29 insertions(+), 14 deletions(-)
> >
> > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> >
> > Thanks,
> > Maxime
Thanks Yinan for reporting the regresion and Gavin for the analysis.
On 9/10/19 11:48 AM, Gavin Hu (Arm Technology China) wrote:
> Hi Yinan,
>
> We have done a comparative analysis and found with the old code the if(weak_barriers) and else branches were saved on x86 as rte_smp_wmb and rte_cio_wmb are identical.
> http://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtqueue.h#n49
> For the new code, with Joyce's patches applied, the branches were not saved, which requir additional cpu cycles, this caused slight degradation on x86.
>
> The patches uplifted the performance on aarch64 about 9% as indicated in the cover letter. While I am thinking over a solution to the degradation on x86,could you help answer:
> 1. Is rte_cio_wmb is sufficient for the non weak-barrier case(HW offloading)?
> I got this question because I see in Intel NIC PMDs, it is almost never used, it is rte_wmb that is more widely used to notify the NIC device, any difference between the virtio ring compatible smartNIC device(or vDPA?) and i40e like devices?
> 2. If the rte_cio_wmb is not sufficient for this case and replaced by stronger barriers, like sfence, then the branches will not be saved by the compiler, then the problem becomes with the correct use of barriers, other than the degradation.
>
> Any comments are welcome!
It may we worth that Yinan tries with rte_wmb instead of rte_cio_wmb
without the series applied, just to confirm this is caused by the etra
branch.
Maxime
> Best Regards,
> Gavin
>
>> -----Original Message-----
>> From: Wang, Yinan <yinan.wang@intel.com>
>> Sent: Tuesday, September 10, 2019 11:54 AM
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm
>> Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
>> Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
>> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
>> <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
>> jfreimann@redhat.com; Honnappa Nagarahalli
>> <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
>> <Gavin.Hu@arm.com>
>> Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed vring
>> desc avail flags
>>
>>
>> Hi Joyce,
>>
>> I just test performance impact of your patch set with code base commit id:
>> d03d8622db48918d14bfe805641b1766ecc40088, after applying your v3 patch
>> set , seven paths of vhost/virtio pvp test shows performance drop as below:
>>
>> PVP vhost/virtio 1c1q test before apply patch apply patch
>> test_perf_pvp_inorder_mergeable 7.603 7.474
>> test_perf_pvp_inorder_no_mergeable 7.642 7.525
>> test_perf_pvp_mergeable 7.556 7.431
>> test_perf_pvp_normal 7.554 7.478
>> test_perf_pvp_vector_rx 7.581 7.469
>> test_perf_pvp_virtio11_mergeable 7.068 6.905
>> test_perf_pvp_virtio11_normal 7.088 6.888
>>
>> Thanks,
>> Yinan
>>
>>> -----Original Message-----
>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
>>> Sent: 2019年9月9日 18:10
>>> To: Joyce Kong <joyce.kong@arm.com>; dev@dpdk.org
>>> Cc: nd@arm.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
>>> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
>>> <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
>>> jfreimann@redhat.com; honnappa.nagarahalli@arm.com;
>> gavin.hu@arm.com
>>> Subject: Re: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
>> vring
>>> desc avail flags
>>>
>>>
>>>
>>> On 9/9/19 11:14 AM, Joyce Kong wrote:
>>>> In case VIRTIO_F_ORDER_PLATFORM(36) is not negotiated, then the
>>>> frontend and backend are assumed to be implemented in software, that
>>>> is they can run on identical CPUs in an SMP configuration.
>>>> Thus a weak form of memory barriers like rte_smp_r/wmb, other than
>>>> rte_cio_r/wmb, is sufficient for this case(vq->hw->weak_barriers == 1)
>>>> and yields better performance.
>>>> For the above case, this patch helps yielding even better performance
>>>> by replacing the two-way barriers with C11 one-way barriers for avail
>>>> flags in packed ring.
>>>>
>>>> Meanwhile, a read barrier is required to ensure ordering between
>>>> descriptor's flags and content reads[1]. With C11, load-acquire can
>>>> enforce the ordering instead of rmb barrier.
>>>>
>>>> [1]https://patchwork.dpdk.org/patch/49109/
>>>>
>>>> Signed-off-by: Joyce Kong <joyce.kong@arm.com>
>>>> Reviewed-by: Gavin Hu <gavin.hu@arm.com>
>>>> Reviewed-by: Phil Yang <phil.yang@arm.com>
>>>> ---
>>>> drivers/net/virtio/virtio_rxtx.c | 13 +++++++------
>>>> drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 +++++-
>>>> drivers/net/virtio/virtqueue.h | 11 +++++++++++
>>>> lib/librte_vhost/vhost.h | 2 +-
>>>> lib/librte_vhost/virtio_net.c | 11 +++++------
>>>> 5 files changed, 29 insertions(+), 14 deletions(-)
>>>
>>> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>
>>> Thanks,
>>> Maxime
> -----Original Message-----
> From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> Sent: Tuesday, September 10, 2019 5:49 PM
> To: Wang, Yinan <yinan.wang@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>; Joyce Kong (Arm Technology China)
> <Joyce.Kong@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> jfreimann@redhat.com; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
> Steve Capper <Steve.Capper@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> vring desc avail flags
>
> Hi Yinan,
>
> We have done a comparative analysis and found with the old code the
> if(weak_barriers) and else branches were saved on x86 as rte_smp_wmb and
> rte_cio_wmb are identical.
> http://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtqueue.h#n49
> For the new code, with Joyce's patches applied, the branches were not saved,
> which requir additional cpu cycles, this caused slight degradation on x86.
>
> The patches uplifted the performance on aarch64 about 9% as indicated in
> the cover letter. While I am thinking over a solution to the degradation on
> x86,could you help answer:
> 1. Is rte_cio_wmb is sufficient for the non weak-barrier case(HW
> offloading)?
> I got this question because I see in Intel NIC PMDs, it is almost never
> used, it is rte_wmb that is more widely used to notify the NIC device, any
> difference between the virtio ring compatible smartNIC device(or vDPA?) and
> i40e like devices?
Hi Gavin,
X86 architecture can guarantee that young store happen later than old store.
So rte_cio_wmb is just compiler memory barrier in x86.
I think compiler barrier is also enough in pmd, rte_wmb is in pmd because of it was inherit from first implementation :)
Thanks,
Marvin
> 2. If the rte_cio_wmb is not sufficient for this case and replaced by
> stronger barriers, like sfence, then the branches will not be saved by the
> compiler, then the problem becomes with the correct use of barriers, other
> than the degradation.
>
> Any comments are welcome!
>
> Best Regards,
> Gavin
>
> > -----Original Message-----
> > From: Wang, Yinan <yinan.wang@intel.com>
> > Sent: Tuesday, September 10, 2019 11:54 AM
> > To: Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm
> > Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > jfreimann@redhat.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> > <Gavin.Hu@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> vring
> > desc avail flags
> >
> >
> > Hi Joyce,
> >
> > I just test performance impact of your patch set with code base commit id:
> > d03d8622db48918d14bfe805641b1766ecc40088, after applying your v3 patch
> > set , seven paths of vhost/virtio pvp test shows performance drop as
> below:
> >
> > PVP vhost/virtio 1c1q test before apply patch apply patch
> > test_perf_pvp_inorder_mergeable 7.603 7.474
> > test_perf_pvp_inorder_no_mergeable 7.642 7.525
> > test_perf_pvp_mergeable 7.556 7.431
> > test_perf_pvp_normal 7.554 7.478
> > test_perf_pvp_vector_rx 7.581 7.469
> > test_perf_pvp_virtio11_mergeable 7.068 6.905
> > test_perf_pvp_virtio11_normal 7.088 6.888
> >
> > Thanks,
> > Yinan
> >
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime Coquelin
> > > Sent: 2019年9月9日 18:10
> > > To: Joyce Kong <joyce.kong@arm.com>; dev@dpdk.org
> > > Cc: nd@arm.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > > jfreimann@redhat.com; honnappa.nagarahalli@arm.com;
> > gavin.hu@arm.com
> > > Subject: Re: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for
> packed
> > vring
> > > desc avail flags
> > >
> > >
> > >
> > > On 9/9/19 11:14 AM, Joyce Kong wrote:
> > > > In case VIRTIO_F_ORDER_PLATFORM(36) is not negotiated, then the
> > > > frontend and backend are assumed to be implemented in software, that
> > > > is they can run on identical CPUs in an SMP configuration.
> > > > Thus a weak form of memory barriers like rte_smp_r/wmb, other than
> > > > rte_cio_r/wmb, is sufficient for this case(vq->hw->weak_barriers == 1)
> > > > and yields better performance.
> > > > For the above case, this patch helps yielding even better performance
> > > > by replacing the two-way barriers with C11 one-way barriers for avail
> > > > flags in packed ring.
> > > >
> > > > Meanwhile, a read barrier is required to ensure ordering between
> > > > descriptor's flags and content reads[1]. With C11, load-acquire can
> > > > enforce the ordering instead of rmb barrier.
> > > >
> > > > [1]https://patchwork.dpdk.org/patch/49109/
> > > >
> > > > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > > > ---
> > > > drivers/net/virtio/virtio_rxtx.c | 13 +++++++------
> > > > drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 +++++-
> > > > drivers/net/virtio/virtqueue.h | 11 +++++++++++
> > > > lib/librte_vhost/vhost.h | 2 +-
> > > > lib/librte_vhost/virtio_net.c | 11 +++++------
> > > > 5 files changed, 29 insertions(+), 14 deletions(-)
> > >
> > > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > >
> > > Thanks,
> > > Maxime
Hi Marvin,
Thanks for your answers, one more question for x86:
1. For CIO memory alone or MMIO memory(eg PCI BAR) alone, the compiler barrier is enough to keep ordering, that's why both rte_io_mb and rte_cio_mb are defined as compiler barriers, right?
2. How about the ordering of interleaved CIO and MMIO accesses, for example, a young store to MMIO can be reordered before an older store to CIO? CIO may be faster than devices, but store buffers or caching may cause the CIO update not visible to the device(in a common doorbell case)?
Best regards,
Gavin
> -----Original Message-----
> From: Liu, Yong <yong.liu@intel.com>
> Sent: Wednesday, September 11, 2019 10:39 AM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Wang, Yinan
> <yinan.wang@intel.com>; Maxime Coquelin <maxime.coquelin@redhat.com>;
> Joyce Kong (Arm Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Steve Capper <Steve.Capper@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed vring
> desc avail flags
>
>
>
> > -----Original Message-----
> > From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> > Sent: Tuesday, September 10, 2019 5:49 PM
> > To: Wang, Yinan <yinan.wang@intel.com>; Maxime Coquelin
> > <maxime.coquelin@redhat.com>; Joyce Kong (Arm Technology China)
> > <Joyce.Kong@arm.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > jfreimann@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>;
> > Steve Capper <Steve.Capper@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> > vring desc avail flags
> >
> > Hi Yinan,
> >
> > We have done a comparative analysis and found with the old code the
> > if(weak_barriers) and else branches were saved on x86 as rte_smp_wmb
> and
> > rte_cio_wmb are identical.
> > http://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtqueue.h#n49
> > For the new code, with Joyce's patches applied, the branches were not
> saved,
> > which requir additional cpu cycles, this caused slight degradation on x86.
> >
> > The patches uplifted the performance on aarch64 about 9% as indicated in
> > the cover letter. While I am thinking over a solution to the degradation on
> > x86,could you help answer:
> > 1. Is rte_cio_wmb is sufficient for the non weak-barrier case(HW
> > offloading)?
> > I got this question because I see in Intel NIC PMDs, it is almost never
> > used, it is rte_wmb that is more widely used to notify the NIC device, any
> > difference between the virtio ring compatible smartNIC device(or vDPA?)
> and
> > i40e like devices?
>
> Hi Gavin,
> X86 architecture can guarantee that young store happen later than old store.
> So rte_cio_wmb is just compiler memory barrier in x86.
>
> I think compiler barrier is also enough in pmd, rte_wmb is in pmd because of
> it was inherit from first implementation :)
>
> Thanks,
> Marvin
>
> > 2. If the rte_cio_wmb is not sufficient for this case and replaced by
> > stronger barriers, like sfence, then the branches will not be saved by the
> > compiler, then the problem becomes with the correct use of barriers, other
> > than the degradation.
> >
> > Any comments are welcome!
> >
> > Best Regards,
> > Gavin
> >
> > > -----Original Message-----
> > > From: Wang, Yinan <yinan.wang@intel.com>
> > > Sent: Tuesday, September 10, 2019 11:54 AM
> > > To: Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm
> > > Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
> > > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > > jfreimann@redhat.com; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> > > <Gavin.Hu@arm.com>
> > > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> > vring
> > > desc avail flags
> > >
> > >
> > > Hi Joyce,
> > >
> > > I just test performance impact of your patch set with code base commit id:
> > > d03d8622db48918d14bfe805641b1766ecc40088, after applying your v3
> patch
> > > set , seven paths of vhost/virtio pvp test shows performance drop as
> > below:
> > >
> > > PVP vhost/virtio 1c1q test before apply patch apply patch
> > > test_perf_pvp_inorder_mergeable 7.603 7.474
> > > test_perf_pvp_inorder_no_mergeable 7.642 7.525
> > > test_perf_pvp_mergeable 7.556 7.431
> > > test_perf_pvp_normal 7.554 7.478
> > > test_perf_pvp_vector_rx 7.581 7.469
> > > test_perf_pvp_virtio11_mergeable 7.068 6.905
> > > test_perf_pvp_virtio11_normal 7.088 6.888
> > >
> > > Thanks,
> > > Yinan
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime
> Coquelin
> > > > Sent: 2019年9月9日 18:10
> > > > To: Joyce Kong <joyce.kong@arm.com>; dev@dpdk.org
> > > > Cc: nd@arm.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > > > jfreimann@redhat.com; honnappa.nagarahalli@arm.com;
> > > gavin.hu@arm.com
> > > > Subject: Re: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for
> > packed
> > > vring
> > > > desc avail flags
> > > >
> > > >
> > > >
> > > > On 9/9/19 11:14 AM, Joyce Kong wrote:
> > > > > In case VIRTIO_F_ORDER_PLATFORM(36) is not negotiated, then the
> > > > > frontend and backend are assumed to be implemented in software,
> that
> > > > > is they can run on identical CPUs in an SMP configuration.
> > > > > Thus a weak form of memory barriers like rte_smp_r/wmb, other than
> > > > > rte_cio_r/wmb, is sufficient for this case(vq->hw->weak_barriers == 1)
> > > > > and yields better performance.
> > > > > For the above case, this patch helps yielding even better performance
> > > > > by replacing the two-way barriers with C11 one-way barriers for avail
> > > > > flags in packed ring.
> > > > >
> > > > > Meanwhile, a read barrier is required to ensure ordering between
> > > > > descriptor's flags and content reads[1]. With C11, load-acquire can
> > > > > enforce the ordering instead of rmb barrier.
> > > > >
> > > > > [1]https://patchwork.dpdk.org/patch/49109/
> > > > >
> > > > > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > > > > ---
> > > > > drivers/net/virtio/virtio_rxtx.c | 13 +++++++------
> > > > > drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 +++++-
> > > > > drivers/net/virtio/virtqueue.h | 11 +++++++++++
> > > > > lib/librte_vhost/vhost.h | 2 +-
> > > > > lib/librte_vhost/virtio_net.c | 11 +++++------
> > > > > 5 files changed, 29 insertions(+), 14 deletions(-)
> > > >
> > > > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > >
> > > > Thanks,
> > > > Maxime
Thanks Gavin, my answers are inline.
> -----Original Message-----
> From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> Sent: Wednesday, September 11, 2019 11:35 AM
> To: Liu, Yong <yong.liu@intel.com>; Wang, Yinan <yinan.wang@intel.com>;
> Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm Technology
> China) <Joyce.Kong@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Steve Capper <Steve.Capper@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> vring desc avail flags
>
> Hi Marvin,
>
> Thanks for your answers, one more question for x86:
> 1. For CIO memory alone or MMIO memory(eg PCI BAR) alone, the compiler
> barrier is enough to keep ordering, that's why both rte_io_mb and
> rte_cio_mb are defined as compiler barriers, right?
Yes, that's right for x86.
> 2. How about the ordering of interleaved CIO and MMIO accesses, for example,
> a young store to MMIO can be reordered before an older store to CIO? CIO
> may be faster than devices, but store buffers or caching may cause the CIO
> update not visible to the device(in a common doorbell case)?
>
There's always one kind of cache coherent engine in x86 uncore sub-system.
When CIO write instruction was retried, data will be in CPU LLC.
When device doing inbound read, request will go to cache engine first and then check memory state and retrieve latest value.
> Best regards,
> Gavin
>
> > -----Original Message-----
> > From: Liu, Yong <yong.liu@intel.com>
> > Sent: Wednesday, September 11, 2019 10:39 AM
> > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Wang, Yinan
> > <yinan.wang@intel.com>; Maxime Coquelin <maxime.coquelin@redhat.com>;
> > Joyce Kong (Arm Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; Steve Capper <Steve.Capper@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> vring
> > desc avail flags
> >
> >
> >
> > > -----Original Message-----
> > > From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> > > Sent: Tuesday, September 10, 2019 5:49 PM
> > > To: Wang, Yinan <yinan.wang@intel.com>; Maxime Coquelin
> > > <maxime.coquelin@redhat.com>; Joyce Kong (Arm Technology China)
> > > <Joyce.Kong@arm.com>; dev@dpdk.org
> > > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > > jfreimann@redhat.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>;
> > > Steve Capper <Steve.Capper@arm.com>
> > > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for
> packed
> > > vring desc avail flags
> > >
> > > Hi Yinan,
> > >
> > > We have done a comparative analysis and found with the old code the
> > > if(weak_barriers) and else branches were saved on x86 as rte_smp_wmb
> > and
> > > rte_cio_wmb are identical.
> > > http://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtqueue.h#n49
> > > For the new code, with Joyce's patches applied, the branches were not
> > saved,
> > > which requir additional cpu cycles, this caused slight degradation on
> x86.
> > >
> > > The patches uplifted the performance on aarch64 about 9% as indicated
> in
> > > the cover letter. While I am thinking over a solution to the
> degradation on
> > > x86,could you help answer:
> > > 1. Is rte_cio_wmb is sufficient for the non weak-barrier case(HW
> > > offloading)?
> > > I got this question because I see in Intel NIC PMDs, it is almost
> never
> > > used, it is rte_wmb that is more widely used to notify the NIC device,
> any
> > > difference between the virtio ring compatible smartNIC device(or vDPA?)
> > and
> > > i40e like devices?
> >
> > Hi Gavin,
> > X86 architecture can guarantee that young store happen later than old
> store.
> > So rte_cio_wmb is just compiler memory barrier in x86.
> >
> > I think compiler barrier is also enough in pmd, rte_wmb is in pmd because
> of
> > it was inherit from first implementation :)
> >
> > Thanks,
> > Marvin
> >
> > > 2. If the rte_cio_wmb is not sufficient for this case and replaced by
> > > stronger barriers, like sfence, then the branches will not be saved by
> the
> > > compiler, then the problem becomes with the correct use of barriers,
> other
> > > than the degradation.
> > >
> > > Any comments are welcome!
> > >
> > > Best Regards,
> > > Gavin
> > >
> > > > -----Original Message-----
> > > > From: Wang, Yinan <yinan.wang@intel.com>
> > > > Sent: Tuesday, September 10, 2019 11:54 AM
> > > > To: Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm
> > > > Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
> > > > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > > > jfreimann@redhat.com; Honnappa Nagarahalli
> > > > <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> > > > <Gavin.Hu@arm.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for
> packed
> > > vring
> > > > desc avail flags
> > > >
> > > >
> > > > Hi Joyce,
> > > >
> > > > I just test performance impact of your patch set with code base
> commit id:
> > > > d03d8622db48918d14bfe805641b1766ecc40088, after applying your v3
> > patch
> > > > set , seven paths of vhost/virtio pvp test shows performance drop as
> > > below:
> > > >
> > > > PVP vhost/virtio 1c1q test before apply patch apply
> patch
> > > > test_perf_pvp_inorder_mergeable 7.603 7.474
> > > > test_perf_pvp_inorder_no_mergeable 7.642 7.525
> > > > test_perf_pvp_mergeable 7.556 7.431
> > > > test_perf_pvp_normal 7.554 7.478
> > > > test_perf_pvp_vector_rx 7.581 7.469
> > > > test_perf_pvp_virtio11_mergeable 7.068
> 6.905
> > > > test_perf_pvp_virtio11_normal 7.088 6.888
> > > >
> > > > Thanks,
> > > > Yinan
> > > >
> > > > > -----Original Message-----
> > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime
> > Coquelin
> > > > > Sent: 2019年9月9日 18:10
> > > > > To: Joyce Kong <joyce.kong@arm.com>; dev@dpdk.org
> > > > > Cc: nd@arm.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > > > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > > > > jfreimann@redhat.com; honnappa.nagarahalli@arm.com;
> > > > gavin.hu@arm.com
> > > > > Subject: Re: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for
> > > packed
> > > > vring
> > > > > desc avail flags
> > > > >
> > > > >
> > > > >
> > > > > On 9/9/19 11:14 AM, Joyce Kong wrote:
> > > > > > In case VIRTIO_F_ORDER_PLATFORM(36) is not negotiated, then the
> > > > > > frontend and backend are assumed to be implemented in software,
> > that
> > > > > > is they can run on identical CPUs in an SMP configuration.
> > > > > > Thus a weak form of memory barriers like rte_smp_r/wmb, other
> than
> > > > > > rte_cio_r/wmb, is sufficient for this case(vq->hw->weak_barriers
> == 1)
> > > > > > and yields better performance.
> > > > > > For the above case, this patch helps yielding even better
> performance
> > > > > > by replacing the two-way barriers with C11 one-way barriers for
> avail
> > > > > > flags in packed ring.
> > > > > >
> > > > > > Meanwhile, a read barrier is required to ensure ordering between
> > > > > > descriptor's flags and content reads[1]. With C11, load-acquire
> can
> > > > > > enforce the ordering instead of rmb barrier.
> > > > > >
> > > > > > [1]https://patchwork.dpdk.org/patch/49109/
> > > > > >
> > > > > > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > > > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > > > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > > > > > ---
> > > > > > drivers/net/virtio/virtio_rxtx.c | 13 +++++++---
> ---
> > > > > > drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 +++++-
> > > > > > drivers/net/virtio/virtqueue.h | 11
> +++++++++++
> > > > > > lib/librte_vhost/vhost.h | 2 +-
> > > > > > lib/librte_vhost/virtio_net.c | 11 +++++-----
> -
> > > > > > 5 files changed, 29 insertions(+), 14 deletions(-)
> > > > >
> > > > > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > > >
> > > > > Thanks,
> > > > > Maxime
Thanks Marvin, my inline comments.
> -----Original Message-----
> From: Liu, Yong <yong.liu@intel.com>
> Sent: Wednesday, September 11, 2019 2:30 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Wang, Yinan
> <yinan.wang@intel.com>; Maxime Coquelin <maxime.coquelin@redhat.com>;
> Joyce Kong (Arm Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
> Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Steve Capper <Steve.Capper@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed vring
> desc avail flags
>
> Thanks Gavin, my answers are inline.
>
> > -----Original Message-----
> > From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> > Sent: Wednesday, September 11, 2019 11:35 AM
> > To: Liu, Yong <yong.liu@intel.com>; Wang, Yinan <yinan.wang@intel.com>;
> > Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm
> Technology
> > China) <Joyce.Kong@arm.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; Steve Capper <Steve.Capper@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> > vring desc avail flags
> >
> > Hi Marvin,
> >
> > Thanks for your answers, one more question for x86:
> > 1. For CIO memory alone or MMIO memory(eg PCI BAR) alone, the compiler
> > barrier is enough to keep ordering, that's why both rte_io_mb and
> > rte_cio_mb are defined as compiler barriers, right?
>
> Yes, that's right for x86.
>
> > 2. How about the ordering of interleaved CIO and MMIO accesses, for
> example,
> > a young store to MMIO can be reordered before an older store to CIO? CIO
> > may be faster than devices, but store buffers or caching may cause the CIO
> > update not visible to the device(in a common doorbell case)?
> >
>
> There's always one kind of cache coherent engine in x86 uncore sub-system.
> When CIO write instruction was retried, data will be in CPU LLC.
> When device doing inbound read, request will go to cache engine first and
> then check memory state and retrieve latest value.
I understand your words that the cache coherent engine is working like a hub/coordinator/arbiter for all the accesses to three types of memory: 1 - normal memory, 2 - CIO memory, 3 - MMIO memory, and the ordering behaviors are no different?
Then in what scenarios mfence/sfence/lfence should be used? Maybe just mfence is enough to keep orderings of store/load(which is the only one might reordered on x86)?
>
> > Best regards,
> > Gavin
> >
> > > -----Original Message-----
> > > From: Liu, Yong <yong.liu@intel.com>
> > > Sent: Wednesday, September 11, 2019 10:39 AM
> > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Wang, Yinan
> > > <yinan.wang@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>;
> > > Joyce Kong (Arm Technology China) <Joyce.Kong@arm.com>;
> dev@dpdk.org
> > > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>
> > > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> > vring
> > > desc avail flags
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> > > > Sent: Tuesday, September 10, 2019 5:49 PM
> > > > To: Wang, Yinan <yinan.wang@intel.com>; Maxime Coquelin
> > > > <maxime.coquelin@redhat.com>; Joyce Kong (Arm Technology China)
> > > > <Joyce.Kong@arm.com>; dev@dpdk.org
> > > > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> Zhihong
> > > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > > > jfreimann@redhat.com; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>;
> > > > Steve Capper <Steve.Capper@arm.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for
> > packed
> > > > vring desc avail flags
> > > >
> > > > Hi Yinan,
> > > >
> > > > We have done a comparative analysis and found with the old code the
> > > > if(weak_barriers) and else branches were saved on x86 as rte_smp_wmb
> > > and
> > > > rte_cio_wmb are identical.
> > > > http://git.dpdk.org/dpdk/tree/drivers/net/virtio/virtqueue.h#n49
> > > > For the new code, with Joyce's patches applied, the branches were not
> > > saved,
> > > > which requir additional cpu cycles, this caused slight degradation on
> > x86.
> > > >
> > > > The patches uplifted the performance on aarch64 about 9% as indicated
> > in
> > > > the cover letter. While I am thinking over a solution to the
> > degradation on
> > > > x86,could you help answer:
> > > > 1. Is rte_cio_wmb is sufficient for the non weak-barrier case(HW
> > > > offloading)?
> > > > I got this question because I see in Intel NIC PMDs, it is almost
> > never
> > > > used, it is rte_wmb that is more widely used to notify the NIC device,
> > any
> > > > difference between the virtio ring compatible smartNIC device(or vDPA?)
> > > and
> > > > i40e like devices?
> > >
> > > Hi Gavin,
> > > X86 architecture can guarantee that young store happen later than old
> > store.
> > > So rte_cio_wmb is just compiler memory barrier in x86.
> > >
> > > I think compiler barrier is also enough in pmd, rte_wmb is in pmd because
> > of
> > > it was inherit from first implementation :)
> > >
> > > Thanks,
> > > Marvin
> > >
> > > > 2. If the rte_cio_wmb is not sufficient for this case and replaced by
> > > > stronger barriers, like sfence, then the branches will not be saved by
> > the
> > > > compiler, then the problem becomes with the correct use of barriers,
> > other
> > > > than the degradation.
> > > >
> > > > Any comments are welcome!
> > > >
> > > > Best Regards,
> > > > Gavin
> > > >
> > > > > -----Original Message-----
> > > > > From: Wang, Yinan <yinan.wang@intel.com>
> > > > > Sent: Tuesday, September 10, 2019 11:54 AM
> > > > > To: Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong
> (Arm
> > > > > Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
> > > > > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> Zhihong
> > > > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > > > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > > > > jfreimann@redhat.com; Honnappa Nagarahalli
> > > > > <Honnappa.Nagarahalli@arm.com>; Gavin Hu (Arm Technology China)
> > > > > <Gavin.Hu@arm.com>
> > > > > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for
> > packed
> > > > vring
> > > > > desc avail flags
> > > > >
> > > > >
> > > > > Hi Joyce,
> > > > >
> > > > > I just test performance impact of your patch set with code base
> > commit id:
> > > > > d03d8622db48918d14bfe805641b1766ecc40088, after applying your
> v3
> > > patch
> > > > > set , seven paths of vhost/virtio pvp test shows performance drop as
> > > > below:
> > > > >
> > > > > PVP vhost/virtio 1c1q test before apply patch apply
> > patch
> > > > > test_perf_pvp_inorder_mergeable 7.603 7.474
> > > > > test_perf_pvp_inorder_no_mergeable 7.642 7.525
> > > > > test_perf_pvp_mergeable 7.556 7.431
> > > > > test_perf_pvp_normal 7.554 7.478
> > > > > test_perf_pvp_vector_rx 7.581 7.469
> > > > > test_perf_pvp_virtio11_mergeable 7.068
> > 6.905
> > > > > test_perf_pvp_virtio11_normal 7.088 6.888
> > > > >
> > > > > Thanks,
> > > > > Yinan
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Maxime
> > > Coquelin
> > > > > > Sent: 2019年9月9日 18:10
> > > > > > To: Joyce Kong <joyce.kong@arm.com>; dev@dpdk.org
> > > > > > Cc: nd@arm.com; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > > > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > > > > <xiao.w.wang@intel.com>; Liu, Yong <yong.liu@intel.com>;
> > > > > > jfreimann@redhat.com; honnappa.nagarahalli@arm.com;
> > > > > gavin.hu@arm.com
> > > > > > Subject: Re: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for
> > > > packed
> > > > > vring
> > > > > > desc avail flags
> > > > > >
> > > > > >
> > > > > >
> > > > > > On 9/9/19 11:14 AM, Joyce Kong wrote:
> > > > > > > In case VIRTIO_F_ORDER_PLATFORM(36) is not negotiated, then
> the
> > > > > > > frontend and backend are assumed to be implemented in software,
> > > that
> > > > > > > is they can run on identical CPUs in an SMP configuration.
> > > > > > > Thus a weak form of memory barriers like rte_smp_r/wmb, other
> > than
> > > > > > > rte_cio_r/wmb, is sufficient for this case(vq->hw->weak_barriers
> > == 1)
> > > > > > > and yields better performance.
> > > > > > > For the above case, this patch helps yielding even better
> > performance
> > > > > > > by replacing the two-way barriers with C11 one-way barriers for
> > avail
> > > > > > > flags in packed ring.
> > > > > > >
> > > > > > > Meanwhile, a read barrier is required to ensure ordering between
> > > > > > > descriptor's flags and content reads[1]. With C11, load-acquire
> > can
> > > > > > > enforce the ordering instead of rmb barrier.
> > > > > > >
> > > > > > > [1]https://patchwork.dpdk.org/patch/49109/
> > > > > > >
> > > > > > > Signed-off-by: Joyce Kong <joyce.kong@arm.com>
> > > > > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > > > > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > > > > > > ---
> > > > > > > drivers/net/virtio/virtio_rxtx.c | 13 +++++++---
> > ---
> > > > > > > drivers/net/virtio/virtio_user/virtio_user_dev.c | 6 +++++-
> > > > > > > drivers/net/virtio/virtqueue.h | 11
> > +++++++++++
> > > > > > > lib/librte_vhost/vhost.h | 2 +-
> > > > > > > lib/librte_vhost/virtio_net.c | 11 +++++-----
> > -
> > > > > > > 5 files changed, 29 insertions(+), 14 deletions(-)
> > > > > >
> > > > > > Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> > > > > >
> > > > > > Thanks,
> > > > > > Maxime
On Wed, Sep 11, 2019 at 08:32:16AM +0000, Gavin Hu (Arm Technology China) wrote:
> Thanks Marvin, my inline comments.
>
> > -----Original Message-----
> > From: Liu, Yong <yong.liu@intel.com>
> > Sent: Wednesday, September 11, 2019 2:30 PM
> > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Wang, Yinan
> > <yinan.wang@intel.com>; Maxime Coquelin <maxime.coquelin@redhat.com>;
> > Joyce Kong (Arm Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org
> > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> > <Honnappa.Nagarahalli@arm.com>; Steve Capper <Steve.Capper@arm.com>
> > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed vring
> > desc avail flags
> >
> > Thanks Gavin, my answers are inline.
> >
> > > -----Original Message-----
> > > From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> > > Sent: Wednesday, September 11, 2019 11:35 AM
> > > To: Liu, Yong <yong.liu@intel.com>; Wang, Yinan <yinan.wang@intel.com>;
> > > Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm
> > Technology
> > > China) <Joyce.Kong@arm.com>; dev@dpdk.org
> > > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; Steve Capper <Steve.Capper@arm.com>
> > > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> > > vring desc avail flags
> > >
> > > Hi Marvin,
> > >
> > > Thanks for your answers, one more question for x86:
> > > 1. For CIO memory alone or MMIO memory(eg PCI BAR) alone, the compiler
> > > barrier is enough to keep ordering, that's why both rte_io_mb and
> > > rte_cio_mb are defined as compiler barriers, right?
> >
> > Yes, that's right for x86.
> >
> > > 2. How about the ordering of interleaved CIO and MMIO accesses, for
> > example,
> > > a young store to MMIO can be reordered before an older store to CIO? CIO
> > > may be faster than devices, but store buffers or caching may cause the CIO
> > > update not visible to the device(in a common doorbell case)?
> > >
> >
> > There's always one kind of cache coherent engine in x86 uncore sub-system.
> > When CIO write instruction was retried, data will be in CPU LLC.
> > When device doing inbound read, request will go to cache engine first and
> > then check memory state and retrieve latest value.
> I understand your words that the cache coherent engine is working like a hub/coordinator/arbiter for all the accesses to three types of memory: 1 - normal memory, 2 - CIO memory, 3 - MMIO memory, and the ordering behaviors are no different?
> Then in what scenarios mfence/sfence/lfence should be used? Maybe just mfence is enough to keep orderings of store/load(which is the only one might reordered on x86)?
> >
The fence types needed will depend on the memory types used, for example,
any memory mapped as write-combining will have different behaviour and
need different fences to the regular write-back memory we are most familiar
with. For the situations we deal with in DPDK, for regular memory writes
and MMIO writes, reads won't be reordered with other reads, and writes
won't be reordered with other writes, so therefore, as you point out, the
mfence instruction is only rarely needed, and barriers to prevent compiler
reordering are sufficient in nearly all cases.
/Bruce
Hi Bruce,
> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Wednesday, September 11, 2019 6:03 PM
> To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> Cc: Liu, Yong <yong.liu@intel.com>; Wang, Yinan <yinan.wang@intel.com>;
> Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm
> Technology China) <Joyce.Kong@arm.com>; dev@dpdk.org; nd
> <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> <Honnappa.Nagarahalli@arm.com>; Steve Capper <Steve.Capper@arm.com>
> Subject: Re: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed vring
> desc avail flags
>
> On Wed, Sep 11, 2019 at 08:32:16AM +0000, Gavin Hu (Arm Technology China)
> wrote:
> > Thanks Marvin, my inline comments.
> >
> > > -----Original Message-----
> > > From: Liu, Yong <yong.liu@intel.com>
> > > Sent: Wednesday, September 11, 2019 2:30 PM
> > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>; Wang, Yinan
> > > <yinan.wang@intel.com>; Maxime Coquelin
> <maxime.coquelin@redhat.com>;
> > > Joyce Kong (Arm Technology China) <Joyce.Kong@arm.com>;
> dev@dpdk.org
> > > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang, Zhihong
> > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa Nagarahalli
> > > <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>
> > > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> vring
> > > desc avail flags
> > >
> > > Thanks Gavin, my answers are inline.
> > >
> > > > -----Original Message-----
> > > > From: Gavin Hu (Arm Technology China) [mailto:Gavin.Hu@arm.com]
> > > > Sent: Wednesday, September 11, 2019 11:35 AM
> > > > To: Liu, Yong <yong.liu@intel.com>; Wang, Yinan
> <yinan.wang@intel.com>;
> > > > Maxime Coquelin <maxime.coquelin@redhat.com>; Joyce Kong (Arm
> > > Technology
> > > > China) <Joyce.Kong@arm.com>; dev@dpdk.org
> > > > Cc: nd <nd@arm.com>; Bie, Tiwei <tiwei.bie@intel.com>; Wang,
> Zhihong
> > > > <zhihong.wang@intel.com>; amorenoz@redhat.com; Wang, Xiao W
> > > > <xiao.w.wang@intel.com>; jfreimann@redhat.com; Honnappa
> Nagarahalli
> > > > <Honnappa.Nagarahalli@arm.com>; Steve Capper
> <Steve.Capper@arm.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v3 1/2] virtio: one way barrier for packed
> > > > vring desc avail flags
> > > >
> > > > Hi Marvin,
> > > >
> > > > Thanks for your answers, one more question for x86:
> > > > 1. For CIO memory alone or MMIO memory(eg PCI BAR) alone, the
> compiler
> > > > barrier is enough to keep ordering, that's why both rte_io_mb and
> > > > rte_cio_mb are defined as compiler barriers, right?
> > >
> > > Yes, that's right for x86.
> > >
> > > > 2. How about the ordering of interleaved CIO and MMIO accesses, for
> > > example,
> > > > a young store to MMIO can be reordered before an older store to CIO?
> CIO
> > > > may be faster than devices, but store buffers or caching may cause the
> CIO
> > > > update not visible to the device(in a common doorbell case)?
> > > >
> > >
> > > There's always one kind of cache coherent engine in x86 uncore sub-
> system.
> > > When CIO write instruction was retried, data will be in CPU LLC.
> > > When device doing inbound read, request will go to cache engine first and
> > > then check memory state and retrieve latest value.
> > I understand your words that the cache coherent engine is working like a
> hub/coordinator/arbiter for all the accesses to three types of memory: 1 -
> normal memory, 2 - CIO memory, 3 - MMIO memory, and the ordering
> behaviors are no different?
> > Then in what scenarios mfence/sfence/lfence should be used? Maybe just
> mfence is enough to keep orderings of store/load(which is the only one might
> reordered on x86)?
> > >
>
> The fence types needed will depend on the memory types used, for example,
> any memory mapped as write-combining will have different behaviour and
> need different fences to the regular write-back memory we are most familiar
> with. For the situations we deal with in DPDK, for regular memory writes
> and MMIO writes, reads won't be reordered with other reads, and writes
> won't be reordered with other writes, so therefore, as you point out, the
> mfence instruction is only rarely needed, and barriers to prevent compiler
> reordering are sufficient in nearly all cases.
Thanks for your explanation about the barriers on x86, it is really helpful for us
to optimize PMDs for aarch64 by using less restrictive barriers while not breaking x86 platforms.
>
> /Bruce
@@ -456,8 +456,10 @@ virtqueue_enqueue_recv_refill_packed(struct virtqueue *vq,
vq->vq_desc_head_idx = dxp->next;
if (vq->vq_desc_head_idx == VQ_RING_DESC_CHAIN_END)
vq->vq_desc_tail_idx = vq->vq_desc_head_idx;
- virtio_wmb(hw->weak_barriers);
- start_dp[idx].flags = flags;
+
+ virtqueue_store_flags_packed(&start_dp[idx], flags,
+ hw->weak_barriers);
+
if (++vq->vq_avail_idx >= vq->vq_nentries) {
vq->vq_avail_idx -= vq->vq_nentries;
vq->vq_packed.cached_flags ^=
@@ -671,8 +673,7 @@ virtqueue_enqueue_xmit_packed_fast(struct virtnet_tx *txvq,
vq->vq_desc_tail_idx = VQ_RING_DESC_CHAIN_END;
}
- virtio_wmb(vq->hw->weak_barriers);
- dp->flags = flags;
+ virtqueue_store_flags_packed(dp, flags, vq->hw->weak_barriers);
}
static inline void
@@ -763,8 +764,8 @@ virtqueue_enqueue_xmit_packed(struct virtnet_tx *txvq, struct rte_mbuf *cookie,
vq->vq_desc_tail_idx = VQ_RING_DESC_CHAIN_END;
}
- virtio_wmb(vq->hw->weak_barriers);
- head_dp->flags = head_flags;
+ virtqueue_store_flags_packed(head_dp, head_flags,
+ vq->hw->weak_barriers);
}
static inline void
@@ -624,7 +624,7 @@ virtio_user_handle_ctrl_msg(struct virtio_user_dev *dev, struct vring *vring,
static inline int
desc_is_avail(struct vring_packed_desc *desc, bool wrap_counter)
{
- uint16_t flags = desc->flags;
+ uint16_t flags = __atomic_load_n(&desc->flags, __ATOMIC_ACQUIRE);
return wrap_counter == !!(flags & VRING_PACKED_DESC_F_AVAIL) &&
wrap_counter != !!(flags & VRING_PACKED_DESC_F_USED);
@@ -684,6 +684,10 @@ virtio_user_handle_cq_packed(struct virtio_user_dev *dev, uint16_t queue_idx)
struct vring_packed *vring = &dev->packed_vrings[queue_idx];
uint16_t n_descs, flags;
+ /* Perform a load-acquire barrier in desc_is_avail to
+ * enforce the ordering between desc flags and desc
+ * content.
+ */
while (desc_is_avail(&vring->desc[vq->used_idx],
vq->used_wrap_counter)) {
@@ -54,6 +54,17 @@ virtio_wmb(uint8_t weak_barriers)
rte_cio_wmb();
}
+static inline void
+virtqueue_store_flags_packed(struct vring_packed_desc *dp,
+ uint16_t flags, uint8_t weak_barriers)
+{
+ if (weak_barriers) {
+ __atomic_store_n(&dp->flags, flags, __ATOMIC_RELEASE);
+ } else {
+ rte_cio_wmb();
+ dp->flags = flags;
+ }
+}
#ifdef RTE_PMD_PACKET_PREFETCH
#define rte_packet_prefetch(p) rte_prefetch1(p)
#else
@@ -344,7 +344,7 @@ vq_is_packed(struct virtio_net *dev)
static inline bool
desc_is_avail(struct vring_packed_desc *desc, bool wrap_counter)
{
- uint16_t flags = *((volatile uint16_t *) &desc->flags);
+ uint16_t flags = __atomic_load_n(&desc->flags, __ATOMIC_ACQUIRE);
return wrap_counter == !!(flags & VRING_DESC_F_AVAIL) &&
wrap_counter != !!(flags & VRING_DESC_F_USED);
@@ -503,14 +503,13 @@ fill_vec_buf_packed(struct virtio_net *dev, struct vhost_virtqueue *vq,
if (avail_idx < vq->last_avail_idx)
wrap_counter ^= 1;
- if (unlikely(!desc_is_avail(&descs[avail_idx], wrap_counter)))
- return -1;
-
/*
- * The ordering between desc flags and desc
- * content reads need to be enforced.
+ * Perform a load-acquire barrier in desc_is_avail to
+ * enforce the ordering between desc flags and desc
+ * content.
*/
- rte_smp_rmb();
+ if (unlikely(!desc_is_avail(&descs[avail_idx], wrap_counter)))
+ return -1;
*desc_count = 0;
*len = 0;