diff mbox series

[v3,2/3] net/virtio: update memory ordering comment for vq notify

Message ID 20190109145015.3010-3-i.maximets@samsung.com (mailing list archive)
State Accepted, archived
Delegated to: Maxime Coquelin
Headers
Series Missing barriers and VIRTIO_F_ORDER_PLATFORM. |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Ilya Maximets Jan. 9, 2019, 2:50 p.m. UTC 
  We're not using io ports in case of modern device even on IA.
Also, this comment useless for other architectures.

Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
---
 drivers/net/virtio/virtqueue.h | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Comments

Gavin Hu Jan. 10, 2019, 8:19 a.m. UTC | #1 
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Ilya Maximets
> Sent: Wednesday, January 9, 2019 10:50 PM
> To: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
> Michael S . Tsirkin <mst@redhat.com>; Xiao Wang
> <xiao.w.wang@intel.com>
> Cc: Tiwei Bie <tiwei.bie@intel.com>; Zhihong Wang
> <zhihong.wang@intel.com>; jfreimann@redhat.com; Jason Wang
> <jasowang@redhat.com>; xiaolong.ye@intel.com;
> alejandro.lucero@netronome.com; Ilya Maximets
> <i.maximets@samsung.com>
> Subject: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering
> comment for vq notify
> 
> We're not using io ports in case of modern device even on IA.
> Also, this comment useless for other architectures.

Agree, it should be architecture neutral. 

> 
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>  drivers/net/virtio/virtqueue.h | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> index dffa03669..53aeac238 100644
> --- a/drivers/net/virtio/virtqueue.h
> +++ b/drivers/net/virtio/virtqueue.h
> @@ -437,14 +437,13 @@ virtqueue_kick_prepare_packed(struct virtqueue
> *vq)
>  	return flags != RING_EVENT_FLAGS_DISABLE;
>  }
> 
> +/*
> + * virtqueue_kick_prepare*() or the virtio_wmb() should be called
> + * before this function to be sure that all the data is visible to vhost.
> + */

C11 _atomic APIs are preferred for new code, other than wmb or rmb,  could you work on that? 

>  static inline void
>  virtqueue_notify(struct virtqueue *vq)
>  {
> -	/*
> -	 * Ensure updated avail->idx is visible to host.
> -	 * For virtio on IA, the notificaiton is through io port operation
> -	 * which is a serialization instruction itself.
> -	 */
>  	VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
>  }
> 
> --
> 2.17.1
Maxime Coquelin Jan. 10, 2019, 9:18 a.m. UTC | #2 
Hi Gavin,

On 1/10/19 9:19 AM, Gavin Hu (Arm Technology China) wrote:
> 
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ilya Maximets
>> Sent: Wednesday, January 9, 2019 10:50 PM
>> To: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
>> Michael S . Tsirkin <mst@redhat.com>; Xiao Wang
>> <xiao.w.wang@intel.com>
>> Cc: Tiwei Bie <tiwei.bie@intel.com>; Zhihong Wang
>> <zhihong.wang@intel.com>; jfreimann@redhat.com; Jason Wang
>> <jasowang@redhat.com>; xiaolong.ye@intel.com;
>> alejandro.lucero@netronome.com; Ilya Maximets
>> <i.maximets@samsung.com>
>> Subject: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering
>> comment for vq notify
>>
>> We're not using io ports in case of modern device even on IA.
>> Also, this comment useless for other architectures.
> 
> Agree, it should be architecture neutral.
> 
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>>   drivers/net/virtio/virtqueue.h | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>> index dffa03669..53aeac238 100644
>> --- a/drivers/net/virtio/virtqueue.h
>> +++ b/drivers/net/virtio/virtqueue.h
>> @@ -437,14 +437,13 @@ virtqueue_kick_prepare_packed(struct virtqueue
>> *vq)
>>   	return flags != RING_EVENT_FLAGS_DISABLE;
>>   }
>>
>> +/*
>> + * virtqueue_kick_prepare*() or the virtio_wmb() should be called
>> + * before this function to be sure that all the data is visible to vhost.
>> + */
> 
> C11 _atomic APIs are preferred for new code, other than wmb or rmb,  could you work on that?

Thanks for the review.
-rc2 deadline is today, so I may apply this series as is if no reply
from Ilya today. If a change is agreed, it could be made on top for
-rc3.

