From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailout1.w1.samsung.com (mailout1.w1.samsung.com [210.118.77.11]) by dpdk.org (Postfix) with ESMTP id 2B098322C for ; Wed, 23 Jan 2019 17:33:26 +0100 (CET) Received: from eucas1p1.samsung.com (unknown [182.198.249.206]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20190123163325euoutp016dfcc81f3d43b13b3a24a6f9f95c3b96~8h10CgdB-1650316503euoutp01H for ; Wed, 23 Jan 2019 16:33:25 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20190123163325euoutp016dfcc81f3d43b13b3a24a6f9f95c3b96~8h10CgdB-1650316503euoutp01H DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1548261205; bh=U/B/1dtka8mNv90ff3Z18EqJdyQC/F5StPYYI+sru6M=; h=Subject:From:To:Date:In-Reply-To:References:From; b=EfvEhSRlhWDCDDz1ZNrm5LHv1duPRdEAHejiK5L/uuNnXn+RZKvTxjIFs+NRSzXfH lUDZ/M6v640exmqfpVhq+1XsE35cdgnyXe0Qdpp1MfA9J5Chw5/bDIWHNTq6UT3SgU AESYquT2BxAxMQH2tACBd+1JxwliSuEBWHhlyE84= Received: from eusmges1new.samsung.com (unknown [203.254.199.242]) by eucas1p2.samsung.com (KnoxPortal) with ESMTP id 20190123163324eucas1p2c4fc1d724fc17918975cb1553afb9c7b~8h1zGII6-0797707977eucas1p2O; Wed, 23 Jan 2019 16:33:24 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges1new.samsung.com (EUCPMTA) with SMTP id D5.84.04441.457984C5; Wed, 23 Jan 2019 16:33:24 +0000 (GMT) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20190123163323eucas1p1baaec2c637cdc656ab9b26dbfd455bae~8h1yTB1jR2456724567eucas1p1v; Wed, 23 Jan 2019 16:33:23 +0000 (GMT) Received: from eusmgms1.samsung.com (unknown [182.198.249.179]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20190123163323eusmtrp2e29f80cb63b702654a9df0940aaec6f3~8h1yEyizb2801128011eusmtrp21; Wed, 23 Jan 2019 16:33:23 +0000 (GMT) X-AuditID: cbfec7f2-5c9ff70000001159-fc-5c489754efaf Received: from eusmtip1.samsung.com ( [203.254.199.221]) by eusmgms1.samsung.com (EUCPMTA) with SMTP id 82.A0.04284.357984C5; Wed, 23 Jan 2019 16:33:23 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip1.samsung.com (KnoxPortal) with ESMTPA id 20190123163322eusmtip1bf19c43e5bb9d8f6e639a5031aae3e84~8h1xXukFn0215902159eusmtip1Z; Wed, 23 Jan 2019 16:33:22 +0000 (GMT) From: Ilya Maximets To: Tiwei Bie , maxime.coquelin@redhat.com, zhihong.wang@intel.com, dev@dpdk.org Message-ID: <9799a0cf-52a5-0ea4-03d8-b812b338ce59@samsung.com> Date: Wed, 23 Jan 2019 19:33:21 +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: H4sIAAAAAAAAA+NgFprNKsWRmVeSWpSXmKPExsWy7djP87oh0z1iDBb2sVu8+7SdyeJK+092 i2Ode1gstjb8Z7LYfHESkwOrx68FS1k9Fu95yeTxft9VNo++LasYA1iiuGxSUnMyy1KL9O0S uDLmzbnBXPDLumJhT1ID4wrDLkZODgkBE4kbG1+ydDFycQgJrGCU+NE6hx3C+cIosWvzKSYI 5zOjxJHmFkaYlq1nYFqWM0qsmXaCGcL5yCjxbtovVpAqYQFLifUnHzGB2GwCOhKnVh8B6xYR SJf4NeUeO4jNK2AnsfbTGWYQm0VAVeLe1kVg9aICERId91ezQdQISpyc+YQFxOYUsJe4+28h mM0sIC7R9GUlK4QtL7H97RywIyQEmtklFsybwApxqotE4+vJTBC2sMSr41vYIWwZif8750PF 6yXut7xkhGjuYJSYfugfVMJeYsvrc0ANHEAbNCXW79IHMSUEHCXadkRBmHwSN94KQpzAJzFp 23RmiDCvREebEMQMFYnfB5czQ9hSEjfffYY6wENi+69NbBMYFWcheXIWksdmIXlsFsIJCxhZ VjGKp5YW56anFhvmpZbrFSfmFpfmpesl5+duYgQmmdP/jn/awfj1UtIhRgEORiUeXo5Cjxgh 1sSy4srcQ4wSHMxKIryuF91ihHhTEiurUovy44tKc1KLDzFKc7AoifNWMzyIFhJITyxJzU5N LUgtgskycXBKNTDqFii5nsn7Wrw/bVtZ4eRvcV8zy6yd9qVmearP3rs29G66I4vObS7zy/+T m1VLlG30JJqE/ycEHr7nY9b1ILda6od69cU/K1etCVp0YQVTd0n08ydmBcb70yVLHr8uiuk6 U7uo6F/j+545U64+EFR8eE+qRHD/FC7nFfFb5oeIPAjNLpT6LqbEUpyRaKjFXFScCAB6Gu4/ LgMAAA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFlrAIsWRmVeSWpSXmKPExsVy+t/xu7rB0z1iDG708Fm8+7SdyeJK+092 i2Ode1gstjb8Z7LYfHESkwOrx68FS1k9Fu95yeTxft9VNo++LasYA1ii9GyK8ktLUhUy8otL bJWiDS2M9AwtLfSMTCz1DI3NY62MTJX07WxSUnMyy1KL9O0S9DLmzbnBXPDLumJhT1ID4wrD LkZODgkBE4mtZ16ygNhCAksZJT7tZoaIS0n8+HWBFcIWlvhzrYsNouY9o0TzD30QW1jAUmL9 yUdMIDabgI7EqdVHGEFsEYF0ibXXZ7JD1OdIvJ53BSzOK2AnsfbTGbD5LAKqEve2LgLrFRWI kDj7ch1UjaDEyZlPwO7hFLCXuPtvIZjNLKAu8WfeJWYIW1yi6ctKVghbXmL72znMExgFZyFp n4WkZRaSlllIWhYwsqxiFEktLc5Nzy021CtOzC0uzUvXS87P3cQIjJ1tx35u3sF4aWPwIUYB DkYlHt4NeR4xQqyJZcWVuYcYJTiYlUR4XS+6xQjxpiRWVqUW5ccXleakFh9iNAV6biKzlGhy PjCu80riDU0NzS0sDc2NzY3NLJTEec8bVEYJCaQnlqRmp6YWpBbB9DFxcEo1MEpL/Zmi7iH0 +X2ww37hjhvH7NZcOvGUV+rr3j8cPS/MYxJfCVUkhMYaWE9t5yszzb05/2+o3mGP77rv8yP/ zc9U5Lj3bqGvVxFT7fanqexXNuWKCkacu8Z6d0+N0JE/pZFr+eoEOUIX/mU02pfucL7rhNvC c0vvW0VkSEexl29ccUKlPpQnXImlOCPRUIu5qDgRAH0FelazAgAA X-CMS-MailID: 20190123163323eucas1p1baaec2c637cdc656ab9b26dbfd455bae X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20190123163323eucas1p1baaec2c637cdc656ab9b26dbfd455bae X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20190123163323eucas1p1baaec2c637cdc656ab9b26dbfd455bae References: <20190122170143.5650-2-tiwei.bie@intel.com> Subject: Re: [dpdk-dev] [1/4] net/virtio: fix the control vq 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, 23 Jan 2019 16:33:26 -0000 Hmm. Nevermind. Please, ignore my previous comments to this patch. Patch seems compliant to spec, but the spec is not very clear. Best regards, Ilya Maximets. On 23.01.2019 16:09, Ilya Maximets wrote: > On 22.01.2019 20:01, Tiwei Bie wrote: >> This patch mainly fixed below issues in the packed ring based >> control vq support in virtio driver: >> >> 1. When parsing the used descriptors, we have to track the >> number of descs that we need to skip; >> 2. vq->vq_free_cnt was decreased twice for a same desc; >> >> Meanwhile, make the function name consistent with other parts. >> >> Fixes: ec194c2f1895 ("net/virtio: support packed queue in send command") >> Fixes: a4270ea4ff79 ("net/virtio: check head desc with correct wrap counter") >> >> Signed-off-by: Tiwei Bie >> --- >> drivers/net/virtio/virtio_ethdev.c | 62 ++++++++++++++---------------- >> drivers/net/virtio/virtqueue.h | 12 +----- >> 2 files changed, 31 insertions(+), 43 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >> index ee5a98b7c..a3fe65599 100644 >> --- a/drivers/net/virtio/virtio_ethdev.c >> +++ b/drivers/net/virtio/virtio_ethdev.c >> @@ -142,16 +142,17 @@ static const struct rte_virtio_xstats_name_off rte_virtio_txq_stat_strings[] = { >> struct virtio_hw_internal virtio_hw_internal[RTE_MAX_ETHPORTS]; >> >> static struct virtio_pmd_ctrl * >> -virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl, >> - int *dlen, int pkt_num) >> +virtio_send_command_packed(struct virtnet_ctl *cvq, >> + struct virtio_pmd_ctrl *ctrl, >> + int *dlen, int pkt_num) >> { >> struct virtqueue *vq = cvq->vq; >> int head; >> struct vring_packed_desc *desc = vq->ring_packed.desc_packed; >> struct virtio_pmd_ctrl *result; >> - bool avail_wrap_counter, used_wrap_counter; >> - uint16_t flags; >> + bool avail_wrap_counter; >> int sum = 0; >> + int nb_descs = 0; >> int k; >> >> /* >> @@ -162,11 +163,10 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl, >> */ >> head = vq->vq_avail_idx; >> avail_wrap_counter = vq->avail_wrap_counter; >> - used_wrap_counter = vq->used_wrap_counter; >> - desc[head].flags = VRING_DESC_F_NEXT; >> desc[head].addr = cvq->virtio_net_hdr_mem; >> desc[head].len = sizeof(struct virtio_net_ctrl_hdr); >> vq->vq_free_cnt--; >> + nb_descs++; >> if (++vq->vq_avail_idx >= vq->vq_nentries) { >> vq->vq_avail_idx -= vq->vq_nentries; >> vq->avail_wrap_counter ^= 1; >> @@ -177,55 +177,51 @@ virtio_pq_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl, >> + sizeof(struct virtio_net_ctrl_hdr) >> + sizeof(ctrl->status) + sizeof(uint8_t) * sum; >> desc[vq->vq_avail_idx].len = dlen[k]; >> - flags = VRING_DESC_F_NEXT; > > Looks like barriers was badly placed here before this patch. > Anyway, you need a write barrier here between {addr, len} and flags updates. > >> + desc[vq->vq_avail_idx].flags = VRING_DESC_F_NEXT | >> + VRING_DESC_F_AVAIL(vq->avail_wrap_counter) | >> + VRING_DESC_F_USED(!vq->avail_wrap_counter); >> sum += dlen[k]; >> vq->vq_free_cnt--; >> - flags |= VRING_DESC_F_AVAIL(vq->avail_wrap_counter) | >> - VRING_DESC_F_USED(!vq->avail_wrap_counter); >> - desc[vq->vq_avail_idx].flags = flags; >> - rte_smp_wmb(); >> - vq->vq_free_cnt--; >> + nb_descs++; >> if (++vq->vq_avail_idx >= vq->vq_nentries) { >> vq->vq_avail_idx -= vq->vq_nentries; >> vq->avail_wrap_counter ^= 1; >> } >> } >> >> - >> desc[vq->vq_avail_idx].addr = cvq->virtio_net_hdr_mem >> + sizeof(struct virtio_net_ctrl_hdr); >> desc[vq->vq_avail_idx].len = sizeof(ctrl->status); >> - flags = VRING_DESC_F_WRITE; >> - flags |= VRING_DESC_F_AVAIL(vq->avail_wrap_counter) | >> - VRING_DESC_F_USED(!vq->avail_wrap_counter); >> - desc[vq->vq_avail_idx].flags = flags; >> - flags = VRING_DESC_F_NEXT; >> - flags |= VRING_DESC_F_AVAIL(avail_wrap_counter) | >> - VRING_DESC_F_USED(!avail_wrap_counter); >> - desc[head].flags = flags; >> - rte_smp_wmb(); >> - > > Same here. We need a write barrier to be sure that {addr, len} written > before updating flags. > > Another way to avoid most of barriers is to work similar to > 'flush_shadow_used_ring_packed', > i.e. update all the data in a loop - write barrier - update all the flags. > >> + desc[vq->vq_avail_idx].flags = VRING_DESC_F_WRITE | >> + VRING_DESC_F_AVAIL(vq->avail_wrap_counter) | >> + VRING_DESC_F_USED(!vq->avail_wrap_counter); >> vq->vq_free_cnt--; >> + nb_descs++; >> if (++vq->vq_avail_idx >= vq->vq_nentries) { >> vq->vq_avail_idx -= vq->vq_nentries; >> vq->avail_wrap_counter ^= 1; >> } >> >> + virtio_wmb(vq->hw->weak_barriers); >> + desc[head].flags = VRING_DESC_F_NEXT | >> + VRING_DESC_F_AVAIL(avail_wrap_counter) | >> + VRING_DESC_F_USED(!avail_wrap_counter); >> + >> + virtio_wmb(vq->hw->weak_barriers); >> virtqueue_notify(vq); >> >> /* wait for used descriptors in virtqueue */ >> - do { >> - rte_rmb(); >> + while (!desc_is_used(&desc[head], vq)) >> usleep(100); >> - } while (!__desc_is_used(&desc[head], used_wrap_counter)); >> + >> + virtio_rmb(vq->hw->weak_barriers); >> >> /* now get used descriptors */ >> - while (desc_is_used(&desc[vq->vq_used_cons_idx], vq)) { >> - vq->vq_free_cnt++; >> - if (++vq->vq_used_cons_idx >= vq->vq_nentries) { >> - vq->vq_used_cons_idx -= vq->vq_nentries; >> - vq->used_wrap_counter ^= 1; >> - } >> + vq->vq_free_cnt += nb_descs; >> + vq->vq_used_cons_idx += nb_descs; >> + if (vq->vq_used_cons_idx >= vq->vq_nentries) { >> + vq->vq_used_cons_idx -= vq->vq_nentries; >> + vq->used_wrap_counter ^= 1; >> } >> >> result = cvq->virtio_net_hdr_mz->addr; >> @@ -266,7 +262,7 @@ virtio_send_command(struct virtnet_ctl *cvq, struct virtio_pmd_ctrl *ctrl, >> sizeof(struct virtio_pmd_ctrl)); >> >> if (vtpci_packed_queue(vq->hw)) { >> - result = virtio_pq_send_command(cvq, ctrl, dlen, pkt_num); >> + result = virtio_send_command_packed(cvq, ctrl, dlen, pkt_num); >> goto out_unlock; >> } >> >> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h >> index 7fcde5643..ca9d8e6e3 100644 >> --- a/drivers/net/virtio/virtqueue.h >> +++ b/drivers/net/virtio/virtqueue.h >> @@ -281,7 +281,7 @@ struct virtio_tx_region { >> }; >> >> static inline int >> -__desc_is_used(struct vring_packed_desc *desc, bool wrap_counter) >> +desc_is_used(struct vring_packed_desc *desc, struct virtqueue *vq) >> { >> uint16_t used, avail, flags; >> >> @@ -289,16 +289,9 @@ __desc_is_used(struct vring_packed_desc *desc, bool wrap_counter) >> used = !!(flags & VRING_DESC_F_USED(1)); >> avail = !!(flags & VRING_DESC_F_AVAIL(1)); >> >> - return avail == used && used == wrap_counter; >> + return avail == used && used == vq->used_wrap_counter; >> } >> >> -static inline int >> -desc_is_used(struct vring_packed_desc *desc, struct virtqueue *vq) >> -{ >> - return __desc_is_used(desc, vq->used_wrap_counter); >> -} >> - >> - >> static inline void >> vring_desc_init_packed(struct virtqueue *vq, int n) >> { >> @@ -354,7 +347,6 @@ virtqueue_enable_intr_packed(struct virtqueue *vq) >> { >> uint16_t *event_flags = &vq->ring_packed.driver_event->desc_event_flags; >> >> - >> if (vq->event_flags_shadow == RING_EVENT_FLAGS_DISABLE) { >> virtio_wmb(vq->hw->weak_barriers); >> vq->event_flags_shadow = RING_EVENT_FLAGS_ENABLE; >>