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 >> __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
