diff mbox series

child process synchronization NIC startup parameters

Message ID 20230705093515.3285574-1-kaisenx.you@intel.com (mailing list archive)
State New
Delegated to: Thomas Monjalon
Headers
Series child process synchronization NIC startup parameters |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/loongarch-compilation success Compilation OK
ci/loongarch-unit-testing success Unit Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/github-robot: build success github build: passed
ci/intel-Functional success Functional PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-abi-testing success Testing PASS
ci/iol-aarch-unit-testing success Testing PASS
ci/iol-unit-testing success Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS

Commit Message

Kaisen You July 5, 2023, 9:35 a.m. UTC 
  In meson_test, because the child process does not synchronize
the NIC startup parameters of the parent process at startup,
it uses all NICs bound by vfio as startup parameters by default,
and an exception occurs in the subsequent hugefile check,
causing the test to fail. Synchronize the NIC startup parameters
of the parent process to the child process to solve this problem.

Fixes: 786b29255c49 ("test: fix file prefix discovery")
Cc: stable@dpdk.org

Signed-off-by: Kaisen You <kaisenx.you@intel.com>
---
 app/test/process.h | 80 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 77 insertions(+), 3 deletions(-)

Comments

Qiming Yang July 6, 2023, 2:16 a.m. UTC | #1 
Hi, kaisen
Your patch missed prefix app/test.

> -----Original Message-----
> From: You, KaisenX <kaisenx.you@intel.com>
> Sent: Wednesday, July 5, 2023 5:35 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> <yidingx.zhou@intel.com>; You, KaisenX <kaisenx.you@intel.com>;
> stable@dpdk.org
> Subject: [PATCH] child process synchronization NIC startup parameters
> 
> In meson_test, because the child process does not synchronize the NIC
> startup parameters of the parent process at startup, it uses all NICs bound by
> vfio as startup parameters by default, and an exception occurs in the
> subsequent hugefile check, causing the test to fail. Synchronize the NIC
> startup parameters of the parent process to the child process to solve this
> problem.
> 
> Fixes: 786b29255c49 ("test: fix file prefix discovery")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> ---
>  app/test/process.h | 80
> ++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 77 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test/process.h b/app/test/process.h index
> 1f073b9c5c..c8a15e8989 100644
> --- a/app/test/process.h
> +++ b/app/test/process.h
> @@ -15,9 +15,11 @@
>  #include <string.h> /* strerror */
>  #include <unistd.h> /* readlink */
>  #include <dirent.h>
> +#include <fcntl.h>
> 
>  #include <rte_string_fns.h> /* strlcpy */
> 
> +#define MAX_EXTRA_ARGS 32
>  #ifdef RTE_EXEC_ENV_FREEBSD
>  #define self "curproc"
>  #define exe "file"
> @@ -34,6 +36,50 @@ extern uint16_t flag_for_send_pkts;  #endif  #endif
> 
> +/* get cmdline form PID. Read process info form /proc/$PID. */ static
> +char *get_cmdline_from_pid(pid_t pid, char *buf, int len) {
> +	char filename[PATH_MAX];
> +	char *name = NULL;
> +	int fd;
> +	int ret;
> +
> +	if (pid < 1 || buf == NULL || len < 0) {
> +		printf("%s: illegal param\n", __func__);
> +		return NULL;
> +	}
> +
> +	snprintf(filename, PATH_MAX, "/proc/%d/cmdline", pid);
> +	memset(buf, 0, len);
> +	fd = open(filename, O_RDONLY);
> +	if (fd < 0) {
> +		perror("open:");
> +		return NULL;
> +	}
> +	ret = read(fd, buf, len);
> +	close(fd);
> +
> +	if (ret < 0)
> +		return NULL;
> +
> +	if (buf[ret-1] == '\n')
> +		buf[--ret] = 0;
> +
> +	name = buf;
> +	while (ret) {
> +		if (((unsigned char)*name) < ' ')
> +			*name = ' ';
> +		name++;
> +		ret--;
> +	}
> +	*name = 0;
> +
> +	if (buf[0])
> +		return buf;
> +
> +	return NULL;
> +}
> +
>  /*
>   * launches a second copy of the test process using the given argv
> parameters,
>   * which should include argv[0] as the process name. To identify in the @@ -
> 44,9 +90,15 @@ static inline int  process_dup(const char *const argv[], int
> numargs, const char *env_value)  {
>  	int num;
> -	char *argv_cpy[numargs + 1];
> -	int i, status;
> +	char *argv_cpy[MAX_EXTRA_ARGS];
> +	int i, status, n, s, j;
>  	char path[32];
> +	char buf[1024];
> +	char *token;
> +	char str_1[] = "-a";
> +	char str_2[] = " ";
> +	char *argv_str[MAX_EXTRA_ARGS];
> +	char *argv_copy[MAX_EXTRA_ARGS];
>  #ifdef RTE_LIB_PDUMP
>  #ifdef RTE_NET_RING
>  	pthread_t thread;
> @@ -113,10 +165,32 @@ process_dup(const char *const argv[], int numargs,
> const char *env_value)
>  			closedir(dir);
>  		}
>  #endif
> +		/* Add the -a parameter to the child process start parameter
> */
> +		get_cmdline_from_pid(getppid(), buf, 1024);
> +		token = strtok(buf, str_2);
> +		argv_str[0] = strdup(token);
> +		n = 0;
> +		j = 0;
> +		while (token != NULL) {
> +			n = n + 1;
> +			argv_str[n] = strdup(token);
> +			token = strtok(NULL, str_2);
> +		}
> +		for (s = 0; s < n; s++) {
> +			if (strcmp(argv_str[s], str_1) == 0 ||
> +				strcmp(argv_str[s + 1], str_1) == 0) {
> +				argv_copy[j] = strdup(argv_str[s + 1]);
> +				j++;
> +			}
> +		}
> +		for (s = 0; s < j; s++)
> +			argv_cpy[numargs + s] = strdup(argv_copy[s]);
> +
>  		printf("Running binary with argv[]:");
> -		for (i = 0; i < num; i++)
> +		for (i = 0; i < num + j; i++)
>  			printf("'%s' ", argv_cpy[i]);
>  		printf("\n");
> +		argv_cpy[numargs + j] = NULL;
>  		fflush(stdout);
> 
>  		/* set the environment variable */
> --
> 2.25.1
Stephen Hemminger July 6, 2023, 7:07 p.m. UTC | #2 
On Wed,  5 Jul 2023 17:35:14 +0800
Kaisen You <kaisenx.you@intel.com> wrote:

