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