[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240816103558.16502b74@kernel.org>
Date: Fri, 16 Aug 2024 10:35:58 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Justin Iurman <justin.iurman@...ege.be>
Cc: netdev@...r.kernel.org, davem@...emloft.net, dsahern@...nel.org,
edumazet@...gle.com, pabeni@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/2] net: ipv6: ioam6: new feature tunsrc
On Tue, 13 Aug 2024 14:27:23 +0200 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. See the comparison:
> - before (= "encap" mode): https://ibb.co/bNCzvf7
> - after (= "encap" mode with "tunsrc"): https://ibb.co/PT8L6yq
No need to extend the selftests ?
> 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).
The wrapping of this text appears excessive
> + */
> + IOAM6_IPTUNNEL_SRC, /* struct in6_addr */
> +
> __IOAM6_IPTUNNEL_MAX,
> };
> @@ -178,6 +186,23 @@ 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]);
> +
> + if (ipv6_addr_any(&ilwt->tunsrc)) {
> + dst_cache_destroy(&ilwt->cache);
> + kfree(lwt);
Let's put the cleanup at the end of the function, and use a goto / label
to jump there.
> + NL_SET_ERR_MSG_ATTR(extack, tb[IOAM6_IPTUNNEL_SRC],
> + "invalid tunnel source address");
> + return -EINVAL;
> + }
> + }
> +
> if (tb[IOAM6_IPTUNNEL_DST])
> ilwt->tundst = nla_get_in6_addr(tb[IOAM6_IPTUNNEL_DST]);
>
> @@ -257,6 +282,8 @@ static int ioam6_do_inline(struct net *net, struct sk_buff *skb,
>
> static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
> struct ioam6_lwt_encap *tuninfo,
> + bool has_tunsrc,
> + struct in6_addr *tunsrc,
> struct in6_addr *tundst)
> {
> struct dst_entry *dst = skb_dst(skb);
> @@ -286,8 +313,13 @@ static int ioam6_do_encap(struct net *net, struct sk_buff *skb,
> hdr->nexthdr = NEXTHDR_HOP;
> hdr->payload_len = cpu_to_be16(skb->len - sizeof(*hdr));
> hdr->daddr = *tundst;
> - ipv6_dev_get_saddr(net, dst->dev, &hdr->daddr,
> - IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
> +
> + if (has_tunsrc) {
> + memcpy(&hdr->saddr, tunsrc, sizeof(*tunsrc));
> + } else {
> + ipv6_dev_get_saddr(net, dst->dev, &hdr->daddr,
> + IPV6_PREFER_SRC_PUBLIC, &hdr->saddr);
> + }
single statement branches, no need for {}
> skb_postpush_rcsum(skb, hdr, len);
>
Powered by blists - more mailing lists