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.
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 <i.maxim...@ovn.org>
Sent: 04 November 2022 03:56
To: Hemanth Aramadaka <hemant...@acldigital.com>; ovs-dev@openvswitch.org
Cc: i.maxim...@ovn.org
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 <hemant...@acldigital.com>
> ---
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/20220315143007.6765-1-hemant...@acldigital.com/#2896253
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
