| Message ID | 1548084757-5373-1-git-send-email-adin@mellanox.com |
|---|---|
| State | Rejected |
| Delegated to: | Simon Horman |
| Headers | show |
| Series | [ovs-dev,v2] lib/tc: Support optional tunnel id | expand |
Hi Simon, Just pinging about this change. Thanks, Roi On 21/01/2019 17:32, 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> > --- > v1->v2: check if mask.tunnel.id == OVS_BE64_MAX > so we won't do match in the case of a partial mask. > > lib/netdev-tc-offloads.c | 13 +++++++++++-- > lib/tc.c | 21 +++++++++++++++------ > lib/tc.h | 1 + > 3 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c > index 73ce7b9..abfbaeb 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 == OVS_BE64_MAX) { > + 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,10 @@ 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 +835,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 +1106,8 @@ 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; > flower_match_to_tun_opt(&flower, tnl, tnl_mask); > flower.tunnel = true; > } > diff --git a/lib/tc.c b/lib/tc.c > index b19f075..e435663 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,9 @@ 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 +2032,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 +2052,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 == OVS_BE64_MAX) { > + nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id); > + } > 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 >
On Thu, Jan 31, 2019 at 07:21:53AM +0000, Roi Dayan wrote: > Hi Simon, > > Just pinging about this change. Sorry for the delay, I am looking at it now.
On Mon, Jan 21, 2019 at 05:32:37PM +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> > --- > v1->v2: check if mask.tunnel.id == OVS_BE64_MAX > so we won't do match in the case of a partial mask. I am still a bit concerned about the partial mask case. It looks like the code will now silently not offload such matches. I think that a partial mask should either be offloaded or offload of the entire flow should be rejected. > > lib/netdev-tc-offloads.c | 13 +++++++++++-- > lib/tc.c | 21 +++++++++++++++------ > lib/tc.h | 1 + > 3 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c > index 73ce7b9..abfbaeb 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 == OVS_BE64_MAX) { > + 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,10 @@ 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 +835,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 +1106,8 @@ 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; > flower_match_to_tun_opt(&flower, tnl, tnl_mask); > flower.tunnel = true; > } > diff --git a/lib/tc.c b/lib/tc.c > index b19f075..e435663 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,9 @@ 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 +2032,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 +2052,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 == OVS_BE64_MAX) { > + nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id); > + } > 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 >
On 31/01/2019 11:58, Simon Horman wrote: > On Mon, Jan 21, 2019 at 05:32:37PM +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> >> --- >> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX >> so we won't do match in the case of a partial mask. > > I am still a bit concerned about the partial mask case. > It looks like the code will now silently not offload such matches. > > I think that a partial mask should either be offloaded or > offload of the entire flow should be rejected. thanks. you are right. I didn't notice it. partial masks should be rejected to fallback to ovs dp instead of ignoring the mask. > >> >> lib/netdev-tc-offloads.c | 13 +++++++++++-- >> lib/tc.c | 21 +++++++++++++++------ >> lib/tc.h | 1 + >> 3 files changed, 27 insertions(+), 8 deletions(-) >> >> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c >> index 73ce7b9..abfbaeb 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 == OVS_BE64_MAX) { >> + 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,10 @@ 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 +835,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 +1106,8 @@ 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; >> flower_match_to_tun_opt(&flower, tnl, tnl_mask); >> flower.tunnel = true; >> } >> diff --git a/lib/tc.c b/lib/tc.c >> index b19f075..e435663 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,9 @@ 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 +2032,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 +2052,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 == OVS_BE64_MAX) { >> + nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id); >> + } >> 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 >>
On 31/01/2019 15:32, Roi Dayan wrote: > > On 31/01/2019 11:58, Simon Horman wrote: >> On Mon, Jan 21, 2019 at 05:32:37PM +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> >>> --- >>> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX >>> so we won't do match in the case of a partial mask. >> I am still a bit concerned about the partial mask case. >> It looks like the code will now silently not offload such matches. >> >> I think that a partial mask should either be offloaded or >> offload of the entire flow should be rejected. > thanks. you are right. I didn't notice it. partial masks should be rejected > to fallback to ovs dp instead of ignoring the mask. > Hi Simon, I did some checks and think the correct fix is to offload exact match. if key is partial we can ignore the mask and offload exact match and it will be correct as we do more strict matching. it is also documented that the kernel datapath is doing the same (from datapath.rst) "The kernel can ignore the mask attribute, installing an exact match flow" So I think the first patch V0 is actually correct as we check the tunnel key flag exists and offload exact match if there was any mask or offload without a key if the mask is 0 or no key. in netdev-tc-offloads.c + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? + tnl_mask->tun_id : 0; in tc.c - 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); + } let me know what you think. Thanks, Roi
Thanks Roi, On Thu, 31 Jan 2019 at 15:52, Roi Dayan <roid@mellanox.com> wrote: > > > On 31/01/2019 15:32, Roi Dayan wrote: > > > > On 31/01/2019 11:58, Simon Horman wrote: > >> On Mon, Jan 21, 2019 at 05:32:37PM +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> > >>> --- > >>> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX > >>> so we won't do match in the case of a partial mask. > >> I am still a bit concerned about the partial mask case. > >> It looks like the code will now silently not offload such matches. > >> > >> I think that a partial mask should either be offloaded or > >> offload of the entire flow should be rejected. > > thanks. you are right. I didn't notice it. partial masks should be > rejected > > to fallback to ovs dp instead of ignoring the mask. > > > > > Hi Simon, > > I did some checks and think the correct fix is to offload exact match. > if key is partial we can ignore the mask and offload exact match and > it will be correct as we do more strict matching. > > it is also documented that the kernel datapath is doing the same > (from datapath.rst) > > "The kernel can ignore the mask attribute, installing an exact > match flow" > > So I think the first patch V0 is actually correct as we > check the tunnel key flag exists and offload exact match if > there was any mask or offload without a key if the mask is 0 > or no key. > > in netdev-tc-offloads.c > > + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > + tnl_mask->tun_id : 0; > I think this is fine so long as tun_id is all-ones. Is that always the case? Should the code check that it is the case? Am I missing the point? > > > in tc.c > > - 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); > + } > > > let me know what you think. > > Thanks, > Roi >
On Fri, Feb 1, 2019 at 4:05 PM Simon Horman <simon.horman@netronome.com> wrote: > > Thanks Roi, > > On Thu, 31 Jan 2019 at 15:52, Roi Dayan <roid@mellanox.com> wrote: > > > > > > > On 31/01/2019 15:32, Roi Dayan wrote: > > > > > > On 31/01/2019 11:58, Simon Horman wrote: > > >> On Mon, Jan 21, 2019 at 05:32:37PM +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> > > >>> --- > > >>> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX > > >>> so we won't do match in the case of a partial mask. > > >> I am still a bit concerned about the partial mask case. > > >> It looks like the code will now silently not offload such matches. > > >> > > >> I think that a partial mask should either be offloaded or > > >> offload of the entire flow should be rejected. > > > thanks. you are right. I didn't notice it. partial masks should be > > rejected > > > to fallback to ovs dp instead of ignoring the mask. > > > > > > > > > Hi Simon, > > > > I did some checks and think the correct fix is to offload exact match. > > if key is partial we can ignore the mask and offload exact match and > > it will be correct as we do more strict matching. > > > > it is also documented that the kernel datapath is doing the same > > (from datapath.rst) > > > > "The kernel can ignore the mask attribute, installing an exact > > match flow" > > > > So I think the first patch V0 is actually correct as we > > check the tunnel key flag exists and offload exact match if > > there was any mask or offload without a key if the mask is 0 > > or no key. > > > > in netdev-tc-offloads.c > > > > + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > > + tnl_mask->tun_id : 0; > > > > I think this is fine so long as tun_id is all-ones. Is that always the case? > Should the code check that it is the case? Am I missing the point? > it looks like tun_id mask is always set to all-ones. but even if it won't be in the future, we shouldn't really care here. tc adds exact match on the tun_id and ignores the tun_id mask. this is considered ok as the matching is more strict. if new match is needed with different tun_id then ovs will try to add another rule for it. so with tc we could have multiple rules vs 1 rule that support mask. > > > > > > > in tc.c > > > > - 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); > > + } > > > > > > let me know what you think. > > > > Thanks, > > Roi > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On Sun, 3 Feb 2019 at 12:03, Roi Dayan <roi.dayan@gmail.com> wrote: > On Fri, Feb 1, 2019 at 4:05 PM Simon Horman <simon.horman@netronome.com> > wrote: > > > > Thanks Roi, > > > > On Thu, 31 Jan 2019 at 15:52, Roi Dayan <roid@mellanox.com> wrote: > > > > > > > > > > > On 31/01/2019 15:32, Roi Dayan wrote: > > > > > > > > On 31/01/2019 11:58, Simon Horman wrote: > > > >> On Mon, Jan 21, 2019 at 05:32:37PM +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> > > > >>> --- > > > >>> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX > > > >>> so we won't do match in the case of a partial mask. > > > >> I am still a bit concerned about the partial mask case. > > > >> It looks like the code will now silently not offload such matches. > > > >> > > > >> I think that a partial mask should either be offloaded or > > > >> offload of the entire flow should be rejected. > > > > thanks. you are right. I didn't notice it. partial masks should be > > > rejected > > > > to fallback to ovs dp instead of ignoring the mask. > > > > > > > > > > > > > Hi Simon, > > > > > > I did some checks and think the correct fix is to offload exact match. > > > if key is partial we can ignore the mask and offload exact match and > > > it will be correct as we do more strict matching. > > > > > > it is also documented that the kernel datapath is doing the same > > > (from datapath.rst) > > > > > > "The kernel can ignore the mask attribute, installing an exact > > > match flow" > > > > > > So I think the first patch V0 is actually correct as we > > > check the tunnel key flag exists and offload exact match if > > > there was any mask or offload without a key if the mask is 0 > > > or no key. > > > > > > in netdev-tc-offloads.c > > > > > > + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > > > + tnl_mask->tun_id : 0; > > > > > > > I think this is fine so long as tun_id is all-ones. Is that always the > case? > > Should the code check that it is the case? Am I missing the point? > > > > it looks like tun_id mask is always set to all-ones. > but even if it won't be in the future, we shouldn't really care here. > tc adds exact match on the tun_id and ignores the tun_id mask. > this is considered ok as the matching is more strict. > if new match is needed with different tun_id then ovs will try to add > another rule for it. > so with tc we could have multiple rules vs 1 rule that support mask. > Thanks for looking into this. That sounds find to me but I wonder if we should make this behaviour explicit. /* * Comment describing why the mask is 0 or all-ones */ flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0; > > > > > > > > > > in tc.c > > > > > > - 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); > > > + } > > > > > > > > > let me know what you think. > > > > > > Thanks, > > > Roi > > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On Mon, Feb 4, 2019 at 11:00 AM Simon Horman <simon.horman@netronome.com> wrote: > > > > On Sun, 3 Feb 2019 at 12:03, Roi Dayan <roi.dayan@gmail.com> wrote: >> >> On Fri, Feb 1, 2019 at 4:05 PM Simon Horman <simon.horman@netronome.com> wrote: >> > >> > Thanks Roi, >> > >> > On Thu, 31 Jan 2019 at 15:52, Roi Dayan <roid@mellanox.com> wrote: >> > >> > > >> > > >> > > On 31/01/2019 15:32, Roi Dayan wrote: >> > > > >> > > > On 31/01/2019 11:58, Simon Horman wrote: >> > > >> On Mon, Jan 21, 2019 at 05:32:37PM +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> >> > > >>> --- >> > > >>> v1->v2: check if mask.tunnel.id == OVS_BE64_MAX >> > > >>> so we won't do match in the case of a partial mask. >> > > >> I am still a bit concerned about the partial mask case. >> > > >> It looks like the code will now silently not offload such matches. >> > > >> >> > > >> I think that a partial mask should either be offloaded or >> > > >> offload of the entire flow should be rejected. >> > > > thanks. you are right. I didn't notice it. partial masks should be >> > > rejected >> > > > to fallback to ovs dp instead of ignoring the mask. >> > > > >> > > >> > > >> > > Hi Simon, >> > > >> > > I did some checks and think the correct fix is to offload exact match. >> > > if key is partial we can ignore the mask and offload exact match and >> > > it will be correct as we do more strict matching. >> > > >> > > it is also documented that the kernel datapath is doing the same >> > > (from datapath.rst) >> > > >> > > "The kernel can ignore the mask attribute, installing an exact >> > > match flow" >> > > >> > > So I think the first patch V0 is actually correct as we >> > > check the tunnel key flag exists and offload exact match if >> > > there was any mask or offload without a key if the mask is 0 >> > > or no key. >> > > >> > > in netdev-tc-offloads.c >> > > >> > > + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? >> > > + tnl_mask->tun_id : 0; >> > > >> > >> > I think this is fine so long as tun_id is all-ones. Is that always the case? >> > Should the code check that it is the case? Am I missing the point? >> > >> >> it looks like tun_id mask is always set to all-ones. >> but even if it won't be in the future, we shouldn't really care here. >> tc adds exact match on the tun_id and ignores the tun_id mask. >> this is considered ok as the matching is more strict. >> if new match is needed with different tun_id then ovs will try to add >> another rule for it. >> so with tc we could have multiple rules vs 1 rule that support mask. > > > Thanks for looking into this. That sounds find to me but I wonder if we should make > this behaviour explicit. > > /* > * Comment describing why the mask is 0 or all-ones > */ > flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0; > I think its nicer like this and symetric currently. here it's generic and "use" mask. in tc.c when we fill the netlink msg we ignore the mas and also when parsing tc dump, tun mask is set to FFFF here (tc.c) and not netdev-tc-offloads.c >> > >> > > >> > > >> > > in tc.c >> > > >> > > - 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); >> > > + } >> > > >> > > >> > > let me know what you think. >> > > >> > > Thanks, >> > > Roi >> > > >> > _______________________________________________ >> > dev mailing list >> > dev@openvswitch.org >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 04/02/2019 15:20, Roi Dayan wrote: >>>>> Hi Simon, >>>>> >>>>> I did some checks and think the correct fix is to offload exact match. >>>>> if key is partial we can ignore the mask and offload exact match and >>>>> it will be correct as we do more strict matching. >>>>> >>>>> it is also documented that the kernel datapath is doing the same >>>>> (from datapath.rst) >>>>> >>>>> "The kernel can ignore the mask attribute, installing an exact >>>>> match flow" >>>>> >>>>> So I think the first patch V0 is actually correct as we >>>>> check the tunnel key flag exists and offload exact match if >>>>> there was any mask or offload without a key if the mask is 0 >>>>> or no key. >>>>> >>>>> in netdev-tc-offloads.c >>>>> >>>>> + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? >>>>> + tnl_mask->tun_id : 0; >>>>> >>>> I think this is fine so long as tun_id is all-ones. Is that always the case? >>>> Should the code check that it is the case? Am I missing the point? >>>> >>> it looks like tun_id mask is always set to all-ones. >>> but even if it won't be in the future, we shouldn't really care here. >>> tc adds exact match on the tun_id and ignores the tun_id mask. >>> this is considered ok as the matching is more strict. >>> if new match is needed with different tun_id then ovs will try to add >>> another rule for it. >>> so with tc we could have multiple rules vs 1 rule that support mask. >> >> Thanks for looking into this. That sounds find to me but I wonder if we should make >> this behaviour explicit. >> >> /* >> * Comment describing why the mask is 0 or all-ones >> */ >> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0; >> > I think its nicer like this and symetric currently. here it's generic > and "use" mask. > in tc.c when we fill the netlink msg we ignore the mas and also when > parsing tc dump, > tun mask is set to FFFF here (tc.c) and not netdev-tc-offloads.c > Hi Simon, any update? i remind i suggested v0 is the correct one. Thanks, Roi >>>>> >>>>> in tc.c >>>>> >>>>> - 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); >>>>> + }
On Sun, Feb 10, 2019 at 08:04:39AM +0000, Roi Dayan wrote: > > > On 04/02/2019 15:20, Roi Dayan wrote: > >>>>> Hi Simon, > >>>>> > >>>>> I did some checks and think the correct fix is to offload exact match. > >>>>> if key is partial we can ignore the mask and offload exact match and > >>>>> it will be correct as we do more strict matching. > >>>>> > >>>>> it is also documented that the kernel datapath is doing the same > >>>>> (from datapath.rst) > >>>>> > >>>>> "The kernel can ignore the mask attribute, installing an exact > >>>>> match flow" > >>>>> > >>>>> So I think the first patch V0 is actually correct as we > >>>>> check the tunnel key flag exists and offload exact match if > >>>>> there was any mask or offload without a key if the mask is 0 > >>>>> or no key. > >>>>> > >>>>> in netdev-tc-offloads.c > >>>>> > >>>>> + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > >>>>> + tnl_mask->tun_id : 0; > >>>>> > >>>> I think this is fine so long as tun_id is all-ones. Is that always the case? > >>>> Should the code check that it is the case? Am I missing the point? > >>>> > >>> it looks like tun_id mask is always set to all-ones. > >>> but even if it won't be in the future, we shouldn't really care here. > >>> tc adds exact match on the tun_id and ignores the tun_id mask. > >>> this is considered ok as the matching is more strict. > >>> if new match is needed with different tun_id then ovs will try to add > >>> another rule for it. > >>> so with tc we could have multiple rules vs 1 rule that support mask. > >> > >> Thanks for looking into this. That sounds find to me but I wonder if we should make > >> this behaviour explicit. > >> > >> /* > >> * Comment describing why the mask is 0 or all-ones > >> */ > >> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0; > >> > > I think its nicer like this and symetric currently. here it's generic > > and "use" mask. > > in tc.c when we fill the netlink msg we ignore the mas and also when > > parsing tc dump, > > tun mask is set to FFFF here (tc.c) and not netdev-tc-offloads.c > > > > Hi Simon, > > any update? i remind i suggested v0 is the correct one. Thanks and sorry for the ongoing delays. I have added this (v2) to a travisci instance and plan to apply the patch to master if that passes. https://travis-ci.org/horms2/ovs/builds/491523655
On 11/02/2019 11:11, Simon Horman wrote: > On Sun, Feb 10, 2019 at 08:04:39AM +0000, Roi Dayan wrote: >> >> >> On 04/02/2019 15:20, Roi Dayan wrote: >>>>>>> Hi Simon, >>>>>>> >>>>>>> I did some checks and think the correct fix is to offload exact match. >>>>>>> if key is partial we can ignore the mask and offload exact match and >>>>>>> it will be correct as we do more strict matching. >>>>>>> >>>>>>> it is also documented that the kernel datapath is doing the same >>>>>>> (from datapath.rst) >>>>>>> >>>>>>> "The kernel can ignore the mask attribute, installing an exact >>>>>>> match flow" >>>>>>> >>>>>>> So I think the first patch V0 is actually correct as we >>>>>>> check the tunnel key flag exists and offload exact match if >>>>>>> there was any mask or offload without a key if the mask is 0 >>>>>>> or no key. >>>>>>> >>>>>>> in netdev-tc-offloads.c >>>>>>> >>>>>>> + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? >>>>>>> + tnl_mask->tun_id : 0; >>>>>>> >>>>>> I think this is fine so long as tun_id is all-ones. Is that always the case? >>>>>> Should the code check that it is the case? Am I missing the point? >>>>>> >>>>> it looks like tun_id mask is always set to all-ones. >>>>> but even if it won't be in the future, we shouldn't really care here. >>>>> tc adds exact match on the tun_id and ignores the tun_id mask. >>>>> this is considered ok as the matching is more strict. >>>>> if new match is needed with different tun_id then ovs will try to add >>>>> another rule for it. >>>>> so with tc we could have multiple rules vs 1 rule that support mask. >>>> >>>> Thanks for looking into this. That sounds find to me but I wonder if we should make >>>> this behaviour explicit. >>>> >>>> /* >>>> * Comment describing why the mask is 0 or all-ones >>>> */ >>>> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0; >>>> >>> I think its nicer like this and symetric currently. here it's generic >>> and "use" mask. >>> in tc.c when we fill the netlink msg we ignore the mas and also when >>> parsing tc dump, >>> tun mask is set to FFFF here (tc.c) and not netdev-tc-offloads.c >>> >> >> Hi Simon, >> >> any update? i remind i suggested v0 is the correct one. > > Thanks and sorry for the ongoing delays. > > I have added this (v2) to a travisci instance and plan to apply the patch > to master if that passes. Hi Simon, I agreed with you v2 has an issue and we should use v0. v2 will not pass id to tc if there is an id with partial mask which is incorrect. v0 will add rule with exact match. can we go with v0 ? Thanks, Roi > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Ftravis-ci.org%2Fhorms2%2Fovs%2Fbuilds%2F491523655&data=02%7C01%7Croid%40mellanox.com%7C8eae4ff8d37947fcd5c208d69000eb6e%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636854730964289416&sdata=iWAaOzcJf%2B5L4ahKeHb2Hcfxp0LpNOQfto%2BMaohkskk%3D&reserved=0 >
On Mon, Feb 11, 2019 at 09:57:10AM +0000, Roi Dayan wrote: > > > On 11/02/2019 11:11, Simon Horman wrote: > > On Sun, Feb 10, 2019 at 08:04:39AM +0000, Roi Dayan wrote: > >> > >> > >> On 04/02/2019 15:20, Roi Dayan wrote: > >>>>>>> Hi Simon, > >>>>>>> > >>>>>>> I did some checks and think the correct fix is to offload exact match. > >>>>>>> if key is partial we can ignore the mask and offload exact match and > >>>>>>> it will be correct as we do more strict matching. > >>>>>>> > >>>>>>> it is also documented that the kernel datapath is doing the same > >>>>>>> (from datapath.rst) > >>>>>>> > >>>>>>> "The kernel can ignore the mask attribute, installing an exact > >>>>>>> match flow" > >>>>>>> > >>>>>>> So I think the first patch V0 is actually correct as we > >>>>>>> check the tunnel key flag exists and offload exact match if > >>>>>>> there was any mask or offload without a key if the mask is 0 > >>>>>>> or no key. > >>>>>>> > >>>>>>> in netdev-tc-offloads.c > >>>>>>> > >>>>>>> + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > >>>>>>> + tnl_mask->tun_id : 0; > >>>>>>> > >>>>>> I think this is fine so long as tun_id is all-ones. Is that always the case? > >>>>>> Should the code check that it is the case? Am I missing the point? > >>>>>> > >>>>> it looks like tun_id mask is always set to all-ones. > >>>>> but even if it won't be in the future, we shouldn't really care here. > >>>>> tc adds exact match on the tun_id and ignores the tun_id mask. > >>>>> this is considered ok as the matching is more strict. > >>>>> if new match is needed with different tun_id then ovs will try to add > >>>>> another rule for it. > >>>>> so with tc we could have multiple rules vs 1 rule that support mask. > >>>> > >>>> Thanks for looking into this. That sounds find to me but I wonder if we should make > >>>> this behaviour explicit. > >>>> > >>>> /* > >>>> * Comment describing why the mask is 0 or all-ones > >>>> */ > >>>> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0; > >>>> > >>> I think its nicer like this and symetric currently. here it's generic > >>> and "use" mask. > >>> in tc.c when we fill the netlink msg we ignore the mas and also when > >>> parsing tc dump, > >>> tun mask is set to FFFF here (tc.c) and not netdev-tc-offloads.c > >>> > >> > >> Hi Simon, > >> > >> any update? i remind i suggested v0 is the correct one. > > > > Thanks and sorry for the ongoing delays. > > > > I have added this (v2) to a travisci instance and plan to apply the patch > > to master if that passes. > > Hi Simon, > > I agreed with you v2 has an issue and we should use v0. > v2 will not pass id to tc if there is an id with partial mask which is incorrect. > v0 will add rule with exact match. > can we go with v0 ? Sure, can I confirm that you mean this version from 17 Jan: "[PATCH] lib/tc: Support optional tunnel id" https://patchwork.ozlabs.org/patch/1026771/
On 11/02/2019 12:13, Simon Horman wrote: > On Mon, Feb 11, 2019 at 09:57:10AM +0000, Roi Dayan wrote: >> >> >> On 11/02/2019 11:11, Simon Horman wrote: >>> On Sun, Feb 10, 2019 at 08:04:39AM +0000, Roi Dayan wrote: >>>> >>>> >>>> On 04/02/2019 15:20, Roi Dayan wrote: >>>>>>>>> Hi Simon, >>>>>>>>> >>>>>>>>> I did some checks and think the correct fix is to offload exact match. >>>>>>>>> if key is partial we can ignore the mask and offload exact match and >>>>>>>>> it will be correct as we do more strict matching. >>>>>>>>> >>>>>>>>> it is also documented that the kernel datapath is doing the same >>>>>>>>> (from datapath.rst) >>>>>>>>> >>>>>>>>> "The kernel can ignore the mask attribute, installing an exact >>>>>>>>> match flow" >>>>>>>>> >>>>>>>>> So I think the first patch V0 is actually correct as we >>>>>>>>> check the tunnel key flag exists and offload exact match if >>>>>>>>> there was any mask or offload without a key if the mask is 0 >>>>>>>>> or no key. >>>>>>>>> >>>>>>>>> in netdev-tc-offloads.c >>>>>>>>> >>>>>>>>> + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? >>>>>>>>> + tnl_mask->tun_id : 0; >>>>>>>>> >>>>>>>> I think this is fine so long as tun_id is all-ones. Is that always the case? >>>>>>>> Should the code check that it is the case? Am I missing the point? >>>>>>>> >>>>>>> it looks like tun_id mask is always set to all-ones. >>>>>>> but even if it won't be in the future, we shouldn't really care here. >>>>>>> tc adds exact match on the tun_id and ignores the tun_id mask. >>>>>>> this is considered ok as the matching is more strict. >>>>>>> if new match is needed with different tun_id then ovs will try to add >>>>>>> another rule for it. >>>>>>> so with tc we could have multiple rules vs 1 rule that support mask. >>>>>> >>>>>> Thanks for looking into this. That sounds find to me but I wonder if we should make >>>>>> this behaviour explicit. >>>>>> >>>>>> /* >>>>>> * Comment describing why the mask is 0 or all-ones >>>>>> */ >>>>>> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0; >>>>>> >>>>> I think its nicer like this and symetric currently. here it's generic >>>>> and "use" mask. >>>>> in tc.c when we fill the netlink msg we ignore the mas and also when >>>>> parsing tc dump, >>>>> tun mask is set to FFFF here (tc.c) and not netdev-tc-offloads.c >>>>> >>>> >>>> Hi Simon, >>>> >>>> any update? i remind i suggested v0 is the correct one. >>> >>> Thanks and sorry for the ongoing delays. >>> >>> I have added this (v2) to a travisci instance and plan to apply the patch >>> to master if that passes. >> >> Hi Simon, >> >> I agreed with you v2 has an issue and we should use v0. >> v2 will not pass id to tc if there is an id with partial mask which is incorrect. >> v0 will add rule with exact match. >> can we go with v0 ? > > Sure, can I confirm that you mean this version from 17 Jan: > > "[PATCH] lib/tc: Support optional tunnel id" > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1026771%2F&data=02%7C01%7Croid%40mellanox.com%7Ca5fa4a3f7136417620d508d69009845d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636854767892746889&sdata=dU0B%2BPf3H6a4fHCTQfXFkuyuuFcZoYZFihfcGb9dIMY%3D&reserved=0 > yes
On Mon, Feb 11, 2019 at 11:25:28AM +0000, Roi Dayan wrote: > > > On 11/02/2019 12:13, Simon Horman wrote: > > On Mon, Feb 11, 2019 at 09:57:10AM +0000, Roi Dayan wrote: > >> > >> > >> On 11/02/2019 11:11, Simon Horman wrote: > >>> On Sun, Feb 10, 2019 at 08:04:39AM +0000, Roi Dayan wrote: > >>>> > >>>> > >>>> On 04/02/2019 15:20, Roi Dayan wrote: > >>>>>>>>> Hi Simon, > >>>>>>>>> > >>>>>>>>> I did some checks and think the correct fix is to offload exact match. > >>>>>>>>> if key is partial we can ignore the mask and offload exact match and > >>>>>>>>> it will be correct as we do more strict matching. > >>>>>>>>> > >>>>>>>>> it is also documented that the kernel datapath is doing the same > >>>>>>>>> (from datapath.rst) > >>>>>>>>> > >>>>>>>>> "The kernel can ignore the mask attribute, installing an exact > >>>>>>>>> match flow" > >>>>>>>>> > >>>>>>>>> So I think the first patch V0 is actually correct as we > >>>>>>>>> check the tunnel key flag exists and offload exact match if > >>>>>>>>> there was any mask or offload without a key if the mask is 0 > >>>>>>>>> or no key. > >>>>>>>>> > >>>>>>>>> in netdev-tc-offloads.c > >>>>>>>>> > >>>>>>>>> + flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? > >>>>>>>>> + tnl_mask->tun_id : 0; > >>>>>>>>> > >>>>>>>> I think this is fine so long as tun_id is all-ones. Is that always the case? > >>>>>>>> Should the code check that it is the case? Am I missing the point? > >>>>>>>> > >>>>>>> it looks like tun_id mask is always set to all-ones. > >>>>>>> but even if it won't be in the future, we shouldn't really care here. > >>>>>>> tc adds exact match on the tun_id and ignores the tun_id mask. > >>>>>>> this is considered ok as the matching is more strict. > >>>>>>> if new match is needed with different tun_id then ovs will try to add > >>>>>>> another rule for it. > >>>>>>> so with tc we could have multiple rules vs 1 rule that support mask. > >>>>>> > >>>>>> Thanks for looking into this. That sounds find to me but I wonder if we should make > >>>>>> this behaviour explicit. > >>>>>> > >>>>>> /* > >>>>>> * Comment describing why the mask is 0 or all-ones > >>>>>> */ > >>>>>> flower.mask.tunnel.id = (tnl->flags & FLOW_TNL_F_KEY) ? UINT32_MAX : 0; > >>>>>> > >>>>> I think its nicer like this and symetric currently. here it's generic > >>>>> and "use" mask. > >>>>> in tc.c when we fill the netlink msg we ignore the mas and also when > >>>>> parsing tc dump, > >>>>> tun mask is set to FFFF here (tc.c) and not netdev-tc-offloads.c > >>>>> > >>>> > >>>> Hi Simon, > >>>> > >>>> any update? i remind i suggested v0 is the correct one. > >>> > >>> Thanks and sorry for the ongoing delays. > >>> > >>> I have added this (v2) to a travisci instance and plan to apply the patch > >>> to master if that passes. > >> > >> Hi Simon, > >> > >> I agreed with you v2 has an issue and we should use v0. > >> v2 will not pass id to tc if there is an id with partial mask which is incorrect. > >> v0 will add rule with exact match. > >> can we go with v0 ? > > > > Sure, can I confirm that you mean this version from 17 Jan: > > > > "[PATCH] lib/tc: Support optional tunnel id" > > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.ozlabs.org%2Fpatch%2F1026771%2F&data=02%7C01%7Croid%40mellanox.com%7Ca5fa4a3f7136417620d508d69009845d%7Ca652971c7d2e4d9ba6a4d149256f461b%7C0%7C0%7C636854767892746889&sdata=dU0B%2BPf3H6a4fHCTQfXFkuyuuFcZoYZFihfcGb9dIMY%3D&reserved=0 > > > > yes Thanks, applied to master.
diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c index 73ce7b9..abfbaeb 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 == OVS_BE64_MAX) { + 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,10 @@ 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 +835,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 +1106,8 @@ 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; flower_match_to_tun_opt(&flower, tnl, tnl_mask); flower.tunnel = true; } diff --git a/lib/tc.c b/lib/tc.c index b19f075..e435663 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,9 @@ 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 +2032,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 +2052,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 == OVS_BE64_MAX) { + nl_msg_put_be32(request, TCA_FLOWER_KEY_ENC_KEY_ID, id); + } 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;