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 3B7735F17 for ; Fri, 7 Dec 2018 15:58:28 +0100 (CET) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20181207145827euoutp029063598286c33fc8813cd678aa5844ce~uFOeWyRRq2528325283euoutp024 for ; Fri, 7 Dec 2018 14:58:27 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20181207145827euoutp029063598286c33fc8813cd678aa5844ce~uFOeWyRRq2528325283euoutp024 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1544194707; bh=/hFPHpZcmEoehJ/l2o7XfIzPPYBPiYPpu6f4l4tHldY=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=Ahwp/lFLMxY07sA71jr+wxrQNjyxE4jb48Y8+TAY3dsAnXC0C4ilqM3cEpH1XEb2r 937/DMAcLdwMHOYluFG90vZuO1hiF5m0I0nZIGGUMGtlWo9fFM6HNxsYdxz4CAreae szmXypuZOb+mJdmEvbmWhGMRtLYAGpGsPbyNA/fY= Received: from eusmges1new.samsung.com (unknown [203.254.199.242]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20181207145826eucas1p2cf1ed5277f0d538b9fa8dd8c63a51c2b~uFOd7ZI1C3195131951eucas1p2D; Fri, 7 Dec 2018 14:58:26 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges1new.samsung.com (EUCPMTA) with SMTP id A0.CF.04441.29A8A0C5; Fri, 7 Dec 2018 14:58:26 +0000 (GMT) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p2.samsung.com (KnoxPortal) with ESMTPA id 20181207145825eucas1p2dfbc3c079ee05d2a7ce1a801dcef4c7f~uFOdI9N1z0039000390eucas1p2g; Fri, 7 Dec 2018 14:58:25 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20181207145825eusmtrp22354586b08f1f6f48981db9527e693fa~uFOc6QpYJ2281822818eusmtrp2H; Fri, 7 Dec 2018 14:58:25 +0000 (GMT) X-AuditID: cbfec7f2-5e3ff70000001159-b2-5c0a8a92ef5e Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 5A.1E.04128.19A8A0C5; Fri, 7 Dec 2018 14:58:25 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20181207145824eusmtip275de623e96e2ffab5f9664e7673e286c~uFOcXEOd12632126321eusmtip2q; Fri, 7 Dec 2018 14:58:24 +0000 (GMT) To: "Michael S. Tsirkin" , Jason Wang Cc: Maxime Coquelin , dev@dpdk.org, jfreimann@redhat.com, tiwei.bie@intel.com, zhihong.wang@intel.com, stable@dpdk.org From: Ilya Maximets Message-ID: <07187c69-54b8-c5bd-9c02-a3f25e437a9a@samsung.com> Date: Fri, 7 Dec 2018 17:58:24 +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: <20181206083048-mutt-send-email-mst@kernel.org> Content-Language: en-GB Content-Transfer-Encoding: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrPKsWRmVeSWpSXmKPExsWy7djP87qTurhiDKYvFbF492k7k8WV9p/s FssufWayOLdmKYvFsc49LBb/f71itfjX8YfdYmvDfyaLzRcnMTlwevxasJTVY/Gel0we7/dd ZfPo27KKMYAlissmJTUnsyy1SN8ugSvj2fYzTAUtahVvvrWyNTCekOti5OSQEDCROLJpCmsX IxeHkMAKRombzVPZIJwvjBKLHp5nhHA+M0r8/vacFaZlxs8VYLaQwHKgluUOEEUfGSVa771l AkkIC7hI3Lv3nRnEFhHwlvg/7QbYWGaBaYwSbScPgXWzCehInFp9hBHE5hWwk1j/9zILiM0i oCLxdfFnsEGiAhESHfdXs0HUCEqcnPkErIZTwEai93szO4jNLCAu0fRlJSuELS/RvHU20GIO oEv3sUscyIU42kXi1o0WRghbWOLV8S3sELaMxP+d85kg7HqJ+y0vwT6WEOhglJh+6B9Uwl5i y+tz7CAzmQU0Jdbv0ocIO0r8bJvLBLGKT+LGW0GIC/gkJm2bDnUBr0RHmxBEtYrE74PLmSFs KYmb7z6zT2BUmoXkr1lIfpmF5JdZCHsXMLKsYhRPLS3OTU8tNsxLLdcrTswtLs1L10vOz93E CExGp/8d/7SD8eulpEOMAhyMSjy8FQ6cMUKsiWXFlbmHGCU4mJVEeJVyuGKEeFMSK6tSi/Lj i0pzUosPMUpzsCiJ81YzPIgWEkhPLEnNTk0tSC2CyTJxcEo1MBqo/Lfdy1C88PyR7N9TY2ru KC8/xHXMskx8o1fTtZ0/F6kJh6c/uzq/rGjrn8Vyt6/va3jcZebJt/v97fXdsulrt36I39iy Y96N31OLc7csWfd88uYvUVM5vEIOivn1hOvcmnGn40mPjGThtq2aM1o/BRaprl91+uIT4ZVH Hu3JfBzKveJOI6OPEktxRqKhFnNRcSIAmd8t5kIDAAA= X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrKIsWRmVeSWpSXmKPExsVy+t/xe7oTu7hiDNZeUbB492k7k8WV9p/s FssufWayOLdmKYvFsc49LBb/f71itfjX8YfdYmvDfyaLzRcnMTlwevxasJTVY/Gel0we7/dd ZfPo27KKMYAlSs+mKL+0JFUhI7+4xFYp2tDCSM/Q0kLPyMRSz9DYPNbKyFRJ384mJTUnsyy1 SN8uQS/j2fYzTAUtahVvvrWyNTCekOti5OSQEDCRmPFzBWsXIxeHkMBSRomF/y6zQySkJH78 usAKYQtL/LnWxQZR9J5R4t2WpWAJYQEXiXv3vjOD2CIC3hL/p90AK2IWmMYosXfXWXaIjrVM Eqt2HAbrYBPQkTi1+ggjiM0rYCex/u9lFhCbRUBF4uviz0wgtqhAhMTZl+ugagQlTs58AlbD KWAj0fu9Gew8ZgF1iT/zLjFD2OISTV9WskLY8hLNW2czT2AUmoWkfRaSlllIWmYhaVnAyLKK USS1tDg3PbfYSK84Mbe4NC9dLzk/dxMjMAq3Hfu5ZQdj17vgQ4wCHIxKPLwVDpwxQqyJZcWV uYcYJTiYlUR4lXK4YoR4UxIrq1KL8uOLSnNSiw8xmgI9N5FZSjQ5H5gg8kriDU0NzS0sDc2N zY3NLJTEec8bVEYJCaQnlqRmp6YWpBbB9DFxcEo1MMZGhbzaGPdKzqU9/oiq2OEDBkeVt0du afpRuf7Qr6VPT+35eZHvxe7Mq7eM4vlidmgctz70ZkbukeBSpmeC5UFdcZma5ys2PVt6Mqll We4z/vVb1jDoLDHoYb/9k2tb9tx2Ee79X24m1Mt3fb9wuIDfRKf1xsuFQR/nPFFkj3fKsJtq 93Re1yclluKMREMt5qLiRABvcSCE2AIAAA== X-CMS-MailID: 20181207145825eucas1p2dfbc3c079ee05d2a7ce1a801dcef4c7f X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20181205113041eucas1p1943b9c13af2fb5b736ba4906b59a9cd5 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20181205113041eucas1p1943b9c13af2fb5b736ba4906b59a9cd5 References: <20181205094957.1938-2-maxime.coquelin@redhat.com> <20181206083048-mutt-send-email-mst@kernel.org> Subject: Re: [dpdk-dev] [1/5] vhost: enforce avail index and desc read ordering 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: Fri, 07 Dec 2018 14:58:28 -0000 On 06.12.2018 16:48, Michael S. Tsirkin wrote: > On Thu, Dec 06, 2018 at 12:17:38PM +0800, Jason Wang wrote: >> >> On 2018/12/5 下午7:30, Ilya Maximets wrote: >>> On 05.12.2018 12:49, Maxime Coquelin wrote: >>>> A read barrier is required to ensure the ordering between >>>> available index and the descriptor reads is enforced. >>>> >>>> Fixes: 4796ad63ba1f ("examples/vhost: import userspace vhost application") >>>> Cc: stable@dpdk.org >>>> >>>> Reported-by: Jason Wang >>>> Signed-off-by: Maxime Coquelin >>>> --- >>>> lib/librte_vhost/virtio_net.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c >>>> index 5e1a1a727..f11ebb54f 100644 >>>> --- a/lib/librte_vhost/virtio_net.c >>>> +++ b/lib/librte_vhost/virtio_net.c >>>> @@ -791,6 +791,12 @@ virtio_dev_rx_split(struct virtio_net *dev, struct vhost_virtqueue *vq, >>>> rte_prefetch0(&vq->avail->ring[vq->last_avail_idx & (vq->size - 1)]); >>>> avail_head = *((volatile uint16_t *)&vq->avail->idx); >>>> + /* >>>> + * The ordering between avail index and >>>> + * desc reads needs to be enforced. >>>> + */ >>>> + rte_smp_rmb(); >>>> + >>> Hmm. This looks weird to me. >>> Could you please describe the bad scenario here? (It'll be good to have it >>> in commit message too) >>> >>> As I understand, you're enforcing the read of avail->idx to happen before >>> reading the avail->ring[avail_idx]. Is it correct? >>> >>> But we have following code sequence: >>> >>> 1. read avail->idx (avail_head). >>> 2. check that last_avail_idx != avail_head. >>> 3. read from the ring using last_avail_idx. >>> >>> So, there is a strict dependency between all 3 steps and the memory >>> transaction will be finished at the step #2 in any case. There is no >>> way to read the ring before reading the avail->idx. >>> >>> Am I missing something? >> >> >> Nope, I kind of get what you meaning now. And even if we will >> >> 4. read descriptor from descriptor ring using the id read from 3 >> >> 5. read descriptor content according to the address from 4 >> >> They still have dependent memory access. So there's no need for rmb. > > I am pretty sure on some architectures there is a need for a barrier > here. This is an execution dependency since avail_head is not used as an > index. And reads can be speculated. So the read from the ring can be > speculated and execute before the read of avail_head and the check. > > However SMP rmb is/should be free on x86. rte_smp_rmd() turns into compiler barrier on x86. And compiler barriers could be harmful too in some cases. > So unless someone on this > thread is actually testing performance on non-x86, you are both wasting > cycles discussing removal of nop macros and also risk pushing untested > software on users. Since DPDK supports not only x86, we have to consider possible performance issues on different architectures. In fact that this patch makes no sense on x86, the only thing we need to consider is the stability and performance on non-x86 architectures. If we'll not pay attention to things like this, vhost-user could become completely unusable on non-x86 architectures someday. It'll be cool if someone could test patches (autotest would be nice too) on ARM at least. But, unfortunately, testing of DPDK is still far from being ideal. And the lack of hardware is the main issue. I'm running vhost with qemu on my ARMv8 platform from time to time, but it's definitely not enough. And I can not test every patch on a list. However I made a few tests on ARMv8 and this patch shows no significant performance difference. But it makes the performance a bit more stable between runs, which is nice. > > >> >>> >>>> for (pkt_idx = 0; pkt_idx < count; pkt_idx++) { >>>> uint32_t pkt_len = pkts[pkt_idx]->pkt_len + dev->vhost_hlen; >>>> uint16_t nr_vec = 0; >>>> @@ -1373,6 +1379,12 @@ virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq, >>>> if (free_entries == 0) >>>> return 0; >>>> + /* >>>> + * The ordering between avail index and >>>> + * desc reads needs to be enforced. >>>> + */ >>>> + rte_smp_rmb(); >>>> + >>> This one is strange too. >>> >>> free_entries = *((volatile uint16_t *)&vq->avail->idx) - >>> vq->last_avail_idx; >>> if (free_entries == 0) >>> return 0; >>> >>> The code reads the value of avail->idx and uses the value on the next >>> line even with any compiler optimizations. There is no way for CPU to >>> postpone the actual read. >> >> >> Yes. >> >> Thanks >> >> >>> >>>> VHOST_LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__); >>>> count = RTE_MIN(count, MAX_PKT_BURST); >>>> > >