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. > > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
