On 3/14/22 15:08, Amber, Kumar wrote:
> Hi Ilya,
>
> Thanks a lot for the valuable feedback.
> Please find the reply inline.
>
>> -----Original Message-----
>> From: Ilya Maximets <[email protected]>
>> Sent: Saturday, March 12, 2022 3:57 AM
>> To: Amber, Kumar <[email protected]>; [email protected]
>> Cc: [email protected]; [email protected]
>> Subject: Re: [ovs-dev] [PATCH v1 0/2] MFEX datapath independent Tests
>>
>> On 2/25/22 12:46, Kumar Amber wrote:
>>> The Patchset Introduces creating of varied lenght packets for testing
>>> MFEX AVX512 implementations as well as Introduces unit-tests which are
>>> independet from ovs-datapath. This also allows us to test a lot more
>>> scenarios testing only MFEX.
>>
>> Hi. First of all, thanks for working on these tests, we really need to
>> improve
>> them.
>>
>> However, beside the fact that test-mfex.c fails to build, we already have
>> this
>> kind of a test. It's called test-flows.c.
>> And we already have a script to generate various packets for that test called
>> flowgen.py.
>>
>> We actually have 2 tests of that kind, the second one is used for fuzzing
>> with the
>> OSS-Fuzz project and located at ./tests/oss-fuzz/miniflow_target.c
>>
>> The main problem with current code, as I many times said in different
>> threads,
>> that we're not testing avx512 version of the miniflow_extract with the same
>> packets we're testing the generic implementation, and we're not testing
>> generic
>> implementation with the traffic we're testing avx512 implementation with.
>> Also
>> we're not testing avx512 code with all the traffic generated in a large
>> variety of
>> common unit tests.
>>
>> This patch set creates a test independent not from the datapath, but from
>> ovs-
>> vswitchd. That might be OK on it's own, but it doesn't really improve the
>> testing
>> situation as we're still not testing datapath-specific avx512 implementations
>> with the flows and packets we're testing generic code with in other existing
>> tests. tests/test-mfex.c seems just a replacement for tests we have in
>> system-
>> dpdk.at, but former are not removed in this patch set, so I'm confused. I
>> don't
>> see big advantages of tests/test-mfex.c against existing tests. There are
>> some,
>> but not very important.
>>
>> Another point is that there is no place for randomness in unit tests.
>> Please,
>> analyze the code and come up with a pack of specific test cases that covers
>> normal paths (positive and
>> negative) and all important corner cases. If there are specific problems you
>> faced before, add regression tests for these specific cases. Tests that may
>> or
>> may not fail are not good unit tests and probably are not unit tests at all.
>>
>> For the "datapath independent" testing, please, make the main code
>> independent from the datapath, so it can be tested with existing test tools
>> (test-
>> flows.c, miniflow_target.c, other unit and system tests). There is no
>> logical
>> dependency between flow parsing and the datapath implementation, it should
>> not be very hard to decouple them.
>
> Again, thanks a lot for the reviews and valuable feedback for improving the
> testing of AVX512 code-path. Based on the feedback would it acceptable to
> change the existing test-flows.c implementation from the present direct call
> to miniflow-extract function to a call to MFEX Auto-validator. This will
> ensure we test all the AVX512 path which is being for scalar in the unit test
> and this will not affect the existing scalar code path as well.
This is only one test of thousands we have in a testsuite. The key point
is to have everything affected, so we don't need to change every single test.
This means that the decision to call a particular implementation should be
made by and only by the miniflow_extract() function itself. All other parts
of OVS, including all the datapath implementations, should just use the common
{flow,miniflow}_extract().
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
