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?
Thanks,
Roi
>
>
>>> __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
