On 8/1/23 12:51, Han Zhou wrote: > On Tue, Jul 18, 2023 at 6:16 AM Dumitru Ceara <[email protected]> wrote: >> >> On 7/18/23 12:14, Han Zhou wrote: >>> On Mon, Jul 17, 2023 at 9:51 PM Dumitru Ceara <[email protected]> wrote: >>>> >>>> On 7/14/23 08:41, Ales Musil wrote: >>>>> On Thu, Jul 13, 2023 at 4:39 PM Dumitru Ceara <[email protected]> > wrote: >>>>> >>>>>> Regular dp-hash is not a canonical L4 hash (at least with the netlink >>>>>> datapath). If the datapath supports l4 symmetrical dp-hash use that >>> one >>>>>> instead. >>>>>> >>>>>> Reported-at: https://github.com/ovn-org/ovn/issues/112 >>>>>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2188679 >>>>>> Signed-off-by: Dumitru Ceara <[email protected]> >>>>>> >>>>> >>>>> Hi Dumitru, >>>>> >>>>> >>>>>> --- >>>>>> include/ovn/features.h | 2 ++ >>>>>> lib/actions.c | 6 ++++++ >>>>>> lib/features.c | 49 > +++++++++++++++++++++++++++++++++--------- >>>>>> 3 files changed, 47 insertions(+), 10 deletions(-) >>>>>> >>>>>> diff --git a/include/ovn/features.h b/include/ovn/features.h >>>>>> index de8f1f5489..3bf536127f 100644 >>>>>> --- a/include/ovn/features.h >>>>>> +++ b/include/ovn/features.h >>>>>> @@ -34,12 +34,14 @@ enum ovs_feature_support_bits { >>>>>> OVS_CT_ZERO_SNAT_SUPPORT_BIT, >>>>>> OVS_DP_METER_SUPPORT_BIT, >>>>>> OVS_CT_TUPLE_FLUSH_BIT, >>>>>> + OVS_DP_HASH_L4_SYM_BIT, >>>>>> }; >>>>>> >>>>>> enum ovs_feature_value { >>>>>> OVS_CT_ZERO_SNAT_SUPPORT = (1 << OVS_CT_ZERO_SNAT_SUPPORT_BIT), >>>>>> OVS_DP_METER_SUPPORT = (1 << OVS_DP_METER_SUPPORT_BIT), >>>>>> OVS_CT_TUPLE_FLUSH_SUPPORT = (1 << OVS_CT_TUPLE_FLUSH_BIT), >>>>>> + OVS_DP_HASH_L4_SYM_SUPPORT = (1 << OVS_DP_HASH_L4_SYM_BIT), >>>>>> }; >>>>>> >>>>>> void ovs_feature_support_destroy(void); >>>>>> diff --git a/lib/actions.c b/lib/actions.c >>>>>> index 037172e606..9d52cb75a8 100644 >>>>>> --- a/lib/actions.c >>>>>> +++ b/lib/actions.c >>>>>> @@ -1625,6 +1625,12 @@ encode_SELECT(const struct ovnact_select >>> *select, >>>>>> struct ds ds = DS_EMPTY_INITIALIZER; >>>>>> ds_put_format(&ds, "type=select,selection_method=dp_hash"); >>>>>> >>>>>> + if (ovs_feature_is_supported(OVS_DP_HASH_L4_SYM_SUPPORT)) { >>>>>> + /* Select dp-hash l4_symmetric by setting the upper 32bits > of >>>>>> + * selection_method_param to 1: */ >>>>>> >>>>> >>>>> This comment is a bit unfortunate, because it reads like you want to > set >>>>> all bits of the upper half >>>>> to 1 e.g. 0xffffffff. Maybe change it to: "selection_method_param to >>> value >>>>> 1 (1 << 32)." during merge, wdyt? >>>>> >>>>> >>>> >>>> Makes sense, thanks for the suggestion! I changed it accordingly. >>>> >>>>>> + ds_put_cstr(&ds, ",selection_method_param=0x100000000"); >>>>>> + } >>>>>> + >>>>>> struct mf_subfield sf = expr_resolve_field(&select->res_field); >>>>>> >>>>>> for (size_t bucket_id = 0; bucket_id < select->n_dsts; >>> bucket_id++) { >>>>>> diff --git a/lib/features.c b/lib/features.c >>>>>> index 97c9c86f00..d24e8f6c5c 100644 >>>>>> --- a/lib/features.c >>>>>> +++ b/lib/features.c >>>>>> @@ -21,6 +21,7 @@ >>>>>> #include "lib/dirs.h" >>>>>> #include "socket-util.h" >>>>>> #include "lib/vswitch-idl.h" >>>>>> +#include "odp-netlink.h" >>>>>> #include "openvswitch/vlog.h" >>>>>> #include "openvswitch/ofpbuf.h" >>>>>> #include "openvswitch/rconn.h" >>>>>> @@ -33,20 +34,48 @@ VLOG_DEFINE_THIS_MODULE(features); >>>>>> >>>>>> #define FEATURES_DEFAULT_PROBE_INTERVAL_SEC 5 >>>>>> >>>>>> +/* Parses 'cap_name' from 'ovs_capabilities' and returns whether the >>>>>> + * type of capability is supported or not. */ >>>>>> +typedef bool ovs_feature_parse_func(const struct smap >>> *ovs_capabilities, >>>>>> + const char *cap_name); >>>>>> + >>>>>> struct ovs_feature { >>>>>> enum ovs_feature_value value; >>>>>> const char *name; >>>>>> + ovs_feature_parse_func *parse; >>>>>> }; >>>>>> >>>>>> +static bool >>>>>> +bool_parser(const struct smap *ovs_capabilities, const char > *cap_name) >>>>>> +{ >>>>>> + return smap_get_bool(ovs_capabilities, cap_name, false); >>>>>> +} >>>>>> + >>>>>> +static bool >>>>>> +dp_hash_l4_sym_support_parser(const struct smap *ovs_capabilities, >>>>>> + const char *cap_name OVS_UNUSED) >>>>>> +{ >>>>>> + int max_hash_alg = smap_get_int(ovs_capabilities, > "max_hash_alg", >>> 0); >>>>>> + >>>>>> + return max_hash_alg == OVS_HASH_ALG_SYM_L4; >>>>>> +} >>>>>> + >>>>>> static struct ovs_feature all_ovs_features[] = { >>>>>> { >>>>>> .value = OVS_CT_ZERO_SNAT_SUPPORT, >>>>>> - .name = "ct_zero_snat" >>>>>> + .name = "ct_zero_snat", >>>>>> + .parse = bool_parser, >>>>>> }, >>>>>> { >>>>>> .value = OVS_CT_TUPLE_FLUSH_SUPPORT, >>>>>> - .name = "ct_flush" >>>>>> - } >>>>>> + .name = "ct_flush", >>>>>> + .parse = bool_parser, >>>>>> + }, >>>>>> + { >>>>>> + .value = OVS_DP_HASH_L4_SYM_SUPPORT, >>>>>> + .name = "dp_hash_l4_sym_support", >>>>>> + .parse = dp_hash_l4_sym_support_parser, >>>>>> + }, >>>>>> }; >>>>>> >>>>>> /* A bitmap of OVS features that have been detected as 'supported'. > */ >>>>>> @@ -65,6 +94,7 @@ ovs_feature_is_valid(enum ovs_feature_value > feature) >>>>>> case OVS_CT_ZERO_SNAT_SUPPORT: >>>>>> case OVS_DP_METER_SUPPORT: >>>>>> case OVS_CT_TUPLE_FLUSH_SUPPORT: >>>>>> + case OVS_DP_HASH_L4_SYM_SUPPORT: >>>>>> return true; >>>>>> default: >>>>>> return false; >>>>>> @@ -183,18 +213,17 @@ ovs_feature_support_run(const struct smap >>>>>> *ovs_capabilities, >>>>>> } >>>>>> >>>>>> for (size_t i = 0; i < ARRAY_SIZE(all_ovs_features); i++) { >>>>>> - enum ovs_feature_value value = all_ovs_features[i].value; >>>>>> - const char *name = all_ovs_features[i].name; >>>>>> - bool old_state = supported_ovs_features & value; >>>>>> - bool new_state = smap_get_bool(ovs_capabilities, name, > false); >>>>>> + struct ovs_feature *feature = &all_ovs_features[i]; >>>>>> + bool old_state = supported_ovs_features & feature->value; >>>>>> + bool new_state = feature->parse(ovs_capabilities, >>> feature->name); >>>>>> if (new_state != old_state) { >>>>>> updated = true; >>>>>> if (new_state) { >>>>>> - supported_ovs_features |= value; >>>>>> + supported_ovs_features |= feature->value; >>>>>> } else { >>>>>> - supported_ovs_features &= ~value; >>>>>> + supported_ovs_features &= ~feature->value; >>>>>> } >>>>>> - VLOG_INFO_RL(&rl, "OVS Feature: %s, state: %s", name, >>>>>> + VLOG_INFO_RL(&rl, "OVS Feature: %s, state: %s", >>> feature->name, >>>>>> new_state ? "supported" : "not supported"); >>>>>> } >>>>>> } >>>>>> -- >>>>>> 2.31.1 >>>>>> >>>>>> _______________________________________________ >>>>>> dev mailing list >>>>>> [email protected] >>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>>>> >>>>>> >>>>> Other than that it looks good. >>>>> >>>>> Acked-by: Ales Musil <[email protected]> >>>>> >>>> >>>> Thanks, I applied this to main. >>>> >>>> I was wondering if we should backport this though. It's not exactly a >>>> bug fix but it does avoid using a broken datapath feature (when >>>> possible) so it avoids a broken behavior. >>>> >>>> The changes are quite contained. >>>> >>>> CC-ing some of the maintainers explicitly to get some more eyes on > this. >>>> >>> Thanks Dumitru. Sorry that I was following the reported issue. I tried > to >>> look for some more details, but I am not authorized to access bug > 2175716. >> >> Sorry about that, that bug used to be public but some internal policies >> made it private. >> >>> It is not quite clear to me what the impact of this problem is. The OVN >>> issue (112) seems to be a very common configuration of OVN ECMP and I >>> assume the problem is not easily reproduced because I have been >>> testing/deploying the feature and it worked as expected. Do you have > more >>> details about the trigger? Are there any related fixes in OVS and > kernel? >>> >> >> The original bug report came from OpenShift: >> https://issues.redhat.com/browse/OCPBUGS-7406 >> >> CC-ing Patryk as he root caused the original issue. >> >> The simplified topology is: >> +--- GW1 ---+ >> client ---- cluster-router ---+ +--- (out of OVN) --- server >> +--- GW2 ---+ >> >> On cluster-router there's an ECMP route to reach 'server' via: >> - GW1 >> - GW2 >> >> On GW1 traffic going out of OVN is SNATed to IP1. >> On GW2 traffic going out of OVN is SNATed to IP2. >> >> The client initiates a TCP session towards the server; a path is chosen >> based on dp-hash, e.g. GW2. >> >> From this point on, during normal operation, traffic on this session >> flows fine via GW2. >> >> If the client closes the connection, then for the closing connection >> there's an exception when computing the dp-hash if the socket is in >> state TIME_WAIT, substate TCP_FIN_WAIT_2: >> https://elixir.bootlin.com/linux/v6.2.1/source/net/ipv4/tcp_ipv4.c#L2218 >> In this last case we call: >> >> tcp_v4_timewait_ack(sk, skb) >> | >> tcp_v4_send_ack(sk, skb, ...) >> >> The latter uses a global, per-cpu "ctl_sk" to build the last ACK packet: >> https://elixir.bootlin.com/linux/v6.2.1/source/net/ipv4/tcp_ipv4.c#L925 >> >> ctl_sk = this_cpu_read(ipv4_tcp_sk); >> sock_net_set(ctl_sk, net); >> ctl_sk->sk_mark = (sk->sk_state == TCP_TIME_WAIT) ? >> inet_twsk(sk)->tw_mark : sk->sk_mark; >> ctl_sk->sk_priority = (sk->sk_state == TCP_TIME_WAIT) ? >> inet_twsk(sk)->tw_priority : sk->sk_priority; >> transmit_time = tcp_transmit_time(sk); >> ip_send_unicast_reply(ctl_sk, >> skb, &TCP_SKB_CB(skb)->header.h4.opt, >> ip_hdr(skb)->saddr, ip_hdr(skb)->daddr, >> &arg, arg.iov[0].iov_len, >> transmit_time); >> >> As ctl_sk's sk_txhash is 0, when transmitting this last ack, its hash is >> computed from the packet contents: >> > https://elixir.bootlin.com/linux/v6.2.1/source/include/linux/skbuff.h#L1540 >> >> static inline __u32 skb_get_hash(struct sk_buff *skb) >> { >> if (!skb->l4_hash && !skb->sw_hash) >> __skb_get_hash(skb); >> >> return skb->hash; >> } >> [...] >> void __skb_get_hash(struct sk_buff *skb) >> { >> struct flow_keys keys; >> u32 hash; >> >> __flow_hash_secret_init(); >> >> hash = ___skb_get_hash(skb, &keys, &hashrnd); >> >> __skb_set_sw_hash(skb, hash, flow_keys_have_l4(&keys)); >> } >> >> But the original socket's txhash was actually a random hash: >> https://elixir.bootlin.com/linux/v6.2.1/source/include/net/sock.h#L2128 >> >> static inline void sk_set_txhash(struct sock *sk) >> { >> /* This pairs with READ_ONCE() in skb_set_hash_from_sk() */ >> WRITE_ONCE(sk->sk_txhash, net_tx_rndhash()); >> } >> >> This means that the last ack might enter the ovs datapath with a >> different dp_hash than all other packets sent on that session in that >> direction. It's possible that this last ack gets hashed to a different >> path of the ECMP route. >> >> This specific issue was fixed in the kernel through: >> >> > https://lore.kernel.org/all/[email protected]/T/#mc84997c4cc755354e5527b41b7a15869536a21f4 >> >> However, while debugging we realized the problem is actually visible >> with _any_ retransmit. That's because a retransmit will trigger a rehash: >> >> https://elixir.bootlin.com/linux/latest/source/net/ipv4/tcp_output.c#L4124 >> >> int tcp_rtx_synack(const struct sock *sk, struct request_sock *req) >> { >> const struct tcp_request_sock_ops *af_ops = > tcp_rsk(req)->af_specific; >> struct flowi fl; >> int res; >> >> /* Paired with WRITE_ONCE() in sock_setsockopt() */ >> if (READ_ONCE(sk->sk_txrehash) == SOCK_TXREHASH_ENABLED) >> tcp_rsk(req)->txhash = net_tx_rndhash(); >> res = af_ops->send_synack(sk, NULL, &fl, req, NULL, > TCP_SYNACK_NORMAL, >> NULL); >> [...] >> >> So suddenly all subsequent packets (skbs) will have a different 'hash' >> value and might get hashed to a different path. >> >> This last part was not fixed. After a discussion on the netdev mailing >> list the conclusion was to not change the hash behavior for locally >> terminated TCP sessions: >> >> > https://lore.kernel.org/all/[email protected]/T/#m1fd8da6d912d5acead66c4c397396e6ac4b160dd >> >> Eventually the documentation change patch was dropped and a v2 of the >> other fixes was accepted: >> https://www.spinics.net/lists/netdev/msg906252.html >> >> The rehash on retransmit problem is still present though making the >> kernel's dp-hash implementation unusable. >> >> During some internal discussions Aaron (in CC) pointed out that OVS >> supports L4 symmetric hashing and that should be a stable L4 hash. OVS >> already reports the maximum supported hashing algorithm through the >> Datapath.Capabilities.max_hash_alg ovsdb field. The only problem was >> that the kernel datapath didn't support L4 symmetric hashing. But Aaron >> posted a patch to implement that and the patch got accepted: >> >> https://www.spinics.net/lists/netdev/msg911045.html >> >> Then we moved OVN to use L4 symmetric hashing if available. That's to >> ensure correctness of the packet processing. We did run performance >> tests and the hit due to the extra hashing was acceptable, max 3% >> throughput degradation in OVN scenarios that simulate common traffic >> patterns (like ovn-kubernetes ones) - in most cases the performance >> degradation was around 2%. >> >> Sorry for the long story but I hope it provides all the required >> context. Thinking more about it I should've probably added some of this >> to the original commit log, I apologize. >> >> Regards, >> Dumitru >> > > Thanks Dumitru for the details! It is now quite clear and I am not against > backporting it. > But I have a question regarding upgrading. For the existing traffic, does > this mean the packets may choose a different path after OVN upgrading? > Should this be called out? >
Hi Han, That's a good question. They might and this affects all traffic that's not locally originated for the kernel datapath and (potentially) all traffic for the userspace datapath. I can post a follow up patch to update the NEWS file. Did you have anything else in mind? Regards, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
