On Thu, Jul 13, 2023 at 4:39 PM Dumitru Ceara <dce...@redhat.com> 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 <dce...@redhat.com> > 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? > + 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 > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Other than that it looks good. Acked-by: Ales Musil <amu...@redhat.com> -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
