[ovs-dev,ovs,V8,13/26] netdev-tc-offloads: Implement netdev flow put using tc interface

From: Paul Blakey <paulb@mellanox.com>

Currently only tunnel offload is supported.

Signed-off-by: Paul Blakey <paulb@mellanox.com>
Reviewed-by: Roi Dayan <roid@mellanox.com>
Reviewed-by: Simon Horman <simon.horman@netronome.com>
---
 lib/dpif-netlink.c       |   4 +-
 lib/netdev-tc-offloads.c | 392 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 385 insertions(+), 11 deletions(-)

On Wed, May 03, 2017 at 06:08:04PM +0300, Roi Dayan wrote:
> From: Paul Blakey <paulb@mellanox.com>
> 
> Currently only tunnel offload is supported.
> 
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> ---
>  lib/dpif-netlink.c       |   4 +-
>  lib/netdev-tc-offloads.c | 392 ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 385 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 81d513d..7e6c647 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1937,8 +1937,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>          return EOPNOTSUPP;
>      }
>  
> -    memset(&match, 0, sizeof match);
> -
>      err = parse_key_and_mask_to_match(put->key, put->key_len, put->mask,
>                                        put->mask_len, &match);
>      if (err) {
> @@ -2011,7 +2009,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>  
>          VLOG_DBG("added flow");
>      } else if (err != EEXIST) {
> -        VLOG_ERR_RL(&rl, "failed to offload flow: %s", ovs_strerror(err));
> +        VLOG_ERR_RL(&rl, "failed adding flow: %s", ovs_strerror(err));
>      }
>  
>  out:
> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index 7e33fff..e3daf62 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -461,16 +461,392 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>      return false;
>  }
>  
> +static int
> +parse_put_flow_set_action(struct tc_flower *flower, const struct nlattr *set,
> +                          size_t set_len)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +    const struct nlattr *set_attr;
> +    size_t set_left;
> +
> +    NL_ATTR_FOR_EACH_UNSAFE(set_attr, set_left, set, set_len) {
> +        if (nl_attr_type(set_attr) == OVS_KEY_ATTR_TUNNEL) {
> +            const struct nlattr *tunnel = nl_attr_get(set_attr);
> +            const size_t tunnel_len = nl_attr_get_size(set_attr);
> +            const struct nlattr *tun_attr;
> +            size_t tun_left;
> +
> +            flower->set.set = true;
> +            NL_ATTR_FOR_EACH_UNSAFE(tun_attr, tun_left, tunnel, tunnel_len) {
> +                switch (nl_attr_type(tun_attr)) {
> +                case OVS_TUNNEL_KEY_ATTR_ID: {
> +                    flower->set.id = nl_attr_get_be64(tun_attr);
> +                }
> +                break;
> +                case OVS_TUNNEL_KEY_ATTR_IPV4_SRC: {
> +                    flower->set.ipv4.ipv4_src = nl_attr_get_be32(tun_attr);
> +                }
> +                break;
> +                case OVS_TUNNEL_KEY_ATTR_IPV4_DST: {
> +                    flower->set.ipv4.ipv4_dst = nl_attr_get_be32(tun_attr);
> +                }
> +                break;
> +                case OVS_TUNNEL_KEY_ATTR_IPV6_SRC: {
> +                    flower->set.ipv6.ipv6_src =
> +                        nl_attr_get_in6_addr(tun_attr);
> +                }
> +                break;
> +                case OVS_TUNNEL_KEY_ATTR_IPV6_DST: {
> +                    flower->set.ipv6.ipv6_dst =
> +                        nl_attr_get_in6_addr(tun_attr);
> +                }
> +                break;
> +                case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
> +                    flower->set.tp_src = nl_attr_get_be16(tun_attr);
> +                }
> +                break;
> +                case OVS_TUNNEL_KEY_ATTR_TP_DST: {
> +                    flower->set.tp_dst = nl_attr_get_be16(tun_attr);
> +                }
> +                break;
> +                }
> +            }
> +        } else {
> +            VLOG_DBG_RL(&rl, "unsupported set action type: %d",
> +                        nl_attr_type(set_attr));
> +            return EOPNOTSUPP;
> +        }
> +    }
> +    return 0;
> +}
> +
> +static int
> +test_key_and_mask(struct match *match) {
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +    const struct flow *key = &match->flow;
> +    struct flow *mask = &match->wc.masks;
> +
> +    if (mask->pkt_mark) {
> +        VLOG_DBG_RL(&rl, "offloading attribute pkt_mark isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->recirc_id && key->recirc_id != 0) {
> +        VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +    mask->recirc_id = 0;
> +
> +    if (mask->dp_hash) {
> +        VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->conj_id) {
> +        VLOG_DBG_RL(&rl, "offloading attribute conj_id isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->skb_priority) {
> +        VLOG_DBG_RL(&rl, "offloading attribute skb_priority isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->actset_output) {
> +        VLOG_DBG_RL(&rl,
> +                    "offloading attribute actset_output isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->ct_state) {
> +        VLOG_DBG_RL(&rl, "offloading attribute ct_state isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->ct_zone) {
> +        VLOG_DBG_RL(&rl, "offloading attribute ct_zone isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->ct_mark) {
> +        VLOG_DBG_RL(&rl, "offloading attribute ct_zone isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (!ovs_u128_is_zero(mask->ct_label)) {
> +        VLOG_DBG_RL(&rl, "offloading attribute ct_lable isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    for (int i = 0; i < FLOW_N_REGS; i++) {
> +        if (mask->regs[i]) {
> +            VLOG_DBG_RL(&rl,
> +                        "offloading attribute regs[%d] isn't supported", i);
> +            return EOPNOTSUPP;
> +        }
> +    }
> +
> +    if (mask->metadata != 0) {
> +        VLOG_DBG_RL(&rl, "offloading attribute metadata isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->nw_tos) {
> +        VLOG_DBG_RL(&rl, "offloading attribute nw_tos isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->nw_ttl) {
> +        VLOG_DBG_RL(&rl, "offloading attribute nw_ttl isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->nw_frag) {
> +        VLOG_DBG_RL(&rl, "offloading attribute nw_frag isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    for (int i = 0; i < FLOW_MAX_MPLS_LABELS; i++) {
> +        if (mask->mpls_lse[i]) {
> +            VLOG_DBG_RL(&rl, "offloading attribute mpls_lse isn't supported");
> +            return EOPNOTSUPP;
> +        }
> +    }
> +
> +    if (key->dl_type == htons(ETH_TYPE_IP) &&
> +        key->nw_proto == IPPROTO_ICMP) {
> +        if (mask->tp_src) {
> +            VLOG_DBG_RL(&rl,
> +                        "offloading attribute icmp_type isn't supported");
> +            return EOPNOTSUPP;
> +        }
> +        if (mask->tp_dst) {
> +            VLOG_DBG_RL(&rl,
> +                        "offloading attribute icmp_code isn't supported");
> +            return EOPNOTSUPP;
> +        }
> +    } else if (key->dl_type == htons(ETH_TYPE_IP) &&
> +               key->nw_proto == IPPROTO_IGMP) {
> +        if (mask->tp_src) {
> +            VLOG_DBG_RL(&rl,
> +                        "offloading attribute igmp_type isn't supported");
> +            return EOPNOTSUPP;
> +        }
> +        if (mask->tp_dst) {
> +            VLOG_DBG_RL(&rl,
> +                        "offloading attribute igmp_code isn't supported");
> +            return EOPNOTSUPP;
> +        }
> +    } else if (key->dl_type == htons(ETH_TYPE_IPV6) &&
> +               key->nw_proto == IPPROTO_ICMPV6) {
> +        if (mask->tp_src) {
> +            VLOG_DBG_RL(&rl,
> +                        "offloading attribute icmp_type isn't supported");
> +            return EOPNOTSUPP;
> +        }
> +        if (mask->tp_dst) {
> +            VLOG_DBG_RL(&rl,
> +                        "offloading attribute icmp_code isn't supported");
> +            return EOPNOTSUPP;
> +        }
> +    }
> +    if (is_ip_any(key) && key->nw_proto == IPPROTO_TCP && mask->tcp_flags) {
> +        if (mask->tcp_flags) {
> +            VLOG_DBG_RL(&rl,
> +                        "offloading attribute tcp_flags isn't supported");
> +            return EOPNOTSUPP;
> +        }
> +    }
> +
> +    if (!is_all_zeros(mask, sizeof *mask)) {
> +        VLOG_DBG_RL(&rl, "offloading isn't supported, unknown attribute");
> +        return EOPNOTSUPP;
> +    }
> +
> +    return 0;
> +}
> +
>  int
> -netdev_tc_flow_put(struct netdev *netdev OVS_UNUSED,
> -                   struct match *match OVS_UNUSED,
> -                   struct nlattr *actions OVS_UNUSED,
> -                   size_t actions_len OVS_UNUSED,
> -                   struct dpif_flow_stats *stats OVS_UNUSED,
> -                   const ovs_u128 *ufid OVS_UNUSED,
> -                   struct offload_info *info OVS_UNUSED)
> +netdev_tc_flow_put(struct netdev *netdev,
> +                   struct match *match,
> +                   struct nlattr *actions,
> +                   size_t actions_len,
> +                   const ovs_u128 *ufid,
> +                   struct offload_info *info,
> +                   struct dpif_flow_stats *stats OVS_UNUSED)
>  {
> -    return EOPNOTSUPP;
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +    struct tc_flower flower;
> +    const struct flow *key = &match->flow;
> +    struct flow *mask = &match->wc.masks;
> +    const struct flow_tnl *tnl = &match->flow.tunnel;
> +    struct nlattr *nla;
> +    size_t left;
> +    int prio = 0;
> +    int handle;
> +    int ifindex;
> +    int err;
> +
> +    ifindex = netdev_get_ifindex(netdev);
> +    if (ifindex < 0) {
> +        VLOG_ERR_RL(&rl_err, "failed to get ifindex for %s: %s",
> +                    netdev_get_name(netdev), ovs_strerror(-ifindex));
> +        return -ifindex;
> +    }
> +
> +    memset(&flower, 0, sizeof flower);
> +
> +    if (tnl->tun_id) {
> +        VLOG_DBG_RL(&rl,
> +                    "tunnel: id %#" PRIx64 " src " IP_FMT
> +                    " dst " IP_FMT " tp_src %d tp_dst %d",
> +                    ntohll(tnl->tun_id),
> +                    IP_ARGS(tnl->ip_src), IP_ARGS(tnl->ip_dst),
> +                    ntohs(tnl->tp_src), ntohs(tnl->tp_dst));
> +        flower.tunnel.id = tnl->tun_id;
> +        flower.tunnel.ipv4.ipv4_src = tnl->ip_src;
> +        flower.tunnel.ipv4.ipv4_dst = tnl->ip_dst;
> +        flower.tunnel.ipv6.ipv6_src = tnl->ipv6_src;
> +        flower.tunnel.ipv6.ipv6_dst = tnl->ipv6_dst;
> +        flower.tunnel.tp_src = tnl->tp_src;
> +        flower.tunnel.tp_dst = tnl->tp_dst;
> +        flower.tunnel.tunnel = true;
> +
> +        memset(&mask->tunnel, 0, sizeof mask->tunnel);
> +    }
> +
> +    flower.key.eth_type = key->dl_type;
> +    flower.mask.eth_type = mask->dl_type;
> +
> +    if (mask->vlans[0].tci) {
> +        ovs_be16 vid_mask = mask->vlans[0].tci & htons(VLAN_VID_MASK);
> +        ovs_be16 pcp_mask = mask->vlans[0].tci & htons(VLAN_PCP_MASK);
> +        ovs_be16 cfi = mask->vlans[0].tci & htons(VLAN_CFI);
> +
> +        if (cfi && key->vlans[0].tci & htons(VLAN_CFI)
> +            && (!vid_mask || vid_mask == htons(VLAN_VID_MASK))
> +            && (!pcp_mask || pcp_mask == htons(VLAN_PCP_MASK))
> +            && (vid_mask || pcp_mask)) {
> +            if (vid_mask) {
> +                flower.key.vlan_id = vlan_tci_to_vid(key->vlans[0].tci);
> +                VLOG_DBG_RL(&rl, "vlan_id: %d\n", flower.key.vlan_id);
> +            }
> +            if (pcp_mask) {
> +                flower.key.vlan_prio = vlan_tci_to_pcp(key->vlans[0].tci);
> +                VLOG_DBG_RL(&rl, "vlan_prio: %d\n", flower.key.vlan_prio);
> +            }
> +            flower.key.encap_eth_type = flower.key.eth_type;
> +            flower.key.eth_type = htons(ETH_TYPE_VLAN);
> +        } else if (mask->vlans[0].tci == htons(0xffff) &&
> +                   ntohs(key->vlans[0].tci) == 0) {
> +            /* exact && no vlan */
> +        } else {
> +            /* partial mask */
> +            return EOPNOTSUPP;
> +        }
> +    } else if (mask->vlans[1].tci) {
> +        return EOPNOTSUPP;
> +    }
> +    memset(mask->vlans, 0, sizeof mask->vlans);
> +
> +    flower.key.dst_mac = key->dl_dst;
> +    memset(&flower.mask.dst_mac, 0xFF, sizeof(flower.mask.dst_mac));
> +    flower.key.src_mac = key->dl_src;
> +    flower.mask.src_mac = mask->dl_src;

Repeating my comment on V7: Why are you fixing the dst_mac mask to
be an exact match?

Flavio

> +    memset(&mask->dl_src, 0, sizeof mask->dl_src);
> +    memset(&mask->dl_dst, 0, sizeof mask->dl_dst);
> +    mask->dl_type = 0;
> +    mask->in_port.odp_port = 0;
> +
> +    if (is_ip_any(key)) {
> +        flower.key.ip_proto = key->nw_proto;
> +        flower.mask.ip_proto = mask->nw_proto;
> +
> +        if (key->nw_proto == IPPROTO_TCP || key->nw_proto == IPPROTO_UDP) {
> +            flower.key.dst_port = key->tp_dst;
> +            flower.mask.dst_port = mask->tp_dst;
> +            flower.key.src_port = key->tp_src;
> +            flower.mask.src_port = mask->tp_src;
> +            mask->tp_src = 0;
> +            mask->tp_dst = 0;
> +        }
> +
> +        mask->nw_frag = 0;
> +        mask->nw_tos = 0;
> +        mask->nw_proto = 0;
> +
> +        if (flower.key.eth_type == htons(ETH_P_IP)) {
> +            flower.key.ipv4.ipv4_src = key->nw_src;
> +            flower.mask.ipv4.ipv4_src = mask->nw_src;
> +            flower.key.ipv4.ipv4_dst = key->nw_dst;
> +            flower.mask.ipv4.ipv4_dst = mask->nw_dst;
> +            mask->nw_src = 0;
> +            mask->nw_dst = 0;
> +        } else if (flower.key.eth_type == htons(ETH_P_IPV6)) {
> +            flower.key.ipv6.ipv6_src = key->ipv6_src;
> +            flower.mask.ipv6.ipv6_src = mask->ipv6_src;
> +            flower.key.ipv6.ipv6_dst = key->ipv6_dst;
> +            flower.mask.ipv6.ipv6_dst = mask->ipv6_dst;
> +            memset(&mask->ipv6_src, 0, sizeof mask->ipv6_src);
> +            memset(&mask->ipv6_dst, 0, sizeof mask->ipv6_dst);
> +        }
> +    }
> +
> +    err = test_key_and_mask(match);
> +    if (err) {
> +        return err;
> +    }
> +
> +    NL_ATTR_FOR_EACH(nla, left, actions, actions_len) {
> +        if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
> +            odp_port_t port = nl_attr_get_odp_port(nla);
> +            struct netdev *outdev = netdev_ports_get(port,
> +                                                     info->port_hmap_obj);
> +
> +            flower.ifindex_out = netdev_get_ifindex(outdev);
> +            flower.set.tp_dst = info->tp_dst_port;
> +            netdev_close(outdev);
> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_PUSH_VLAN) {
> +            const struct ovs_action_push_vlan *vlan_push = nl_attr_get(nla);
> +
> +            flower.vlan_push_id = vlan_tci_to_vid(vlan_push->vlan_tci);
> +            flower.vlan_push_prio = vlan_tci_to_pcp(vlan_push->vlan_tci);
> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_POP_VLAN) {
> +            flower.vlan_pop = 1;
> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SET) {
> +            const struct nlattr *set = nl_attr_get(nla);
> +            const size_t set_len = nl_attr_get_size(nla);
> +
> +            err = parse_put_flow_set_action(&flower, set, set_len);
> +            if (err) {
> +                return err;
> +            }
> +        } else {
> +            VLOG_DBG_RL(&rl, "unsupported put action type: %d",
> +                        nl_attr_type(nla));
> +            return EOPNOTSUPP;
> +        }
> +    }
> +
> +    handle = get_ufid_tc_mapping(ufid, &prio, NULL);
> +    if (handle && prio) {
> +        VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", handle, prio);
> +        tc_del_filter(ifindex, prio, handle);
> +    }
> +
> +    if (!prio) {
> +        prio = get_prio_for_tc_flower(&flower);
> +    }
> +
> +    flower.act_cookie.data = ufid;
> +    flower.act_cookie.len = sizeof *ufid;
> +
> +    err = tc_replace_flower(ifindex, prio, handle, &flower);
> +    if (!err) {
> +        add_ufid_tc_mapping(ufid, flower.prio, flower.handle, netdev, ifindex);
> +    }
> +
> +    return err;
>  }
>  
>  int
> -- 
> 2.7.4
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

On Wed, May 03, 2017 at 06:08:04PM +0300, Roi Dayan wrote:
> From: Paul Blakey <paulb@mellanox.com>
> 
> Currently only tunnel offload is supported.
> 
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Reviewed-by: Simon Horman <simon.horman@netronome.com>
> ---
>  lib/dpif-netlink.c       |   4 +-
>  lib/netdev-tc-offloads.c | 392 ++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 385 insertions(+), 11 deletions(-)
> 
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 81d513d..7e6c647 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -1937,8 +1937,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>          return EOPNOTSUPP;
>      }
>  
> -    memset(&match, 0, sizeof match);
> -
>      err = parse_key_and_mask_to_match(put->key, put->key_len, put->mask,
>                                        put->mask_len, &match);
>      if (err) {
> @@ -2011,7 +2009,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>  
>          VLOG_DBG("added flow");
>      } else if (err != EEXIST) {
> -        VLOG_ERR_RL(&rl, "failed to offload flow: %s", ovs_strerror(err));
> +        VLOG_ERR_RL(&rl, "failed adding flow: %s", ovs_strerror(err));
>      }
>  
>  out:

I may be misunderstanding things but perhaps the above changes
belong in an earlier patch?

> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
> index 7e33fff..e3daf62 100644
> --- a/lib/netdev-tc-offloads.c
> +++ b/lib/netdev-tc-offloads.c
> @@ -461,16 +461,392 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>      return false;
>  }

...

> +static int
> +test_key_and_mask(struct match *match) {
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> +    const struct flow *key = &match->flow;
> +    struct flow *mask = &match->wc.masks;
> +
> +    if (mask->pkt_mark) {
> +        VLOG_DBG_RL(&rl, "offloading attribute pkt_mark isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +
> +    if (mask->recirc_id && key->recirc_id != 0) {
> +        VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't supported");
> +        return EOPNOTSUPP;
> +    }
> +    mask->recirc_id = 0;

Its not clear to me why the recirc_id mask is reset here.

> +
> +    if (mask->dp_hash) {
> +        VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't supported");
> +        return EOPNOTSUPP;
> +    }

...

> +    if (mask->metadata != 0) {

It seems to me that "if (!mask->metdata)" would make the above more
consistent with other checks in this function.

> +        VLOG_DBG_RL(&rl, "offloading attribute metadata isn't supported");
> +        return EOPNOTSUPP;
> +    }

...

On 09/05/2017 21:12, Flavio Leitner wrote:
> On Wed, May 03, 2017 at 06:08:04PM +0300, Roi Dayan wrote:
>> From: Paul Blakey <paulb@mellanox.com>
>>
>> Currently only tunnel offload is supported.
>>
>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>> Reviewed-by: Simon Horman <simon.horman@netronome.com>
>> ---
>>  lib/dpif-netlink.c       |   4 +-
>>  lib/netdev-tc-offloads.c | 392 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 385 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 81d513d..7e6c647 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -1937,8 +1937,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>>          return EOPNOTSUPP;
>>      }
>>
>> -    memset(&match, 0, sizeof match);
>> -
>>      err = parse_key_and_mask_to_match(put->key, put->key_len, put->mask,
>>                                        put->mask_len, &match);
>>      if (err) {
>> @@ -2011,7 +2009,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>>
>>          VLOG_DBG("added flow");
>>      } else if (err != EEXIST) {
>> -        VLOG_ERR_RL(&rl, "failed to offload flow: %s", ovs_strerror(err));
>> +        VLOG_ERR_RL(&rl, "failed adding flow: %s", ovs_strerror(err));
>>      }
>>
>>  out:
>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
>> index 7e33fff..e3daf62 100644
>> --- a/lib/netdev-tc-offloads.c
>> +++ b/lib/netdev-tc-offloads.c
>> @@ -461,16 +461,392 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>>      return false;
>>  }
>>
>> +static int
>> +parse_put_flow_set_action(struct tc_flower *flower, const struct nlattr *set,
>> +                          size_t set_len)
>> +{
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>> +    const struct nlattr *set_attr;
>> +    size_t set_left;
>> +
>> +    NL_ATTR_FOR_EACH_UNSAFE(set_attr, set_left, set, set_len) {
>> +        if (nl_attr_type(set_attr) == OVS_KEY_ATTR_TUNNEL) {
>> +            const struct nlattr *tunnel = nl_attr_get(set_attr);
>> +            const size_t tunnel_len = nl_attr_get_size(set_attr);
>> +            const struct nlattr *tun_attr;
>> +            size_t tun_left;
>> +
>> +            flower->set.set = true;
>> +            NL_ATTR_FOR_EACH_UNSAFE(tun_attr, tun_left, tunnel, tunnel_len) {
>> +                switch (nl_attr_type(tun_attr)) {
>> +                case OVS_TUNNEL_KEY_ATTR_ID: {
>> +                    flower->set.id = nl_attr_get_be64(tun_attr);
>> +                }
>> +                break;
>> +                case OVS_TUNNEL_KEY_ATTR_IPV4_SRC: {
>> +                    flower->set.ipv4.ipv4_src = nl_attr_get_be32(tun_attr);
>> +                }
>> +                break;
>> +                case OVS_TUNNEL_KEY_ATTR_IPV4_DST: {
>> +                    flower->set.ipv4.ipv4_dst = nl_attr_get_be32(tun_attr);
>> +                }
>> +                break;
>> +                case OVS_TUNNEL_KEY_ATTR_IPV6_SRC: {
>> +                    flower->set.ipv6.ipv6_src =
>> +                        nl_attr_get_in6_addr(tun_attr);
>> +                }
>> +                break;
>> +                case OVS_TUNNEL_KEY_ATTR_IPV6_DST: {
>> +                    flower->set.ipv6.ipv6_dst =
>> +                        nl_attr_get_in6_addr(tun_attr);
>> +                }
>> +                break;
>> +                case OVS_TUNNEL_KEY_ATTR_TP_SRC: {
>> +                    flower->set.tp_src = nl_attr_get_be16(tun_attr);
>> +                }
>> +                break;
>> +                case OVS_TUNNEL_KEY_ATTR_TP_DST: {
>> +                    flower->set.tp_dst = nl_attr_get_be16(tun_attr);
>> +                }
>> +                break;
>> +                }
>> +            }
>> +        } else {
>> +            VLOG_DBG_RL(&rl, "unsupported set action type: %d",
>> +                        nl_attr_type(set_attr));
>> +            return EOPNOTSUPP;
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int
>> +test_key_and_mask(struct match *match) {
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>> +    const struct flow *key = &match->flow;
>> +    struct flow *mask = &match->wc.masks;
>> +
>> +    if (mask->pkt_mark) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute pkt_mark isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->recirc_id && key->recirc_id != 0) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +    mask->recirc_id = 0;
>> +
>> +    if (mask->dp_hash) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->conj_id) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute conj_id isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->skb_priority) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute skb_priority isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->actset_output) {
>> +        VLOG_DBG_RL(&rl,
>> +                    "offloading attribute actset_output isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->ct_state) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute ct_state isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->ct_zone) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute ct_zone isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->ct_mark) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute ct_zone isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (!ovs_u128_is_zero(mask->ct_label)) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute ct_lable isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    for (int i = 0; i < FLOW_N_REGS; i++) {
>> +        if (mask->regs[i]) {
>> +            VLOG_DBG_RL(&rl,
>> +                        "offloading attribute regs[%d] isn't supported", i);
>> +            return EOPNOTSUPP;
>> +        }
>> +    }
>> +
>> +    if (mask->metadata != 0) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute metadata isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->nw_tos) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute nw_tos isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->nw_ttl) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute nw_ttl isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->nw_frag) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute nw_frag isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    for (int i = 0; i < FLOW_MAX_MPLS_LABELS; i++) {
>> +        if (mask->mpls_lse[i]) {
>> +            VLOG_DBG_RL(&rl, "offloading attribute mpls_lse isn't supported");
>> +            return EOPNOTSUPP;
>> +        }
>> +    }
>> +
>> +    if (key->dl_type == htons(ETH_TYPE_IP) &&
>> +        key->nw_proto == IPPROTO_ICMP) {
>> +        if (mask->tp_src) {
>> +            VLOG_DBG_RL(&rl,
>> +                        "offloading attribute icmp_type isn't supported");
>> +            return EOPNOTSUPP;
>> +        }
>> +        if (mask->tp_dst) {
>> +            VLOG_DBG_RL(&rl,
>> +                        "offloading attribute icmp_code isn't supported");
>> +            return EOPNOTSUPP;
>> +        }
>> +    } else if (key->dl_type == htons(ETH_TYPE_IP) &&
>> +               key->nw_proto == IPPROTO_IGMP) {
>> +        if (mask->tp_src) {
>> +            VLOG_DBG_RL(&rl,
>> +                        "offloading attribute igmp_type isn't supported");
>> +            return EOPNOTSUPP;
>> +        }
>> +        if (mask->tp_dst) {
>> +            VLOG_DBG_RL(&rl,
>> +                        "offloading attribute igmp_code isn't supported");
>> +            return EOPNOTSUPP;
>> +        }
>> +    } else if (key->dl_type == htons(ETH_TYPE_IPV6) &&
>> +               key->nw_proto == IPPROTO_ICMPV6) {
>> +        if (mask->tp_src) {
>> +            VLOG_DBG_RL(&rl,
>> +                        "offloading attribute icmp_type isn't supported");
>> +            return EOPNOTSUPP;
>> +        }
>> +        if (mask->tp_dst) {
>> +            VLOG_DBG_RL(&rl,
>> +                        "offloading attribute icmp_code isn't supported");
>> +            return EOPNOTSUPP;
>> +        }
>> +    }
>> +    if (is_ip_any(key) && key->nw_proto == IPPROTO_TCP && mask->tcp_flags) {
>> +        if (mask->tcp_flags) {
>> +            VLOG_DBG_RL(&rl,
>> +                        "offloading attribute tcp_flags isn't supported");
>> +            return EOPNOTSUPP;
>> +        }
>> +    }
>> +
>> +    if (!is_all_zeros(mask, sizeof *mask)) {
>> +        VLOG_DBG_RL(&rl, "offloading isn't supported, unknown attribute");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>>  int
>> -netdev_tc_flow_put(struct netdev *netdev OVS_UNUSED,
>> -                   struct match *match OVS_UNUSED,
>> -                   struct nlattr *actions OVS_UNUSED,
>> -                   size_t actions_len OVS_UNUSED,
>> -                   struct dpif_flow_stats *stats OVS_UNUSED,
>> -                   const ovs_u128 *ufid OVS_UNUSED,
>> -                   struct offload_info *info OVS_UNUSED)
>> +netdev_tc_flow_put(struct netdev *netdev,
>> +                   struct match *match,
>> +                   struct nlattr *actions,
>> +                   size_t actions_len,
>> +                   const ovs_u128 *ufid,
>> +                   struct offload_info *info,
>> +                   struct dpif_flow_stats *stats OVS_UNUSED)
>>  {
>> -    return EOPNOTSUPP;
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>> +    struct tc_flower flower;
>> +    const struct flow *key = &match->flow;
>> +    struct flow *mask = &match->wc.masks;
>> +    const struct flow_tnl *tnl = &match->flow.tunnel;
>> +    struct nlattr *nla;
>> +    size_t left;
>> +    int prio = 0;
>> +    int handle;
>> +    int ifindex;
>> +    int err;
>> +
>> +    ifindex = netdev_get_ifindex(netdev);
>> +    if (ifindex < 0) {
>> +        VLOG_ERR_RL(&rl_err, "failed to get ifindex for %s: %s",
>> +                    netdev_get_name(netdev), ovs_strerror(-ifindex));
>> +        return -ifindex;
>> +    }
>> +
>> +    memset(&flower, 0, sizeof flower);
>> +
>> +    if (tnl->tun_id) {
>> +        VLOG_DBG_RL(&rl,
>> +                    "tunnel: id %#" PRIx64 " src " IP_FMT
>> +                    " dst " IP_FMT " tp_src %d tp_dst %d",
>> +                    ntohll(tnl->tun_id),
>> +                    IP_ARGS(tnl->ip_src), IP_ARGS(tnl->ip_dst),
>> +                    ntohs(tnl->tp_src), ntohs(tnl->tp_dst));
>> +        flower.tunnel.id = tnl->tun_id;
>> +        flower.tunnel.ipv4.ipv4_src = tnl->ip_src;
>> +        flower.tunnel.ipv4.ipv4_dst = tnl->ip_dst;
>> +        flower.tunnel.ipv6.ipv6_src = tnl->ipv6_src;
>> +        flower.tunnel.ipv6.ipv6_dst = tnl->ipv6_dst;
>> +        flower.tunnel.tp_src = tnl->tp_src;
>> +        flower.tunnel.tp_dst = tnl->tp_dst;
>> +        flower.tunnel.tunnel = true;
>> +
>> +        memset(&mask->tunnel, 0, sizeof mask->tunnel);
>> +    }
>> +
>> +    flower.key.eth_type = key->dl_type;
>> +    flower.mask.eth_type = mask->dl_type;
>> +
>> +    if (mask->vlans[0].tci) {
>> +        ovs_be16 vid_mask = mask->vlans[0].tci & htons(VLAN_VID_MASK);
>> +        ovs_be16 pcp_mask = mask->vlans[0].tci & htons(VLAN_PCP_MASK);
>> +        ovs_be16 cfi = mask->vlans[0].tci & htons(VLAN_CFI);
>> +
>> +        if (cfi && key->vlans[0].tci & htons(VLAN_CFI)
>> +            && (!vid_mask || vid_mask == htons(VLAN_VID_MASK))
>> +            && (!pcp_mask || pcp_mask == htons(VLAN_PCP_MASK))
>> +            && (vid_mask || pcp_mask)) {
>> +            if (vid_mask) {
>> +                flower.key.vlan_id = vlan_tci_to_vid(key->vlans[0].tci);
>> +                VLOG_DBG_RL(&rl, "vlan_id: %d\n", flower.key.vlan_id);
>> +            }
>> +            if (pcp_mask) {
>> +                flower.key.vlan_prio = vlan_tci_to_pcp(key->vlans[0].tci);
>> +                VLOG_DBG_RL(&rl, "vlan_prio: %d\n", flower.key.vlan_prio);
>> +            }
>> +            flower.key.encap_eth_type = flower.key.eth_type;
>> +            flower.key.eth_type = htons(ETH_TYPE_VLAN);
>> +        } else if (mask->vlans[0].tci == htons(0xffff) &&
>> +                   ntohs(key->vlans[0].tci) == 0) {
>> +            /* exact && no vlan */
>> +        } else {
>> +            /* partial mask */
>> +            return EOPNOTSUPP;
>> +        }
>> +    } else if (mask->vlans[1].tci) {
>> +        return EOPNOTSUPP;
>> +    }
>> +    memset(mask->vlans, 0, sizeof mask->vlans);
>> +
>> +    flower.key.dst_mac = key->dl_dst;
>> +    memset(&flower.mask.dst_mac, 0xFF, sizeof(flower.mask.dst_mac));
>> +    flower.key.src_mac = key->dl_src;
>> +    flower.mask.src_mac = mask->dl_src;
>
> Repeating my comment on V7: Why are you fixing the dst_mac mask to
> be an exact match?
>
> Flavio

missed your comment. was for testing. fixed for V9.
thanks

>
>> +    memset(&mask->dl_src, 0, sizeof mask->dl_src);
>> +    memset(&mask->dl_dst, 0, sizeof mask->dl_dst);
>> +    mask->dl_type = 0;
>> +    mask->in_port.odp_port = 0;
>> +
>> +    if (is_ip_any(key)) {
>> +        flower.key.ip_proto = key->nw_proto;
>> +        flower.mask.ip_proto = mask->nw_proto;
>> +
>> +        if (key->nw_proto == IPPROTO_TCP || key->nw_proto == IPPROTO_UDP) {
>> +            flower.key.dst_port = key->tp_dst;
>> +            flower.mask.dst_port = mask->tp_dst;
>> +            flower.key.src_port = key->tp_src;
>> +            flower.mask.src_port = mask->tp_src;
>> +            mask->tp_src = 0;
>> +            mask->tp_dst = 0;
>> +        }
>> +
>> +        mask->nw_frag = 0;
>> +        mask->nw_tos = 0;
>> +        mask->nw_proto = 0;
>> +
>> +        if (flower.key.eth_type == htons(ETH_P_IP)) {
>> +            flower.key.ipv4.ipv4_src = key->nw_src;
>> +            flower.mask.ipv4.ipv4_src = mask->nw_src;
>> +            flower.key.ipv4.ipv4_dst = key->nw_dst;
>> +            flower.mask.ipv4.ipv4_dst = mask->nw_dst;
>> +            mask->nw_src = 0;
>> +            mask->nw_dst = 0;
>> +        } else if (flower.key.eth_type == htons(ETH_P_IPV6)) {
>> +            flower.key.ipv6.ipv6_src = key->ipv6_src;
>> +            flower.mask.ipv6.ipv6_src = mask->ipv6_src;
>> +            flower.key.ipv6.ipv6_dst = key->ipv6_dst;
>> +            flower.mask.ipv6.ipv6_dst = mask->ipv6_dst;
>> +            memset(&mask->ipv6_src, 0, sizeof mask->ipv6_src);
>> +            memset(&mask->ipv6_dst, 0, sizeof mask->ipv6_dst);
>> +        }
>> +    }
>> +
>> +    err = test_key_and_mask(match);
>> +    if (err) {
>> +        return err;
>> +    }
>> +
>> +    NL_ATTR_FOR_EACH(nla, left, actions, actions_len) {
>> +        if (nl_attr_type(nla) == OVS_ACTION_ATTR_OUTPUT) {
>> +            odp_port_t port = nl_attr_get_odp_port(nla);
>> +            struct netdev *outdev = netdev_ports_get(port,
>> +                                                     info->port_hmap_obj);
>> +
>> +            flower.ifindex_out = netdev_get_ifindex(outdev);
>> +            flower.set.tp_dst = info->tp_dst_port;
>> +            netdev_close(outdev);
>> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_PUSH_VLAN) {
>> +            const struct ovs_action_push_vlan *vlan_push = nl_attr_get(nla);
>> +
>> +            flower.vlan_push_id = vlan_tci_to_vid(vlan_push->vlan_tci);
>> +            flower.vlan_push_prio = vlan_tci_to_pcp(vlan_push->vlan_tci);
>> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_POP_VLAN) {
>> +            flower.vlan_pop = 1;
>> +        } else if (nl_attr_type(nla) == OVS_ACTION_ATTR_SET) {
>> +            const struct nlattr *set = nl_attr_get(nla);
>> +            const size_t set_len = nl_attr_get_size(nla);
>> +
>> +            err = parse_put_flow_set_action(&flower, set, set_len);
>> +            if (err) {
>> +                return err;
>> +            }
>> +        } else {
>> +            VLOG_DBG_RL(&rl, "unsupported put action type: %d",
>> +                        nl_attr_type(nla));
>> +            return EOPNOTSUPP;
>> +        }
>> +    }
>> +
>> +    handle = get_ufid_tc_mapping(ufid, &prio, NULL);
>> +    if (handle && prio) {
>> +        VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", handle, prio);
>> +        tc_del_filter(ifindex, prio, handle);
>> +    }
>> +
>> +    if (!prio) {
>> +        prio = get_prio_for_tc_flower(&flower);
>> +    }
>> +
>> +    flower.act_cookie.data = ufid;
>> +    flower.act_cookie.len = sizeof *ufid;
>> +
>> +    err = tc_replace_flower(ifindex, prio, handle, &flower);
>> +    if (!err) {
>> +        add_ufid_tc_mapping(ufid, flower.prio, flower.handle, netdev, ifindex);
>> +    }
>> +
>> +    return err;
>>  }
>>
>>  int
>> --
>> 2.7.4
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

On 10/05/2017 16:28, Simon Horman wrote:
> On Wed, May 03, 2017 at 06:08:04PM +0300, Roi Dayan wrote:
>> From: Paul Blakey <paulb@mellanox.com>
>>
>> Currently only tunnel offload is supported.
>>
>> Signed-off-by: Paul Blakey <paulb@mellanox.com>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
>> Reviewed-by: Simon Horman <simon.horman@netronome.com>
>> ---
>>  lib/dpif-netlink.c       |   4 +-
>>  lib/netdev-tc-offloads.c | 392 ++++++++++++++++++++++++++++++++++++++++++++++-
>>  2 files changed, 385 insertions(+), 11 deletions(-)
>>
>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
>> index 81d513d..7e6c647 100644
>> --- a/lib/dpif-netlink.c
>> +++ b/lib/dpif-netlink.c
>> @@ -1937,8 +1937,6 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>>          return EOPNOTSUPP;
>>      }
>>
>> -    memset(&match, 0, sizeof match);
>> -
>>      err = parse_key_and_mask_to_match(put->key, put->key_len, put->mask,
>>                                        put->mask_len, &match);
>>      if (err) {
>> @@ -2011,7 +2009,7 @@ parse_flow_put(struct dpif_netlink *dpif, struct dpif_flow_put *put)
>>
>>          VLOG_DBG("added flow");
>>      } else if (err != EEXIST) {
>> -        VLOG_ERR_RL(&rl, "failed to offload flow: %s", ovs_strerror(err));
>> +        VLOG_ERR_RL(&rl, "failed adding flow: %s", ovs_strerror(err));
>>      }
>>
>>  out:
>
> I may be misunderstanding things but perhaps the above changes
> belong in an earlier patch?

also noticed it and fixed for V9.
the msg was updated in an earlier patch and from rebases the original 
string was updated again again here and we missed it.
also for the memset above that was removed. should be in earlier patch. 
fixed.

>
>> diff --git a/lib/netdev-tc-offloads.c b/lib/netdev-tc-offloads.c
>> index 7e33fff..e3daf62 100644
>> --- a/lib/netdev-tc-offloads.c
>> +++ b/lib/netdev-tc-offloads.c
>> @@ -461,16 +461,392 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>>      return false;
>>  }
>
> ...
>
>> +static int
>> +test_key_and_mask(struct match *match) {
>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
>> +    const struct flow *key = &match->flow;
>> +    struct flow *mask = &match->wc.masks;
>> +
>> +    if (mask->pkt_mark) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute pkt_mark isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +
>> +    if (mask->recirc_id && key->recirc_id != 0) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute recirc_id isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>> +    mask->recirc_id = 0;
>
> Its not clear to me why the recirc_id mask is reset here.

What I recall is we always get a mask for recirc_id but if the key is 0 
we want to ignore the mask and not fail the end test that checked all masks.


>
>> +
>> +    if (mask->dp_hash) {
>> +        VLOG_DBG_RL(&rl, "offloading attribute dp_hash isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>
> ...
>
>> +    if (mask->metadata != 0) {
>
> It seems to me that "if (!mask->metdata)" would make the above more
> consistent with other checks in this function.

ok

>
>> +        VLOG_DBG_RL(&rl, "offloading attribute metadata isn't supported");
>> +        return EOPNOTSUPP;
>> +    }
>
> ...
>