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 <[email protected]> > Acked-by: Paul Blakey <[email protected]> > --- > 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 > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
