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

[Aaron Conole <aconole@redhat.com>]

"Ananyev, Konstantin" <konstantin.ananyev@intel.com> writes:

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

The tests were failing for a number of reasons when built with meson,
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.  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).

WDYT?

> Konstantin