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.
> >
> 
> I assume the same issues occurs for AVX2, 

Yes, I just used sse because it is always available on x86. 

but for SSE specifically why do we even compile up two copies of the function for x86 platforms,
> since SSE will always be supported?

for non IA  platforms.
Konstantin