> In meson_test, because the child process does not synchronize
> the NIC startup parameters of the parent process at startup,
> it uses all NICs bound by vfio as startup parameters by default,
> and an exception occurs in the subsequent hugefile check,
> causing the test to fail. Synchronize the NIC startup parameters
> of the parent process to the child process to solve this problem.
> 
> Fixes: 786b29255c49 ("test: fix file prefix discovery")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Kaisen You <kaisenx.you@intel.com>

Not portable outside of Linux.
Seems really bug prone to have test diving into /proc to find
command line of parent. Couldn't this be passed one of many other
ways into child?
Kaisen You July 7, 2023, 1:21 a.m. UTC | #3 
> -----Original Message-----
> From: Yang, Qiming <qiming.yang@intel.com>
> Sent: 2023年7月6日 10:17
> To: You, KaisenX <kaisenx.you@intel.com>; dev@dpdk.org
> Cc: Zhou, YidingX <yidingx.zhou@intel.com>; stable@dpdk.org
> Subject: RE: [PATCH] child process synchronization NIC startup parameters
> 
> Hi, kaisen
> Your patch missed prefix app/test.

Thank you for your reply, I will send v2 to fix this problem as soon as possible.
> 
> > -----Original Message-----
> > From: You, KaisenX <kaisenx.you@intel.com>
> > Sent: Wednesday, July 5, 2023 5:35 PM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Zhou, YidingX
> > <yidingx.zhou@intel.com>; You, KaisenX <kaisenx.you@intel.com>;
> > stable@dpdk.org
> > Subject: [PATCH] child process synchronization NIC startup parameters
> >
> > In meson_test, because the child process does not synchronize the NIC
> > startup parameters of the parent process at startup, it uses all NICs
> > bound by vfio as startup parameters by default, and an exception
> > occurs in the subsequent hugefile check, causing the test to fail.
> > Synchronize the NIC startup parameters of the parent process to the
> > child process to solve this problem.
> >
> > Fixes: 786b29255c49 ("test: fix file prefix discovery")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Kaisen You <kaisenx.you@intel.com>
> > ---
> >  app/test/process.h | 80
> > ++++++++++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 77 insertions(+), 3 deletions(-)
> >
> > diff --git a/app/test/process.h b/app/test/process.h index
> > 1f073b9c5c..c8a15e8989 100644
> > --- a/app/test/process.h
> > +++ b/app/test/process.h
> > @@ -15,9 +15,11 @@
> >  #include <string.h> /* strerror */
> >  #include <unistd.h> /* readlink */
> >  #include <dirent.h>
> > +#include <fcntl.h>
> >
> >  #include <rte_string_fns.h> /* strlcpy */
> >
> > +#define MAX_EXTRA_ARGS 32
> >  #ifdef RTE_EXEC_ENV_FREEBSD
> >  #define self "curproc"
> >  #define exe "file"
> > @@ -34,6 +36,50 @@ extern uint16_t flag_for_send_pkts;  #endif  #endif
> >
> > +/* get cmdline form PID. Read process info form /proc/$PID. */ static
> > +char *get_cmdline_from_pid(pid_t pid, char *buf, int len) {
> > +	char filename[PATH_MAX];
> > +	char *name = NULL;
> > +	int fd;
> > +	int ret;
> > +
> > +	if (pid < 1 || buf == NULL || len < 0) {
> > +		printf("%s: illegal param\n", __func__);
> > +		return NULL;
> > +	}
> > +
> > +	snprintf(filename, PATH_MAX, "/proc/%d/cmdline", pid);
> > +	memset(buf, 0, len);
> > +	fd = open(filename, O_RDONLY);
> > +	if (fd < 0) {
> > +		perror("open:");
> > +		return NULL;
> > +	}
> > +	ret = read(fd, buf, len);
> > +	close(fd);
> > +
> > +	if (ret < 0)
> > +		return NULL;
> > +
> > +	if (buf[ret-1] == '\n')
> > +		buf[--ret] = 0;
> > +
> > +	name = buf;
> > +	while (ret) {
> > +		if (((unsigned char)*name) < ' ')
> > +			*name = ' ';
> > +		name++;
> > +		ret--;
> > +	}
> > +	*name = 0;
> > +
> > +	if (buf[0])
> > +		return buf;
> > +
> > +	return NULL;
> > +}
> > +
> >  /*
> >   * launches a second copy of the test process using the given argv
> > parameters,
> >   * which should include argv[0] as the process name. To identify in
> > the @@ -
> > 44,9 +90,15 @@ static inline int  process_dup(const char *const
> > argv[], int numargs, const char *env_value)  {
> >  	int num;
> > -	char *argv_cpy[numargs + 1];
> > -	int i, status;
> > +	char *argv_cpy[MAX_EXTRA_ARGS];
> > +	int i, status, n, s, j;
> >  	char path[32];
> > +	char buf[1024];
> > +	char *token;
> > +	char str_1[] = "-a";
> > +	char str_2[] = " ";
> > +	char *argv_str[MAX_EXTRA_ARGS];
> > +	char *argv_copy[MAX_EXTRA_ARGS];
> >  #ifdef RTE_LIB_PDUMP
> >  #ifdef RTE_NET_RING
> >  	pthread_t thread;
> > @@ -113,10 +165,32 @@ process_dup(const char *const argv[], int
> > numargs, const char *env_value)
> >  			closedir(dir);
> >  		}
> >  #endif
> > +		/* Add the -a parameter to the child process start parameter
> > */
> > +		get_cmdline_from_pid(getppid(), buf, 1024);
> > +		token = strtok(buf, str_2);
> > +		argv_str[0] = strdup(token);
> > +		n = 0;
> > +		j = 0;
> > +		while (token != NULL) {
> > +			n = n + 1;
> > +			argv_str[n] = strdup(token);
> > +			token = strtok(NULL, str_2);
> > +		}
> > +		for (s = 0; s < n; s++) {
> > +			if (strcmp(argv_str[s], str_1) == 0 ||
> > +				strcmp(argv_str[s + 1], str_1) == 0) {
> > +				argv_copy[j] = strdup(argv_str[s + 1]);
> > +				j++;
> > +			}
> > +		}
> > +		for (s = 0; s < j; s++)
> > +			argv_cpy[numargs + s] = strdup(argv_copy[s]);
> > +
> >  		printf("Running binary with argv[]:");
> > -		for (i = 0; i < num; i++)
> > +		for (i = 0; i < num + j; i++)
> >  			printf("'%s' ", argv_cpy[i]);
> >  		printf("\n");
> > +		argv_cpy[numargs + j] = NULL;
> >  		fflush(stdout);
> >
> >  		/* set the environment variable */
> > --
> > 2.25.1
diff mbox series

