[PATCH 0/2] examples/vhost: rename a CLI option and support multiple socket files

STALE3692d

14 messages, 3 authors, 2016-08-23 · open the first message on its own page

[PATCH 0/2] examples/vhost: rename a CLI option and support multiple socket files

From: Jiayu Hu <hidden>
Date: 2016-08-17 08:06:00

Patch 1: rename the CLI option "dev-basename".
Patch 2: add multiple socket files support. 

Jiayu Hu (2):
  examples/vhost: rename dev-basename
  examples/vhost: support multiple socket files

 examples/vhost/main.c | 81 ++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 54 insertions(+), 27 deletions(-)

-- 
2.7.4

[PATCH 1/2] examples/vhost: rename dev-basename

From: Jiayu Hu <hidden>
Date: 2016-08-17 08:06:01

In examples/vhost, "dev-basename" is a program option, which is to set
the vhost-net socket used by vhost-user, or the character device used
by vhost-cuse. Since vhost-cuse should be dropped, and "dev-basename"
is not a suitable name for the vhost-net socket. Therefore, this patch
is to change this option name for examples/vhost.

Signed-off-by: Jiayu Hu <redacted>
---
 examples/vhost/main.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 92a9823..a718577 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c

@@ -90,9 +90,6 @@
 /* Size of buffers used for snprintfs. */
 #define MAX_PRINT_BUFF 6072
 
-/* Maximum character device basename size. */
-#define MAX_BASENAME_SZ 10
-
 /* Maximum long option length for option parsing. */
 #define MAX_LONG_OPT_SZ 64

@@ -139,8 +136,8 @@ static uint32_t burst_rx_delay_time = BURST_RX_WAIT_US;
 /* Specify the number of retries on RX. */
 static uint32_t burst_rx_retry_num = BURST_RX_RETRIES;
 
-/* Character device basename. Can be set by user. */
-static char dev_basename[MAX_BASENAME_SZ] = "vhost-net";
+/* Socket file path. Can be set by user */
+static char socket_file[PATH_MAX] = "vhost-net";
 
 /* empty vmdq configuration structure. Filled in programatically */
 static struct rte_eth_conf vmdq_conf_default = {

@@ -392,17 +389,17 @@ port_init(uint8_t port)
 }
 
 /*
- * Set character device basename.
+ * Set socket file path.
  */
 static int
-us_vhost_parse_basename(const char *q_arg)
+us_vhost_parse_socket_path(const char *q_arg)
 {
 	/* parse number string */
 
-	if (strnlen(q_arg, MAX_BASENAME_SZ) > MAX_BASENAME_SZ)
+	if (strnlen(q_arg, PATH_MAX) > PATH_MAX)
 		return -1;
 	else
-		snprintf((char*)&dev_basename, MAX_BASENAME_SZ, "%s", q_arg);
+		snprintf((char *)&socket_file, PATH_MAX, "%s", q_arg);
 
 	return 0;
 }