Regards,
Maxime
>>   static inline void
>>   virtqueue_notify(struct virtqueue *vq)
>>   {
>> -	/*
>> -	 * Ensure updated avail->idx is visible to host.
>> -	 * For virtio on IA, the notificaiton is through io port operation
>> -	 * which is a serialization instruction itself.
>> -	 */
>>   	VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
>>   }
>>
>> --
>> 2.17.1
>
Ilya Maximets Jan. 10, 2019, 9:55 a.m. UTC | #3 
On 10.01.2019 12:18, Maxime Coquelin wrote:
> Hi Gavin,
> 
> On 1/10/19 9:19 AM, Gavin Hu (Arm Technology China) wrote:
>>
>>
>>> -----Original Message-----
>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ilya Maximets
>>> Sent: Wednesday, January 9, 2019 10:50 PM
>>> To: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
>>> Michael S . Tsirkin <mst@redhat.com>; Xiao Wang
>>> <xiao.w.wang@intel.com>
>>> Cc: Tiwei Bie <tiwei.bie@intel.com>; Zhihong Wang
>>> <zhihong.wang@intel.com>; jfreimann@redhat.com; Jason Wang
>>> <jasowang@redhat.com>; xiaolong.ye@intel.com;
>>> alejandro.lucero@netronome.com; Ilya Maximets
>>> <i.maximets@samsung.com>
>>> Subject: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering
>>> comment for vq notify
>>>
>>> We're not using io ports in case of modern device even on IA.
>>> Also, this comment useless for other architectures.
>>
>> Agree, it should be architecture neutral.
>>
>>>
>>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>>> ---
>>>   drivers/net/virtio/virtqueue.h | 9 ++++-----
>>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
>>> index dffa03669..53aeac238 100644
>>> --- a/drivers/net/virtio/virtqueue.h
>>> +++ b/drivers/net/virtio/virtqueue.h
>>> @@ -437,14 +437,13 @@ virtqueue_kick_prepare_packed(struct virtqueue
>>> *vq)
>>>       return flags != RING_EVENT_FLAGS_DISABLE;
>>>   }
>>>
>>> +/*
>>> + * virtqueue_kick_prepare*() or the virtio_wmb() should be called
>>> + * before this function to be sure that all the data is visible to vhost.
>>> + */
>>
>> C11 _atomic APIs are preferred for new code, other than wmb or rmb,  could you work on that?
> 
> Thanks for the review.
> -rc2 deadline is today, so I may apply this series as is if no reply
> from Ilya today. If a change is agreed, it could be made on top for
> -rc3.

IMHO, If we'll decide to move to C11 atomics, we'll need to rewrite all
the significant memory accesses in virtio driver at once. We can't do
this partially. This will require significant amount of work to understand
how to do that and will require a lot of testing. So, it's definitely not
for current release. Also, there are possible performance concerns about
such solution.

But I have even more significant concern: C11 atomics are designed for
inter-thread synchronization on multi-core systems. But in case of vDPA we
have real hardware and I'm not sure if we can use C11 atomics for
cross-domain synchronizations. Do you know if some of the memory ordering
types in C11 provide outer domain sync on ARMv8, for example ?

