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

Shahaf Shuler shahafs at mellanox.com
Thu Dec 27 11:07:46 CET 2018


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 at 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&data=02%7C01%7Cshahafs%40mellanox.co
> m%7C147684bb9b754e30781408d66b506965%7Ca652971c7d2e4d9ba6a4d149
> 256f461b%7C0%7C0%7C636814390449088148&sdata=f4W1YLftBtZ4zAQ3
> %2Bv7LV5w%2FDhM5aWpROV2c2gHY8iU%3D&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? 


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



More information about the dev mailing list