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

Simon Horman simon.horman at netronome.com
Wed May 10 13:28:49 UTC 2017


On Wed, May 03, 2017 at 06:08:04PM +0300, Roi Dayan wrote:
> From: Paul Blakey <paulb at mellanox.com>
> 
> Currently only tunnel offload is supported.
> 
> Signed-off-by: Paul Blakey <paulb at mellanox.com>
> Reviewed-by: Roi Dayan <roid at mellanox.com>
> Reviewed-by: Simon Horman <simon.horman at 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;
> +    }

...


More information about the dev mailing list