diff mbox series

[v7] app/test: secondary process passes allow parameters

Message ID 20231114102815.409998-1-mingjinx.ye@intel.com (mailing list archive)
State Accepted, archived
Delegated to: Thomas Monjalon
Headers
Series [v7] app/test: secondary process passes allow 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/iol-compile-amd64-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/github-robot: build success github build: passed
ci/iol-sample-apps-testing success Testing PASS
ci/iol-unit-arm64-testing success Testing PASS
ci/iol-compile-arm64-testing success Testing PASS
ci/iol-unit-amd64-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/intel-Testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/intel-Functional success Functional PASS

Commit Message

Mingjin Ye Nov. 14, 2023, 10:28 a.m. UTC 
  In EAL related test cases, the allow parameters are not passed to
the secondary process, resulting in unexpected NICs being loaded.

This patch fixes this issue by appending the allow parameters to
the secondary process.

Fixes: af75078fece3 ("first public release")
Cc: stable@dpdk.org

Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
---
v5: Optimized.
---
v6: Optimized.
---
v7: Fix CI errors.
---
 app/test/process.h | 46 ++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 42 insertions(+), 4 deletions(-)

Comments

Huang, ZhiminX Nov. 17, 2023, 10:05 a.m. UTC | #1 
> -----Original Message-----
> From: Mingjin Ye <mingjinx.ye@intel.com>
> Sent: Tuesday, November 14, 2023 6:28 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Ye, MingjinX
> <mingjinx.ye@intel.com>; stable@dpdk.org
> Subject: [PATCH v7] app/test: secondary process passes allow parameters
> 
> In EAL related test cases, the allow parameters are not passed to the
> secondary process, resulting in unexpected NICs being loaded.
> 
> This patch fixes this issue by appending the allow parameters to the secondary
> process.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> v5: Optimized.
> ---
> v6: Optimized.
> ---
> v7: Fix CI errors.
> ---

Verified pass for meson test:
dpdk-devbind.py -b vfio-pci 31:00.0 31:00.1 b1:00.0 b1:00.1
DPDK_TEST=eal_flags_file_prefix_autotest MALLOC_PERTURB_=192 /root/dpdk/x86_64-native-linuxapp-gcc/app/test/dpdk-test --file-prefix=eal_flags_file_prefix_autotest -a 31:00.0 -a 31:00.1

Tested-by: Zhimin Huang <zhiminx.huang@intel.com >
Mingjin Ye Nov. 17, 2023, 10:24 a.m. UTC | #2 
Hi Richardson,

can you please take a look at this patch.

Thanks,
Mingjin


