Hi Ilya, Thanks for the comments. Replies Inline.
> -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Thursday, March 17, 2022 6:21 PM > 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 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(). > So, as I Understand it, the goal is to either put the auto-validator inside scalar miniflow_extract or to modify the miniflow_extract signature itself to be hidden inside an API which can be chosen based on some #ifdefs and selected as auto-validator in a test-builds or simple miniflow_extract in a release build. If this is so I can do a POC and come back for feedback of the same. Regards Amber > Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
