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 519471B444 for ; Fri, 14 Dec 2018 18:23:33 +0100 (CET) Received: from eucas1p2.samsung.com (unknown [182.198.249.207]) by mailout1.w1.samsung.com (KnoxPortal) with ESMTP id 20181214172332euoutp0179233be7f10650f57d99fa293e0d92d1~wQuJjIVkT2116021160euoutp01C for ; Fri, 14 Dec 2018 17:23:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20181214172332euoutp0179233be7f10650f57d99fa293e0d92d1~wQuJjIVkT2116021160euoutp01C DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=samsung.com; s=mail20170921; t=1544808212; bh=bjvB0iEXc/KysshJ/kEY5C+EOeerkaHdUwnYY3bOJlk=; h=Subject:To:Cc:From:Date:In-Reply-To:References:From; b=OZ/CWI110TOI4iPVFkfV+RB6IfxHFJUcSuxbB8UgPDEJJo/Ofxk6kHFEIfLmO9x5i Bs7UzXeH+Y9KN90e63VpCTJFi+HhtKL9GfLYJ3uqNj+SdA05mXNaiph5vSOpn0II6K kW9pFBFiIjYE9A2GWn7/A6CSX7U8qa3jAsJsEDu8= Received: from eusmges3new.samsung.com (unknown [203.254.199.245]) by eucas1p1.samsung.com (KnoxPortal) with ESMTP id 20181214172326eucas1p1e2aefab63dc9f20e2ec704b8c8090b8d~wQuEGxrwE2152921529eucas1p16; Fri, 14 Dec 2018 17:23:26 +0000 (GMT) Received: from eucas1p2.samsung.com ( [182.198.249.207]) by eusmges3new.samsung.com (EUCPMTA) with SMTP id 45.C7.04806.E07E31C5; Fri, 14 Dec 2018 17:23:26 +0000 (GMT) Received: from eusmtrp2.samsung.com (unknown [182.198.249.139]) by eucas1p1.samsung.com (KnoxPortal) with ESMTPA id 20181214172325eucas1p1268c9696d9da27fac93439551c2dca56~wQuDI86us2741927419eucas1p1_; Fri, 14 Dec 2018 17:23:25 +0000 (GMT) Received: from eusmgms2.samsung.com (unknown [182.198.249.180]) by eusmtrp2.samsung.com (KnoxPortal) with ESMTP id 20181214172325eusmtrp2d72f41db138c4b0db236010ad8cc0bf8~wQuC6IItX3230632306eusmtrp2R; Fri, 14 Dec 2018 17:23:25 +0000 (GMT) X-AuditID: cbfec7f5-367ff700000012c6-ed-5c13e70e593f Received: from eusmtip2.samsung.com ( [203.254.199.222]) by eusmgms2.samsung.com (EUCPMTA) with SMTP id 03.00.04128.D07E31C5; Fri, 14 Dec 2018 17:23:25 +0000 (GMT) Received: from [106.109.129.180] (unknown [106.109.129.180]) by eusmtip2.samsung.com (KnoxPortal) with ESMTPA id 20181214172324eusmtip27964c82a3d8565af65e910187f9505b2~wQuCOFmHb2120321203eusmtip2P; Fri, 14 Dec 2018 17:23:24 +0000 (GMT) To: "Michael S. Tsirkin" Cc: dev@dpdk.org, Maxime Coquelin , Xiao Wang , Tiwei Bie , Zhihong Wang , jfreimann@redhat.com, Jason Wang , xiaolong.ye@intel.com, alejandro.lucero@netronome.com, Yongseok Koh From: Ilya Maximets Message-ID: <5618717e-3e92-b760-a7d5-9b9edde29b8e@samsung.com> Date: Fri, 14 Dec 2018 20:23:23 +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: <20181214120038-mutt-send-email-mst@kernel.org> Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA01SbUhTYRjtvfduu8omr7PwyQRj2A/Dr6zgGmaFQSvBrB8RJdQ1b8vapm1q WkQqpWYi5g+XFiiImq4Qzc+0aVOsVAzFz0JN0fyYGk0NK525XSX/neec57znOfDSpPSTwIWO VMdwGjWrlAntqZq235+9HKacwnxHf9JMl7mYYBbMtQTTm/pbxBT3LBJM16siiml73Egx639m BUx14jrBNE21iJjVucNMb76BZN50ZxPHxfI/BUUCeWHjDCH/rmsh5LonY6T8h6FPKM+sKkOh wkv2ARGcMjKO0/gEXrW/Ye5sF0Q3BMaP59SLElG3XzqyowEfgjGjHqUje1qKXyKo7KsU8MMS goykHoIfFhHUlOcKtyyjpiSBFUtxCYKPrVH80k8Ea5MGZBWc8FkwplSLrHgn9oDJLybbSyRu IUD/OtkmCLEntOtbbQYJDoRfSc9sCRTeB3NJJsqKd+GLkDaqF/I7jvApd8LG2+EAGBwx2HgS O0PyUqmAx25QO/+CtIYBHhHB0PiwiD/7JGT1f0U8doLZD1WbvCus1+cTPH4Aow9nEG9OQ6Az WjaFY1Bl6tow0BsJHlD+1oenT4Clo5+y0oAdYHDekb/BAbJrdCRPSyAtRcpvu8Pf9yUkj11g aGFRlIVkedua5W1rk7etTd7/3AJElSFnLlarUnDag2rujreWVWlj1Qrva1GqSrTxwTosH5br kGE13IgwjWRiie+gU5hUwMZpE1RGBDQp2ym58GiDkkSwCXc5TdQVTayS0xrRHpqSOUvu7fh2 WYoVbAx3i+OiOc2WStB2LoloaDYrPDI4uFSJ35nqdIWl7MB5NieScM04EoNPea40eCl3q7vM 45lu+jV395C96ozwKEwF1FXM3favFFuE12/i56VPO4fvNwd5505Pet5lglbcTGfi546mNte2 nwvoictclle1VQyIxVNKxV//lezT7t6h003trhUT/SG/Vv1klPYGe2A/qdGy/wBLI2cEXAMA AA== X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFrrLIsWRmVeSWpSXmKPExsVy+t/xe7q8z4VjDJb8ZrM492kZk8W7T9uZ LK60/2S3WHbpM5PFuTVLWSyOde5hsfj/6xWrxdaG/0wW+58fZrf488bU4sr8fcwWmy9OYnLg 8fi1YCmrx+I9L5k8nk0/zOQxvfshs8f7fVfZPPq2rGIMYIvSsynKLy1JVcjILy6xVYo2tDDS M7S00DMysdQzNDaPtTIyVdK3s0lJzcksSy3St0vQy/h05hRrwW67ikfTdrI3MF406mLk5JAQ MJG4/7qRtYuRi0NIYCmjxKbV85ghElISP35dYIWwhSX+XOtigyh6zyhx6H43WEJYwFfiyb5H 7CC2iICmxNNbr5lAipgFDjNJLPiylhGi4wCjRPON62AdbAI6EqdWH2EEsXkF7CS+Nc5gA7FZ BFQl3jS+ZgGxRQUiJM6+XAdVIyhxcuYTsDingI3EjXv7wOqZBdQl/sy7xAxhi0s0fVnJCmHL S2x/O4d5AqPQLCTts5C0zELSMgtJywJGllWMIqmlxbnpucVGesWJucWleel6yfm5mxiBUbvt 2M8tOxi73gUfYhTgYFTi4TW4IRwjxJpYVlyZe4hRgoNZSYQ3rBUoxJuSWFmVWpQfX1Sak1p8 iNEU6LmJzFKiyfnAhJJXEm9oamhuYWlobmxubGahJM573qAySkggPbEkNTs1tSC1CKaPiYNT qoExJFrCSSTluNKH8+KRDz75/uKeF7OS+fjp3W96QzXqq62WGEuWzGn4UfxUVlVcaW1tw8rt F7amti93W3su43p5bLJyncisjJ9S2TeuPtnx3fmsv2AU17SM+ZPND0SH98a1ulce+CTvmWS7 WbVXgHWby30v9iX1TQ9NZ60I/ewnJmnKHdHynFuJpTgj0VCLuag4EQCIoKWx8AIAAA== X-CMS-MailID: 20181214172325eucas1p1268c9696d9da27fac93439551c2dca56 X-Msg-Generator: CA Content-Type: text/plain; charset="utf-8" X-RootMTR: 20181214153817eucas1p19a41cdd791879252e1f3a5d77c427845 X-EPHeader: CA CMS-TYPE: 201P X-CMS-RootMailID: 20181214153817eucas1p19a41cdd791879252e1f3a5d77c427845 References: <20181214153812.3878-1-i.maximets@samsung.com> <20181214120038-mutt-send-email-mst@kernel.org> Subject: Re: [dpdk-dev] [PATCH] 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: Fri, 14 Dec 2018 17:23:33 -0000 On 14.12.2018 20:00, Michael S. Tsirkin wrote: > On Fri, Dec 14, 2018 at 06:38:12PM +0300, Ilya Maximets wrote: >> 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 >> --- >> >> Based on "[RFC] net/virtio: use real barriers for vDPA". >> >> RFC --> PATCH: >> * 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://www.mail-archive.com/virtio-dev@lists.oasis-open.org/msg04114.html >> >> 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 | 14 ++++++------ >> drivers/net/virtio/virtqueue.h | 35 +++++++++++++++++++++++++----- >> 5 files changed, 48 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/virtio/virtio_ethdev.c b/drivers/net/virtio/virtio_ethdev.c >> index cb2b2e0bf..5ae7a9650 100644 >> --- a/drivers/net/virtio/virtio_ethdev.c >> +++ b/drivers/net/virtio/virtio_ethdev.c >> @@ -1474,6 +1474,8 @@ virtio_init_device(struct rte_eth_dev *eth_dev, uint64_t req_features) >> if (virtio_negotiate_features(hw, req_features) < 0) >> return -1; >> >> + hw->weak_barriers = !vtpci_with_feature(hw, VIRTIO_F_ORDER_PLATFORM); >> + >> if (!hw->virtio_user_dev) { >> pci_dev = RTE_ETH_DEV_TO_PCI(eth_dev); >> rte_eth_copy_pci_info(eth_dev, pci_dev); >> diff --git a/drivers/net/virtio/virtio_ethdev.h b/drivers/net/virtio/virtio_ethdev.h >> index e0f80e5a4..c098e5ac0 100644 >> --- a/drivers/net/virtio/virtio_ethdev.h >> +++ b/drivers/net/virtio/virtio_ethdev.h >> @@ -34,7 +34,8 @@ >> 1u << VIRTIO_RING_F_INDIRECT_DESC | \ >> 1ULL << VIRTIO_F_VERSION_1 | \ >> 1ULL << VIRTIO_F_IN_ORDER | \ >> - 1ULL << VIRTIO_F_IOMMU_PLATFORM) >> + 1ULL << VIRTIO_F_IOMMU_PLATFORM | \ >> + 1ULL << VIRTIO_F_ORDER_PLATFORM) >> >> #define VIRTIO_PMD_SUPPORTED_GUEST_FEATURES \ >> (VIRTIO_PMD_DEFAULT_GUEST_FEATURES | \ >> diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h >> index e961a58ca..e2f096185 100644 >> --- a/drivers/net/virtio/virtio_pci.h >> +++ b/drivers/net/virtio/virtio_pci.h >> @@ -128,6 +128,12 @@ struct virtnet_ctl; >> */ >> #define VIRTIO_F_IN_ORDER 35 >> >> +/* >> + * This feature indicates that memory accesses by the driver and the device >> + * are ordered in a way described by the platform. >> + */ >> +#define VIRTIO_F_ORDER_PLATFORM 36 >> + >> /* The Guest publishes the used index for which it expects an interrupt >> * at the end of the avail ring. Host should ignore the avail->flags field. */ >> /* The Host publishes the avail index for which it expects a kick >> @@ -240,6 +246,7 @@ struct virtio_hw { >> uint8_t use_simple_rx; >> uint8_t use_inorder_rx; >> uint8_t use_inorder_tx; >> + uint8_t weak_barriers; >> bool has_tx_offload; >> bool has_rx_offload; >> uint16_t port_id; >> diff --git a/drivers/net/virtio/virtio_rxtx.c b/drivers/net/virtio/virtio_rxtx.c >> index cb8f89f18..66195bf47 100644 >> --- a/drivers/net/virtio/virtio_rxtx.c >> +++ b/drivers/net/virtio/virtio_rxtx.c >> @@ -906,7 +906,7 @@ virtio_recv_pkts(void *rx_queue, struct rte_mbuf **rx_pkts, uint16_t nb_pkts) >> >> nb_used = VIRTQUEUE_NUSED(vq); >> >> - virtio_rmb(); >> + virtio_rmb(hw->weak_barriers); >> >> num = likely(nb_used <= nb_pkts) ? nb_used : nb_pkts; >> if (unlikely(num > VIRTIO_MBUF_BURST_SZ)) >> @@ -1017,7 +1017,7 @@ virtio_recv_mergeable_pkts_inorder(void *rx_queue, >> nb_used = RTE_MIN(nb_used, nb_pkts); >> nb_used = RTE_MIN(nb_used, VIRTIO_MBUF_BURST_SZ); >> >> - virtio_rmb(); >> + virtio_rmb(hw->weak_barriers); >> >> PMD_RX_LOG(DEBUG, "used:%d", nb_used); >> >> @@ -1202,7 +1202,7 @@ virtio_recv_mergeable_pkts(void *rx_queue, >> >> nb_used = VIRTQUEUE_NUSED(vq); >> >> - virtio_rmb(); >> + virtio_rmb(hw->weak_barriers); >> >> PMD_RX_LOG(DEBUG, "used:%d", nb_used); >> >> @@ -1365,7 +1365,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts) >> PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts); >> nb_used = VIRTQUEUE_NUSED(vq); >> >> - virtio_rmb(); >> + virtio_rmb(hw->weak_barriers); >> if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh)) >> virtio_xmit_cleanup(vq, nb_used); >> >> @@ -1407,7 +1407,7 @@ virtio_xmit_pkts(void *tx_queue, struct rte_mbuf **tx_pkts, uint16_t nb_pkts) >> /* Positive value indicates it need free vring descriptors */ >> if (unlikely(need > 0)) { >> nb_used = VIRTQUEUE_NUSED(vq); >> - virtio_rmb(); >> + virtio_rmb(hw->weak_barriers); >> need = RTE_MIN(need, (int)nb_used); >> >> virtio_xmit_cleanup(vq, need); >> @@ -1463,7 +1463,7 @@ virtio_xmit_pkts_inorder(void *tx_queue, >> PMD_TX_LOG(DEBUG, "%d packets to xmit", nb_pkts); >> nb_used = VIRTQUEUE_NUSED(vq); >> >> - virtio_rmb(); >> + virtio_rmb(hw->weak_barriers); >> if (likely(nb_used > vq->vq_nentries - vq->vq_free_thresh)) >> virtio_xmit_cleanup_inorder(vq, nb_used); >> >> @@ -1511,7 +1511,7 @@ virtio_xmit_pkts_inorder(void *tx_queue, >> need = slots - vq->vq_free_cnt; >> if (unlikely(need > 0)) { >> nb_used = VIRTQUEUE_NUSED(vq); >> - virtio_rmb(); >> + virtio_rmb(hw->weak_barriers); >> need = RTE_MIN(need, (int)nb_used); >> >> virtio_xmit_cleanup_inorder(vq, need); >> diff --git a/drivers/net/virtio/virtqueue.h b/drivers/net/virtio/virtqueue.h >> index 26518ed98..6b9055a1f 100644 >> --- a/drivers/net/virtio/virtqueue.h >> +++ b/drivers/net/virtio/virtqueue.h >> @@ -19,15 +19,40 @@ >> struct rte_mbuf; >> >> /* >> - * 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(); >> +} >> + > > Why doesn't rte_cio_rmb exit? Assuming your question was "Why doesn't rte_cio_mb exist?". I guess 'cio' barriers was copied from 'dma_' barriers from kernel. And 'rte_cio_mb' does not exist because there is no 'dma_mb' in kernel. OTOH, maybe we can use 'rte_io_mb' here to be more consistent, but it equals to the 'rte_mb' on all supported architectures. So, I'm not sure if it's needed. > >> +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(); >> +} >> >> #ifdef RTE_PMD_PACKET_PREFETCH >> #define rte_packet_prefetch(p) rte_prefetch1(p) >> @@ -312,7 +337,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; >> } >> >> -- >> 2.17.1 > >