> -----Original Message-----
> From: Ye, MingjinX <mingjinx.ye@intel.com>
> Sent: Tuesday, November 14, 2023 6:28 PM
> To: dev@dpdk.org
> Cc: Yang, Qiming <qiming.yang@intel.com>; Ye, MingjinX
> <mingjinx.ye@intel.com>; stable@dpdk.org
> Subject: [PATCH v7] app/test: secondary process passes allow parameters
> 
> In EAL related test cases, the allow parameters are not passed to the
> secondary process, resulting in unexpected NICs being loaded.
> 
> This patch fixes this issue by appending the allow parameters to the
> secondary process.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> ---
> v5: Optimized.
> ---
> v6: Optimized.
> ---
> v7: Fix CI errors.
> ---
>  app/test/process.h | 46
> ++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 42 insertions(+), 4 deletions(-)
> 
> diff --git a/app/test/process.h b/app/test/process.h index
> af7bc3e0de..06b2f8b192 100644
> --- a/app/test/process.h
> +++ b/app/test/process.h
> @@ -17,6 +17,7 @@
>  #include <dirent.h>
> 
>  #include <rte_string_fns.h> /* strlcpy */
> +#include <rte_devargs.h>
> 
>  #ifdef RTE_EXEC_ENV_FREEBSD
>  #define self "curproc"
> @@ -34,6 +35,34 @@ extern uint16_t flag_for_send_pkts;  #endif  #endif
> 
> +#define PREFIX_ALLOW "--allow="
> +
> +static int
> +add_parameter_allow(char **argv, int max_capacity) {
> +	struct rte_devargs *devargs;
> +	int count = 0;
> +
> +	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> +		if (strlen(devargs->name) == 0)
> +			continue;
> +
> +		if (devargs->data == NULL || strlen(devargs->data) == 0) {
> +			if (asprintf(&argv[count], PREFIX_ALLOW"%s",
> devargs->name) < 0)
> +				break;
> +		} else {
> +			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
> +					 devargs->name, devargs->data) < 0)
> +				break;
> +		}
> +
> +		if (++count == max_capacity)
> +			break;
> +	}
> +
> +	return count;
> +}
> +
>  /*
>   * 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 @@ -
> 43,8 +72,10 @@ extern uint16_t flag_for_send_pkts;  static inline int
> process_dup(const char *const argv[], int numargs, const char *env_value)  {
> -	int num;
> -	char *argv_cpy[numargs + 1];
> +	int num = 0;
> +	char **argv_cpy;
> +	int allow_num;
> +	int argv_num;
>  	int i, status;
>  	char path[32];
>  #ifdef RTE_LIB_PDUMP
> @@ -58,11 +89,18 @@ process_dup(const char *const argv[], int numargs,
> const char *env_value)
>  	if (pid < 0)
>  		return -1;
>  	else if (pid == 0) {
> +		allow_num =
> rte_devargs_type_count(RTE_DEVTYPE_ALLOWED);
> +		argv_num = numargs + allow_num + 1;
> +		argv_cpy = calloc(argv_num, sizeof(char *));
> +		if (!argv_cpy)
> +			rte_panic("Memory allocation failed\n");
> +
>  		/* make a copy of the arguments to be passed to exec */
>  		for (i = 0; i < numargs; i++)
>  			argv_cpy[i] = strdup(argv[i]);
> -		argv_cpy[i] = NULL;
> -		num = numargs;
> +		if (allow_num > 0)
> +			num = add_parameter_allow(&argv_cpy[i],
> allow_num);
> +		num += numargs;
> 
>  #ifdef RTE_EXEC_ENV_LINUX
>  		{
> --
> 2.25.1
Bruce Richardson Nov. 17, 2023, 11:17 a.m. UTC | #3 
On Fri, Nov 17, 2023 at 10:24:41AM +0000, Ye, MingjinX wrote:
> Hi Richardson,
> 
> can you please take a look at this patch.
> 
> Thanks,
> Mingjin
> 

Hi,

I acked v6. If nothing major has changed in the patch, then the ack can be
kept for v7. If there is something that has changed that you think I should
look at, please include it in the change-log, so I can check it out.

For now, assuming no major changes:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

/Bruce

> 
> > -----Original Message-----
> > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > Sent: Tuesday, November 14, 2023 6:28 PM
> > To: dev@dpdk.org
> > Cc: Yang, Qiming <qiming.yang@intel.com>; Ye, MingjinX
> > <mingjinx.ye@intel.com>; stable@dpdk.org
> > Subject: [PATCH v7] app/test: secondary process passes allow parameters
> > 
> > In EAL related test cases, the allow parameters are not passed to the
> > secondary process, resulting in unexpected NICs being loaded.
> > 
> > This patch fixes this issue by appending the allow parameters to the
> > secondary process.
> > 
> > Fixes: af75078fece3 ("first public release")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > ---
> > v5: Optimized.
> > ---
> > v6: Optimized.
> > ---
> > v7: Fix CI errors.
> > ---
> >  app/test/process.h | 46
> > ++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 42 insertions(+), 4 deletions(-)
> > 
> > diff --git a/app/test/process.h b/app/test/process.h index
> > af7bc3e0de..06b2f8b192 100644
> > --- a/app/test/process.h
> > +++ b/app/test/process.h
> > @@ -17,6 +17,7 @@
> >  #include <dirent.h>
> > 
> >  #include <rte_string_fns.h> /* strlcpy */
> > +#include <rte_devargs.h>
> > 
> >  #ifdef RTE_EXEC_ENV_FREEBSD
> >  #define self "curproc"
> > @@ -34,6 +35,34 @@ extern uint16_t flag_for_send_pkts;  #endif  #endif
> > 
> > +#define PREFIX_ALLOW "--allow="
> > +
> > +static int
> > +add_parameter_allow(char **argv, int max_capacity) {
> > +	struct rte_devargs *devargs;
> > +	int count = 0;
> > +
> > +	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> > +		if (strlen(devargs->name) == 0)
> > +			continue;
> > +
> > +		if (devargs->data == NULL || strlen(devargs->data) == 0) {
> > +			if (asprintf(&argv[count], PREFIX_ALLOW"%s",
> > devargs->name) < 0)
> > +				break;
> > +		} else {
> > +			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
> > +					 devargs->name, devargs->data) < 0)
> > +				break;
> > +		}
> > +
> > +		if (++count == max_capacity)
> > +			break;
> > +	}
> > +
> > +	return count;
> > +}
> > +
> >  /*
> >   * 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 @@ -
> > 43,8 +72,10 @@ extern uint16_t flag_for_send_pkts;  static inline int
> > process_dup(const char *const argv[], int numargs, const char *env_value)  {
> > -	int num;
> > -	char *argv_cpy[numargs + 1];
> > +	int num = 0;
> > +	char **argv_cpy;
> > +	int allow_num;
> > +	int argv_num;
> >  	int i, status;
> >  	char path[32];
> >  #ifdef RTE_LIB_PDUMP
> > @@ -58,11 +89,18 @@ process_dup(const char *const argv[], int numargs,
> > const char *env_value)
> >  	if (pid < 0)
> >  		return -1;
> >  	else if (pid == 0) {
> > +		allow_num =
> > rte_devargs_type_count(RTE_DEVTYPE_ALLOWED);
> > +		argv_num = numargs + allow_num + 1;
> > +		argv_cpy = calloc(argv_num, sizeof(char *));
> > +		if (!argv_cpy)
> > +			rte_panic("Memory allocation failed\n");
> > +
> >  		/* make a copy of the arguments to be passed to exec */
> >  		for (i = 0; i < numargs; i++)
> >  			argv_cpy[i] = strdup(argv[i]);
> > -		argv_cpy[i] = NULL;
> > -		num = numargs;
> > +		if (allow_num > 0)
> > +			num = add_parameter_allow(&argv_cpy[i],
> > allow_num);
> > +		num += numargs;
> > 
> >  #ifdef RTE_EXEC_ENV_LINUX
> >  		{
> > --
> > 2.25.1
>
Mingjin Ye Nov. 24, 2023, 10:28 a.m. UTC | #4 
Hi, 

Thanks for the ack. would like to change the state of the patch, which will be merged into the dpdk.

Change: The "--allow" parameter is added when the number of allow is greater than zero.

Thanks,
Mingjin

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Friday, November 17, 2023 7:17 PM
> To: Ye, MingjinX <mingjinx.ye@intel.com>
> Cc: dev@dpdk.org; Yang, Qiming <qiming.yang@intel.com>;
> stable@dpdk.org; Xu, HailinX <hailinx.xu@intel.com>
> Subject: Re: [PATCH v7] app/test: secondary process passes allow
> parameters
> 
> On Fri, Nov 17, 2023 at 10:24:41AM +0000, Ye, MingjinX wrote:
> > Hi Richardson,
> >
> > can you please take a look at this patch.
> >
> > Thanks,
> > Mingjin
> >
> 
> Hi,
> 
> I acked v6. If nothing major has changed in the patch, then the ack can be
> kept for v7. If there is something that has changed that you think I should
> look at, please include it in the change-log, so I can check it out.
> 
> For now, assuming no major changes:
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> /Bruce
> 
> >
> > > -----Original Message-----
> > > From: Ye, MingjinX <mingjinx.ye@intel.com>
> > > Sent: Tuesday, November 14, 2023 6:28 PM
> > > To: dev@dpdk.org
> > > Cc: Yang, Qiming <qiming.yang@intel.com>; Ye, MingjinX
> > > <mingjinx.ye@intel.com>; stable@dpdk.org
> > > Subject: [PATCH v7] app/test: secondary process passes allow
> > > parameters
> > >
> > > In EAL related test cases, the allow parameters are not passed to
> > > the secondary process, resulting in unexpected NICs being loaded.
> > >
> > > This patch fixes this issue by appending the allow parameters to the
> > > secondary process.
> > >
> > > Fixes: af75078fece3 ("first public release")
> > > Cc: stable@dpdk.org
> > >
> > > Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>
> > > ---
> > > v5: Optimized.
> > > ---
> > > v6: Optimized.
> > > ---
> > > v7: Fix CI errors.
> > > ---
> > >  app/test/process.h | 46
> > > ++++++++++++++++++++++++++++++++++++++++++----
> > >  1 file changed, 42 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/app/test/process.h b/app/test/process.h index
> > > af7bc3e0de..06b2f8b192 100644
> > > --- a/app/test/process.h
> > > +++ b/app/test/process.h
> > > @@ -17,6 +17,7 @@
> > >  #include <dirent.h>
> > >
> > >  #include <rte_string_fns.h> /* strlcpy */
> > > +#include <rte_devargs.h>
> > >
> > >  #ifdef RTE_EXEC_ENV_FREEBSD
> > >  #define self "curproc"
> > > @@ -34,6 +35,34 @@ extern uint16_t flag_for_send_pkts;  #endif
> > > #endif
> > >
> > > +#define PREFIX_ALLOW "--allow="
> > > +
> > > +static int
> > > +add_parameter_allow(char **argv, int max_capacity) {
> > > +	struct rte_devargs *devargs;
> > > +	int count = 0;
> > > +
> > > +	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
> > > +		if (strlen(devargs->name) == 0)
> > > +			continue;
> > > +
> > > +		if (devargs->data == NULL || strlen(devargs->data) == 0) {
> > > +			if (asprintf(&argv[count], PREFIX_ALLOW"%s",
> > > devargs->name) < 0)
> > > +				break;
> > > +		} else {
> > > +			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
> > > +					 devargs->name, devargs->data) < 0)
> > > +				break;
> > > +		}
> > > +
> > > +		if (++count == max_capacity)
> > > +			break;
> > > +	}
> > > +
> > > +	return count;
> > > +}
> > > +
> > >  /*
> > >   * 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 @@ -
> > > 43,8 +72,10 @@ extern uint16_t flag_for_send_pkts;  static inline
> > > int process_dup(const char *const argv[], int numargs, const char
> *env_value)  {
> > > -	int num;
> > > -	char *argv_cpy[numargs + 1];
> > > +	int num = 0;
> > > +	char **argv_cpy;
> > > +	int allow_num;
> > > +	int argv_num;
> > >  	int i, status;
> > >  	char path[32];
> > >  #ifdef RTE_LIB_PDUMP
> > > @@ -58,11 +89,18 @@ process_dup(const char *const argv[], int
> > > numargs, const char *env_value)
> > >  	if (pid < 0)
> > >  		return -1;
> > >  	else if (pid == 0) {
> > > +		allow_num =
> > > rte_devargs_type_count(RTE_DEVTYPE_ALLOWED);
> > > +		argv_num = numargs + allow_num + 1;
> > > +		argv_cpy = calloc(argv_num, sizeof(char *));
> > > +		if (!argv_cpy)
> > > +			rte_panic("Memory allocation failed\n");
> > > +
> > >  		/* make a copy of the arguments to be passed to exec */
> > >  		for (i = 0; i < numargs; i++)
> > >  			argv_cpy[i] = strdup(argv[i]);
> > > -		argv_cpy[i] = NULL;
> > > -		num = numargs;
> > > +		if (allow_num > 0)
> > > +			num = add_parameter_allow(&argv_cpy[i],
> > > allow_num);
> > > +		num += numargs;
> > >
> > >  #ifdef RTE_EXEC_ENV_LINUX
> > >  		{
> > > --
> > > 2.25.1
> >
Thomas Monjalon March 6, 2024, 1:50 p.m. UTC | #5 
14/11/2023 11:28, Mingjin Ye:
> In EAL related test cases, the allow parameters are not passed to
> the secondary process, resulting in unexpected NICs being loaded.
> 
> This patch fixes this issue by appending the allow parameters to
> the secondary process.
> 
> Fixes: af75078fece3 ("first public release")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Mingjin Ye <mingjinx.ye@intel.com>

Applied, thanks.
diff mbox series

Patch

diff --git a/app/test/process.h b/app/test/process.h
index af7bc3e0de..06b2f8b192 100644
--- a/app/test/process.h
+++ b/app/test/process.h
@@ -17,6 +17,7 @@ 
 #include <dirent.h>
 
 #include <rte_string_fns.h> /* strlcpy */
+#include <rte_devargs.h>
 
 #ifdef RTE_EXEC_ENV_FREEBSD
 #define self "curproc"
@@ -34,6 +35,34 @@  extern uint16_t flag_for_send_pkts;
 #endif
 #endif
 
+#define PREFIX_ALLOW "--allow="
+
+static int
+add_parameter_allow(char **argv, int max_capacity)
+{
+	struct rte_devargs *devargs;
+	int count = 0;
+
+	RTE_EAL_DEVARGS_FOREACH(NULL, devargs) {
+		if (strlen(devargs->name) == 0)
+			continue;
+
+		if (devargs->data == NULL || strlen(devargs->data) == 0) {
+			if (asprintf(&argv[count], PREFIX_ALLOW"%s", devargs->name) < 0)
+				break;
+		} else {
+			if (asprintf(&argv[count], PREFIX_ALLOW"%s,%s",
+					 devargs->name, devargs->data) < 0)
+				break;
+		}
+
+		if (++count == max_capacity)
+			break;
+	}
+
+	return count;
+}
+
 /*
  * 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
@@ -43,8 +72,10 @@  extern uint16_t flag_for_send_pkts;
 static inline int
 process_dup(const char *const argv[], int numargs, const char *env_value)
 {
-	int num;
-	char *argv_cpy[numargs + 1];
+	int num = 0;
+	char **argv_cpy;
+	int allow_num;
+	int argv_num;
 	int i, status;
 	char path[32];
 #ifdef RTE_LIB_PDUMP
@@ -58,11 +89,18 @@  process_dup(const char *const argv[], int numargs, const char *env_value)
 	if (pid < 0)
 		return -1;
 	else if (pid == 0) {
+		allow_num = rte_devargs_type_count(RTE_DEVTYPE_ALLOWED);
+		argv_num = numargs + allow_num + 1;
+		argv_cpy = calloc(argv_num, sizeof(char *));
+		if (!argv_cpy)
+			rte_panic("Memory allocation failed\n");
+
 		/* make a copy of the arguments to be passed to exec */
 		for (i = 0; i < numargs; i++)
 			argv_cpy[i] = strdup(argv[i]);
-		argv_cpy[i] = NULL;
-		num = numargs;
+		if (allow_num > 0)
+			num = add_parameter_allow(&argv_cpy[i], allow_num);
+		num += numargs;
 
 #ifdef RTE_EXEC_ENV_LINUX
 		{