Hi Cian, Thanks for the comments please see my replies inline.
> -----Original Message----- > From: Ferriter, Cian <cian.ferri...@intel.com> > Sent: Monday, April 25, 2022 5:05 PM > To: Amber, Kumar <kumar.am...@intel.com>; ovs-dev@openvswitch.org; > i.maxim...@ovn.org > Cc: Stokes, Ian <ian.sto...@intel.com>; echau...@redhat.com; > ktray...@redhat.com; f...@sysclose.org; Van Haaren, Harry > <harry.van.haa...@intel.com> > Subject: RE: [PATCH v2 0/4] Miniflow Extract Testing Improvements > > > > > -----Original Message----- > > From: Amber, Kumar <kumar.am...@intel.com> > > Sent: Sunday 27 March 2022 08:09 > > To: ovs-dev@openvswitch.org > > Cc: Stokes, Ian <ian.sto...@intel.com>; echau...@redhat.com; > > ktray...@redhat.com; i.maxim...@ovn.org; Ferriter, Cian > > <cian.ferri...@intel.com>; f...@sysclose.org; Van Haaren, Harry > > <harry.van.haa...@intel.com>; Amber, Kumar <kumar.am...@intel.com> > > Subject: [PATCH v2 0/4] Miniflow Extract Testing Improvements > > > > The patch-set introduces changes which would improve the testing of > > miniflow_extract for AVX512 based miniflow_extract optimizations > > whithout affecting scalar code path. > > > > Kumar Amber (4): > > dpif-netdev: Refactor per thread recirc data allocation. > > dpif-netdev: Refactor hashing function. > > Miniflow_extract: Refactor miniflow_extract into api. > > miniflow_extract: Add autovalidator support to miniflow_extract. > > > > lib/dpif-netdev-avx512.c | 2 +- > > lib/dpif-netdev-private-dpcls.h | 23 ++++++++++++++ > > lib/dpif-netdev-private-dpif.c | 2 ++ > > lib/dpif-netdev-private-dpif.h | 5 +++ > > lib/dpif-netdev-private-extract.c | 6 ++-- > > lib/dpif-netdev.c | 27 +--------------- > > lib/flow.c | 52 +++++++++++++++++++++++++------ > > lib/flow.h | 6 +++- > > ofproto/ofproto.c | 10 +++--- > > 9 files changed, 87 insertions(+), 46 deletions(-) > > > > -- > > 2.25.1 > > Adding Ilya to the To: line since I looked at his feedback from the v1 of this > patch set to see if it was addressed. > > Hi Amber and Ilya, > > I've been looking at and testing this patch set to see what improvements it > brings to MFEX testing. I intentionally introduced errors to both generic and > AVX512 implementations of miniflow extract to check what the testing was > picking up on. > > Thanks to the patch set, when I build with the '--enable-mfex-default- > autovalidator' at configure time, the intentionally introduced errors in the > AVX512 implementations of miniflow extract are found. > The 'make check' tests return 98 failures. > > I looked closely at the 'flow extractor' unit test that Ilya mentioned > previously. It's in 'tests/test-flows.c', uses flows and packets generated > from > 'tests/flowgen.py' and can be run 2 ways: > make check TESTSUITEFLAGS='-k extractor -v' > OR > $OVS_DIR/tests/ovstest test-flows flows pcap > > The intentionally introduced errors in the AVX512 implementations of > miniflow extract are found. All packets used in 'test/test-flows.c' are passed > through the autovalidator. The errors are reported by the miniflow extract > autovalidator, not the actual 'test/test-flows.c' checks. > I did have to fix one segfault happening in autovalidator because of the new > way in which this patch set uses it. I'll highlight the cause of this > segfault in > the actual patch (patch 4/4). > > So with these changes, we are testing the avx512 versions of the > miniflow_extract with all the packets we're testing the generic > implementation with. However, since we are not removing the MFEX related > .at tests, we still test the avx512 versions of miniflow extract with more > packets than the generic implementation. > @Ilya, I can see in your feedback on the v1, you would like to see the .at > miniflow extract traffic tests to be removed as well as the 'mfex_fuzzy.py' > file. I see value in what this patch set is doing, the non-fuzzy .at miniflow > extract test is covered by the 'test-flows.c' test since the same types of > packets are covered. The fuzz testing .at miniflow extract test still provides > additional value and removing it would reduce testing coverage. > > This patch set adds value as is but I can see 2 shortcomings with the test > coverage of this patch set if we want to remove the .at miniflow extract > traffic tests and the 'mfex_fuzzy.py' file: > 1. If the eventual goal is to remove the use of 'mfex_fuzzy.py', is > 'flowgen.py' > viable for all the AVX512 miniflow extract testing? I'm just thinking about > testing IPv6 in the future when AVX512 implementations are added for it. > 'flowgen.py' doesn't currently generate IPv6 packets. Is adding that support > easy? I think it's a great idea because we can benefit all implementations of > miniflow extract with improvements to 'flowgen.py', but I find Scapy is the > easiest tool for generating PCAPs. > 2. We lose fuzzy style testing of the AVX512 miniflow extract > implementations. > > @Amber and Ilya, thoughts on the above shortcomings? > I do agree with the idea to keep both the testing mechanisms in place as each one provides its own merit and shortcomings. Since the optimized versions of MFEX is based on the bit-manipulations in form of AVX512 and other intrinsic functions thus validating the AVX512 functions with large fuzzing and random packets generated for various values of packet lengths, payloads and different values in header gives us higher confidence to find any bug at functional level. Keeping that in mind it's important we keep both the tests both will provide excellent coverage and protection against any breakage or change. Also flow gen doesn't use a modern industry standard tool like Scapy to generate traffic and is harder to work with. The intention to keep the mfex_fuzzy.py script is to keep it advancing to generate more and more complex varied traffic types of scenarios to test the AVX512 bit-wise results thoroughly and also use to test IPv6 and Vxlan and other tunnels in future. I think this is easier with Scapy and the mfex_fuzzy.py script. Regards Amber > Thanks, > Cian _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
