Re: [dpdk-dev] [PATCH v3 2/7] net/virtio-user: add vectorized packed ring parameter

["Liu, Yong" <yong.liu@intel.com>]

> -----Original Message-----
> From: Ye, Xiaolong <xiaolong.ye@intel.com>
> Sent: Wednesday, April 8, 2020 2:23 PM
> To: Liu, Yong <yong.liu@intel.com>
> Cc: maxime.coquelin@redhat.com; Wang, Zhihong
> <zhihong.wang@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>; dev@dpdk.org
> Subject: Re: [PATCH v3 2/7] net/virtio-user: add vectorized packed ring
> parameter
> 
> On 04/08, Marvin Liu wrote:
> >Add new parameter "packed_vec" which can disable vectorized packed
> ring
> >datapath explicitly. When "packed_vec" option is on, driver will check
> >packed ring vectorized datapath prerequisites. If any one of them not
> >matched, vectorized datapath won't be selected.
> >
> >Signed-off-by: Marvin Liu <yong.liu@intel.com>
> >
> >diff --git a/drivers/net/virtio/virtio_pci.h b/drivers/net/virtio/virtio_pci.h
> >index 7433d2f08..8103b7a18 100644
> >--- a/drivers/net/virtio/virtio_pci.h
> >+++ b/drivers/net/virtio/virtio_pci.h
> >@@ -251,6 +251,8 @@ struct virtio_hw {
> > 	uint8_t	    use_msix;
> > 	uint8_t     modern;
> > 	uint8_t     use_simple_rx;
> >+	uint8_t     packed_vec_rx;
> >+	uint8_t     packed_vec_tx;
> > 	uint8_t     use_inorder_rx;
> > 	uint8_t     use_inorder_tx;
> > 	uint8_t     weak_barriers;
> >diff --git a/drivers/net/virtio/virtio_user_ethdev.c
> b/drivers/net/virtio/virtio_user_ethdev.c
> >index e61af4068..399ac5511 100644
> >--- a/drivers/net/virtio/virtio_user_ethdev.c
> >+++ b/drivers/net/virtio/virtio_user_ethdev.c
> >@@ -450,6 +450,8 @@ static const char *valid_args[] = {
> > 	VIRTIO_USER_ARG_IN_ORDER,
> > #define VIRTIO_USER_ARG_PACKED_VQ      "packed_vq"
> > 	VIRTIO_USER_ARG_PACKED_VQ,
> >+#define VIRTIO_USER_ARG_PACKED_VEC     "packed_vec"
> >+	VIRTIO_USER_ARG_PACKED_VEC,
> > 	NULL
> > };
> >
> >@@ -552,6 +554,8 @@ virtio_user_pmd_probe(struct rte_vdev_device
> *dev)
> > 	uint64_t mrg_rxbuf = 1;
> > 	uint64_t in_order = 1;
> > 	uint64_t packed_vq = 0;
> >+	uint64_t packed_vec = 0;
> >+
> > 	char *path = NULL;
> > 	char *ifname = NULL;
> > 	char *mac_addr = NULL;
> >@@ -668,6 +672,15 @@ virtio_user_pmd_probe(struct rte_vdev_device
> *dev)
> > 		}
> > 	}
> >
> >+	if (rte_kvargs_count(kvlist, VIRTIO_USER_ARG_PACKED_VEC) == 1) {
> >+		if (rte_kvargs_process(kvlist,
> VIRTIO_USER_ARG_PACKED_VEC,
> >+				       &get_integer_arg, &packed_vec) < 0) {
> >+			PMD_INIT_LOG(ERR, "error to parse %s",
> >+				     VIRTIO_USER_ARG_PACKED_VQ);
> >+			goto end;
> >+		}
> >+	}
> >+
> > 	if (queues > 1 && cq == 0) {
> > 		PMD_INIT_LOG(ERR, "multi-q requires ctrl-q");
> > 		goto end;
> >@@ -705,6 +718,17 @@ virtio_user_pmd_probe(struct rte_vdev_device
> *dev)
> > 	}
> >
> > 	hw = eth_dev->data->dev_private;
> >+#if defined(RTE_ARCH_X86) && defined(CC_AVX512_SUPPORT)
> >+	if (packed_vec) {
> >+		hw->packed_vec_rx = 1;
> >+		hw->packed_vec_tx = 1;
> >+	}
> >+#else
> >+	if (packed_vec)
> >+		PMD_INIT_LOG(ERR, "building environment not match
> vectorized "
> >+				  "packed ring datapath requirement");
> 
> Minor nit:
> 
> s/not match/doesn't match/
> 
> And better to avoid breaking error message strings across multiple source
> lines.
> It makes it harder to use tools like grep to find errors in source.
> E.g. user uses "vectorized packed ring datapath" to grep the code.
> 
> Thanks,
> Xiaolong
> 

Thanks for remind. Will change in next release.

> >+#endif
> >+
> > 	if (virtio_user_dev_init(hw->virtio_user_dev, path, queues, cq,
> > 			 queue_size, mac_addr, &ifname, server_mode,
> > 			 mrg_rxbuf, in_order, packed_vq) < 0) {
> >@@ -777,4 +801,5 @@
> RTE_PMD_REGISTER_PARAM_STRING(net_virtio_user,
> > 	"server=<0|1> "
> > 	"mrg_rxbuf=<0|1> "
> > 	"in_order=<0|1> "
> >-	"packed_vq=<0|1>");
> >+	"packed_vq=<0|1>"
> >+	"packed_vec=<0|1>");
> >--
> >2.17.1
> >