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. 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?
Regards, Han > Regards, > Dumitru > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
