[ovs-dev] lib/tc: Support optional tunnel id

Currently the TC tunnel_key action is always
initialized with the given tunnel id value. However,
some tunneling protocols define the tunnel id as an optional field.

This patch initializes the id field of tunnel_key:set and tunnel_key:unset
only if a value is provided.

In the case that a tunnel key value is not provided by the user
the key flag will not be set.

Signed-off-by: Adi Nissim <adin@mellanox.com>
Acked-by: Paul Blakey <paulb@mellanox.com>
---
 lib/netdev-tc-offloads.c | 11 +++++++++--
 lib/tc.c                 | 20 ++++++++++++++------
 lib/tc.h                 |  1 +
 3 files changed, 24 insertions(+), 8 deletions(-)

--
1.8.3.1

Bleep bloop.  Greetings Adi Nissim, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line is 83 characters long (recommended limit is 79)
#45 FILE: lib/netdev-tc-offloads.c:634:
                    nl_msg_put_be64(buf, OVS_TUNNEL_KEY_ATTR_ID, action->encap.id);

WARNING: Line is 85 characters long (recommended limit is 79)
#68 FILE: lib/netdev-tc-offloads.c:1108:
        flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;

WARNING: Line is 80 characters long (recommended limit is 79)
#121 FILE: lib/tc.c:1913:
                nl_msg_put_act_tunnel_key_set(request, action->encap.id_present,

Lines checked: 160, Warnings: 3, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@bytheb.org

Thanks,
0-day Robot

Hi Adi,

thanks for your patch.

On Thu, Jan 17, 2019 at 05:41:36PM +0200, Adi Nissim wrote:
> Currently the TC tunnel_key action is always
> initialized with the given tunnel id value. However,
> some tunneling protocols define the tunnel id as an optional field.
> 
> This patch initializes the id field of tunnel_key:set and tunnel_key:unset
> only if a value is provided.
> 
> In the case that a tunnel key value is not provided by the user
> the key flag will not be set.
> 
> Signed-off-by: Adi Nissim <adin@mellanox.com>
> Acked-by: Paul Blakey <paulb@mellanox.com>
> ---
>  lib/netdev-tc-offloads.c | 11 +++++++++--
>  lib/tc.c                 | 20 ++++++++++++++------
>  lib/tc.h                 |  1 +
>  3 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index 73ce7b9..b4a397f 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -574,7 +574,9 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>      }
> 
>      if (flower->tunnel) {
> -        match_set_tun_id(match, flower->key.tunnel.id);
> +        if (flower->mask.tunnel.id) {
> +            match_set_tun_id(match, flower->key.tunnel.id);
> +        }
>          if (flower->key.tunnel.ipv4.ipv4_dst) {
>              match_set_tun_src(match, flower->key.tunnel.ipv4.ipv4_src);
>              match_set_tun_dst(match, flower->key.tunnel.ipv4.ipv4_dst);
> @@ -628,7 +630,9 @@ parse_tc_flower_to_match(struct tc_flower *flower,
>                  size_t tunnel_offset =
>                      nl_msg_start_nested(buf, OVS_KEY_ATTR_TUNNEL);
> 
> -                nl_msg_put_be64(buf, OVS_TUNNEL_KEY_ATTR_ID, action->encap.id);
> +                if (action->encap.id_present) {
> +                    nl_msg_put_be64(buf, OVS_TUNNEL_KEY_ATTR_ID, action->encap.id);
> +                }
>                  if (action->encap.ipv4.ipv4_src) {
>                      nl_msg_put_be32(buf, OVS_TUNNEL_KEY_ATTR_IPV4_SRC,
>                                      action->encap.ipv4.ipv4_src);
> @@ -830,11 +834,13 @@ parse_put_flow_set_action(struct tc_flower *flower, struct tc_action *action,
>      tunnel_len = nl_attr_get_size(set);
> 
>      action->type = TC_ACT_ENCAP;
> +    action->encap.id_present = false;
>      flower->action_count++;
>      NL_ATTR_FOR_EACH_UNSAFE(tun_attr, tun_left, tunnel, tunnel_len) {
>          switch (nl_attr_type(tun_attr)) {
>          case OVS_TUNNEL_KEY_ATTR_ID: {
>              action->encap.id = nl_attr_get_be64(tun_attr);
> +            action->encap.id_present = true;
>          }
>          break;
>          case OVS_TUNNEL_KEY_ATTR_IPV4_SRC: {
> @@ -1099,6 +1105,7 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>          flower.key.tunnel.tp_dst = tnl->tp_dst;
>          flower.mask.tunnel.tos = tnl_mask->ip_tos;
>          flower.mask.tunnel.ttl = tnl_mask->ip_ttl;
> +        flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? tnl_mask->tun_id : 0;

Is the tnl->flags check necessary? What is the meaning of a match on the
flags if the tunnel type doesn't support that field. Is it possible to
configure such a flow?

>          flower_match_to_tun_opt(&flower, tnl, tnl_mask);
>          flower.tunnel = true;
>      }
> diff --git a/lib/tc.c b/lib/tc.c
> index b19f075..04dac5f 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -571,6 +571,7 @@ nl_parse_flower_tunnel(struct nlattr **attrs, struct tc_flower *flower)
>          ovs_be32 id = nl_attr_get_be32(attrs[TCA_FLOWER_KEY_ENC_KEY_ID]);
> 
>          flower->key.tunnel.id = be32_to_be64(id);
> +        flower->mask.tunnel.id = OVS_BE64_MAX;
>      }
>      if (attrs[TCA_FLOWER_KEY_ENC_IPV4_SRC_MASK]) {
>          flower->key.tunnel.ipv4.ipv4_src =
> @@ -1014,6 +1015,7 @@ nl_parse_act_tunnel_key(struct nlattr *options, struct tc_flower *flower)
>              action->encap.ipv6.ipv6_dst = nl_attr_get_in6_addr(ipv6_dst);
>          }
>          action->encap.id = id ? be32_to_be64(nl_attr_get_be32(id)) : 0;
> +        action->encap.id_present = id ? true : false;
>          action->encap.tp_dst = dst_port ? nl_attr_get_be16(dst_port) : 0;
>          action->encap.tos = tos ? nl_attr_get_u8(tos) : 0;
>          action->encap.ttl = ttl ? nl_attr_get_u8(ttl) : 0;
> @@ -1631,9 +1633,9 @@ nl_msg_put_act_tunnel_geneve_option(struct ofpbuf *request,
>  }
> 
>  static void
> -nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, ovs_be64 id,
> -                              ovs_be32 ipv4_src, ovs_be32 ipv4_dst,
> -                              struct in6_addr *ipv6_src,
> +nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, bool id_present,
> +                              ovs_be64 id, ovs_be32 ipv4_src,
> +                              ovs_be32 ipv4_dst, struct in6_addr *ipv6_src,
>                                struct in6_addr *ipv6_dst,
>                                ovs_be16 tp_dst, uint8_t tos, uint8_t ttl,
>                                struct tun_metadata tun_metadata,
> @@ -1650,7 +1652,9 @@ nl_msg_put_act_tunnel_key_set(struct ofpbuf *request, ovs_be64 id,
>          nl_msg_put_unspec(request, TCA_TUNNEL_KEY_PARMS, &tun, sizeof tun);
> 
>          ovs_be32 id32 = be64_to_be32(id);
> -        nl_msg_put_be32(request, TCA_TUNNEL_KEY_ENC_KEY_ID, id32);
> +        if (id_present) {
> +            nl_msg_put_be32(request, TCA_TUNNEL_KEY_ENC_KEY_ID, id32);
> +        }
>          if (ipv4_dst) {
>              nl_msg_put_be32(request, TCA_TUNNEL_KEY_ENC_IPV4_SRC, ipv4_src);
>              nl_msg_put_be32(request, TCA_TUNNEL_KEY_ENC_IPV4_DST, ipv4_dst);
> @@ -1906,7 +1910,8 @@ nl_msg_put_flower_acts(struct ofpbuf *request, struct tc_flower *flower)
>              break;
>              case TC_ACT_ENCAP: {
>                  act_offset = nl_msg_start_nested(request, act_index++);
> -                nl_msg_put_act_tunnel_key_set(request, action->encap.id,
> +                nl_msg_put_act_tunnel_key_set(request, action->encap.id_present,
> +                                              action->encap.id,
>                                                action->encap.ipv4.ipv4_src,
>                                                action->encap.ipv4.ipv4_dst,
>                                                &action->encap.ipv6.ipv6_src,
> @@ -2026,6 +2031,7 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>      uint8_t ttl = flower->key.tunnel.ttl;
>      uint8_t tos_mask = flower->mask.tunnel.tos;
>      uint8_t ttl_mask = flower->mask.tunnel.ttl;
> +    ovs_be64 id_mask = flower->mask.tunnel.id;
> 
>      if (ipv4_dst) {
>          nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_IPV4_SRC, ipv4_src);
> @@ -2045,7 +2051,9 @@ nl_msg_put_flower_tunnel(struct ofpbuf *request, struct tc_flower *flower)
>      if (tp_dst) {
>          nl_msg_put_be16(request, TCA_FLOWER_KEY_ENC_UDP_DST_PORT, tp_dst);
>      }
> -    nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);
> +    if (id_mask) {
> +        nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id);

It seems to me that the code without your patch assumes that
the mask is all-ones, or in other words an exact-match. This is
the only match currently supported by TC flower.

So I think that checking the mask.tunnel.id is a good idea, but
shouldn't the check be such that OVS:

1. Offloads matches that include an exact match on the tunnel id
2. Skips matching the tunnel id is the mask is 0 (the purpose of this patch)
3. Rejects offloading flows where the tunnel id mask is neither 0 nor
   all-ones

> +    }
>      nl_msg_put_flower_tunnel_opts(request, TCA_FLOWER_KEY_ENC_OPTS,
>                                    flower->key.tunnel.metadata);
>      nl_msg_put_flower_tunnel_opts(request, TCA_FLOWER_KEY_ENC_OPTS_MASK,
> diff --git a/lib/tc.h b/lib/tc.h
> index 7196a32..6643f24 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -147,6 +147,7 @@ struct tc_action {
>          } vlan;
> 
>          struct {
> +            bool id_present;
>              ovs_be64 id;
>              ovs_be16 tp_src;
>              ovs_be16 tp_dst;
> --
> 1.8.3.1
>

Hi Simon,

thank you for the comments

Is the tnl->flags check necessary? What is the meaning of a match on the flags if the tunnel type doesn't support that field. Is it possible to configure such a flow?


it is possible to configure such flow, for example GRE tunnel without key is a possible option and in this case the key flag will not be set.

this is why the check tnl->flags is needed.

It seems to me that the code without your patch assumes that the mask is all-ones, or in other words an exact-match. This is the only match currently supported by TC flower.

So I think that checking the mask.tunnel.id is a good idea, but shouldn't the check be such that OVS:

1. Offloads matches that include an exact match on the tunnel id 2. Skips matching the tunnel id is the mask is 0 (the purpose of this patch) 3. Rejects offloading flows where the tunnel id mask is neither 0 nor
   all-ones

I will make the necessary changes and send a new patch.

Thanks,

Adi

On Mon, Jan 21, 2019 at 11:53:20AM +0000, Adi Nissim wrote:
> Hi Simon,
> 
> thank you for the comments
> 
> Is the tnl->flags check necessary? What is the meaning of a match on the flags if the tunnel type doesn't support that field. Is it possible to configure such a flow?
> 
> 
> it is possible to configure such flow, for example GRE tunnel without key is a possible option and in this case the key flag will not be set.
> 
> this is why the check tnl->flags is needed.

Thanks, that makes sense.

> 
> It seems to me that the code without your patch assumes that the mask is all-ones, or in other words an exact-match. This is the only match currently supported by TC flower.
> 
> So I think that checking the mask.tunnel.id is a good idea, but shouldn't the check be such that OVS:
> 
> 1. Offloads matches that include an exact match on the tunnel id 2. Skips matching the tunnel id is the mask is 0 (the purpose of this patch) 3. Rejects offloading flows where the tunnel id mask is neither 0 nor
>    all-ones
> 
> I will make the necessary changes and send a new patch.

Thanks, much appreciated.