On 9 Oct 2024, at 12:06, Roi Dayan wrote:
> On 09/10/2024 11:27, Eelco Chaudron wrote: >> >> >> On 9 Oct 2024, at 8:50, Roi Dayan wrote: >> >>> On 08/10/2024 16:52, Eelco Chaudron wrote: >>>> This patch checks to see if the TCA_FLOWER_KEY_ENC_FLAGS key is supported. >>>> >>>> Signed-off-by: Eelco Chaudron <[email protected]> >>>> --- >>>> acinclude.m4 | 6 +++--- >>>> include/linux/pkt_cls.h | 28 ++++++++++++++++++++++++- >>>> lib/netdev-offload-tc.c | 45 +++++++++++++++++++++++++++++++++++++++++ >>>> lib/tc.c | 29 +++++++++++++++++++++++--- >>>> lib/tc.h | 2 ++ >>>> 5 files changed, 103 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/acinclude.m4 b/acinclude.m4 >>>> index 1ace70c92..f4cb4a146 100644 >>>> --- a/acinclude.m4 >>>> +++ b/acinclude.m4 >>>> @@ -163,10 +163,10 @@ dnl Configure Linux tc compat. >>>> AC_DEFUN([OVS_CHECK_LINUX_TC], [ >>>> AC_COMPILE_IFELSE([ >>>> AC_LANG_PROGRAM([#include <linux/pkt_cls.h>], [ >>>> - int x = TCA_ACT_FLAGS_SKIP_HW; >>>> + int x = TCA_FLOWER_KEY_ENC_FLAGS_MASK; >>>> ])], >>>> - [AC_DEFINE([HAVE_TCA_ACT_FLAGS_SKIP_HW], [1], >>>> - [Define to 1 if TCA_ACT_FLAGS_SKIP_HW is available.])]) >>>> + [AC_DEFINE([HAVE_TCA_FLOWER_KEY_ENC_FLAGS_MASK], [1], >>>> + [Define to 1 if TCA_FLOWER_KEY_ENC_FLAGS_MASK is >>>> available.])]) >>>> >>>> AC_CHECK_MEMBERS([struct tcf_t.firstuse], [], [], [#include >>>> <linux/pkt_cls.h>]) >>>> >>>> diff --git a/include/linux/pkt_cls.h b/include/linux/pkt_cls.h >>>> index fb4a7ecea..5890dc157 100644 >>>> --- a/include/linux/pkt_cls.h >>>> +++ b/include/linux/pkt_cls.h >>>> @@ -1,7 +1,7 @@ >>>> #ifndef __LINUX_PKT_CLS_WRAPPER_H >>>> #define __LINUX_PKT_CLS_WRAPPER_H 1 >>>> >>>> -#if defined(__KERNEL__) || defined(HAVE_TCA_ACT_FLAGS_SKIP_HW) >>>> +#if defined(__KERNEL__) || defined(HAVE_TCA_FLOWER_KEY_ENC_FLAGS_MASK) >>>> #include_next <linux/pkt_cls.h> >>>> #else >>>> >>>> @@ -252,6 +252,28 @@ enum { >>>> TCA_FLOWER_KEY_CT_LABELS, /* u128 */ >>>> TCA_FLOWER_KEY_CT_LABELS_MASK, /* u128 */ >>>> >>>> + TCA_FLOWER_KEY_MPLS_OPTS, >>>> + >>>> + TCA_FLOWER_KEY_HASH, /* u32 */ >>>> + TCA_FLOWER_KEY_HASH_MASK, /* u32 */ >>>> + >>>> + TCA_FLOWER_KEY_NUM_OF_VLANS, /* u8 */ >>>> + >>>> + TCA_FLOWER_KEY_PPPOE_SID, /* be16 */ >>>> + TCA_FLOWER_KEY_PPP_PROTO, /* be16 */ >>>> + >>>> + TCA_FLOWER_KEY_L2TPV3_SID, /* be32 */ >>>> + >>>> + TCA_FLOWER_L2_MISS, /* u8 */ >>>> + >>>> + TCA_FLOWER_KEY_CFM, /* nested */ >>>> + >>>> + TCA_FLOWER_KEY_SPI, /* be32 */ >>>> + TCA_FLOWER_KEY_SPI_MASK, /* be32 */ >>>> + >>>> + TCA_FLOWER_KEY_ENC_FLAGS, /* be32 */ >>>> + TCA_FLOWER_KEY_ENC_FLAGS_MASK, /* be32 */ >>>> + >>>> __TCA_FLOWER_MAX, >>>> }; >>>> >>>> @@ -306,6 +328,10 @@ enum { >>>> enum { >>>> TCA_FLOWER_KEY_FLAGS_IS_FRAGMENT = (1 << 0), >>>> TCA_FLOWER_KEY_FLAGS_FRAG_IS_FIRST = (1 << 1), >>>> + TCA_FLOWER_KEY_FLAGS_TUNNEL_CSUM = (1 << 2), >>>> + TCA_FLOWER_KEY_FLAGS_TUNNEL_DONT_FRAGMENT = (1 << 3), >>>> + TCA_FLOWER_KEY_FLAGS_TUNNEL_OAM = (1 << 4), >>>> + TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT = (1 << 5), >>>> }; >>>> >>>> /* Match-all classifier */ >>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c >>>> index 3be1c08d2..fcd206f2c 100644 >>>> --- a/lib/netdev-offload-tc.c >>>> +++ b/lib/netdev-offload-tc.c >>>> @@ -53,6 +53,7 @@ static bool multi_mask_per_prio = false; >>>> static bool block_support = false; >>>> static uint16_t ct_state_support; >>>> static bool vxlan_gbp_support = false; >>>> +static bool enc_flags_support = false; >>>> >>>> struct netlink_field { >>>> int offset; >>>> @@ -2863,6 +2864,49 @@ out: >>>> tc_add_del_qdisc(ifindex, false, block_id, TC_INGRESS); >>>> } >>>> >>>> +static void >>>> +probe_enc_flags_support(int ifindex) >>>> +{ >>>> + struct tc_flower flower; >>>> + struct tcf_id id; >>>> + int block_id = 0; >>>> + int prio = TC_RESERVED_PRIORITY_FEATURE_PROBE; >>>> + int error; >>>> + >>>> + error = tc_add_del_qdisc(ifindex, true, block_id, TC_INGRESS); >>>> + if (error) { >>>> + return; >>>> + } >>>> + >>>> + memset(&flower, 0, sizeof flower); >>>> + flower.tc_policy = TC_POLICY_SKIP_HW; >>>> + flower.key.eth_type = htons(ETH_P_IP); >>>> + flower.mask.eth_type = OVS_BE16_MAX; >>>> + flower.tunnel = true; >>>> + flower.mask.tunnel.id = OVS_BE64_MAX; >>>> + flower.mask.tunnel.ipv4.ipv4_src = OVS_BE32_MAX; >>>> + flower.mask.tunnel.ipv4.ipv4_dst = OVS_BE32_MAX; >>>> + flower.mask.tunnel.tp_dst = OVS_BE16_MAX; >>>> + flower.mask.tunnel.tc_enc_flags = >>>> TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT; >>>> + flower.key.tunnel.ipv4.ipv4_src = htonl(0x01010101); >>>> + flower.key.tunnel.ipv4.ipv4_dst = htonl(0x01010102); >>>> + flower.key.tunnel.tp_dst = htons(46354); >>>> + flower.key.tunnel.tc_enc_flags = TCA_FLOWER_KEY_FLAGS_TUNNEL_CRIT_OPT; >>>> + >>>> + id = tc_make_tcf_id(ifindex, block_id, prio, TC_INGRESS); >>>> + error = tc_replace_flower(&id, &flower); >>>> + if (error) { >>>> + goto out; >>>> + } >>>> + >>>> + tc_del_flower_filter(&id); >>>> + >>>> + enc_flags_support = true; >>>> + VLOG_INFO("probe tc: enc flags are supported."); >>>> +out: >>>> + tc_add_del_qdisc(ifindex, false, block_id, TC_INGRESS); >>>> +} >>>> + >>>> static int >>>> tc_get_policer_action_ids(struct hmap *map) >>>> { >>>> @@ -2991,6 +3035,7 @@ netdev_tc_init_flow_api(struct netdev *netdev) >>>> probe_multi_mask_per_prio(ifindex); >>>> probe_ct_state_support(ifindex); >>>> probe_vxlan_gbp_support(ifindex); >>>> + probe_enc_flags_support(ifindex); >>>> >>>> ovs_mutex_lock(&meter_police_ids_mutex); >>>> meter_police_ids = id_pool_create(METER_POLICE_IDS_BASE, >>>> diff --git a/lib/tc.c b/lib/tc.c >>>> index e55ba3b1b..6c853b8e6 100644 >>>> --- a/lib/tc.c >>>> +++ b/lib/tc.c >>>> @@ -454,6 +454,9 @@ static const struct nl_policy tca_flower_policy[] = { >>>> [TCA_FLOWER_KEY_ENC_OPTS] = { .type = NL_A_NESTED, .optional = true, >>>> }, >>>> [TCA_FLOWER_KEY_ENC_OPTS_MASK] = { .type = NL_A_NESTED, >>>> .optional = true, }, >>>> + [TCA_FLOWER_KEY_ENC_FLAGS] = { .type = NL_A_BE32, .optional = true, }, >>>> + [TCA_FLOWER_KEY_ENC_FLAGS_MASK] = { .type = NL_A_BE32, >>>> + .optional = true, }, >>>> [TCA_FLOWER_KEY_CT_STATE] = { .type = NL_A_U16, .optional = true, }, >>>> [TCA_FLOWER_KEY_CT_STATE_MASK] = { .type = NL_A_U16, .optional = >>>> true, }, >>>> [TCA_FLOWER_KEY_CT_ZONE] = { .type = NL_A_U16, .optional = true, }, >>>> @@ -865,6 +868,13 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct >>>> tc_flower *flower) >>>> flower->tunnel = true; >>>> } >>>> >>>> + if (attrs[TCA_FLOWER_KEY_ENC_FLAGS_MASK]) { >>>> + flower->key.tunnel.tc_enc_flags = ntohl( >>>> + nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_FLAGS])); >>>> + flower->mask.tunnel.tc_enc_flags = ntohl( >>>> + nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_FLAGS_MASK])); >>>> + } >>>> + >>>> if (attrs[TCA_FLOWER_KEY_ENC_OPTS] && >>>> attrs[TCA_FLOWER_KEY_ENC_OPTS_MASK]) { >>>> err = nl_parse_flower_tunnel_opts(attrs[TCA_FLOWER_KEY_ENC_OPTS], >>>> @@ -3611,6 +3621,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, >>>> struct tc_flower *flower) >>>> struct in6_addr *ipv6_src = &flower->key.tunnel.ipv6.ipv6_src; >>>> struct in6_addr *ipv6_dst = &flower->key.tunnel.ipv6.ipv6_dst; >>>> ovs_be32 id = be64_to_be32(flower->key.tunnel.id); >>>> + ovs_be32 enc_flags = htonl(flower->key.tunnel.tc_enc_flags); >>>> ovs_be16 tp_src = flower->key.tunnel.tp_src; >>>> ovs_be16 tp_dst = flower->key.tunnel.tp_dst; >>>> uint8_t tos = flower->key.tunnel.tos; >>>> @@ -3618,6 +3629,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, >>>> struct tc_flower *flower) >>>> uint8_t tos_mask = flower->mask.tunnel.tos; >>>> uint8_t ttl_mask = flower->mask.tunnel.ttl; >>>> ovs_be64 id_mask = flower->mask.tunnel.id; >>>> + ovs_be32 enc_flags_mask = htonl(flower->mask.tunnel.tc_enc_flags); >>>> ovs_be16 tp_src_mask = flower->mask.tunnel.tp_src; >>>> ovs_be16 tp_dst_mask = flower->mask.tunnel.tp_dst; >>>> >>>> @@ -3655,6 +3667,11 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, >>>> struct tc_flower *flower) >>>> nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT_MASK, >>>> tp_dst_mask); >>>> } >>>> + if (enc_flags_mask) { >>>> + nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_FLAGS, enc_flags); >>>> + nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_FLAGS_MASK, >>>> + enc_flags_mask); >>>> + } >>>> if (id_mask) { >>>> nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id); >>>> } >>>> @@ -3961,6 +3978,7 @@ tc_replace_flower(struct tcf_id *id, struct >>>> tc_flower *flower) >>>> struct ofpbuf b = ofpbuf_const_initializer(reply->data, >>>> reply->size); >>>> struct nlmsghdr *nlmsg = ofpbuf_try_pull(&b, sizeof *nlmsg); >>>> struct tcmsg *tc = ofpbuf_try_pull(&b, sizeof *tc); >>>> + bool is_probe = id->prio == TC_RESERVED_PRIORITY_FEATURE_PROBE; >>>> >>>> if (!nlmsg || !tc) { >>>> COVERAGE_INC(tc_netlink_malformed_reply); >>>> @@ -3980,9 +3998,14 @@ tc_replace_flower(struct tcf_id *id, struct >>>> tc_flower *flower) >>>> false); >>>> >>>> if (ret || !cmp_tc_flower_match_action(flower, &flower_out)) { >>>> - VLOG_WARN_RL(&error_rl, "Kernel flower acknowledgment >>>> does " >>>> - "not match request! Set dpif_netlink to dbg >>>> to " >>>> - "see which rule caused this error."); >>>> + if (is_probe) { >>>> + error = EINVAL; >>> >>> maybe can do that in another way, marking its a probe. >>> see my comment about reserved prio below. >>> >>>> + } else { >>>> + VLOG_WARN_RL(&error_rl, "Kernel flower acknowledgment >>>> " >>>> + "does not match request! Set >>>> " >>>> + "dpif_netlink to dbg to see " >>>> + "which rule caused this >>>> error."); >>>> + } >>>> } >>>> } >>>> ofpbuf_delete(reply); >>>> diff --git a/lib/tc.h b/lib/tc.h >>>> index 8442c8d8b..8ec4857b7 100644 >>>> --- a/lib/tc.h >>>> +++ b/lib/tc.h >>>> @@ -52,6 +52,7 @@ enum tc_flower_reserved_prio { >>>> TC_RESERVED_PRIORITY_IPV4, >>>> TC_RESERVED_PRIORITY_IPV6, >>>> TC_RESERVED_PRIORITY_VLAN, >>>> + TC_RESERVED_PRIORITY_FEATURE_PROBE, >>> >>> Hi, >>> >>> Why would you want to reserve a low priority for feature probe? >>> The reserved priority was to make sure those types gets lower >>> priority for better performance in HW offloading. so now one >>> level will be saved for feature probing which is not needed >>> and will never be used again after all probing was done. >>> Unless I miss something I think we should not save a priority >>> for feature probe. >> >> Hi Roi, >> >> First, thanks for the review! Yes, there are alternative ways to handle >> this, but I felt using one of the 64K priorities would be a cleaner >> solution. As far as I know, we’ve never run into any issues with running out >> of them. >> >> We could add a flag to tc_replace_flower(), such as fail_match_action_cmp. >> Does anyone else have any other suggestions/opinions? If not, I’ll send out >> a v2 with this change, along with some include fixes to make the patch pass >> on GitHub. >> >> Cheers, >> >> Eelco > > Hi, > > The issue was not running out of prios but having lower prio > for better performance as how hw handling the rule. > So catching now a lower prio means all rules are by default added > with higher prio. > This will probably cause many changes later to save more prios before > before feature_probe for different type of rules. > > The flag could also be the simple bool is_probe instead of the local > bool you added. > > Another option is to save a prio for feature_probe as maybe the last > possible prio and in get_next_available_prio() to account for that. > For example we can define the following 2 and check for last prio instead of > UINT16_MAX > > > TC_RESERVED_PRIORITY_FEATURE_PROBE = UINT16_MAX > TC_LAST_PRIO = TC_RESERVED_PRIORITY_FEATURE_PROBE - 1 > > if (last_prio == TC_LAST_PRIO) { > return TC_RESERVED_PRIORITY_NONE; > } > > In this case you dont need to add a flag and pass it froma ll callers now. > what do you think? I like this idea :) Let me think about it a bit more, and I will send out a v2 next week. >>>> __TC_RESERVED_PRIORITY_MAX >>>> }; >>>> #define TC_RESERVED_PRIORITY_MAX (__TC_RESERVED_PRIORITY_MAX -1) >>>> @@ -125,6 +126,7 @@ struct tc_flower_tunnel { >>>> uint8_t ttl; >>>> ovs_be16 tp_src; >>>> ovs_be16 tp_dst; >>>> + uint32_t tc_enc_flags; >>>> struct tc_tunnel_gbp gbp; >>>> ovs_be64 id; >>>> struct tun_metadata metadata; >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