> 
> Regards,
> Maxime
>>>   static inline void
>>>   virtqueue_notify(struct virtqueue *vq)
>>>   {
>>> -    /*
>>> -     * Ensure updated avail->idx is visible to host.
>>> -     * For virtio on IA, the notificaiton is through io port operation
>>> -     * which is a serialization instruction itself.
>>> -     */
>>>       VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
>>>   }
>>>
>>> -- 
>>> 2.17.1
>>
> 
>
Maxime Coquelin Jan. 10, 2019, 2:31 p.m. UTC | #4 
On 1/9/19 3:50 PM, Ilya Maximets wrote:
> We're not using io ports in case of modern device even on IA.
> Also, this comment useless for other architectures.
> 
> Signed-off-by: Ilya Maximets<i.maximets@samsung.com>
> ---
>   drivers/net/virtio/virtqueue.h | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)


Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime
Michael S. Tsirkin Jan. 10, 2019, 2:56 p.m. UTC | #5 
On Thu, Jan 10, 2019 at 12:55:19PM +0300, Ilya Maximets wrote:
> On 10.01.2019 12:18, Maxime Coquelin wrote:
> > Hi Gavin,
> > 
> > On 1/10/19 9:19 AM, Gavin Hu (Arm Technology China) wrote:
> >>
> >>
> >>> -----Original Message-----
> >>> From: dev <dev-bounces@dpdk.org> On Behalf Of Ilya Maximets
> >>> Sent: Wednesday, January 9, 2019 10:50 PM
> >>> To: dev@dpdk.org; Maxime Coquelin <maxime.coquelin@redhat.com>;
> >>> Michael S . Tsirkin <mst@redhat.com>; Xiao Wang
> >>> <xiao.w.wang@intel.com>
> >>> Cc: Tiwei Bie <tiwei.bie@intel.com>; Zhihong Wang
> >>> <zhihong.wang@intel.com>; jfreimann@redhat.com; Jason Wang
> >>> <jasowang@redhat.com>; xiaolong.ye@intel.com;
> >>> alejandro.lucero@netronome.com; Ilya Maximets
> >>> <i.maximets@samsung.com>
> >>> Subject: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering
> >>> comment for vq notify
> >>>
> >>> We're not using io ports in case of modern device even on IA.
> >>> Also, this comment useless for other architectures.
> >>
> >> Agree, it should be architecture neutral.
> >>
> >>>
> >>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> >>> ---
> >>>   drivers/net/virtio/virtqueue.h | 9 ++++-----
> >>>   1 file changed, 4 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
> >>> index dffa03669..53aeac238 100644
> >>> --- a/drivers/net/virtio/virtqueue.h
> >>> +++ b/drivers/net/virtio/virtqueue.h
> >>> @@ -437,14 +437,13 @@ virtqueue_kick_prepare_packed(struct virtqueue
> >>> *vq)
> >>>       return flags != RING_EVENT_FLAGS_DISABLE;
> >>>   }
> >>>
> >>> +/*
> >>> + * virtqueue_kick_prepare*() or the virtio_wmb() should be called
> >>> + * before this function to be sure that all the data is visible to vhost.
> >>> + */
> >>
> >> C11 _atomic APIs are preferred for new code, other than wmb or rmb,  could you work on that?
> > 
> > Thanks for the review.
> > -rc2 deadline is today, so I may apply this series as is if no reply
> > from Ilya today. If a change is agreed, it could be made on top for
> > -rc3.
> 
> IMHO, If we'll decide to move to C11 atomics, we'll need to rewrite all
> the significant memory accesses in virtio driver at once. We can't do
> this partially. This will require significant amount of work to understand
> how to do that and will require a lot of testing. So, it's definitely not
> for current release. Also, there are possible performance concerns about
> such solution.
> 
> But I have even more significant concern: C11 atomics are designed for
> inter-thread synchronization on multi-core systems. But in case of vDPA we
> have real hardware and I'm not sure if we can use C11 atomics for
> cross-domain synchronizations. Do you know if some of the memory ordering
> types in C11 provide outer domain sync on ARMv8, for example ?

I would add to that - compiler support might not work well in all
versions which can be used to build dpdk.
E.g. this article https://lwn.net/Articles/691128/
says that "there will be some seriously suboptimal code production
before gcc-7.1".

HTH

> > 
> > Regards,
> > Maxime
> >>>   static inline void
> >>>   virtqueue_notify(struct virtqueue *vq)
> >>>   {
> >>> -    /*
> >>> -     * Ensure updated avail->idx is visible to host.
> >>> -     * For virtio on IA, the notificaiton is through io port operation
> >>> -     * which is a serialization instruction itself.
> >>> -     */
> >>>       VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
> >>>   }
> >>>
> >>> -- 
> >>> 2.17.1
> >>
> > 
> >
diff mbox series

Patch

diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h
index dffa03669..53aeac238 100644
--- a/drivers/net/virtio/virtqueue.h
+++ b/drivers/net/virtio/virtqueue.h
@@ -437,14 +437,13 @@  virtqueue_kick_prepare_packed(struct virtqueue *vq)
 	return flags != RING_EVENT_FLAGS_DISABLE;
 }
 
+/*
+ * virtqueue_kick_prepare*() or the virtio_wmb() should be called
+ * before this function to be sure that all the data is visible to vhost.
+ */
 static inline void
 virtqueue_notify(struct virtqueue *vq)
 {
-	/*
-	 * Ensure updated avail->idx is visible to host.
-	 * For virtio on IA, the notificaiton is through io port operation
-	 * which is a serialization instruction itself.
-	 */
 	VTPCI_OPS(vq->hw)->notify_queue(vq->hw, vq);
 }