On 10/30/23 11:03, Hemanth Aramadaka wrote: > Hi Ilya, > > The changes are not required for flow_hash_5tuple() function. I have added > debug statements, and we are not hitting this function as part of this code > flow.
It doesn't matter if you hit this particular function in your scenario or not. All the hash functions must produce the same result. Best regards, Ilya Maximets. > > Thanks, > Hemanth. > > -----Original Message----- > From: Ilya Maximets <[email protected]> > Sent: Monday, June 26, 2023 7:59 PM > To: Hemanth Aramadaka <[email protected]>; 'Simon Horman' > <[email protected]>; [email protected] > Cc: [email protected] > Subject: Re: [ovs-dev] [PATCH] flow: Consistent VXLAN UDP src ports for > fragmented packets > > On 6/26/23 08:59, Hemanth Aramadaka via dev wrote: >> Hi Simon, >> >> Sorry for the late response. Yes, the changes are specific to IPV6 >> protocol only. > > Please, fix the flow_hash_5tuple() function as well. All variants of the > same hash function should give the same result. For some reason you just > removed the fix for this function from the patch instead of addressing a > review comment. > > And add the unit test for the issue. > > Thanks. > Best regards, Ilya Maximets. > >> >> Thanks, >> Hemanth. >> >> -----Original Message----- >> From: Simon Horman <[email protected]> >> Sent: Friday, January 20, 2023 8:52 PM >> To: [email protected] >> Cc: Hemanth Aramadaka <[email protected]> >> Subject: Re: [ovs-dev] [PATCH] flow: Consistent VXLAN UDP src ports >> for fragmented packets >> >> On Thu, Jan 05, 2023 at 06:55:03PM +0530, 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]> >>> --- >>> lib/flow.c | 18 +++++++++++++++--- >>> 1 file changed, 15 insertions(+), 3 deletions(-) >>> >>> diff --git a/lib/flow.c b/lib/flow.c >>> index c3a3aa3ce..4f4197b1c 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,14 @@ miniflow_hash_5tuple(const struct miniflow *flow, >>> uint32_t basis) >>> >>> nw_proto = MINIFLOW_GET_U8(flow, nw_proto); >>> hash = hash_add(hash, nw_proto); >>> + >>> + /* Skip l4 header fields if IP packet is fragmented since >>> + * only first fragment will carry l4 header. >>> + */ >>> + nw_frag = MINIFLOW_GET_U8(flow, nw_frag); >>> + if (nw_frag & FLOW_NW_FRAG_MASK) { >>> + goto out; >>> + } >> >> Maybe I am reading things wrong, but it seems to me that this change >> seems to effect all protocols, and in particular both >> IPv4 and IPv6. Whereas the changes above to miniflow_extract() only >> affect IPv6. Is this intentional? >> >>> if (nw_proto != IPPROTO_TCP && nw_proto != IPPROTO_UDP >>> && nw_proto != IPPROTO_SCTP && nw_proto != IPPROTO_ICMP >>> && nw_proto != IPPROTO_ICMPV6) { >>> -- >>> 2.34.1 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
