Re: [PATCH net-next 2/2] net: ipv6: ioam6: new feature tunsrc

From: Justin Iurman
Date: Tue Aug 13 2024 - 07:21:03 EST


On 8/13/24 13:06, Paolo Abeni wrote:
On 8/9/24 14:39, Justin Iurman wrote:
This patch provides a new feature (i.e., "tunsrc") for the tunnel (i.e.,
"encap") mode of ioam6. Just like seg6 already does, except it is
attached to a route. The "tunsrc" is optional: when not provided (by
default), the automatic resolution is applied. Using "tunsrc" when
possible has a benefit: performance.

It's customary to include performances figures in performance related changeset ;)

Indeed, I realized it too late... thx for the reminder!

Before (= "encap" mode): https://ibb.co/bNCzvf7
After (= "encap" mode with "tunsrc"): https://ibb.co/PT8L6yq

I'll add these again to -v2.

Signed-off-by: Justin Iurman <justin.iurman@xxxxxxxxx>
---
  include/uapi/linux/ioam6_iptunnel.h |  7 +++++
  net/ipv6/ioam6_iptunnel.c           | 48 ++++++++++++++++++++++++++---
  2 files changed, 51 insertions(+), 4 deletions(-)

diff --git a/include/uapi/linux/ioam6_iptunnel.h b/include/uapi/linux/ioam6_iptunnel.h
index 38f6a8fdfd34..6cdbd0da7ad8 100644
--- a/include/uapi/linux/ioam6_iptunnel.h
+++ b/include/uapi/linux/ioam6_iptunnel.h
@@ -50,6 +50,13 @@ enum {
      IOAM6_IPTUNNEL_FREQ_K,        /* u32 */
      IOAM6_IPTUNNEL_FREQ_N,        /* u32 */
+    /* Tunnel src address.
+     * For encap,auto modes.
+     * Optional (automatic if
+     * not provided).
+     */
+    IOAM6_IPTUNNEL_SRC,        /* struct in6_addr */
+
      __IOAM6_IPTUNNEL_MAX,
  };
diff --git a/net/ipv6/ioam6_iptunnel.c b/net/ipv6/ioam6_iptunnel.c
index cd2522f04edf..e0e73faf9969 100644
--- a/net/ipv6/ioam6_iptunnel.c
+++ b/net/ipv6/ioam6_iptunnel.c
@@ -42,6 +42,8 @@ struct ioam6_lwt {
      struct ioam6_lwt_freq freq;
      atomic_t pkt_cnt;
      u8 mode;
+    bool has_tunsrc;
+    struct in6_addr tunsrc;
      struct in6_addr tundst;
      struct ioam6_lwt_encap tuninfo;
  };
@@ -72,6 +74,7 @@ static const struct nla_policy ioam6_iptunnel_policy[IOAM6_IPTUNNEL_MAX + 1] = {
      [IOAM6_IPTUNNEL_MODE]    = NLA_POLICY_RANGE(NLA_U8,
                             IOAM6_IPTUNNEL_MODE_MIN,
                             IOAM6_IPTUNNEL_MODE_MAX),
+    [IOAM6_IPTUNNEL_SRC]    = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
      [IOAM6_IPTUNNEL_DST]    = NLA_POLICY_EXACT_LEN(sizeof(struct in6_addr)),
      [IOAM6_IPTUNNEL_TRACE]    = NLA_POLICY_EXACT_LEN(
                      sizeof(struct ioam6_trace_hdr)),
@@ -144,6 +147,11 @@ static int ioam6_build_state(struct net *net, struct nlattr *nla,
      else
          mode = nla_get_u8(tb[IOAM6_IPTUNNEL_MODE]);
+    if (tb[IOAM6_IPTUNNEL_SRC] && mode == IOAM6_IPTUNNEL_MODE_INLINE) {
+        NL_SET_ERR_MSG(extack, "no tunnel source expected in this mode");
+        return -EINVAL;
+    }

when mode is IOAM6_IPTUNNEL_MODE_AUTO, the data path could still add the encapsulation for forwarded packets, why explicitly preventing this optimization in such scenario?

Good catch! I guess we can just ignore "tunsrc" if provided with inline mode, instead of returning an error.

+
      if (!tb[IOAM6_IPTUNNEL_DST] && mode != IOAM6_IPTUNNEL_MODE_INLINE) {
          NL_SET_ERR_MSG(extack, "this mode needs a tunnel destination");
          return -EINVAL;
@@ -178,6 +186,14 @@ static int ioam6_build_state(struct net *net, struct nlattr *nla,
      ilwt->freq.n = freq_n;
      ilwt->mode = mode;
+
+    if (!tb[IOAM6_IPTUNNEL_SRC]) {
+        ilwt->has_tunsrc = false;
+    } else {
+        ilwt->has_tunsrc = true;
+        ilwt->tunsrc = nla_get_in6_addr(tb[IOAM6_IPTUNNEL_SRC]);

Since you are going to use the source address only if != ANY, I think it would be cleaner to refuse such addresses here. That will avoid an additional check in the datapath.

+1.

Thanks,
Justin

Cheers,

Paolo