Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support

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

On 27.12.2018 13:07, Shahaf Shuler wrote:
> Hi Ilya, 
> 
> Wednesday, December 26, 2018 6:37 PM, Ilya Maximets:
>> Subject: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering
>> feature support
>>
>> VIRTIO_F_ORDER_PLATFORM is required to use proper memory barriers in
>> case of HW vhost implementations like vDPA.
>>
>> DMA barriers (rte_cio_*) are sufficent for that purpose.
>>
>> Previously known as VIRTIO_F_IO_BARRIER.
>>
>> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
>> ---
>>
>> Version 2:
>>   * rebased on current master (packed rings).
>>
>> RFC --> Version 1:
>>   * Dropped vendor-specific hack to determine if we need real barriers.
>>   * Added VIRTIO_F_ORDER_PLATFORM feature definition and checking.
>>
>> Note: Patch to change the name of the feature from VIRTIO_F_IO_BARRIER
>>       to VIRTIO_F_ORDER_PLATFORM is not merged yet:
>>
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.m
>> ail-archive.com%2Fvirtio-dev%40lists.oasis-
>> open.org%2Fmsg04114.html&amp;data=02%7C01%7Cshahafs%40mellanox.co
>> m%7C147684bb9b754e30781408d66b506965%7Ca652971c7d2e4d9ba6a4d149
>> 256f461b%7C0%7C0%7C636814390449088148&amp;sdata=f4W1YLftBtZ4zAQ3
>> %2Bv7LV5w%2FDhM5aWpROV2c2gHY8iU%3D&amp;reserved=0
>>
>>  drivers/net/virtio/virtio_ethdev.c |  2 ++  drivers/net/virtio/virtio_ethdev.h |  3
>> ++-
>>  drivers/net/virtio/virtio_pci.h    |  7 ++++++
>>  drivers/net/virtio/virtio_rxtx.c   | 16 ++++++------
>>  drivers/net/virtio/virtqueue.h     | 39 ++++++++++++++++++++++++------
>>  5 files changed, 51 insertions(+), 16 deletions(-)
> 
> [...]
> 
>>
>>  /*
>> - * Per virtio_config.h in Linux.
>> + * Per virtio_ring.h in Linux.
>>   *     For virtio_pci on SMP, we don't need to order with respect to MMIO
>>   *     accesses through relaxed memory I/O windows, so smp_mb() et al are
>>   *     sufficient.
>>   *
>> + *     For using virtio to talk to real devices (eg. vDPA) we do need real
>> + *     barriers.
>>   */
>> -#define virtio_mb()	rte_smp_mb()
>> -#define virtio_rmb()	rte_smp_rmb()
>> -#define virtio_wmb()	rte_smp_wmb()
>> +static inline void
>> +virtio_mb(uint8_t weak_barriers)
>> +{
>> +	if (weak_barriers)
>> +		rte_smp_mb();
>> +	else
>> +		rte_mb();
>> +}
>> +
>> +static inline void
>> +virtio_rmb(uint8_t weak_barriers)
>> +{
>> +	if (weak_barriers)
>> +		rte_smp_rmb();
>> +	else
>> +		rte_cio_rmb();
>> +}
>> +
>> +static inline void
>> +virtio_wmb(uint8_t weak_barriers)
>> +{
>> +	if (weak_barriers)
>> +		rte_smp_wmb();
>> +	else
>> +		rte_cio_wmb();
> 
> Just wondering if the cio barrier is enough here.
> This virtio_wmb will be called, for example in the following sequence for transmit (not of packed queue):
> if (likely(nb_tx)) {                                                 
>         vq_update_avail_idx(vq);                <--- virito_wmb()                     
>                                                                      
>         if (unlikely(virtqueue_kick_prepare(vq))) {                 <--- no barrier 
>                 virtqueue_notify(vq);                                
>                 PMD_TX_LOG(DEBUG, "Notified backend after xmit");    
>         }                                                            
> }                                                                    
> 
> Assuming the backhand has vDPA device. The notify will be most likely mapped to the device PCI bar as write combining.
> This means we need to keep ordering here between two memory domains - the PCI and the host local memory. We must make sure that when the notify reaches the device, the store to the host local memory is already written to memory (and not in the write buffer).
> Is complier barrier is enough for such purpose? Or we need something stronger like sfence? 
> 
> Note #1 - This issue (if exists) is not caused directly by your code, however you are making things right here w/ respect to memory ordering so worth taking care also on this one
> Note #2 - if indeed there is an issue here, for packed queue we are somewhat OK, since virtio_mb() will be called before the kick. I am not sure why we need such hard barrier and sfence is not enough. Do you know? 

Hmm. Thanks for spotting this.
The issue exists, but it's a bit different.
Looks like we have missed virtio_mb() for the split case inside the
virtqueue_kick_prepare(). It's required because we must ensure the
ordering between writing the data (updating avail->idx) and reading the
used->flags. Otherwise we could catch the case where data written, but
virtqueue wasn't notified and vhost waits for notification.
This is not the vDPA related issue. This could happen with kernel vhost.
Adding the virtio_mb() to virtqueue_kick_prepare() will solve the vDPA
case automatically. I'll prepare the patches.


Beside that, Jens, between v10 and v11 of the packed queues support
patch-set you added the virtio_mb() to kick_prepare function.
I guess, this could be the root cause of the performance drop you
spotted in compare with split queues. Maybe you could check and prove
that point?
I didn't found why you done that change (there are no such review
comments on the list), but it was right decision.

virtio_mb() is really heavy. I'd like to avoid it somehow, but I don't
know how to do this yet.

> 
> 
>> +}
>>
>>  #ifdef RTE_PMD_PACKET_PREFETCH
>>  #define rte_packet_prefetch(p)  rte_prefetch1(p) @@ -325,7 +350,7 @@
>> virtqueue_enable_intr_packed(struct virtqueue *vq)
>>
>>
>>  	if (vq->event_flags_shadow == RING_EVENT_FLAGS_DISABLE) {
>> -		virtio_wmb();
>> +		virtio_wmb(vq->hw->weak_barriers);
>>  		vq->event_flags_shadow = RING_EVENT_FLAGS_ENABLE;
>>  		*event_flags = vq->event_flags_shadow;
>>  	}
>> @@ -391,7 +416,7 @@ void vq_ring_free_inorder(struct virtqueue *vq,
>> uint16_t desc_idx,  static inline void  vq_update_avail_idx(struct virtqueue *vq)
>> {
>> -	virtio_wmb();
>> +	virtio_wmb(vq->hw->weak_barriers);
>>  	vq->vq_ring.avail->idx = vq->vq_avail_idx;  }
>>
>> @@ -423,7 +448,7 @@ virtqueue_kick_prepare_packed(struct virtqueue *vq)  {
>>  	uint16_t flags;
>>
>> -	virtio_mb();
>> +	virtio_mb(vq->hw->weak_barriers);
>>  	flags = vq->ring_packed.device_event->desc_event_flags;
>>
>>  	return flags != RING_EVENT_FLAGS_DISABLE;
>> --
>> 2.17.1
>