On 11/4/22 14:03, Hemanth Aramadaka wrote: > Hi , > > Thanks for the review and comments. I have addressed the comments and raised > the review request again. > > Ran these tests (tunnel_push_pop - packet_out and tunnel_push_pop - > packet_out debug_slow ). I am attaching the test results as well here.
Hi, sorry for not being clear, but I was talking about creating a new test, not just running existing ones. Existing tests do not cover the use case you're trying to fix. Best regards, Ilya Maximets. > > > R620-10-CSSCI-5:/home/sdn/zarahem/ovs # make check TESTSUITEFLAGS='795' > make check-am > make[1]: Entering directory '/home/sdn/zarahem/ovs' > make utilities/ovs-appctl-bashcomp.bash utilities/ovs-vsctl-bashcomp.bash > tests/atlocal tests/testpki-cacert.pem tests/testpki-cert.pem > tests/testpki-privkey.pem tests/testpki-req.pem tests/testpki-cert2.pem > tests/testpki-privkey2.pem tests/testpki-req2.pem > make[2]: Entering directory '/home/sdn/zarahem/ovs' > make[2]: Nothing to be done for 'utilities/ovs-appctl-bashcomp.bash'. > make[2]: Nothing to be done for 'utilities/ovs-vsctl-bashcomp.bash'. > make[2]: 'tests/atlocal' is up to date. > make[2]: 'tests/testpki-cacert.pem' is up to date. > make[2]: 'tests/testpki-cert.pem' is up to date. > make[2]: 'tests/testpki-privkey.pem' is up to date. > make[2]: 'tests/testpki-req.pem' is up to date. > make[2]: 'tests/testpki-cert2.pem' is up to date. > make[2]: 'tests/testpki-privkey2.pem' is up to date. > make[2]: 'tests/testpki-req2.pem' is up to date. > make[2]: Leaving directory '/home/sdn/zarahem/ovs' > make check-local > make[2]: Entering directory '/home/sdn/zarahem/ovs' > set /bin/sh './tests/testsuite' -C tests > AUTOTEST_PATH=utilities:vswitchd:ovsdb:vtep:tests:ipsec::; \ > "$@" 795 || \ > (test -z "$(find /home/sdn/zarahem/ovs/tests/testsuite.dir -name 'asan.*')" > && \ > test -z "$(find /home/sdn/zarahem/ovs/tests/testsuite.dir -name 'ubsan.*')" > && \ > test X'' = Xyes && "$@" --recheck) > Illegal "police" > ## ------------------------------ ## > ## openvswitch 3.0.90 test suite. ## > ## ------------------------------ ## > 795: tunnel_push_pop - packet_out ok > > ## ------------- ## > ## Test results. ## > ## ------------- ## > > 1 test was successful. > make[2]: Leaving directory '/home/sdn/zarahem/ovs' > make[1]: Leaving directory '/home/sdn/zarahem/ovs' > R620-10-CSSCI-5:/home/sdn/zarahem/ovs # make check TESTSUITEFLAGS='796' > make check-am > make[1]: Entering directory '/home/sdn/zarahem/ovs' > make utilities/ovs-appctl-bashcomp.bash utilities/ovs-vsctl-bashcomp.bash > tests/atlocal tests/testpki-cacert.pem tests/testpki-cert.pem > tests/testpki-privkey.pem tests/testpki-req.pem tests/testpki-cert2.pem > tests/testpki-privkey2.pem tests/testpki-req2.pem > make[2]: Entering directory '/home/sdn/zarahem/ovs' > make[2]: Nothing to be done for 'utilities/ovs-appctl-bashcomp.bash'. > make[2]: Nothing to be done for 'utilities/ovs-vsctl-bashcomp.bash'. > make[2]: 'tests/atlocal' is up to date. > make[2]: 'tests/testpki-cacert.pem' is up to date. > make[2]: 'tests/testpki-cert.pem' is up to date. > make[2]: 'tests/testpki-privkey.pem' is up to date. > make[2]: 'tests/testpki-req.pem' is up to date. > make[2]: 'tests/testpki-cert2.pem' is up to date. > make[2]: 'tests/testpki-privkey2.pem' is up to date. > make[2]: 'tests/testpki-req2.pem' is up to date. > make[2]: Leaving directory '/home/sdn/zarahem/ovs' > make check-local > make[2]: Entering directory '/home/sdn/zarahem/ovs' > set /bin/sh './tests/testsuite' -C tests > AUTOTEST_PATH=utilities:vswitchd:ovsdb:vtep:tests:ipsec::; \ > "$@" 796 || \ > (test -z "$(find /home/sdn/zarahem/ovs/tests/testsuite.dir -name 'asan.*')" > && \ > test -z "$(find /home/sdn/zarahem/ovs/tests/testsuite.dir -name 'ubsan.*')" > && \ > test X'' = Xyes && "$@" --recheck) > Illegal "police" > ## ------------------------------ ## > ## openvswitch 3.0.90 test suite. ## > ## ------------------------------ ## > 796: tunnel_push_pop - packet_out debug_slow ok > > ## ------------- ## > ## Test results. ## > ## ------------- ## > > 1 test was successful. > make[2]: Leaving directory '/home/sdn/zarahem/ovs' > make[1]: Leaving directory '/home/sdn/zarahem/ovs' > > > Thanks, > Hemanth. > > -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: 04 November 2022 03:56 > To: Hemanth Aramadaka <[email protected]>; [email protected] > Cc: [email protected] > Subject: Re: [ovs-dev] [PATCH] flow: Consistent VXLAN UDP src ports for > fragmented packets > > On 11/3/22 13:11, Hemanth Aramadaka via dev wrote: >> Issue: >> >> The src-port for UDP is based on RSS hash in the packet metadata. >> In case of packets coming from VM it will be 5-tuple, if available, >> otherwise just IP addresses.If the VM fragments a large IP packet and >> sends the fragments to ovs, only the first fragment will contain the >> L4 header. Therefore, the first fragment and subsequent fragments get >> different UDP src ports in the outgoing VXLAN header.This can lead to >> fragment re-ordering in the fabric as packet will take different >> paths. >> >> Fix: >> >> Intention of this is to avoid fragment packets taking different paths. >> For example, due to presence of firewalls, fragment packets will take >> different paths and will get dropped.To avoid this we ignore the L4 >> header during hash calculation only in the case of fragmented packets. >> >> Signed-off-by: Hemanth Aramadaka <[email protected]> >> --- > > Hi. Thanks for the update. > > Nit: Please, add a version number to the patch subject when sending a new > version. E.g. this one should actually be '[PATCH v3]'. > > And here between the '---' and the summary of the changed you may put some > version history describing what changed between the versions. > > Come more comments inline. > > >> lib/flow.c | 17 ++++++++++++++--- >> 1 file changed, 14 insertions(+), 3 deletions(-) >> >> diff --git a/lib/flow.c b/lib/flow.c >> index c3a3aa3ce..170c825ab 100644 >> --- a/lib/flow.c >> +++ b/lib/flow.c >> @@ -1018,7 +1018,9 @@ miniflow_extract(struct dp_packet *packet, struct >> miniflow *dst) >> miniflow_push_be16(mf, ct_tp_src, ct_tp_src); >> miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst); >> if (dl_type == htons(ETH_TYPE_IP)) { >> - dp_packet_update_rss_hash_ipv4_tcp_udp(packet); >> + if (!(nw_frag & FLOW_NW_FRAG_MASK)) { >> + dp_packet_update_rss_hash_ipv4_tcp_udp(packet); >> + } >> } else if (dl_type == htons(ETH_TYPE_IPV6)) { >> dp_packet_update_rss_hash_ipv6_tcp_udp(packet); >> } >> @@ -1033,7 +1035,9 @@ miniflow_extract(struct dp_packet *packet, struct >> miniflow *dst) >> miniflow_push_be16(mf, ct_tp_src, ct_tp_src); >> miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst); >> if (dl_type == htons(ETH_TYPE_IP)) { >> - dp_packet_update_rss_hash_ipv4_tcp_udp(packet); >> + if (!(nw_frag & FLOW_NW_FRAG_MASK)) { >> + dp_packet_update_rss_hash_ipv4_tcp_udp(packet); >> + } >> } else if (dl_type == htons(ETH_TYPE_IPV6)) { >> dp_packet_update_rss_hash_ipv6_tcp_udp(packet); >> } >> @@ -2248,7 +2252,7 @@ miniflow_hash_5tuple(const struct miniflow >> *flow, uint32_t basis) >> >> if (flow) { >> ovs_be16 dl_type = MINIFLOW_GET_BE16(flow, dl_type); >> - uint8_t nw_proto; >> + uint8_t nw_proto, nw_frag; >> >> if (dl_type == htons(ETH_TYPE_IPV6)) { >> struct flowmap map = FLOWMAP_EMPTY_INITIALIZER; @@ >> -2270,6 +2274,9 @@ miniflow_hash_5tuple(const struct miniflow *flow, >> uint32_t basis) >> >> nw_proto = MINIFLOW_GET_U8(flow, nw_proto); >> hash = hash_add(hash, nw_proto); >> + if (nw_frag & FLOW_NW_FRAG_MASK) { > > The 'nw_frag' is not initialized here, so the code doesn't even compile > without warnings: > > lib/flow.c:2277:13: error: variable 'nw_frag' is uninitialized when used here > [-Werror,-Wuninitialized] > if (nw_frag & FLOW_NW_FRAG_MASK) { > ^~~~~~~ > > >> + goto out; >> + } >> if (nw_proto != IPPROTO_TCP && nw_proto != IPPROTO_UDP >> && nw_proto != IPPROTO_SCTP && nw_proto != IPPROTO_ICMP >> && nw_proto != IPPROTO_ICMPV6) { @@ -2292,6 +2299,7 @@ >> flow_hash_5tuple(const struct flow *flow, uint32_t basis) { >> BUILD_ASSERT_DECL(FLOW_WC_SEQ == 42); >> uint32_t hash = basis; >> + uint8_t nw_frag; >> >> if (flow) { >> >> @@ -2312,6 +2320,9 @@ flow_hash_5tuple(const struct flow *flow, uint32_t >> basis) >> } >> >> hash = hash_add(hash, flow->nw_proto); >> + if (nw_frag & FLOW_NW_FRAG_MASK) { > > Same here. > > > I think, that highlights the necessity of some kind of unit test for this > functionality. > > I described one possible variant of the unit test in my review for the > previous version of the patch here: > > https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/#2896253 > > > Best regards, Ilya Maximets. > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
