Re: [dpdk-dev] [PATCH v3 2/3] net/virtio: update memory ordering comment for vq notify

[Ilya Maximets <i.maximets@samsung.com>]

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