Ruifeng Wang <ruifeng.w...@arm.com> writes:
>> -----Original Message-----
>> From: Aaron Conole <acon...@redhat.com>
>> Sent: Thursday, March 5, 2020 22:37
>> To: Ruifeng Wang <ruifeng.w...@arm.com>
>> Cc: maicolgabr...@hotmail.com; bruce.richard...@intel.com; dev@dpdk.org;
>> david.march...@redhat.com; Gavin Hu <gavin...@arm.com>; Honnappa
>> Nagarahalli <honnappa.nagaraha...@arm.com>; juraj.lin...@pantheon.tech;
>> nd <n...@arm.com>
>> Subject: Re: [PATCH v2 2/2] ci: add test suite run without hugepage
>>
>> Ruifeng Wang <ruifeng.w...@arm.com> writes:
>>
>> >> -----Original Message-----
>> >> From: Aaron Conole <acon...@redhat.com>
>> >> Sent: Thursday, March 5, 2020 01:31
>> >> To: Ruifeng Wang <ruifeng.w...@arm.com>
>> >> Cc: maicolgabr...@hotmail.com; bruce.richard...@intel.com;
>> >> dev@dpdk.org; david.march...@redhat.com; Gavin Hu
>> <gavin...@arm.com>;
>> >> Honnappa Nagarahalli <honnappa.nagaraha...@arm.com>;
>> >> juraj.lin...@pantheon.tech; nd <n...@arm.com>
>> >> Subject: Re: [PATCH v2 2/2] ci: add test suite run without hugepage
>> >>
>> >> Ruifeng Wang <ruifeng.w...@arm.com> writes:
>> >>
>> >> > This test suite is derived from fast-tests suite. Cases in this
>> >> > suite are run with '--no-huge' flag.
>> >> >
>> >> > The suite aims to cover as many as possible test cases out of the
>> >> > fast-tests suites in the environments without huge pages support,
>> >> > like containers.
>> >> >
>> >> > Signed-off-by: Ruifeng Wang <ruifeng.w...@arm.com>
>> >> > Reviewed-by: Gavin Hu <gavin...@arm.com>
>> >> > ---
>> >>
>> >> I like this much more. Few comments.
>> >>
>> >> > .travis.yml | 10 +++++--
>> >> > app/test/meson.build | 71
>> >> > ++++++++++++++++++++++++++++++++++++++++++++
>> >>
>> >> You should update doc/guides/prog_guide/meson_ut.rst to include some
>> >> detail about the new tests suite.
>> >>
>> > Thanks. Will update document in next version.
>> >
>> >> > 2 files changed, 79 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/.travis.yml b/.travis.yml index b64a81bd0..eed1d96db
>> >> > 100644
>> >> > --- a/.travis.yml
>> >> > +++ b/.travis.yml
>> >> > @@ -40,7 +40,7 @@ jobs:
>> >> > - env: DEF_LIB="static"
>> >> > arch: amd64
>> >> > compiler: gcc
>> >> > - - env: DEF_LIB="shared" RUN_TESTS=1
>> >> > + - env: DEF_LIB="shared" RUN_TESTS=1 TEST_SUITES="fast-tests
>> >> nohuge-tests"
>> >> > arch: amd64
>> >> > compiler: gcc
>> >> > - env: DEF_LIB="shared" BUILD_DOCS=1 @@ -63,7 +63,7 @@ jobs:
>> >> > - env: DEF_LIB="static"
>> >> > arch: amd64
>> >> > compiler: clang
>> >> > - - env: DEF_LIB="shared" RUN_TESTS=1
>> >> > + - env: DEF_LIB="shared" RUN_TESTS=1 TEST_SUITES="fast-tests
>> >> nohuge-tests"
>> >> > arch: amd64
>> >> > compiler: clang
>> >> > - env: DEF_LIB="shared" BUILD_DOCS=1 @@ -101,6 +101,9 @@ jobs:
>> >> > - env: DEF_LIB="static"
>> >> > arch: arm64
>> >> > compiler: gcc
>> >> > + - env: DEF_LIB="shared" RUN_TESTS=1 TEST_SUITES="nohuge-tests"
>> >> > + arch: arm64
>> >> > + compiler: gcc
>> >> > - env: DEF_LIB="shared" BUILD_DOCS=1
>> >> > arch: arm64
>> >> > compiler: gcc
>> >> > @@ -124,3 +127,6 @@ jobs:
>> >> > - env: DEF_LIB="shared"
>> >> > arch: arm64
>> >> > compiler: clang
>> >> > + - env: DEF_LIB="shared" RUN_TESTS=1 TEST_SUITES="nohuge-tests"
>> >> > + arch: arm64
>> >> > + compiler: clang
>> >> > diff --git a/app/test/meson.build b/app/test/meson.build index
>> >> > 0a2ce710f..162a1a76f 100644
>> >> > --- a/app/test/meson.build
>> >> > +++ b/app/test/meson.build
>> >> > @@ -237,6 +237,60 @@ fast_test_names = [
>> >> > 'thash_autotest',
>> >> > ]
>> >>
>> >> Shouldn't we also trim the list of fast-tests? Otherwise, these
>> >> tests will run twice.
>> >>
>> > I think you mean to have exclusive lists for fast-tests and nohuge-tests.
>>
>> That's what I was thinking.
>>
>> > Overlapped cases will run twice if both test suites are opted in.
>> > But the two runs are not the same, one runs with hugepage and the
>> > other runs in no-huge mode.
>>
>> Is it really so different between huge and no-huge? Most of the libraries
>> won't care - they call the rte_**alloc functions, and it returns blocks of
>> memory. Maybe I am simplifying it too much.
>>
>> > If fast-tests list is splited, we will need to always run multiple
>> > suites to cover all fast tests.
>> > We can keep x86 to run only fast-tests suite to avoid extra test runs
>> > if they are not necessary. Thoughts?
>>
>> I guess since most DPDK usage will be with hugepages, we should prefer to
>> test with it. I don't care too much about the color of this particular
>> shed. If
>> you want to do it that way, it's okay by me - it gives us the coverage, and
>> doesn't duplicate tests between those environments.
>>
>> BUT it means when we add a new test to the suite, we need to remember to
>> add it in two places - fast_test and nohuge_test. That almost guarantees we
>> will miss tests (because we accidentally don't add it to nohuge). Maybe
>> there's another way, like keep a list of all the tests and some information
>> on
>> whether the test needs hugepages to run. Then if there are no hugepages
>> available, we can write that we SKIP the tests that don't support huge pages.
>> In that way, we don't need two different lists - and if there are hugepages,
>> we will run all the test cases.
>> WDYT?
>>
> Yes. Agree with you that having duplicate tests in suites is error prone.
Cool!
> IIUC, cases in a suite is determined at build time, as well as command
> options to run cases.
> This implies hugepage availability needs to be detected at build time if we
> want to run only
> suitable cases in suite in an environment. It could be something we don't
> want.
>
> I'll trim fast-tests in next version to remove duplicated cases.
I think it might be better to make the array something like (just a
psuedo-code example):
# psuedo-code to check for hugepages
has_hugepages = check_for_hugepages()
...
fast_test_names = [
['acl_autotest', true],
['alarm_autotest', true],
['atomic_autotest', true],
...
Then in the code:
foreach arg : fast_test_names
....
if not arg[1]
test(arg[0], ...)
if has_hugepages and arg[1]
test(arg[0], )
Does it make sense? Do you see a problem?
> Thank you.
>
>> >> ex: https://travis-ci.com/ovsrobot/dpdk/jobs/292037684
>> >>
>> >> > +nohuge_test_names = [
>> >> > + 'byteorder_autotest',
>> >> > + 'cmdline_autotest',
>> >> > + 'common_autotest',
>> >> > + 'cpuflags_autotest',
>> >> > + 'cycles_autotest',
>> >> > + 'debug_autotest',
>> >> > + 'eal_flags_n_opt_autotest',
>> >> > + 'eal_flags_no_huge_autotest',
>> >> > + 'eal_flags_vdev_opt_autotest',
>> >> > + 'eal_fs_autotest',
>> >> > + 'errno_autotest',
>> >> > + 'event_ring_autotest',
>> >> > + 'fib_autotest',
>> >> > + 'fib6_autotest',
>> >> > + 'interrupt_autotest',
>> >> > + 'logs_autotest',
>> >> > + 'lpm_autotest',
>> >> > + 'lpm6_autotest',
>> >> > + 'memcpy_autotest',
>> >> > + 'meter_autotest',
>> >> > + 'per_lcore_autotest',
>> >> > + 'prefetch_autotest',
>> >> > + 'rcu_qsbr_autotest',
>> >> > + 'red_autotest',
>> >> > + 'rib_autotest',
>> >> > + 'rib6_autotest',
>> >> > + 'ring_autotest',
>> >> > + 'rwlock_rda_autotest',
>> >> > + 'rwlock_rds_wrm_autotest',
>> >> > + 'rwlock_rde_wro_autotest',
>> >> > + 'sched_autotest',
>> >> > + 'spinlock_autotest',
>> >> > + 'string_autotest',
>> >> > + 'tailq_autotest',
>> >> > + 'user_delay_us',
>> >> > + 'version_autotest',
>> >> > + 'crc_autotest',
>> >> > + 'delay_us_sleep_autotest',
>> >> > + 'eventdev_common_autotest',
>> >> > + 'fbarray_autotest',
>> >> > + 'ipsec_autotest',
>> >> > + 'kni_autotest',
>> >> > + 'kvargs_autotest',
>> >> > + 'member_autotest',
>> >> > + 'metrics_autotest',
>> >> > + 'power_cpufreq_autotest',
>> >> > + 'power_autotest',
>> >> > + 'power_kvm_vm_autotest',
>> >> > + 'reorder_autotest',
>> >> > + 'service_autotest',
>> >> > + 'thash_autotest',
>> >> > +]
>> >> > +
>> >> > perf_test_names = [
>> >> > 'ring_perf_autotest',
>> >> > 'mempool_perf_autotest',
>> >> > @@ -341,6 +395,10 @@ if dpdk_conf.has('RTE_LIBRTE_RING_PMD')
>> >> > fast_test_names += 'latencystats_autotest'
>> >> > driver_test_names += 'link_bonding_mode4_autotest'
>> >> > fast_test_names += 'pdump_autotest'
>> >> > + nohuge_test_names += 'ring_pmd_autotest'
>> >> > + nohuge_test_names += 'bitratestats_autotest'
>> >> > + nohuge_test_names += 'latencystats_autotest'
>> >> > + nohuge_test_names += 'pdump_autotest'
>> >> > endif
>> >> >
>> >> > if dpdk_conf.has('RTE_LIBRTE_POWER') @@ -430,6 +488,19 @@
>> foreach
>> >> > arg : fast_test_names
>> >> > endif
>> >> > endforeach
>> >> >
>> >> > +foreach arg : nohuge_test_names
>> >> > + if host_machine.system() == 'linux'
>> >> > + test(arg, dpdk_test,
>> >> > + env : ['DPDK_TEST=' + arg],
>> >> > + args : test_args +
>> >> > + ['--no-huge'] + ['-m 1024'] +
>> >> > + ['--file-prefix=@0@'.format(arg)],
>> >> > + timeout : timeout_seconds_fast,
>> >> > + is_parallel : false,
>> >> > + suite : 'nohuge-tests')
>> >> > + endif
>> >> > +endforeach
>> >> > +
>> >> > foreach arg : perf_test_names
>> >> > test(arg, dpdk_test,
>> >> > env : ['DPDK_TEST=' + arg],
