Re: [dpdk-dev] [PATCH v2 2/2] ci: add test suite run without hugepage

[Aaron Conole <aconole@redhat.com>]

David Marchand <david.marchand@redhat.com> writes:

> On Fri, Mar 6, 2020 at 4:57 PM Aaron Conole <aconole@redhat.com> wrote:
>>
>> Ruifeng Wang <Ruifeng.Wang@arm.com> writes:
>>
>> >> -----Original Message-----
>> >> From: Aaron Conole <aconole@redhat.com>
>> >> Sent: Thursday, March 5, 2020 22:37
>> >> To: Ruifeng Wang <Ruifeng.Wang@arm.com>
>> >> Cc: maicolgabriel@hotmail.com; bruce.richardson@intel.com; dev@dpdk.org;
>> >> david.marchand@redhat.com; Gavin Hu <Gavin.Hu@arm.com>; Honnappa
>> >> Nagarahalli <Honnappa.Nagarahalli@arm.com>; juraj.linkes@pantheon.tech;
>> >> nd <nd@arm.com>
>> >> Subject: Re: [PATCH v2 2/2] ci: add test suite run without hugepage
>> >>
>> >> Ruifeng Wang <Ruifeng.Wang@arm.com> writes:
>> >>
>> >> >> -----Original Message-----
>> >> >> From: Aaron Conole <aconole@redhat.com>
>> >> >> Sent: Thursday, March 5, 2020 01:31
>> >> >> To: Ruifeng Wang <Ruifeng.Wang@arm.com>
>> >> >> Cc: maicolgabriel@hotmail.com; bruce.richardson@intel.com;
>> >> >> dev@dpdk.org; david.marchand@redhat.com; Gavin Hu
>> >> <Gavin.Hu@arm.com>;
>> >> >> Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>;
>> >> >> juraj.linkes@pantheon.tech; nd <nd@arm.com>
>> >> >> Subject: Re: [PATCH v2 2/2] ci: add test suite run without hugepage
>> >> >>
>> >> >> Ruifeng Wang <ruifeng.wang@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.wang@arm.com>
>> >> >> > Reviewed-by: Gavin Hu <gavin.hu@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?
>
> I just hope meson won't create a dependency on hugepage availability...
> Thinking about stuff like:
> https://git.dpdk.org/dpdk/commit/?id=599d67b6a4bf96096352cc5fbc8fc28e54a1ca62

Yes, I agree.  We probably need to do some kind of similar abstraction.