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

Message ID 20190411195229.7841-4-aconole@redhat.com
State Superseded
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 warning coding style issues

Commit Message

Aaron Conole April 11, 2019, 7:52 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>
---
Conflicts with http://patches.dpdk.org/patch/50850/

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

Comments

David Marchand April 12, 2019, 7:46 a.m. | #1
On Thu, Apr 11, 2019 at 9:52 PM Aaron Conole <aconole@redhat.com> 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>
> ---
> Conflicts with http://patches.dpdk.org/patch/50850/
>
>  app/test/meson.build | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
>
> diff --git a/app/test/meson.build b/app/test/meson.build
> index 867cc5863..1010bfbc8 100644
> --- a/app/test/meson.build
> +++ b/app/test/meson.build
> @@ -344,17 +344,32 @@ if get_option('tests')
>         timeout_seconds = 600
>         timeout_seconds_fast = 10
>
> +       # Retreive the number of CPU cores
>

nit: Retrieve

Little concern here on the approach of getting the max available cpu index.
If we have non contiguous cpus (let's say hyper threading is disabled),
this won't work.
But we can just assume this won't happen for non regression setups (vms).


+       num_cores = run_command('lscpu',
> '-p=cpu').stdout().strip().split('\n')[-1]
>

lscpu is a linux command afaik.

Maybe for FreeBSD:
root@freebsd-10:~ # sysctl dev.cpu |cut -d . -f 3 |sort |tail -1
3

Not sure if FreeBSD ensures that the keys names/objects won't change
accross the versions.


+       num_cores_arg = '-l 0-' + 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 +378,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 +387,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 +396,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')
> --
> 2.19.1
>

The rest looks good to me.
Reviewed-by: David Marchand <david.marchand@redhat.com>
Bruce Richardson April 12, 2019, 9:06 a.m. | #2
On Fri, Apr 12, 2019 at 09:46:17AM +0200, David Marchand wrote:
>    On Thu, Apr 11, 2019 at 9:52 PM Aaron Conole <[1]aconole@redhat.com>
>    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 <[2]aconole@redhat.com>
>      ---
>      Conflicts with [3]http://patches.dpdk.org/patch/50850/
>       app/test/meson.build | 24 +++++++++++++++++++++---
>       1 file changed, 21 insertions(+), 3 deletions(-)
>      diff --git a/app/test/meson.build b/app/test/meson.build
>      index 867cc5863..1010bfbc8 100644
>      --- a/app/test/meson.build
>      +++ b/app/test/meson.build
>      @@ -344,17 +344,32 @@ if get_option('tests')
>              timeout_seconds = 600
>              timeout_seconds_fast = 10
>      +       # Retreive the number of CPU cores
> 
>    nit: Retrieve
>    Little concern here on the approach of getting the max available cpu
>    index.
>    If we have non contiguous cpus (let's say hyper threading is disabled),
>    this won't work.
>    But we can just assume this won't happen for non regression setups
>    (vms).
> 
>      +       num_cores = run_command('lscpu',
>      '-p=cpu').stdout().strip().split('\n')[-1]
> 
>    lscpu is a linux command afaik.
>    Maybe for FreeBSD:
>    root@freebsd-10:~ # sysctl dev.cpu |cut -d . -f 3 |sort |tail -1
>    3
>    Not sure if FreeBSD ensures that the keys names/objects won't change
>    accross the versions.
> 

Very similar thoughs/concerns here. I think doing this per OS is probably
safer in the long term - even though lscpu is available for FreeBSD as an
extra package.

My suggestion: for FreeBSD, get the number of CPUs using
	"/sbin/sysctl -n hw.ncpu"
For Linux, rather than using a "0-N" range, use output from 
"cat /sys/devices/system/cpu/present", which will ensure that it works
even in the non-contiguous CPU numbering case. [Though I suspect we get
non-contiguous numbers only in the case a core or two has been hotplugged
out, I think - disabling hyperthreading won't leave gaps, just fewer cores]

/Bruce
Luca Boccassi April 12, 2019, 9:17 a.m. | #3
On Thu, 2019-04-11 at 15:52 -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
> >
> ---
> Conflicts with 
> http://patches.dpdk.org/patch/50850/
> 
> 
>  app/test/meson.build | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 

With the provision that the changes suggested by Bruce and David are
applied,

Acked-by: Luca Boccassi <bluca@debian.org>

Patch

diff --git a/app/test/meson.build b/app/test/meson.build
index 867cc5863..1010bfbc8 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -344,17 +344,32 @@  if get_option('tests')
 	timeout_seconds = 600
 	timeout_seconds_fast = 10
 
+	# Retreive the number of CPU cores
+	num_cores = run_command('lscpu', '-p=cpu').stdout().strip().split('\n')[-1]
+	num_cores_arg = '-l 0-' + 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 +378,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 +387,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 +396,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')