@@ -462,7 +459,7 @@ us_vhost_usage(const char *prgname)
 	RTE_LOG(INFO, VHOST_CONFIG, "%s [EAL options] -- -p PORTMASK\n"
 	"		--vm2vm [0|1|2]\n"
 	"		--rx_retry [0|1] --mergeable [0|1] --stats [0-N]\n"
-	"		--dev-basename <name>\n"
+	"		--socket-file <path>\n"
 	"		--nb-devices ND\n"
 	"		-p PORTMASK: Set mask for ports to be used by application\n"
 	"		--vm2vm [0|1|2]: disable/software(default)/hardware vm2vm comms\n"

@@ -472,7 +469,7 @@ us_vhost_usage(const char *prgname)
 	"		--mergeable [0|1]: disable(default)/enable RX mergeable buffers\n"
 	"		--vlan-strip [0|1]: disable/enable(default) RX VLAN strip on host\n"
 	"		--stats [0-N]: 0: Disable stats, N: Time in seconds to print stats\n"
-	"		--dev-basename: The basename to be used for the character device.\n"
+	"		--socket-file: The path of the socket file.\n"
 	"		--tx-csum [0|1] disable/enable TX checksum offload.\n"
 	"		--tso [0|1] disable/enable TCP segment offload.\n"
 	"		--client register a vhost-user socket as client mode.\n",

@@ -497,7 +494,7 @@ us_vhost_parse_args(int argc, char **argv)
 		{"mergeable", required_argument, NULL, 0},
 		{"vlan-strip", required_argument, NULL, 0},
 		{"stats", required_argument, NULL, 0},
-		{"dev-basename", required_argument, NULL, 0},
+		{"socket-file", required_argument, NULL, 0},
 		{"tx-csum", required_argument, NULL, 0},
 		{"tso", required_argument, NULL, 0},
 		{"client", no_argument, &client_mode, 1},

@@ -638,7 +635,8 @@ us_vhost_parse_args(int argc, char **argv)
 			if (!strncmp(long_option[option_index].name, "stats", MAX_LONG_OPT_SZ)) {
 				ret = parse_num_opt(optarg, INT32_MAX);
 				if (ret == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for stats [0..N]\n");
+					RTE_LOG(INFO, VHOST_CONFIG,
+						"Invalid argument for stats [0..N]\n");
 					us_vhost_usage(prgname);
 					return -1;
 				} else {

@@ -646,10 +644,13 @@ us_vhost_parse_args(int argc, char **argv)
 				}
 			}
 
-			/* Set character device basename. */
-			if (!strncmp(long_option[option_index].name, "dev-basename", MAX_LONG_OPT_SZ)) {
-				if (us_vhost_parse_basename(optarg) == -1) {
-					RTE_LOG(INFO, VHOST_CONFIG, "Invalid argument for character device basename (Max %d characters)\n", MAX_BASENAME_SZ);
+			/* Set socket file path. */
+			if (!strncmp(long_option[option_index].name,
+						"socket-file", MAX_LONG_OPT_SZ)) {
+				if (us_vhost_parse_socket_path(optarg) == -1) {
+					RTE_LOG(INFO, VHOST_CONFIG,
+					"Invalid argument for socket name (Max %d characters)\n",
+					PATH_MAX);
 					us_vhost_usage(prgname);
 					return -1;
 				}

@@ -1345,7 +1346,7 @@ static void
 sigint_handler(__rte_unused int signum)
 {
 	/* Unregister vhost driver. */
-	int ret = rte_vhost_driver_unregister((char *)&dev_basename);
+	int ret = rte_vhost_driver_unregister((char *)&socket_file);
 	if (ret != 0)
 		rte_exit(EXIT_FAILURE, "vhost driver unregister failure.\n");
 	exit(0);

@@ -1509,8 +1510,8 @@ main(int argc, char *argv[])
 	if (client_mode)
 		flags |= RTE_VHOST_USER_CLIENT;
 
-	/* Register vhost(cuse or user) driver to handle vhost messages. */
-	ret = rte_vhost_driver_register(dev_basename, flags);
+	/* Register vhost user driver to handle vhost messages. */
+	ret = rte_vhost_driver_register(socket_file, flags);
 	if (ret != 0)
 		rte_exit(EXIT_FAILURE, "vhost driver register failure.\n");

-- 
2.7.4

[PATCH 2/2] examples/vhost: support multiple socket files

From: Jiayu Hu <hidden>
Date: 2016-08-17 08:06:03

When examples/vhost runs in client mode, only one QEMU can be connected.
This is because that examples/vhost just supports one socket file. This
patch is to add multiple sockets support for examples/vhost.

Signed-off-by: Jiayu Hu <redacted>
---
 examples/vhost/main.c | 50 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index a718577..9974f0b 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c

@@ -136,8 +136,9 @@ static uint32_t burst_rx_delay_time = BURST_RX_WAIT_US;
 /* Specify the number of retries on RX. */
 static uint32_t burst_rx_retry_num = BURST_RX_RETRIES;
 
-/* Socket file path. Can be set by user */
-static char socket_file[PATH_MAX] = "vhost-net";
+/* Socket file paths. Can be set by user */
+static char *socket_files;
+int nb_sockets;
 
 /* empty vmdq configuration structure. Filled in programatically */
 static struct rte_eth_conf vmdq_conf_default = {

@@ -395,11 +396,12 @@ static int
 us_vhost_parse_socket_path(const char *q_arg)
 {
 	/* parse number string */
-
 	if (strnlen(q_arg, PATH_MAX) > PATH_MAX)
 		return -1;
-	else
-		snprintf((char *)&socket_file, PATH_MAX, "%s", q_arg);
+
+	socket_files = realloc(socket_files, PATH_MAX * (nb_sockets + 1));
+	snprintf(socket_files + nb_sockets * PATH_MAX, PATH_MAX, "%s", q_arg);
+	nb_sockets++;
 
 	return 0;
 }

@@ -1341,14 +1343,30 @@ print_stats(void)
 	}
 }
 
+/*
+ * This function is used to unregister drivers.
+ */
+static void
+unregister_drivers(int socket_num)
+{
+	int i, ret;
+
+	for (i = 0; i < socket_num; i++) {
+		ret = rte_vhost_driver_unregister(socket_files + i * PATH_MAX);
+		if (ret != 0)
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"Fail to unregister vhost driver for %s.\n",
+				socket_files + i * PATH_MAX);
+	}
+}
+
 /* When we receive a INT signal, unregister vhost driver */
 static void
 sigint_handler(__rte_unused int signum)
 {
 	/* Unregister vhost driver. */
-	int ret = rte_vhost_driver_unregister((char *)&socket_file);
-	if (ret != 0)
-		rte_exit(EXIT_FAILURE, "vhost driver unregister failure.\n");
+	unregister_drivers(nb_sockets);
+
 	exit(0);
 }

@@ -1412,12 +1430,15 @@ main(int argc, char *argv[])
 {
 	unsigned lcore_id, core_id = 0;
 	unsigned nb_ports, valid_num_ports;
-	int ret;
+	int ret, i;
 	uint8_t portid;
 	static pthread_t tid;
 	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 	uint64_t flags = 0;
 
+	nb_sockets = 0;
+	socket_files = NULL;
+
 	signal(SIGINT, sigint_handler);
 
 	/* init EAL */

@@ -1511,9 +1532,14 @@ main(int argc, char *argv[])
 		flags |= RTE_VHOST_USER_CLIENT;
 
 	/* Register vhost user driver to handle vhost messages. */
-	ret = rte_vhost_driver_register(socket_file, flags);
-	if (ret != 0)
-		rte_exit(EXIT_FAILURE, "vhost driver register failure.\n");
+	for (i = 0; i < nb_sockets; i++) {
+		ret = rte_vhost_driver_register
+				(socket_files + i * PATH_MAX, flags);
+		if (ret != 0) {
+			unregister_drivers(i);
+			rte_exit(EXIT_FAILURE, "vhost driver register failure.\n");
+		}
+	}
 
 	rte_vhost_driver_callback_register(&virtio_net_device_ops);

-- 
2.7.4

Re: [PATCH 2/2] examples/vhost: support multiple socket files

From: Yuanhan Liu <hidden>
Date: 2016-08-18 07:52:52

On Tue, Aug 16, 2016 at 12:14:39PM -0400, Jiayu Hu wrote:

quoted hunk

+/*
+ * This function is used to unregister drivers.
+ */
+static void
+unregister_drivers(int socket_num)
+{

Redundant comment. The function name already explains it well.

quoted hunk

 	/* Register vhost user driver to handle vhost messages. */
-	ret = rte_vhost_driver_register(socket_file, flags);
-	if (ret != 0)
-		rte_exit(EXIT_FAILURE, "vhost driver register failure.\n");
+	for (i = 0; i < nb_sockets; i++) {
+		ret = rte_vhost_driver_register
+				(socket_files + i * PATH_MAX, flags);
+		if (ret != 0) {
+			unregister_drivers(i);
+			rte_exit(EXIT_FAILURE, "vhost driver register failure.\n");

Lines over 80 chars.

Besides, please cc corresponding maintainers while sending patches, say
cc me for virtio/vhost changes. From MAINTAINERS you could find the
names.

So, please make a v2, with above 2 minor fixed. And also, please follow
the guide on http://dpdk.org/dev to send v2:

    If a previous version of the patch has already been sent, a version
    number and changelog annotations are helpful:

    git send-email -1 -v2 --annotate --in-reply-to <Message-ID of the previous patch>
    --to dev@dpdk.org --cc <everybody discussing the patch>


	--yliu

Re: [PATCH 1/2] examples/vhost: rename dev-basename

From: Maxime Coquelin <hidden>
Date: 2016-08-18 08:22:41

Hi Jiayu,

On 08/16/2016 06:14 PM, Jiayu Hu wrote:

quoted hunk

In examples/vhost, "dev-basename" is a program option, which is to set
the vhost-net socket used by vhost-user, or the character device used
by vhost-cuse. Since vhost-cuse should be dropped, and "dev-basename"
is not a suitable name for the vhost-net socket. Therefore, this patch
is to change this option name for examples/vhost.

Signed-off-by: Jiayu Hu <redacted>
---
 examples/vhost/main.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 92a9823..a718577 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c

@@ -90,9 +90,6 @@
 /* Size of buffers used for snprintfs. */
 #define MAX_PRINT_BUFF 6072

-/* Maximum character device basename size. */
-#define MAX_BASENAME_SZ 10
-
 /* Maximum long option length for option parsing. */
 #define MAX_LONG_OPT_SZ 64

@@ -139,8 +136,8 @@ static uint32_t burst_rx_delay_time = BURST_RX_WAIT_US;
 /* Specify the number of retries on RX. */
 static uint32_t burst_rx_retry_num = BURST_RX_RETRIES;

-/* Character device basename. Can be set by user. */
-static char dev_basename[MAX_BASENAME_SZ] = "vhost-net";
+/* Socket file path. Can be set by user */
+static char socket_file[PATH_MAX] = "vhost-net";

Not very important, but now that we only support vhost-user,
maybe we could default the name to "vhost-user"?

There is no real convention I think, but this is what OVS is
used to use in its examples.

Other than that:
Reviewed-by: Maxime Coquelin <redacted>

Thanks,
Maxime

Re: [PATCH 1/2] examples/vhost: rename dev-basename

From: Yuanhan Liu <hidden>
Date: 2016-08-18 08:27:01

On Thu, Aug 18, 2016 at 10:22:38AM +0200, Maxime Coquelin wrote:

Hi Jiayu,

On 08/16/2016 06:14 PM, Jiayu Hu wrote:

quoted

In examples/vhost, "dev-basename" is a program option, which is to set
the vhost-net socket used by vhost-user, or the character device used
by vhost-cuse. Since vhost-cuse should be dropped, and "dev-basename"
is not a suitable name for the vhost-net socket. Therefore, this patch
is to change this option name for examples/vhost.

Signed-off-by: Jiayu Hu <redacted>
---
examples/vhost/main.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 92a9823..a718577 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c

@@ -90,9 +90,6 @@

/* Size of buffers used for snprintfs. */
#define MAX_PRINT_BUFF 6072

-/* Maximum character device basename size. */
-#define MAX_BASENAME_SZ 10
-
/* Maximum long option length for option parsing. */
#define MAX_LONG_OPT_SZ 64

@@ -139,8 +136,8 @@ static uint32_t burst_rx_delay_time = BURST_RX_WAIT_US;

/* Specify the number of retries on RX. */
static uint32_t burst_rx_retry_num = BURST_RX_RETRIES;

-/* Character device basename. Can be set by user. */
-static char dev_basename[MAX_BASENAME_SZ] = "vhost-net";
+/* Socket file path. Can be set by user */
+static char socket_file[PATH_MAX] = "vhost-net";

Not very important, but now that we only support vhost-user,
maybe we could default the name to "vhost-user"?

There is no real convention I think, but this is what OVS is
used to use in its examples.

I think it doesn't matter now, since since the 2nd patch, --socket-path
is a must but not optional any more, meaning there is no default
socket file path.

	--yliu

Other than that:
Reviewed-by: Maxime Coquelin <redacted>

Thanks,
Maxime

Re: [PATCH 2/2] examples/vhost: support multiple socket files

From: Maxime Coquelin <hidden>
Date: 2016-08-18 08:27:58


On 08/16/2016 06:14 PM, Jiayu Hu wrote:

quoted hunk

When examples/vhost runs in client mode, only one QEMU can be connected.
This is because that examples/vhost just supports one socket file. This
patch is to add multiple sockets support for examples/vhost.

Signed-off-by: Jiayu Hu <redacted>
---
 examples/vhost/main.c | 50 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index a718577..9974f0b 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c

@@ -136,8 +136,9 @@ static uint32_t burst_rx_delay_time = BURST_RX_WAIT_US;
 /* Specify the number of retries on RX. */
 static uint32_t burst_rx_retry_num = BURST_RX_RETRIES;

-/* Socket file path. Can be set by user */
-static char socket_file[PATH_MAX] = "vhost-net";

Default name being removed, you can drop my comment on patch 1. :)

quoted hunk

+/* Socket file paths. Can be set by user */
+static char *socket_files;
+int nb_sockets;

Any reason not to make it static?

quoted hunk

 /* empty vmdq configuration structure. Filled in programatically */
 static struct rte_eth_conf vmdq_conf_default = {

@@ -395,11 +396,12 @@ static int
 us_vhost_parse_socket_path(const char *q_arg)
 {
 	/* parse number string */
-
 	if (strnlen(q_arg, PATH_MAX) > PATH_MAX)
 		return -1;
-	else
-		snprintf((char *)&socket_file, PATH_MAX, "%s", q_arg);
+
+	socket_files = realloc(socket_files, PATH_MAX * (nb_sockets + 1));
+	snprintf(socket_files + nb_sockets * PATH_MAX, PATH_MAX, "%s", q_arg);
+	nb_sockets++;

 	return 0;
 }
@@ -1341,14 +1343,30 @@ print_stats(void)
 	}
 }

+/*
+ * This function is used to unregister drivers.
+ */
+static void
+unregister_drivers(int socket_num)
+{
+	int i, ret;
+
+	for (i = 0; i < socket_num; i++) {
+		ret = rte_vhost_driver_unregister(socket_files + i * PATH_MAX);
+		if (ret != 0)
+			RTE_LOG(ERR, VHOST_CONFIG,
+				"Fail to unregister vhost driver for %s.\n",
+				socket_files + i * PATH_MAX);
+	}
+}
+
 /* When we receive a INT signal, unregister vhost driver */
 static void
 sigint_handler(__rte_unused int signum)
 {
 	/* Unregister vhost driver. */
-	int ret = rte_vhost_driver_unregister((char *)&socket_file);
-	if (ret != 0)
-		rte_exit(EXIT_FAILURE, "vhost driver unregister failure.\n");
+	unregister_drivers(nb_sockets);
+
 	exit(0);
 }
@@ -1412,12 +1430,15 @@ main(int argc, char *argv[])
 {
 	unsigned lcore_id, core_id = 0;
 	unsigned nb_ports, valid_num_ports;
-	int ret;
+	int ret, i;
 	uint8_t portid;
 	static pthread_t tid;
 	char thread_name[RTE_MAX_THREAD_NAME_LEN];
 	uint64_t flags = 0;

+	nb_sockets = 0;
+	socket_files = NULL;

Since socket_files is static, no need to initialize it to NULL.
If you staticize nb_sockets, same remark will apply.

Re: [PATCH 1/2] examples/vhost: rename dev-basename

From: Maxime Coquelin <hidden>
Date: 2016-08-18 08:29:15


On 08/18/2016 10:35 AM, Yuanhan Liu wrote:

On Thu, Aug 18, 2016 at 10:22:38AM +0200, Maxime Coquelin wrote:

quoted

Hi Jiayu,

On 08/16/2016 06:14 PM, Jiayu Hu wrote:

quoted

In examples/vhost, "dev-basename" is a program option, which is to set
the vhost-net socket used by vhost-user, or the character device used
by vhost-cuse. Since vhost-cuse should be dropped, and "dev-basename"
is not a suitable name for the vhost-net socket. Therefore, this patch
is to change this option name for examples/vhost.

Signed-off-by: Jiayu Hu <redacted>
---
examples/vhost/main.c | 41 +++++++++++++++++++++--------------------
1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index 92a9823..a718577 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c

@@ -90,9 +90,6 @@

/* Size of buffers used for snprintfs. */
#define MAX_PRINT_BUFF 6072

-/* Maximum character device basename size. */
-#define MAX_BASENAME_SZ 10
-
/* Maximum long option length for option parsing. */
#define MAX_LONG_OPT_SZ 64

@@ -139,8 +136,8 @@ static uint32_t burst_rx_delay_time = BURST_RX_WAIT_US;

/* Specify the number of retries on RX. */
static uint32_t burst_rx_retry_num = BURST_RX_RETRIES;

-/* Character device basename. Can be set by user. */
-static char dev_basename[MAX_BASENAME_SZ] = "vhost-net";
+/* Socket file path. Can be set by user */
+static char socket_file[PATH_MAX] = "vhost-net";

Not very important, but now that we only support vhost-user,
maybe we could default the name to "vhost-user"?

There is no real convention I think, but this is what OVS is
used to use in its examples.

I think it doesn't matter now, since since the 2nd patch, --socket-path
is a must but not optional any more, meaning there is no default
socket file path.

Yes, just noticed it while reviewing patch 2.
So this is all good to me for this patch.

Thanks,
Maxime

Re: [PATCH 2/2] examples/vhost: support multiple socket files

From: Yuanhan Liu <hidden>
Date: 2016-08-18 08:34:23

On Thu, Aug 18, 2016 at 10:27:55AM +0200, Maxime Coquelin wrote:


On 08/16/2016 06:14 PM, Jiayu Hu wrote:

quoted

When examples/vhost runs in client mode, only one QEMU can be connected.
This is because that examples/vhost just supports one socket file. This
patch is to add multiple sockets support for examples/vhost.

Signed-off-by: Jiayu Hu <redacted>
---
examples/vhost/main.c | 50 ++++++++++++++++++++++++++++++++++++++------------
1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/examples/vhost/main.c b/examples/vhost/main.c
index a718577..9974f0b 100644
--- a/examples/vhost/main.c
+++ b/examples/vhost/main.c

@@ -136,8 +136,9 @@ static uint32_t burst_rx_delay_time = BURST_RX_WAIT_US;

/* Specify the number of retries on RX. */
static uint32_t burst_rx_retry_num = BURST_RX_RETRIES;

-/* Socket file path. Can be set by user */
-static char socket_file[PATH_MAX] = "vhost-net";

Default name being removed, you can drop my comment on patch 1. :)

quoted

+/* Socket file paths. Can be set by user */
+static char *socket_files;
+int nb_sockets;

Any reason not to make it static?

Right, it should be "static". Hmm, I missed it in review :(

	--yliu