From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout2.w1.samsung.com (mailout2.w1.samsung.com [210.118.77.12]) by dpdk.org (Postfix) with ESMTP id D758B1B48D for ; Wed, 9 Jan 2019 15:34:47 +0100 (CET) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20190109143446euoutp028f1cfc12d425ab22be9210076ed8e80b~4NMOWAKIS1016310163euoutp02J for ; Wed, 9 Jan 2019 14:34:46 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20190109143446euoutp028f1cfc12d425ab22be9210076ed8e80b~4NMOWAKIS1016310163euoutp02J DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1547044486; bh=LHoCOSDUfc/d9zja1iWNj/MV5Sgm2nQFtwW/pMaaPXs=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=tlPNjhcf5ivNVxE8zQA2Y3s3XlPxmCvIldXrJfB2xkAXW7GWACDWnJXL0bJZmKMKb ato7q9myiECkljD6U9Z9Rg118eUDW+GvMKXaK8VnXnILyoqA2gJoXNTXCKrGDsBk1+ O3XMNwBnS/xIr1c4oMlBcOuq7wDOXD3oVH4NtnwY= Received: from eusmges1new.samsung.com (unknown [203.254.199.242]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20190109143446eucas1p24a3c41ca97f2b7d15bfa701b8b3fa89c~4NMN_UYs22757827578eucas1p2j; Wed, 9 Jan 2019 14:34:46 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges1new.samsung.com (EUCPMTA) with SMTP id 80.C2.04441.586063C5; Wed, 9 Jan 2019 14:34:45 +0000 (GMT) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20190109143445eucas1p187fd8e18d3d08184b5fb1989d0f97336~4NMNO6tY10390203902eucas1p1Y; Wed, 9 Jan 2019 14:34:45 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20190109143445eusmtrp2165ad908b5b9b19261ac5e9212b39788~4NMM-56_52613826138eusmtrp2h; Wed, 9 Jan 2019 14:34:45 +0000 (GMT) X-AuditID: cbfec7f2-5e3ff70000001159-bc-5c360685886c Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 54.A3.04128.586063C5; Wed, 9 Jan 2019 14:34:45 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20190109143443eusmtip15797956a88702b075d12598fa9c0b796~4NMLp1-a61726117261eusmtip1O; Wed, 9 Jan 2019 14:34:43 +0000 (GMT) To: Shahaf Shuler , "dev@dpdk.org" , Maxime Coquelin , "Michael S . Tsirkin" , Xiao Wang , "jfreimann@redhat.com" Cc: Tiwei Bie , Zhihong Wang , Jason Wang , "xiaolong.ye@intel.com" , "alejandro.lucero@netronome.com" , Daniel Marcovitch From: Ilya Maximets Message-ID: <2832c88f-9b43-997c-5937-ef5ae6482fd5@samsung.com> Date: Wed, 9 Jan 2019 17:34:38 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA01SaUhUYRTte+/NvKc48TkaXqwoR1GK0hST9yNapB9D2CaIVlq+8mGSjjKj pgU5LeZklGKSzmColaOpra6VS07lkmIkWpaRSeKWuY0iLmnOPCX/3XvOufeeA5chpS0ieyZM Ec0rFVy4TGxJldfPfNyeKPYK3KHW7GJbJ/QE+3W6kGZHJioItj1phmb1bUaCbS3Oo9j6G1UU uzg7JGK7sg6wZepFgq3tf0uz88M72ZJPacReiXw2J08kf1A1SMj7Mt4S8oybPaR8tKZDLL9d WoiOiI9b7grhw8NieaXb7mDLs3fm6qgorUfcyPSwWI2KnZORBQPYExLHq8lkZMlIcQGCz+15 tImQ4kkExlwvgTAiuNX0i1yZyE1VI4HIR5DZWSgWmnEE3foxsUllg4OgoCmJMBG2eGqJaKwy NyROJaCjYVhkUonxNvhQ9G5pF8NI8G74VmdlginsBHdTJs2SdTgANN1F5qUSbA1N2l7KJLdY OtD0N8gEk9gOrkw+Egn1Jqj4k2XOA3iQBt3DLkqwvR/6q8eXI9jAUEMpLdQbYPFlNiHUCdB9 bRAJwxoEGYaFZWIPlP5upU2HSbwFnr5yE+B9MDeTb/YDeC10/rEWPKyFtPIMUoAloLkuFdRO MFeXv+zAHr6OGOlUJNOtCqZblUa3Ko3u/90cRBUiOz5GFRHKq9wV/HlXFRehilGEup6JjHiB ln6seaFhohJNtZ02IMwgmZUkeGFnoFTExariIwwIGFJmK3H57hkolYRw8Rd4ZeQpZUw4rzKg 9Qwls5NcXPPzhBSHctH8OZ6P4pUrLMFY2KuRg4Ps/Y/LNbWveyfLiuOKxtP7m/38+70BNlba +7SUu3gfCni+Zk/jlO98ymHtG2cmoGfu3sAx2t/ZqiLsaDZTWzJGKKIdH2de9E0P97vaHuWo 9ZjqderRnrQOUyZc4uQHre+rR/oMX7JtB66oQzgHjXG0/slmotrnmd61LbdxVEapznLuW0ml ivsHY7qKTl8DAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrAIsWRmVeSWpSXmKPExsVy+t/xu7qtbGYxBs8ms1mc+7SMyeLm91Xs Fu8+bWeyuNL+k91i2aXPTBbn1ixlsTjWuYfF4v+vV6wWt+d4WWxt+M9ksf/5YXaLP29MLTZf nMTkwOvxa8FSVo/Fe14yeTybfpjJY3r3Q2aP9/uusnn0bVnFGMAWpWdTlF9akqqQkV9cYqsU bWhhpGdoaaFnZGKpZ2hsHmtlZKqkb2eTkpqTWZZapG+XoJcx+fdBloKZRhXvvr9ha2Bco9bF yMkhIWAisXBCA2MXIxeHkMBSRollR6cyQySkJH78usAKYQtL/LnWxQZR9J5R4t+fA2AJYYFY iRUn25lAEiICX4G6p70Ac5gFJjFJ9K99ywLRMplJon3/BiaQFjYBHYlTq48ALeTg4BWwk7h1 kAckzCKgIjGt/wvYVFGBCImzL9cxgti8AoISJ2c+YQEp5wTadvJvLEiYWUBd4s+8S8wQtrhE 05eVrBC2vMT2t3OYJzAKzULSPQtJyywkLbOQtCxgZFnFKJJaWpybnltspFecmFtcmpeul5yf u4kRGLvbjv3csoOx613wIUYBDkYlHt6Ef6YxQqyJZcWVuYcYJTiYlUR41e+YxAjxpiRWVqUW 5ccXleakFh9iNAX6bSKzlGhyPjCt5JXEG5oamltYGpobmxubWSiJ8543qIwSEkhPLEnNTk0t SC2C6WPi4JRqYAxxu3dLPSpL1q2haOe2Nxq+RnJSgg9zgo8oLD19/8GDCjmHau3dsb7sRhOs 3DRYAh8xTp/MzHjha8n6qvqw11NWGN9c+KIrdbFdS+QL3thLTicyM9RkhR7NlWNbPTWq9wrP udkyJZ9WPkj6VVzfNY+FeY+3WzgrzxP94884lvEqJ7L8SD25XYmlOCPRUIu5qDgRAMzgei3z AgAA X-CMS-MailID: 20190109143445eucas1p187fd8e18d3d08184b5fb1989d0f97336 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20181226163717eucas1p15276eb45e35abe2c9cf3e7c1e0050823 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20181226163717eucas1p15276eb45e35abe2c9cf3e7c1e0050823 References: <20181214153812.3878-1-i.maximets@samsung.com> <20181226163712.31596-1-i.maximets@samsung.com> Subject: Re: [dpdk-dev] [PATCH v2] net/virtio: add platform memory ordering feature support X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 09 Jan 2019 14:34:48 -0000 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 >> --- >> >> 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? 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 >