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 955086833 for ; Thu, 6 Dec 2018 13:48:35 +0100 (CET) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout2.w1.samsung.com (KnoxPortal) with ESMTP id 20181206124834euoutp028511305723359f76a01405bcc5bac8e3~tvzyezeuF0089400894euoutp02S for ; Thu, 6 Dec 2018 12:48:34 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout2.w1.samsung.com 20181206124834euoutp028511305723359f76a01405bcc5bac8e3~tvzyezeuF0089400894euoutp02S DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1544100514; bh=p5wIis/gqey2djhgN96cT7t41PycMv+S1EfGEQDqGPo=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=DrdM02slK9GYM3yR+KCCL2esUccfYoXDJtdOzi/m/jQbpjGIf1VC+6gnDmx6eqgzR FgjpnelcIECduw4eclytyvytYsyh1Z6TlPCA3HK+NYgKadqnEMgR3LFSVB7iZonL62 8jpfoh57ULQ9QNr2klkzW+RLl4tIZGXihxf8rGCU= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20181206124833eucas1p21c7e57c134b1172802839ebdd4879e59~tvzyCQ-Cy0324203242eucas1p2E; Thu, 6 Dec 2018 12:48:33 +0000 (GMT) Received: from eucas1p1.samsung.com ( [182.198.249.206]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id 63.41.04806.1AA190C5; Thu, 6 Dec 2018 12:48:33 +0000 (GMT) Received: from eusmtrp1.samsung.com (unknown [182.198.249.138]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20181206124833eucas1p17ffb632a0191360ddab0e79a4a2c23f8~tvzxUslNP2571125711eucas1p1V; Thu, 6 Dec 2018 12:48:33 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp1.samsung.com (KnoxPortal) with ESMTP id 20181206124832eusmtrp145f2d5d31263f18784e8978efe3ff2ba~tvzxGHRH_0948609486eusmtrp1k; Thu, 6 Dec 2018 12:48:32 +0000 (GMT) X-AuditID: cbfec7f5-367ff700000012c6-8f-5c091aa13c02 Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id F8.E8.04284.0AA190C5; Thu, 6 Dec 2018 12:48:32 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20181206124832eusmtip2345ca7be61d99eadb8efd76b63c8bc8c~tvzwkRLCd2088220882eusmtip26; Thu, 6 Dec 2018 12:48:32 +0000 (GMT) To: Jason Wang , Maxime Coquelin , dev@dpdk.org, jfreimann@redhat.com, tiwei.bie@intel.com, zhihong.wang@intel.com Cc: stable@dpdk.org, "Michael S. Tsirkin" From: Ilya Maximets Message-ID: Date: Thu, 6 Dec 2018 15:48:31 +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: 8bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrHKsWRmVeSWpSXmKPExsWy7djPc7oLpThjDP7fUrF492k7k8WV9p/s FssufWayOLdmKYvFsc49LBb/f71itfjX8YfdYmvDfyaLzRcnMTlwevxasJTVY/Gel0we7/dd ZfPo27KKMYAlissmJTUnsyy1SN8ugSvj16LJzAVLlCo+vdFrYHwv3cXIySEhYCLxdMVCFhBb SGAFo8Sv6ZYQ9hdGiXOPo7sYuYDsz4wSH36tY4ZpuNTYyA6RWM4o0bpjMguE85FR4trxuWBV wgIuEvfufWcGSYiAVPWdOM0OkmAWsJK4smYPK4jNJqAjcWr1EUYQm1fATqLz93KgBg4OFgEV iQ+vM0HCogIREh33V7NBlAhKnJz5BOxUTqDyU/MmsEKMFJdo+rISypaXaN46G2yvhMAudokr dw4zQpztIvHr7mSoF4QlXh3fwg5hy0icntzDAmHXS9xveckI0dzBKDH90D8miIS9xJbX59hB jmMW0JRYv0sfIuwo8bNtLhNIWEKAT+LGW0GIG/gkJm2bzgwR5pXoaBOCqFaR+H1wOdQFUhI3 331mn8CoNAvJZ7OQfDMLyTezEPYuYGRZxSieWlqcm55abJyXWq5XnJhbXJqXrpecn7uJEZiK Tv87/nUH474/SYcYBTgYlXh4XzxgjxFiTSwrrsw9xCjBwawkwsugxxEjxJuSWFmVWpQfX1Sa k1p8iFGag0VJnLea4UG0kEB6YklqdmpqQWoRTJaJg1OqgbHoRrer9bp7Bwt9zS7NdNq7oj77 9h3/J1f/+D2bwPWkulV3QVOj0rwdq/f7iCdl32V4Jb/H4eF5Tq6VT3U0rp7vCN53hCmT8bDz z9UPYk4WPphy5LrX7MCwPLGr5yU4vJnK7Z5c2WjCoi3KOe1FUrju6QkirTuV722ZwV8cdN6l 8QN31wGXGVOVWIozEg21mIuKEwGIhZcXQQMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFjrEIsWRmVeSWpSXmKPExsVy+t/xe7oLpDhjDJb/MrJ492k7k8WV9p/s FssufWayOLdmKYvFsc49LBb/f71itfjX8YfdYmvDfyaLzRcnMTlwevxasJTVY/Gel0we7/dd ZfPo27KKMYAlSs+mKL+0JFUhI7+4xFYp2tDCSM/Q0kLPyMRSz9DYPNbKyFRJ384mJTUnsyy1 SN8uQS/j16LJzAVLlCo+vdFrYHwv3cXIySEhYCJxqbGRvYuRi0NIYCmjxI5Zz9ghElISP35d YIWwhSX+XOtiA7GFBN4zSkw5FgZiCwu4SNy7950ZpFlEYDmjxKdL18AamAWsJK6s2cMKMfU3 o8Tun39YQBJsAjoSp1YfYQSxeQXsJDp/Lwfq5uBgEVCR+PA6EyQsKhAhcfblOqgSQYmTM5+A tXIClZ+aNwFqvrrEn3mXmCFscYmmLyuh4vISzVtnM09gFJqFpH0WkpZZSFpmIWlZwMiyilEk tbQ4Nz232FCvODG3uDQvXS85P3cTIzD+th37uXkH46WNwYcYBTgYlXh4XzxgjxFiTSwrrsw9 xCjBwawkwsugxxEjxJuSWFmVWpQfX1Sak1p8iNEU6LeJzFKiyfnA1JBXEm9oamhuYWlobmxu bGahJM573qAySkggPbEkNTs1tSC1CKaPiYNTqoGRz9D11xtx52yuuHU2D+vcVl9x473isdN9 c+I7xd515+zj19wOn350Nbv5OpFpl9WXnHj14mG9PM/9ux2LyiryKl/k5ZX86n489YFRWWuC +zer+mVHwy/WnthUpnnLwfTrhpuHdwhffCauWbOlvTYxVnrS57vPptgkxyYeuXlL+tWWt+Lv j3T3KrEUZyQaajEXFScCAJoY3CHVAgAA X-CMS-MailID: 20181206124833eucas1p17ffb632a0191360ddab0e79a4a2c23f8 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> 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: Thu, 06 Dec 2018 12:48:36 -0000 On 06.12.2018 7:17, 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. > On a second glance I changed my mind. The code looks like this: 1. read avail_head = avail->idx 2. read cur_idx = last_avail_idx if (cur_idx != avail_head) { 3. read idx = avail->ring[cur_idx] 4. read desc[idx] } There is an address (data) dependency: 2 -> 3 -> 4. These reads could not be reordered. But it's only control dependency between 1 and (3, 4), because 'avail_head' is not used to calculate 'cur_idx'. In case of aggressive speculative execution, 1 could be reordered with 3 resulting with reading of not yet updated 'idx'. Not sure if speculative execution could go so far while 'avail_head' is not read yet, but it's should be possible in theory. Thoughts ? >> >>>       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. > It's kind of similar situation here, but 'avail_head' is involved somehow in 'cur_idx' calculation because of fill_vec_buf_split(..., vq->last_avail_idx + i, ...) And 'i' depends on 'free_entries'. But we need to look at the exact asm code to be sure. I think, we may add barrier here to avoid possible issues. > Thanks > > >> >>>       VHOST_LOG_DEBUG(VHOST_DATA, "(%d) %s\n", dev->vid, __func__); >>>         count = RTE_MIN(count, MAX_PKT_BURST); >>> > >