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