Patch

diff --git a/app/test/process.h b/app/test/process.h
index 1f073b9c5c..c8a15e8989 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -15,9 +15,11 @@ 
 #include <string.h> /* strerror */
 #include <unistd.h> /* readlink */
 #include <dirent.h>
+#include <fcntl.h>
 
 #include <rte_string_fns.h> /* strlcpy */
 
+#define MAX_EXTRA_ARGS 32
 #ifdef RTE_EXEC_ENV_FREEBSD
 #define self "curproc"
 #define exe "file"
@@ -34,6 +36,50 @@  extern uint16_t flag_for_send_pkts;
 #endif
 #endif
 
+/* get cmdline form PID. Read process info form /proc/$PID. */
+static char *get_cmdline_from_pid(pid_t pid, char *buf, int len)
+{
+	char filename[PATH_MAX];
+	char *name = NULL;
+	int fd;
+	int ret;
+
+	if (pid < 1 || buf == NULL || len < 0) {
+		printf("%s: illegal param\n", __func__);
+		return NULL;
+	}
+
+	snprintf(filename, PATH_MAX, "/proc/%d/cmdline", pid);
+	memset(buf, 0, len);
+	fd = open(filename, O_RDONLY);
+	if (fd < 0) {
+		perror("open:");
+		return NULL;
+	}
+	ret = read(fd, buf, len);
+	close(fd);
+
+	if (ret < 0)
+		return NULL;
+
+	if (buf[ret-1] == '\n')
+		buf[--ret] = 0;
+
+	name = buf;
+	while (ret) {
+		if (((unsigned char)*name) < ' ')
+			*name = ' ';
+		name++;
+		ret--;
+	}
+	*name = 0;
+
+	if (buf[0])
+		return buf;
+
+	return NULL;
+}
+
 /*
  * launches a second copy of the test process using the given argv parameters,
  * which should include argv[0] as the process name. To identify in the
@@ -44,9 +90,15 @@  static inline int
 process_dup(const char *const argv[], int numargs, const char *env_value)
 {
 	int num;
-	char *argv_cpy[numargs + 1];
-	int i, status;
+	char *argv_cpy[MAX_EXTRA_ARGS];
+	int i, status, n, s, j;
 	char path[32];
+	char buf[1024];
+	char *token;
+	char str_1[] = "-a";
+	char str_2[] = " ";
+	char *argv_str[MAX_EXTRA_ARGS];
+	char *argv_copy[MAX_EXTRA_ARGS];
 #ifdef RTE_LIB_PDUMP
 #ifdef RTE_NET_RING
 	pthread_t thread;
@@ -113,10 +165,32 @@  process_dup(const char *const argv[], int numargs, const char *env_value)
 			closedir(dir);
 		}
 #endif
+		/* Add the -a parameter to the child process start parameter */
+		get_cmdline_from_pid(getppid(), buf, 1024);
+		token = strtok(buf, str_2);
+		argv_str[0] = strdup(token);
+		n = 0;
+		j = 0;
+		while (token != NULL) {
+			n = n + 1;
+			argv_str[n] = strdup(token);
+			token = strtok(NULL, str_2);
+		}
+		for (s = 0; s < n; s++) {
+			if (strcmp(argv_str[s], str_1) == 0 ||
+				strcmp(argv_str[s + 1], str_1) == 0) {
+				argv_copy[j] = strdup(argv_str[s + 1]);
+				j++;
+			}
+		}
+		for (s = 0; s < j; s++)
+			argv_cpy[numargs + s] = strdup(argv_copy[s]);
+
 		printf("Running binary with argv[]:");
-		for (i = 0; i < num; i++)
+		for (i = 0; i < num + j; i++)
 			printf("'%s' ", argv_cpy[i]);
 		printf("\n");
+		argv_cpy[numargs + j] = NULL;
 		fflush(stdout);
 
 		/* set the environment variable */