Hi Ilya, Thanks for all the comments. I will do the needful and provide next version of the patch soon. Regards, Somnath -----Original Message----- From: Ilya Maximets <i.maxim...@ovn.org> Sent: 16 June 2025 17:00 To: Somnath Chatterjee B <somnath.b.chatter...@ericsson.com>; ovs-dev@openvswitch.org Cc: i.maxim...@ovn.org; Aaron Conole <acon...@redhat.com> Subject: Re: [ovs-dev] [PATCH v3] flow:Ovs shouldn't consider L4 header for frag pkt.
[You don't often get email from i.maxim...@ovn.org. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] On 6/16/25 12:51 PM, Somnath Chatterjee via dev wrote: > When IP layer fragments packets, L4 header > is only present in the first packet. The next fragmented > packets donot contain L4 headers. So hash calculation based > on 5tuple should ignore the L4 headers for all fragmented > packets. > If we consider L4 header for any fragments , say first or > subsequent ones, the hash will vary which may result in > selecting different dpdk port for egressing out packet. > > Signed-off-by: Somnath Chatterjee <somnath.b.chatter...@ericsson.com> > --- > lib/flow.c | 31 +++++++++++++++++++------------ > 1 file changed, 19 insertions(+), 12 deletions(-) Hi, Somnath. Thanks for the patch. We had a few previous attempts to address the same issue: https://patchwork.ozlabs.org/project/openvswitch/list/?submitter=83637&state=* Please, take a look at these changes and the review comments and adjust your patch accordingly. TL;DR: 1. We need to change all the hash functions in this file, so they all provide exactly the same results, i.e. we need to change minilow and symmetric varians of the functions as well. 2. We need new unit tests for this issue in e.g. tests/tunnel-push-pop* files. Also, please, do not add leading spaces in the commit message, the lines should start from the beginning, without extra indentation. See couple of small comments below as well. Best regards, Ilya Maximets. > > diff --git a/lib/flow.c b/lib/flow.c > index c2e1e4ad6..d01fa103f 100644 > --- a/lib/flow.c > +++ b/lib/flow.c > @@ -1075,10 +1075,12 @@ miniflow_extract(struct dp_packet *packet, struct > miniflow *dst) > miniflow_push_be16(mf, tp_dst, tcp->tcp_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); > - } else if (dl_type == htons(ETH_TYPE_IPV6)) { > - dp_packet_update_rss_hash_ipv6_tcp_udp(packet); > + if (OVS_LIKELY(!nw_frag)) { We should check only the FLOW_NW_FRAG_MASK bits. > + if (dl_type == htons(ETH_TYPE_IP)) { > + 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); > + } > } > dp_packet_ol_l4_csum_check_partial(packet); > if (dp_packet_l4_checksum_good(packet) > @@ -1095,10 +1097,12 @@ miniflow_extract(struct dp_packet *packet, struct > miniflow *dst) > miniflow_push_be16(mf, tp_dst, udp->udp_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); > - } else if (dl_type == htons(ETH_TYPE_IPV6)) { > - dp_packet_update_rss_hash_ipv6_tcp_udp(packet); > + if (OVS_LIKELY(!nw_frag)) { Same here. > + if (dl_type == htons(ETH_TYPE_IP)) { > + 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); > + } > } > dp_packet_ol_l4_csum_check_partial(packet); > if (dp_packet_l4_checksum_good(packet) > @@ -2426,10 +2430,13 @@ flow_hash_5tuple(const struct flow *flow, uint32_t > basis) > goto out; > } > > - /* Add both ports at once. */ > - hash = hash_add(hash, > - ((const uint32_t *)flow)[offsetof(struct flow, > tp_src) > - / sizeof(uint32_t)]); > + /* Add both L4 ports, only if the packet is not fragmented. */ > + if (OVS_LIKELY(!(flow->nw_frag & FLOW_NW_FRAG_MASK))) { > + hash = hash_add(hash, > + ((const uint32_t *) flow) > + [offsetof(struct flow, tp_src) / > + sizeof(uint32_t)]); > + } Don't need to move this code under the if. Just 'goto out' before checking the protocol. > } > out: > return hash_finish(hash, 42); /* Arbitrary number. */ _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
