[v2,3/3] app/test/meson: auto detect number of cores

Message ID 20190412162141.23327-4-aconole@redhat.com
State Accepted
Headers show
Series
  • travis: enhancements for build (plus a meson fix)
Related show

Checks

Context Check Description
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Aaron Conole April 12, 2019, 4:21 p.m.
The arguments being passed will cause failures on laptops that have,
for instance, 2 cores only.  Most of the tests don't require more
than a single core.  Some require multiple cores (but those tests
should be modified to 'SKIP' when the correct number of cores
aren't available).

The unit test results shouldn't be impacted by this change, but it
allows for a future enhancement to pass flags such as '--no-huge'.

Also include a fix to a reported issue with running on FreeBSD.

Signed-off-by: Aaron Conole <aconole@redhat.com>
Reviewed-by: David Marchand <david.marchand@redhat.com>
Acked-by: Luca Boccassi <bluca@debian.org>
---
v2:
* Fix a spelling mistake
* Add support for FreeBSD
* Include a default fallback
* Use a more robust core-mask argument source (rather than lscpu)

Conflicts with http://patches.dpdk.org/patch/50850/

 app/test/meson.build | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

Comments

Bruce Richardson April 12, 2019, 4:36 p.m. | #1
On Fri, Apr 12, 2019 at 12:21:41PM -0400, Aaron Conole wrote:
> The arguments being passed will cause failures on laptops that have,
> for instance, 2 cores only.  Most of the tests don't require more
> than a single core.  Some require multiple cores (but those tests
> should be modified to 'SKIP' when the correct number of cores
> aren't available).
> 
> The unit test results shouldn't be impacted by this change, but it
> allows for a future enhancement to pass flags such as '--no-huge'.
> 
> Also include a fix to a reported issue with running on FreeBSD.
> 
> Signed-off-by: Aaron Conole <aconole@redhat.com>
> Reviewed-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Luca Boccassi <bluca@debian.org>
> ---
> v2:
> * Fix a spelling mistake
> * Add support for FreeBSD
> * Include a default fallback
> * Use a more robust core-mask argument source (rather than lscpu)
> 
> Conflicts with http://patches.dpdk.org/patch/50850/
> 
>  app/test/meson.build | 35 ++++++++++++++++++++++++++++++++---
>  1 file changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 867cc5863..5e056eb59 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -344,17 +344,43 @@ if get_option('tests')
>  	timeout_seconds = 600
>  	timeout_seconds_fast = 10
>  
> +	# Retrieve the number of CPU cores, defaulting to 4.
> +	num_cores = '0-3'
> +	if host_machine.system() == 'linux'
> +		num_cores = run_command('cat',
> +					'/sys/devices/system/cpu/present'
> +				       ).stdout().strip()
> +	elif host_machine.system() == 'freebsd'
> +		snum_cores = run_command('/sbin/sysctl', '-n',
> +					 'hw.ncpu').stdout().strip()
> +		inum_cores = snum_cores.to_int() - 1
> +                num_cores = '0-@0@'.format(inum_cores)
> +	endif
> +
> +	num_cores_arg = '-l ' + num_cores
> +
> +	test_args = [num_cores_arg, '-n 4']

This -n 4 parameter can be dropped. Four is the default setting IIRC.
I also wonder are the parameters coming through to the app correctly,
generally meson does not work well with parameters with spaces in them -
I'd expect the "-l" and the num_cores values to be separated in the array.
I also think num_cores_arg value could be dropped too.

If it works though, I'm ok to keep as-is though.

>  	foreach arg : fast_parallel_test_names
> -		test(arg, dpdk_test,
> -			env : ['DPDK_TEST=' + arg],
> -			args : ['-c f','-n 4', '--file-prefix=@0@'.format(arg)],
> +		if host_machine.system() == 'linux'
> +			test(arg, dpdk_test,
> +				  env : ['DPDK_TEST=' + arg],
> +				  args : test_args +
> +					 ['--file-prefix=@0@'.format(arg)],
> +			timeout : timeout_seconds_fast,
> +			suite : 'fast-tests')
> +		else
> +			test(arg, dpdk_test,
> +				env : ['DPDK_TEST=' + arg],
> +				args : test_args,
>  			timeout : timeout_seconds_fast,
>  			suite : 'fast-tests')
> +		endif
>  	endforeach

While this is needed now, I think in the medium term we should have the
"file-prefix" flag being a warning rather than a hard-error on FreeBSD.
[i.e. keep this, but we should fix it in 19.08 to be shorter]

/Bruce
Aaron Conole April 12, 2019, 6:21 p.m. | #2
Bruce Richardson <bruce.richardson@intel.com> writes:

> On Fri, Apr 12, 2019 at 12:21:41PM -0400, Aaron Conole wrote:
>> The arguments being passed will cause failures on laptops that have,
>> for instance, 2 cores only.  Most of the tests don't require more
>> than a single core.  Some require multiple cores (but those tests
>> should be modified to 'SKIP' when the correct number of cores
>> aren't available).
>> 
>> The unit test results shouldn't be impacted by this change, but it
>> allows for a future enhancement to pass flags such as '--no-huge'.
>> 
>> Also include a fix to a reported issue with running on FreeBSD.
>> 
>> Signed-off-by: Aaron Conole <aconole@redhat.com>
>> Reviewed-by: David Marchand <david.marchand@redhat.com>
>> Acked-by: Luca Boccassi <bluca@debian.org>
>> ---
>> v2:
>> * Fix a spelling mistake
>> * Add support for FreeBSD
>> * Include a default fallback
>> * Use a more robust core-mask argument source (rather than lscpu)
>> 
>> Conflicts with http://patches.dpdk.org/patch/50850/
>> 
>>  app/test/meson.build | 35 ++++++++++++++++++++++++++++++++---
>>  1 file changed, 32 insertions(+), 3 deletions(-)
>> 
>> diff --git a/app/test/meson.build b/app/test/meson.build
>> index 867cc5863..5e056eb59 100644
>> --- a/app/test/meson.build
>> +++ b/app/test/meson.build
>> @@ -344,17 +344,43 @@ if get_option('tests')
>>  	timeout_seconds = 600
>>  	timeout_seconds_fast = 10
>>  
>> +	# Retrieve the number of CPU cores, defaulting to 4.
>> +	num_cores = '0-3'
>> +	if host_machine.system() == 'linux'
>> +		num_cores = run_command('cat',
>> +					'/sys/devices/system/cpu/present'
>> +				       ).stdout().strip()
>> +	elif host_machine.system() == 'freebsd'
>> +		snum_cores = run_command('/sbin/sysctl', '-n',
>> +					 'hw.ncpu').stdout().strip()
>> +		inum_cores = snum_cores.to_int() - 1
>> +                num_cores = '0-@0@'.format(inum_cores)
>> +	endif
>> +
>> +	num_cores_arg = '-l ' + num_cores
>> +
>> +	test_args = [num_cores_arg, '-n 4']
>
> This -n 4 parameter can be dropped. Four is the default setting IIRC.

For another patch.  I thought about doing it with this one, but I'd
rather keep the changes a little bit traceable.  If you think I should
resubmit with it dropped, I will.

> I also wonder are the parameters coming through to the app correctly,
> generally meson does not work well with parameters with spaces in them -
> I'd expect the "-l" and the num_cores values to be separated in the array.
> I also think num_cores_arg value could be dropped too.
>
> If it works though, I'm ok to keep as-is though.

It does work.  Actually, I needed to change because on some of the VM
setups (including the one used by Travis) the '-c f' arg errors because
it wants 4 cores, and only 2 exist.  Either way, this patch doesn't
change the spacing being passed with args :)

>>  	foreach arg : fast_parallel_test_names
>> -		test(arg, dpdk_test,
>> -			env : ['DPDK_TEST=' + arg],
>> -			args : ['-c f','-n 4', '--file-prefix=@0@'.format(arg)],
>> +		if host_machine.system() == 'linux'
>> +			test(arg, dpdk_test,
>> +				  env : ['DPDK_TEST=' + arg],
>> +				  args : test_args +
>> +					 ['--file-prefix=@0@'.format(arg)],
>> +			timeout : timeout_seconds_fast,
>> +			suite : 'fast-tests')
>> +		else
>> +			test(arg, dpdk_test,
>> +				env : ['DPDK_TEST=' + arg],
>> +				args : test_args,
>>  			timeout : timeout_seconds_fast,
>>  			suite : 'fast-tests')
>> +		endif
>>  	endforeach
>
> While this is needed now, I think in the medium term we should have the
> "file-prefix" flag being a warning rather than a hard-error on FreeBSD.
> [i.e. keep this, but we should fix it in 19.08 to be shorter]

Agreed.  Probably a good cleanup in the future.

> /Bruce
Bruce Richardson April 15, 2019, 9:14 a.m. | #3
On Fri, Apr 12, 2019 at 02:21:41PM -0400, Aaron Conole wrote:
> Bruce Richardson <bruce.richardson@intel.com> writes:
> 
> > On Fri, Apr 12, 2019 at 12:21:41PM -0400, Aaron Conole wrote:
> >> The arguments being passed will cause failures on laptops that have,
> >> for instance, 2 cores only.  Most of the tests don't require more
> >> than a single core.  Some require multiple cores (but those tests
> >> should be modified to 'SKIP' when the correct number of cores
> >> aren't available).
> >> 
> >> The unit test results shouldn't be impacted by this change, but it
> >> allows for a future enhancement to pass flags such as '--no-huge'.
> >> 
> >> Also include a fix to a reported issue with running on FreeBSD.
> >> 
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> Reviewed-by: David Marchand <david.marchand@redhat.com>
> >> Acked-by: Luca Boccassi <bluca@debian.org>
> >> ---
> >> v2:
> >> * Fix a spelling mistake
> >> * Add support for FreeBSD
> >> * Include a default fallback
> >> * Use a more robust core-mask argument source (rather than lscpu)
> >> 
> >> Conflicts with http://patches.dpdk.org/patch/50850/
> >> 
> >>  app/test/meson.build | 35 ++++++++++++++++++++++++++++++++---
> >>  1 file changed, 32 insertions(+), 3 deletions(-)
> >> 
> >> diff --git a/app/test/meson.build b/app/test/meson.build
> >> index 867cc5863..5e056eb59 100644
> >> --- a/app/test/meson.build
> >> +++ b/app/test/meson.build
> >> @@ -344,17 +344,43 @@ if get_option('tests')
> >>  	timeout_seconds = 600
> >>  	timeout_seconds_fast = 10
> >>  
> >> +	# Retrieve the number of CPU cores, defaulting to 4.
> >> +	num_cores = '0-3'
> >> +	if host_machine.system() == 'linux'
> >> +		num_cores = run_command('cat',
> >> +					'/sys/devices/system/cpu/present'
> >> +				       ).stdout().strip()
> >> +	elif host_machine.system() == 'freebsd'
> >> +		snum_cores = run_command('/sbin/sysctl', '-n',
> >> +					 'hw.ncpu').stdout().strip()
> >> +		inum_cores = snum_cores.to_int() - 1
> >> +                num_cores = '0-@0@'.format(inum_cores)
> >> +	endif
> >> +
> >> +	num_cores_arg = '-l ' + num_cores
> >> +
> >> +	test_args = [num_cores_arg, '-n 4']
> >
> > This -n 4 parameter can be dropped. Four is the default setting IIRC.
> 
> For another patch.  I thought about doing it with this one, but I'd
> rather keep the changes a little bit traceable.  If you think I should
> resubmit with it dropped, I will.
> 
> > I also wonder are the parameters coming through to the app correctly,
> > generally meson does not work well with parameters with spaces in them -
> > I'd expect the "-l" and the num_cores values to be separated in the array.
> > I also think num_cores_arg value could be dropped too.
> >
> > If it works though, I'm ok to keep as-is though.
> 
> It does work.  Actually, I needed to change because on some of the VM
> setups (including the one used by Travis) the '-c f' arg errors because
> it wants 4 cores, and only 2 exist.  Either way, this patch doesn't
> change the spacing being passed with args :)
> 
> >>  	foreach arg : fast_parallel_test_names
> >> -		test(arg, dpdk_test,
> >> -			env : ['DPDK_TEST=' + arg],
> >> -			args : ['-c f','-n 4', '--file-prefix=@0@'.format(arg)],
> >> +		if host_machine.system() == 'linux'
> >> +			test(arg, dpdk_test,
> >> +				  env : ['DPDK_TEST=' + arg],
> >> +				  args : test_args +
> >> +					 ['--file-prefix=@0@'.format(arg)],
> >> +			timeout : timeout_seconds_fast,
> >> +			suite : 'fast-tests')
> >> +		else
> >> +			test(arg, dpdk_test,
> >> +				env : ['DPDK_TEST=' + arg],
> >> +				args : test_args,
> >>  			timeout : timeout_seconds_fast,
> >>  			suite : 'fast-tests')
> >> +		endif
> >>  	endforeach
> >
> > While this is needed now, I think in the medium term we should have the
> > "file-prefix" flag being a warning rather than a hard-error on FreeBSD.
> > [i.e. keep this, but we should fix it in 19.08 to be shorter]
> 
> Agreed.  Probably a good cleanup in the future.
> 

Fair responses to all comments.

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

Patch

diff --git a/app/test/meson.build b/app/test/meson.build
index 867cc5863..5e056eb59 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -344,17 +344,43 @@  if get_option('tests')
 	timeout_seconds = 600
 	timeout_seconds_fast = 10
 
+	# Retrieve the number of CPU cores, defaulting to 4.
+	num_cores = '0-3'
+	if host_machine.system() == 'linux'
+		num_cores = run_command('cat',
+					'/sys/devices/system/cpu/present'
+				       ).stdout().strip()
+	elif host_machine.system() == 'freebsd'
+		snum_cores = run_command('/sbin/sysctl', '-n',
+					 'hw.ncpu').stdout().strip()
+		inum_cores = snum_cores.to_int() - 1
+                num_cores = '0-@0@'.format(inum_cores)
+	endif
+
+	num_cores_arg = '-l ' + num_cores
+
+	test_args = [num_cores_arg, '-n 4']
 	foreach arg : fast_parallel_test_names
-		test(arg, dpdk_test,
-			env : ['DPDK_TEST=' + arg],
-			args : ['-c f','-n 4', '--file-prefix=@0@'.format(arg)],
+		if host_machine.system() == 'linux'
+			test(arg, dpdk_test,
+				  env : ['DPDK_TEST=' + arg],
+				  args : test_args +
+					 ['--file-prefix=@0@'.format(arg)],
+			timeout : timeout_seconds_fast,
+			suite : 'fast-tests')
+		else
+			test(arg, dpdk_test,
+				env : ['DPDK_TEST=' + arg],
+				args : test_args,
 			timeout : timeout_seconds_fast,
 			suite : 'fast-tests')
+		endif
 	endforeach
 
 	foreach arg : fast_non_parallel_test_names
 		test(arg, dpdk_test,
 			env : ['DPDK_TEST=' + arg],
+			args : test_args,
 			timeout : timeout_seconds_fast,
 			is_parallel : false,
 			suite : 'fast-tests')
@@ -363,6 +389,7 @@  if get_option('tests')
 	foreach arg : perf_test_names
 		test(arg, dpdk_test,
 		env : ['DPDK_TEST=' + arg],
+		args : test_args,
 		timeout : timeout_seconds,
 		is_parallel : false,
 		suite : 'perf-tests')
@@ -371,6 +398,7 @@  if get_option('tests')
 	foreach arg : driver_test_names
 		test(arg, dpdk_test,
 			env : ['DPDK_TEST=' + arg],
+			args : test_args,
 			timeout : timeout_seconds,
 			is_parallel : false,
 			suite : 'driver-tests')
@@ -379,6 +407,7 @@  if get_option('tests')
 	foreach arg : dump_test_names
 		test(arg, dpdk_test,
 			env : ['DPDK_TEST=' + arg],
+			args : test_args,
 			timeout : timeout_seconds,
 			is_parallel : false,
 			suite : 'debug-tests')