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

[Bruce Richardson <bruce.richardson@intel.com>]

On Tue, Apr 09, 2019 at 07:29:09PM +0100, Ananyev, Konstantin wrote:
> 
> > >
> > > > > 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.

Yes, I realise that, but there is no point in compiling the weak version
for IA platforms, since the normal version will be guaranteed available. In
any case, it doesn't matter much, since the issue needs fixing for AVX2
anyway.

/Bruce