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