Re: [dpdk-dev] [PATCH 3/3] acl: adjust the tests

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> > Hi Aaron,
> >
> >>
> >> This makes the tests pass, and also ensures that on platforms where the
> >> testing is supported, we can properly test the implementation specific
> >> code.  One edge case is when we run on x86_64 systems that don't support
> >> AVX2, but where the compiler can generate such instructions.  That could
> >> be an enhancement in the future, but for now at least the tests will
> >> pass.
> >>
> >> Signed-off-by: Aaron Conole <aconole@redhat.com>
> >> ---
> >>  app/test/test_acl.c             | 62 +++++++++++++--------------------
> >>  lib/librte_acl/Makefile         |  1 +
> >>  lib/librte_acl/acl_run_notsup.c | 46 ++++++++++++++++++++++++
> >>  lib/librte_acl/meson.build      |  4 +--
> >>  4 files changed, 73 insertions(+), 40 deletions(-)
> >>  create mode 100644 lib/librte_acl/acl_run_notsup.c
> >>
> >> diff --git a/app/test/test_acl.c b/app/test/test_acl.c
> >> index b1f75d1bc..c44faa251 100644
> >> --- a/app/test/test_acl.c
> >> +++ b/app/test/test_acl.c
> >> @@ -408,6 +408,9 @@ test_classify(void)
> >>  		return -1;
> >>  	}
> >>
> >> +	/* Always use the scalar testing for now. */
> >> +	rte_acl_set_ctx_classify(acx, RTE_ACL_CLASSIFY_SCALAR);
> >> +
> >>  	ret = 0;
> >>  	for (i = 0; i != TEST_CLASSIFY_ITER; i++) {
> >>
> >> @@ -547,6 +550,7 @@ test_build_ports_range(void)
> >>  	for (i = 0; i != RTE_DIM(test_data); i++)
> >>  		data[i] = (uint8_t *)&test_data[i];
> >>
> >> +	rte_acl_set_ctx_classify(acx, RTE_ACL_CLASSIFY_SCALAR);
> >>  	for (i = 0; i != RTE_DIM(test_rules); i++) {
> >>  		rte_acl_reset(acx);
> >>  		ret = test_classify_buid(acx, test_rules, i + 1);
> >> @@ -911,6 +915,8 @@ test_convert_rules(const char *desc,
> >>  		return -1;
> >>  	}
> >>
> >> +	rte_acl_set_ctx_classify(acx, RTE_ACL_CLASSIFY_SCALAR);
> >> +
> >
> > As I understand here and above, on x86 you replaced default algo (SSE, AVX2)
> > with scalar one, right?
> > That looks like reduction of test coverage for x86.
> 
> In one way, you're right.  However, the tests weren't testing what they
> purported anyway.

Could you explain a bit more here?
What I am seeing: tests were running bot sse(or avx2) and scalar classify() method.
Now they always running scalar only.
To me it definitely looks like reduction in coverage.

>  Actually, it's just a shift I think (previously, it
> would have tested the AVX2 but I don't see AVX2 having a fallback into
> the SSE code - unlike the SSE code falling back into scalar).

Not sure I understand you here.
What fallback for AVX2 you expect that you think is missing?

> 
> The tests were failing for a number of reasons when built with meson,

Ok, but with legacy build system (make) on x86 all tests passes, right?
So the problem is in new build system, not in the test itself.
Why we should compromise our test coverage to make it work with
new tools? 
That just hides the problem without fixing it.
Instead I think the build system needs to be fixed.
Looking at it a bit closely, for .so meson+ninja generates code with
correct version of the function:
 
nm x86_64-native-linuxapp-gcc-meson/lib/librte_acl.so.2 | grep acl_classify_sse
000000000000fa50 t rte_acl_classify_sse

So for 'meson -Ddefault_library=shared'
acl_autotest passes without the problem. 

Though for static lib we have both:
nm x86_64-native-linuxapp-gcc-meson/lib/librte_acl.a | grep acl_classify_sse
0000000000000000 W rte_acl_classify_sse
0000000000004880 T rte_acl_classify_sse

And then linker pickups the wrong one:
nm x86_64-native-linuxapp-gcc-meson/app/test/dpdk-test | grep acl_classify_sse
00000000005f6100 W rte_acl_classify_sse

While for make:
$ nm x86_64-native-linuxapp-gcc-aesmb/lib/librte_acl.a | grep acl_classify_sse 
0000000000000000 W rte_acl_classify_sse
0000000000004880 T rte_acl_classify_sse
$ nm x86_64-native-linuxapp-gcc-aesmb/app/test | grep acl_classify_sse
0000000000240440 T rte_acl_classify_sse

Linker pickups the right one.


> and on the systems I tested with (including tests under travis).
> 
> 1. Any meson build that I observed didn't correctly fill anything but
>    the scalar variable.  I had to remove the -ENOTSUP definitions in the
>    rte_acl.c file (forgot to git add it), and make the second version.
> 
> 2. The tests never selected scalar, or nor sse implementations. 

As I can see test_classify_run() *always* run both default method (sse/avx2 on x86)
and then scalar one. 

>  Rather,
>    they selected only what the currently running platform provided.
>    This meant that I was always seeing the AVX2 code executed, but never
>    SSE nor scalar (but for one case) - at least as far as I could see.
> 
> There were others - I iterated on these for a few days.
> 
> This is why I changed a block to run through each implementation in one
> of the versions.
> 
> HOWEVER, it's still deficient.
> 
> We need to fully cover all the cases.  BUT it's better than the failure
> that currently happens on almost every system I've tried - including
> shipping the build to travis to run.  So, I figured running > failing with
> almost no reason why.  And looking into the failure revealed that the
> meson build didn't even include the platform specific builds.
> 
> During my rework, I can change the test cases to iterate as in other
> test cases.  It will extend the time.  And I don't know how to resolve
> the case where we run on a system that doesn't support AVX2 but we have
> a compiler that supports AVX2 (since that case will fail - but we
> shouldn't even attempt it).

I don't see why that should happen.
At rte_acl_init() we do check does that machine supports AVX2(SSE, NEON) 
instructions or not.
Are you saying under some circumstances rte_acl_init() are not working properly,
or not get invoked?

Konstantin