On 5/26/2023 8:18 PM, Eelco Chaudron wrote:
External email: Use caution opening links or attachments


On 15 May 2023, at 10:23, Roi Dayan wrote:

From: Gavin Li <[email protected]>

Add TC offload support for vxlan encap with gbp option

Signed-off-by: Gavin Li <[email protected]>
Reviewed-by: Gavi Teitz <[email protected]>
Reviewed-by: Roi Dayan <[email protected]>
I have a couple of comments below, and some general ones.

1) Can we add some unit tests for this to check-offload?

2) Can you explain what happens if the kernel does not support the newly added 
option? I guess it will return -EINVAL when trying to install a flow, which 
will probably result in an error message. This is because EOPNOTSUPP is the 
only one ignored as not a problem. So maybe we need some probing?

This does conclude my review of v3.

//Eelco

ACK

Will add unit tests for this to check-offload and add probing.


---
  acinclude.m4                         |  7 ++++
  include/linux/tc_act/tc_tunnel_key.h | 17 +++++++-
  lib/netdev-offload-tc.c              | 31 ++++++++++++++-
  lib/odp-util.c                       | 41 ++++++++++++++------
  lib/odp-util.h                       |  4 +-
  lib/tc.c                             | 58 ++++++++++++++++++++++++++++
  lib/tc.h                             |  1 +
  7 files changed, 143 insertions(+), 16 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index ac1eab790041..690a13c25967 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -191,6 +191,13 @@ AC_DEFUN([OVS_CHECK_LINUX_TC], [
      [AC_DEFINE([HAVE_TCA_TUNNEL_KEY_ENC_TTL], [1],
                 [Define to 1 if TCA_TUNNEL_KEY_ENC_TTL is available.])])

+  AC_COMPILE_IFELSE([
+    AC_LANG_PROGRAM([#include <linux/tc_act/tc_tunnel_key.h>], [
+        int x = TCA_TUNNEL_KEY_ENC_OPTS_VXLAN;
+    ])],
+    [AC_DEFINE([HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN], [1],
+               [Define to 1 if TCA_TUNNEL_KEY_ENC_OPTS_VXLAN is available.])])
+
    AC_COMPILE_IFELSE([
      AC_LANG_PROGRAM([#include <linux/tc_act/tc_pedit.h>], [
          int x = TCA_PEDIT_KEY_EX_HDR_TYPE_UDP;
diff --git a/include/linux/tc_act/tc_tunnel_key.h 
b/include/linux/tc_act/tc_tunnel_key.h
index f13acf17dd75..17291b90bf3c 100644
--- a/include/linux/tc_act/tc_tunnel_key.h
+++ b/include/linux/tc_act/tc_tunnel_key.h
@@ -1,7 +1,7 @@
  #ifndef __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H
  #define __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H 1

-#if defined(__KERNEL__) || defined(HAVE_TCA_TUNNEL_KEY_ENC_TTL)
+#if defined(__KERNEL__) || defined(HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN)
  #include_next <linux/tc_act/tc_tunnel_key.h>
  #else

@@ -53,6 +53,10 @@ enum {
                                        * TCA_TUNNEL_KEY_ENC_OPTS_GENEVE
                                        * attributes
                                        */
+     TCA_TUNNEL_KEY_ENC_OPTS_VXLAN,  /* Nested
+                                      * TCA_TUNNEL_KEY_ENC_OPTS_VXLAN
+                                      * attributes
+                                      */
       __TCA_TUNNEL_KEY_ENC_OPTS_MAX,
  };

@@ -70,6 +74,15 @@ enum {
  #define TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX \
       (__TCA_TUNNEL_KEY_ENC_OPT_GENEVE_MAX - 1)

-#endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_TTL */
+enum {
+     TCA_TUNNEL_KEY_ENC_OPT_VXLAN_UNSPEC,
+     TCA_TUNNEL_KEY_ENC_OPT_VXLAN_GBP,       /* u32 */
+     __TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX,
+};
+
+#define TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX \
+     (__TCA_TUNNEL_KEY_ENC_OPT_VXLAN_MAX - 1)
+
+#endif /* __KERNEL__ || HAVE_TCA_TUNNEL_KEY_ENC_OPTS_VXLAN */

  #endif /* __LINUX_TC_ACT_TC_TUNNEL_KEY_WRAPPER_H */
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 1c97681bc92b..a32ed8021c4b 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -668,6 +668,24 @@ static void parse_tc_flower_geneve_opts(struct tc_action 
*action,
      nl_msg_end_nested(buf, geneve_off);
  }

+static void
+parse_tc_flower_vxlan_tun_opts(struct tc_action *action,
+                                  struct ofpbuf *buf)
+{
+    size_t gbp_off;
+    uint32_t gbp_raw;
+
+    if (!action->encap.gbp.id_present) {
+        return;
+    }
+
+    gbp_off = nl_msg_start_nested(buf, OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS);
+    gbp_raw = odp_encode_gbp_raw(action->encap.gbp.flags,
+                                 action->encap.gbp.id);
+    nl_msg_put_u32(buf, OVS_VXLAN_EXT_GBP, gbp_raw);
+    nl_msg_end_nested(buf, gbp_off);
+}
+
  static void
  flower_tun_opt_to_match(struct match *match, struct tc_flower *flower)
  {
@@ -863,7 +881,7 @@ parse_tc_flower_to_actions__(struct tc_flower *flower, 
struct ofpbuf *buf,
              if (!action->encap.no_csum) {
                  nl_msg_put_flag(buf, OVS_TUNNEL_KEY_ATTR_CSUM);
              }
-
+            parse_tc_flower_vxlan_tun_opts(action, buf);
              parse_tc_flower_geneve_opts(action, buf);
              nl_msg_end_nested(buf, tunnel_offset);
              nl_msg_end_nested(buf, set_offset);
@@ -1552,6 +1570,7 @@ parse_put_flow_set_action(struct tc_flower *flower, 
struct tc_action *action,

      action->type = TC_ACT_ENCAP;
      action->encap.id_present = false;
+    action->encap.gbp.id_present = false;
      action->encap.no_csum = 1;
      flower->action_count++;
      NL_ATTR_FOR_EACH_UNSAFE(tun_attr, tun_left, tunnel, tunnel_len) {
@@ -1613,6 +1632,16 @@ parse_put_flow_set_action(struct tc_flower *flower, 
struct tc_action *action,
              action->encap.data.present.len = nl_attr_get_size(tun_attr);
          }
          break;
+        case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS: {
+            if (odp_vxlan_tun_opts_from_attr(tun_attr,
+                                             &action->encap.gbp.id,
+                                             &action->encap.gbp.flags,
+                                             &action->encap.gbp.id_present)) {
+                VLOG_ERR_RL(&rl, "error parsing VXLAN options");
+                return EINVAL;
+            }
+        }
+        break;
          default:
              VLOG_DBG_RL(&rl, "unsupported tunnel key attribute %d",
                          nl_attr_type(tun_attr));
diff --git a/lib/odp-util.c b/lib/odp-util.c
index bf34c61fec58..34c9001f85fc 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3149,22 +3149,13 @@ odp_tun_key_from_attr__(const struct nlattr *attr, bool 
is_mask,
              tun->flags |= FLOW_TNL_F_OAM;
              break;
          case OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS: {
-            static const struct nl_policy vxlan_opts_policy[] = {
-                [OVS_VXLAN_EXT_GBP] = { .type = NL_A_U32 },
-            };
-            struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)];
-
-            if (!nl_parse_nested(a, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) {
+            if (odp_vxlan_tun_opts_from_attr(a, &tun->gbp_id,
+                                             &tun->gbp_flags,
+                                             NULL)) {
                  odp_parse_error(&rl, errorp, "error parsing VXLAN options");
                  return ODP_FIT_ERROR;
              }

-            if (ext[OVS_VXLAN_EXT_GBP]) {
-                uint32_t gbp = nl_attr_get_u32(ext[OVS_VXLAN_EXT_GBP]);
-
-                odp_decode_gbp_raw(gbp, &tun->gbp_id, &tun->gbp_flags);
-            }
-
              break;
          }
          case OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS:
@@ -8843,3 +8834,29 @@ commit_odp_actions(const struct flow *flow, struct flow 
*base,

      return slow1 ? slow1 : slow2;
  }
+
+int
+odp_vxlan_tun_opts_from_attr(const struct nlattr *tun_attr, ovs_be16 *id,
+                             uint8_t *flags, bool *id_present)
+{
+    static const struct nl_policy vxlan_opts_policy[] = {
+        [OVS_VXLAN_EXT_GBP] = { .type = NL_A_U32 },
+    };
+    struct nlattr *ext[ARRAY_SIZE(vxlan_opts_policy)];
+
+    if (!nl_parse_nested(tun_attr, vxlan_opts_policy, ext, ARRAY_SIZE(ext))) {
+        return EINVAL;
+    }
+
+    if (ext[OVS_VXLAN_EXT_GBP]) {
+        uint32_t gbp_raw = nl_attr_get_u32(ext[OVS_VXLAN_EXT_GBP]);
+
+        odp_decode_gbp_raw(gbp_raw, id, flags);
+    }
+
+    if (id_present) {
+        *id_present = !!ext[OVS_VXLAN_EXT_GBP];
+    }
+
+    return 0;
+}
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 163efe7a87b5..03f15ef5052b 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -291,7 +291,9 @@ enum slow_path_reason commit_odp_actions(const struct flow 
*,
                                           bool pending_encap,
                                           bool pending_decap,
                                           struct ofpbuf *encap_data);
-
You removed ‘form feed’ here, some people want this to stay in, see code 
guidelines…
ACK

+int odp_vxlan_tun_opts_from_attr(const struct nlattr *tun_attr, ovs_be16 *id,
+                                 uint8_t *flags, bool *id_present);
+
  /* ofproto-dpif interface.
   *
   * The following types and functions are logically part of ofproto-dpif.
diff --git a/lib/tc.c b/lib/tc.c
index 94df4c507f14..7a13aaabbe59 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1287,6 +1287,35 @@ nl_parse_act_geneve_opts(const struct nlattr *in_nlattr,
      return 0;
  }

+static int
+nl_parse_act_vxlan_opts(struct nlattr *in_nlattr, struct tc_action *action)
+{
+    const struct ofpbuf *msg;
+    struct nlattr *nla;
+    struct ofpbuf buf;
+    size_t left;
+
+    nl_attr_get_nested(in_nlattr, &buf);
+    msg = &buf;
+
+    NL_ATTR_FOR_EACH (nla, left, ofpbuf_at(msg, 0, 0), msg->size) {
+        uint16_t type = nl_attr_type(nla);
+        int32_t gbp_raw;
+
+        switch (type) {
+        case TCA_FLOWER_KEY_ENC_OPT_VXLAN_GBP:
+            gbp_raw = nl_attr_get_u32(nla);
+            odp_decode_gbp_raw(gbp_raw, &action->encap.gbp.id,
+                               &action->encap.gbp.flags);
+            action->encap.gbp.id_present = true;
+
+            break;
+        }
+    }
+
+    return 0;
+}
+
  static int
  nl_parse_act_tunnel_opts(struct nlattr *options, struct tc_action *action)
  {
@@ -1312,6 +1341,13 @@ nl_parse_act_tunnel_opts(struct nlattr *options, struct 
tc_action *action)
                  return err;
              }

+            break;
+        case TCA_TUNNEL_KEY_ENC_OPTS_VXLAN:
+            err = nl_parse_act_vxlan_opts(nla, action);
+            if (err) {
+                return err;
+            }
+
              break;
          }
      }
@@ -2627,6 +2663,27 @@ nl_msg_put_act_tunnel_geneve_option(struct ofpbuf 
*request,
      nl_msg_end_nested(request, outer);
  }

+static void
+nl_msg_put_act_tunnel_vxlan_opts(struct ofpbuf *request,
+                                 struct tc_action_encap *encap)
+{
+    size_t outer, inner;
+    uint32_t gbp_raw;
+
+    if (!encap->gbp.id_present) {
+        return;
+    }
+
+    gbp_raw = odp_encode_gbp_raw(encap->gbp.flags,
+                                 encap->gbp.id);
+    outer = nl_msg_start_nested_with_flag(request, TCA_TUNNEL_KEY_ENC_OPTS);
+    inner = nl_msg_start_nested_with_flag(request,
+                                          TCA_TUNNEL_KEY_ENC_OPTS_VXLAN);
+    nl_msg_put_u32(request, TCA_TUNNEL_KEY_ENC_OPT_VXLAN_GBP, gbp_raw);
+    nl_msg_end_nested(request, inner);
+    nl_msg_end_nested(request, outer);
+}
+
  static void
  nl_msg_put_act_tunnel_key_set(struct ofpbuf *request,
                                struct tc_action_encap *encap,
@@ -2667,6 +2724,7 @@ nl_msg_put_act_tunnel_key_set(struct ofpbuf *request,
              nl_msg_put_be16(request, TCA_TUNNEL_KEY_ENC_DST_PORT,
                              encap->tp_dst);
          }
+        nl_msg_put_act_tunnel_vxlan_opts(request, encap);
          nl_msg_put_act_tunnel_geneve_option(request, &encap->data);
          nl_msg_put_u8(request, TCA_TUNNEL_KEY_NO_CSUM, encap->no_csum);
      }
diff --git a/lib/tc.h b/lib/tc.h
index 1d648282a004..b493cb52c4ca 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -218,6 +218,7 @@ struct tc_action_encap {
      uint8_t tos;
      uint8_t ttl;
      uint8_t no_csum;
+    struct tc_tunnel_gbp gbp;
Any reason why you put it here? If not, I would suggest putting it at the 
bottom as it might be used less frequent.
ACK


      struct {
          ovs_be32 ipv4_src;
          ovs_be32 ipv4_dst;
--
2.38.0
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to