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 the changes below make linker to pick-up the proper version of the function
and make acl_autotest to pass for static build too.

diff --git a/app/test/meson.build b/app/test/meson.build
index 867cc5863..4364be932 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -328,6 +328,7 @@ test_dep_objs += cc.find_library('execinfo', required: false)
 link_libs = []
 if get_option('default_library') == 'static'
        link_libs = dpdk_drivers
+       link_libs += dpdk_static_libraries
 endif

 if get_option('tests')
diff --git a/meson.build b/meson.build
index a96486597..df1e1c41c 100644
--- a/meson.build
+++ b/meson.build
@@ -62,6 +62,7 @@ configure_file(output: build_cfg,
 # for static builds, include the drivers as libs and we need to "whole-archive"
 # them.
 dpdk_drivers = ['-Wl,--whole-archive'] + dpdk_drivers + ['-Wl,--no-whole-archive']
+dpdk_static_libraries = ['-Wl,--whole-archive'] + dpdk_static_libraries + ['-Wl,--no-whole-archive']

Not saying that's the proper patch, but just to prove that linking librte_acl.a
with '--whole-archive' does fix the problem.
Bruce, could you point is the best way to fix things here
(my meson knowledge is very limited)?
Do we need extra container here as 'whole_archive_static_libraries[]' or so?
Thanks
Konstantin


> 
> 
> > 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