[ovs-dev,RFC,v2,3/8] tc: Introduce tc_id to specify a tc filter

Move all that is needed to identify a tc filter to a
new structure, tc_id. This removes a lot of duplication
in accessing/creating tc filters.

Signed-off-by: Paul Blakey <paulb@mellanox.com>
---
 lib/netdev-linux.c      |   6 +-
 lib/netdev-offload-tc.c | 207 ++++++++++++++++++++----------------------------
 lib/tc.c                | 122 +++++++++++-----------------
 lib/tc.h                |  28 ++++---
 4 files changed, 151 insertions(+), 212 deletions(-)

Hi Paul,

On Thu, Jul 04, 2019 at 05:28:22PM +0300, Paul Blakey wrote:
> Move all that is needed to identify a tc filter to a
> new structure, tc_id. This removes a lot of duplication
> in accessing/creating tc filters.
> 
> Signed-off-by: Paul Blakey <paulb@mellanox.com>

This is a rather large patch.
I'm wondering if you could comment on how thoroughly it has
been tested.

> ---
>  lib/netdev-linux.c      |   6 +-
>  lib/netdev-offload-tc.c | 207 ++++++++++++++++++++----------------------------
>  lib/tc.c                | 122 +++++++++++-----------------
>  lib/tc.h                |  28 ++++---
>  4 files changed, 151 insertions(+), 212 deletions(-)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index e4ea94c..69e0982 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2421,6 +2421,8 @@ static int
>  tc_del_matchall_policer(struct netdev *netdev)
>  {
>      uint32_t block_id = 0;
> +    int prio = TC_RESERVED_PRIORITY_POLICE;
> +    struct tc_id id;
>      int ifindex;
>      int err;
>  
> @@ -2429,8 +2431,8 @@ tc_del_matchall_policer(struct netdev *netdev)
>          return err;
>      }
>  
> -    err = tc_del_filter(ifindex, TC_RESERVED_PRIORITY_POLICE, 1, block_id,
> -                        TC_INGRESS);
> +    id = make_tc_id(ifindex, block_id, prio, TC_INGRESS);
> +    err = tc_del_filter(&id);
>      if (err) {
>          return err;
>      }
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 2af0f10..e37049c 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -145,20 +145,16 @@ static struct ovs_mutex ufid_lock = OVS_MUTEX_INITIALIZER;
>  /**
>   * struct ufid_tc_data - data entry for ufid_tc hmap.
>   * @ufid_node: Element in @ufid_tc hash table by ufid key.
> - * @tc_node: Element in @ufid_tc hash table by prio/handle/ifindex key.
> + * @tc_node: Element in @ufid_tc hash table by tc_id key.
>   * @ufid: ufid assigned to the flow
> - * @prio: tc priority
> - * @handle: tc handle
> - * @ifindex: netdev ifindex.
> + * @id: tc id
>   * @netdev: netdev associated with the tc rule
>   */
>  struct ufid_tc_data {
>      struct hmap_node ufid_node;
>      struct hmap_node tc_node;
>      ovs_u128 ufid;
> -    uint16_t prio;
> -    uint32_t handle;
> -    int ifindex;
> +    struct tc_id id;
>      struct netdev *netdev;
>  };
>  
> @@ -190,32 +186,27 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)
>  
>  /* Wrapper function to delete filter and ufid tc mapping */
>  static int
> -del_filter_and_ufid_mapping(int ifindex, int prio, int handle,
> -                            uint32_t block_id, const ovs_u128 *ufid,
> -                            enum tc_qdisc_hook hook)
> +del_filter_and_ufid_mapping(struct tc_id *id, const ovs_u128 *ufid)
>  {
>      int err;
>  
> -    err = tc_del_filter(ifindex, prio, handle, block_id, hook);
> +    err = tc_del_filter(id);
>      del_ufid_tc_mapping(ufid);
> -
>      return err;
>  }
>  
>  /* Add ufid entry to ufid_tc hashmap. */
>  static void
> -add_ufid_tc_mapping(const ovs_u128 *ufid, int prio, int handle,
> -                    struct netdev *netdev, int ifindex)
> +add_ufid_tc_mapping(struct netdev *netdev, const ovs_u128 *ufid,
> +                    struct tc_id *id)

Adding the id parameter after the ufid parameter does not
seem consistent with some other functions, f.e. find_ufid(),
while consistent with others, f.e. get_ufid_tc_mapping().

Could a more consistent parameter ordering be used throughout
this patch?

>  {
>      size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0);
> -    size_t tc_hash = hash_int(hash_int(prio, handle), ifindex);
> +    size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex);
>      struct ufid_tc_data *new_data = xzalloc(sizeof *new_data);
>  
>      new_data->ufid = *ufid;
> -    new_data->prio = prio;
> -    new_data->handle = handle;
> +    new_data->id = *id;
>      new_data->netdev = netdev_ref(netdev);
> -    new_data->ifindex = ifindex;
>  
>      ovs_mutex_lock(&ufid_lock);
>      hmap_insert(&ufid_tc, &new_data->ufid_node, ufid_hash);
> @@ -223,56 +214,49 @@ add_ufid_tc_mapping(const ovs_u128 *ufid, int prio, int handle,
>      ovs_mutex_unlock(&ufid_lock);
>  }
>  
> -/* Get ufid from ufid_tc hashmap.
> +/* Get tc id from ufid_tc hashmap.
>   *
> - * If netdev output param is not NULL then the function will return
> - * associated netdev on success and a refcount is taken on that netdev.
> - * The caller is then responsible to close the netdev.
> - *
> - * Returns handle if successful and fill prio and netdev for that ufid.
> - * Otherwise returns 0.
> + * Returns 0 if successful and fills id.
> + * Otherwise returns the error.
>   */
>  static int
> -get_ufid_tc_mapping(const ovs_u128 *ufid, int *prio, struct netdev **netdev)
> +get_ufid_tc_mapping(const ovs_u128 *ufid, struct tc_id *id)
>  {
>      size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0);
>      struct ufid_tc_data *data;
> -    int handle = 0;
>  
>      ovs_mutex_lock(&ufid_lock);
>      HMAP_FOR_EACH_WITH_HASH(data, ufid_node, ufid_hash, &ufid_tc) {
>          if (ovs_u128_equals(*ufid, data->ufid)) {
> -            if (prio) {
> -                *prio = data->prio;
> -            }
> -            if (netdev) {
> -                *netdev = netdev_ref(data->netdev);
> -            }
> -            handle = data->handle;
> -            break;
> +            *id = data->id;

Did you consider returning &data-id (or NULL on error)?

As this function now only returns one value (id) rather than two
(prio and netdev) returning a reference to the id may be cleaner.

> +            ovs_mutex_unlock(&ufid_lock);
> +            return 0;
>          }
>      }
>      ovs_mutex_unlock(&ufid_lock);
>  
> -    return handle;
> +    return ENOENT;
>  }
>  
> -/* Find ufid entry in ufid_tc hashmap using prio, handle and netdev.
> +/* Find ufid entry in ufid_tc hashmap using tc_id id.
>   * The result is saved in ufid.
>   *
>   * Returns true on success.
>   */
>  static bool
> -find_ufid(int prio, int handle, struct netdev *netdev, ovs_u128 *ufid)
> +find_ufid(struct netdev *netdev, struct tc_id *id, ovs_u128 *ufid)

I think this could also return a reference to id.
But by that reasoning it could have also returned a reference before the id
change.  So perhaps I am missing something here.

>  {
> -    int ifindex = netdev_get_ifindex(netdev);
> +    size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex);
>      struct ufid_tc_data *data;
> -    size_t tc_hash = hash_int(hash_int(prio, handle), ifindex);
>  
>      ovs_mutex_lock(&ufid_lock);
>      HMAP_FOR_EACH_WITH_HASH(data, tc_node, tc_hash,  &ufid_tc) {
> -        if (data->prio == prio && data->handle == handle
> -            && data->ifindex == ifindex) {
> +        if (netdev == data->netdev
> +            && data->id.prio == id->prio
> +            && data->id.handle == id->handle
> +            && data->id.hook == id->hook
> +            && data->id.block_id == id->block_id
> +            && data->id.ifindex == id->ifindex) {
>              *ufid = data->ufid;
>              break;
>          }
> @@ -356,6 +340,8 @@ netdev_tc_flow_flush(struct netdev *netdev)
>      enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
>      int ifindex = netdev_get_ifindex(netdev);
>      uint32_t block_id = 0;
> +    struct tc_id id;
> +    int prio = 0;
>  
>      if (ifindex < 0) {
>          VLOG_ERR_RL(&error_rl, "flow_flush: failed to get ifindex for %s: %s",
> @@ -364,8 +350,8 @@ netdev_tc_flow_flush(struct netdev *netdev)
>      }
>  
>      block_id = get_block_id_from_netdev(netdev);
> -
> -    return tc_flush(ifindex, block_id, hook);
> +    id = make_tc_id(ifindex, block_id, prio, hook);
> +    return tc_del_filter(&id);
>  }
>  
>  static int
> @@ -375,6 +361,8 @@ netdev_tc_flow_dump_create(struct netdev *netdev,
>      enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
>      struct netdev_flow_dump *dump;
>      uint32_t block_id = 0;
> +    struct tc_id id;
> +    int prio = 0;
>      int ifindex;
>  
>      ifindex = netdev_get_ifindex(netdev);
> @@ -388,7 +376,9 @@ netdev_tc_flow_dump_create(struct netdev *netdev,
>      dump = xzalloc(sizeof *dump);
>      dump->nl_dump = xzalloc(sizeof *dump->nl_dump);
>      dump->netdev = netdev_ref(netdev);
> -    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, hook);
> +
> +    id = make_tc_id(ifindex, block_id, prio, hook);
> +    tc_dump_flower_start(&id, dump->nl_dump);
>  
>      *dump_out = dump;
>  
> @@ -739,13 +729,18 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>                           struct ofpbuf *rbuffer,
>                           struct ofpbuf *wbuffer)
>  {
> +    struct netdev *netdev = dump->netdev;
>      struct ofpbuf nl_flow;
> +    struct tc_id id;
> +
> +    id.hook = get_tc_qdisc_hook(netdev);
> +    id.block_id = get_block_id_from_netdev(netdev);
> +    id.ifindex = netdev_get_ifindex(netdev);
>  
>      while (nl_dump_next(dump->nl_dump, &nl_flow, rbuffer)) {
>          struct tc_flower flower;
> -        struct netdev *netdev = dump->netdev;
>  
> -        if (parse_netlink_to_tc_flower(&nl_flow, &flower)) {
> +        if (parse_netlink_to_tc_flower(&nl_flow, &id, &flower)) {
>              continue;
>          }
>  
> @@ -756,7 +751,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
>  
>          if (flower.act_cookie.len) {
>              *ufid = *((ovs_u128 *) flower.act_cookie.data);
> -        } else if (!find_ufid(flower.prio, flower.handle, netdev, ufid)) {
> +        } else if (!find_ufid(netdev, &id, ufid)) {
>              continue;
>          }
>  
> @@ -1098,9 +1093,9 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>      struct tc_action *action;
>      uint32_t block_id = 0;
>      struct nlattr *nla;
> +    struct tc_id id;
>      size_t left;
>      int prio = 0;
> -    int handle;
>      int ifindex;
>      int err;
>  
> @@ -1352,35 +1347,32 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
>          }
>      }
>  
> -    block_id = get_block_id_from_netdev(netdev);
> -    handle = get_ufid_tc_mapping(ufid, &prio, NULL);
> -    if (handle && prio) {
> -        VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", handle, prio);
> -        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
> -                                    hook);
> -    }
> -
> -    if (!prio) {
> -        prio = get_prio_for_tc_flower(&flower);
> -        if (prio == 0) {
> -            VLOG_ERR_RL(&rl, "couldn't get tc prio: %s", ovs_strerror(ENOSPC));
> -            return ENOSPC;
> -        }
> +    if (!get_ufid_tc_mapping(ufid, &id)) {
> +        VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d",
> +                    id.handle, id.prio);
> +        del_filter_and_ufid_mapping(&id, ufid);
> +    }
> +
> +    prio = get_prio_for_tc_flower(&flower);
> +    if (prio == 0) {
> +        VLOG_ERR_RL(&rl, "couldn't get tc prio: %s", ovs_strerror(ENOSPC));
> +        return ENOSPC;
>      }
>  
>      flower.act_cookie.data = ufid;
>      flower.act_cookie.len = sizeof *ufid;
>  
> -    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id, hook);
> +    id = make_tc_id(ifindex, block_id, prio, hook);
> +    err = tc_replace_flower(&id, &flower);
>      if (!err) {
> -        add_ufid_tc_mapping(ufid, flower.prio, flower.handle, netdev, ifindex);
> +        add_ufid_tc_mapping(netdev, ufid, &id);
>      }
>  
>      return err;
>  }
>  
>  static int
> -netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
> +netdev_tc_flow_get(struct netdev *netdev,
>                     struct match *match,
>                     struct nlattr **actions,
>                     const ovs_u128 *ufid,
> @@ -1389,43 +1381,27 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
>                     struct ofpbuf *buf)
>  {
>      static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
> -    struct netdev *dev;
>      struct tc_flower flower;
> -    enum tc_qdisc_hook hook;
> -    uint32_t block_id = 0;
>      odp_port_t in_port;
> -    int prio = 0;
> -    int ifindex;
> -    int handle;
> +    struct tc_id id;
>      int err;
>  
> -    handle = get_ufid_tc_mapping(ufid, &prio, &dev);
> -    if (!handle) {
> -        return ENOENT;
> -    }
> -
> -    hook = get_tc_qdisc_hook(dev);
> -
> -    ifindex = netdev_get_ifindex(dev);
> -    if (ifindex < 0) {
> -        VLOG_ERR_RL(&error_rl, "flow_get: failed to get ifindex for %s: %s",
> -                    netdev_get_name(dev), ovs_strerror(-ifindex));
> -        netdev_close(dev);
> -        return -ifindex;
> +    err = get_ufid_tc_mapping(ufid, &id);
> +    if (err) {
> +        return err;
>      }
>  
> -    block_id = get_block_id_from_netdev(dev);
>      VLOG_DBG_RL(&rl, "flow get (dev %s prio %d handle %d block_id %d)",
> -                netdev_get_name(dev), prio, handle, block_id);
> -    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, hook);
> -    netdev_close(dev);
> +                netdev_get_name(netdev), id.prio, id.handle, id.block_id);
> +
> +    err = tc_get_flower(&id, &flower);
>      if (err) {
>          VLOG_ERR_RL(&error_rl, "flow get failed (dev %s prio %d handle %d): %s",
> -                    netdev_get_name(dev), prio, handle, ovs_strerror(err));
> +                    netdev_get_name(netdev), id.prio, id.handle, ovs_strerror(err));
>          return err;
>      }
>  
> -    in_port = netdev_ifindex_to_odp_port(ifindex);
> +    in_port = netdev_ifindex_to_odp_port(id.ifindex);
>      parse_tc_flower_to_match(&flower, match, actions, stats, attrs, buf);
>  
>      match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX);
> @@ -1440,44 +1416,24 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
>                     struct dpif_flow_stats *stats)
>  {
>      struct tc_flower flower;
> -    enum tc_qdisc_hook hook;
> -    uint32_t block_id = 0;
> -    struct netdev *dev;
> -    int prio = 0;
> -    int ifindex;
> -    int handle;
> +    struct tc_id id;
>      int error;
>  
> -    handle = get_ufid_tc_mapping(ufid, &prio, &dev);
> -    if (!handle) {
> -        return ENOENT;
> -    }
> -
> -    hook = get_tc_qdisc_hook(dev);
> -
> -    ifindex = netdev_get_ifindex(dev);
> -    if (ifindex < 0) {
> -        VLOG_ERR_RL(&error_rl, "flow_del: failed to get ifindex for %s: %s",
> -                    netdev_get_name(dev), ovs_strerror(-ifindex));
> -        netdev_close(dev);
> -        return -ifindex;
> +    error = get_ufid_tc_mapping(ufid, &id);
> +    if (error) {
> +        return error;
>      }
>  
> -    block_id = get_block_id_from_netdev(dev);
> -
>      if (stats) {
>          memset(stats, 0, sizeof *stats);
> -        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id, hook)) {
> +        if (!tc_get_flower(&id, &flower)) {
>              stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
>              stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
>              stats->used = flower.lastused;
>          }
>      }
>  
> -    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
> -                                        hook);
> -
> -    netdev_close(dev);
> +    error = del_filter_and_ufid_mapping(&id, ufid);
>  
>      return error;
>  }
> @@ -1486,7 +1442,9 @@ static void
>  probe_multi_mask_per_prio(int ifindex)
>  {
>      struct tc_flower flower;
> +    struct tc_id id1, id2;
>      int block_id = 0;
> +    int prio = 1;
>      int error;
>  
>      error = tc_add_del_qdisc(ifindex, true, block_id, TC_INGRESS);
> @@ -1501,7 +1459,8 @@ probe_multi_mask_per_prio(int ifindex)
>      memset(&flower.key.dst_mac, 0x11, sizeof flower.key.dst_mac);
>      memset(&flower.mask.dst_mac, 0xff, sizeof flower.mask.dst_mac);
>  
> -    error = tc_replace_flower(ifindex, 1, 1, &flower, block_id, TC_INGRESS);
> +    id1 = make_tc_id(ifindex, block_id, prio, TC_INGRESS);
> +    error = tc_replace_flower(&id1, &flower);
>      if (error) {
>          goto out;
>      }
> @@ -1509,14 +1468,15 @@ probe_multi_mask_per_prio(int ifindex)
>      memset(&flower.key.src_mac, 0x11, sizeof flower.key.src_mac);
>      memset(&flower.mask.src_mac, 0xff, sizeof flower.mask.src_mac);
>  
> -    error = tc_replace_flower(ifindex, 1, 2, &flower, block_id, TC_INGRESS);
> -    tc_del_filter(ifindex, 1, 1, block_id, TC_INGRESS);
> +    id2 = make_tc_id(ifindex, block_id, prio, TC_INGRESS);
> +    error = tc_replace_flower(&id2, &flower);
> +    tc_del_filter(&id1);
>  
>      if (error) {
>          goto out;
>      }
>  
> -    tc_del_filter(ifindex, 1, 2, block_id, TC_INGRESS);
> +    tc_del_filter(&id2);
>  
>      multi_mask_per_prio = true;
>      VLOG_INFO("probe tc: multiple masks on single tc prio is supported.");
> @@ -1530,6 +1490,8 @@ probe_tc_block_support(int ifindex)
>  {
>      struct tc_flower flower;
>      uint32_t block_id = 1;
> +    struct tc_id id;
> +    int prio = 0;
>      int error;
>  
>      error = tc_add_del_qdisc(ifindex, true, block_id, TC_INGRESS);
> @@ -1544,7 +1506,8 @@ probe_tc_block_support(int ifindex)
>      memset(&flower.key.dst_mac, 0x11, sizeof flower.key.dst_mac);
>      memset(&flower.mask.dst_mac, 0xff, sizeof flower.mask.dst_mac);
>  
> -    error = tc_replace_flower(ifindex, 1, 1, &flower, block_id, TC_INGRESS);
> +    id = make_tc_id(ifindex, block_id, prio, TC_INGRESS);
> +    error = tc_replace_flower(&id, &flower);
>  
>      tc_add_del_qdisc(ifindex, false, block_id, TC_INGRESS);
>  
> diff --git a/lib/tc.c b/lib/tc.c
> index 1eca356..d7cb43e 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -193,6 +193,21 @@ tc_make_request(int ifindex, int type, unsigned int flags,
>      return tcmsg;
>  }
>  
> +static void request_from_tc_id(struct tc_id *id, uint16_t eth_type,
> +                               int type, unsigned int flags,
> +                               struct ofpbuf *request)
> +{
> +    int ifindex = id->block_id ? TCM_IFINDEX_MAGIC_BLOCK : id->ifindex;
> +    struct tcmsg *tcmsg;
> +
> +    tcmsg = tc_make_request(ifindex, type, flags, request);
> +    tcmsg->tcm_parent = (id->hook == TC_EGRESS) ?
> +                        TC_EGRESS_PARENT : (id->block_id ? : TC_INGRESS_PARENT);
> +    tcmsg->tcm_info = tc_make_handle(id->prio, eth_type);
> +    tcmsg->tcm_handle = id->handle;
> +}


The above helper seems nice, leading to a good reduction in code
duplication. But I wonder if it would be nicer to add it in a earlier
patch in this series - for one it would make this patch shorter and
easier to review. But I do not feel strongly about this.

> +
> +
>  int
>  tc_transact(struct ofpbuf *request, struct ofpbuf **replyp)
>  {
> @@ -1429,7 +1444,8 @@ nl_parse_flower_options(struct nlattr *nl_options, struct tc_flower *flower)
>  }
>  
>  int
> -parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tc_flower *flower)
> +parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tc_id *id,
> +                           struct tc_flower *flower)
>  {
>      struct tcmsg *tc;
>      struct nlattr *ta[ARRAY_SIZE(tca_policy)];
> @@ -1442,16 +1458,17 @@ parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tc_flower *flower)
>      memset(flower, 0, sizeof *flower);
>  
>      tc = ofpbuf_at_assert(reply, NLMSG_HDRLEN, sizeof *tc);
> -    flower->handle = tc->tcm_handle;
> +
>      flower->key.eth_type = (OVS_FORCE ovs_be16) tc_get_minor(tc->tcm_info);
>      flower->mask.eth_type = OVS_BE16_MAX;
> -    flower->prio = tc_get_major(tc->tcm_info);
> +    id->prio = tc_get_major(tc->tcm_info);
> +    id->handle = tc->tcm_handle;
>  
> -    if (flower->prio == TC_RESERVED_PRIORITY_POLICE) {
> +    if (id->prio == TC_RESERVED_PRIORITY_POLICE) {
>          return 0;
>      }
>  
> -    if (!flower->handle) {
> +    if (!id->handle) {
>          return EAGAIN;
>      }
>  
> @@ -1471,20 +1488,11 @@ parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tc_flower *flower)
>  }
>  
>  int
> -tc_dump_flower_start(int ifindex, struct nl_dump *dump, uint32_t block_id,
> -                     enum tc_qdisc_hook hook)
> +tc_dump_flower_start(struct tc_id *id, struct nl_dump *dump)
>  {
>      struct ofpbuf request;
> -    struct tcmsg *tcmsg;
> -    int index;
> -
> -    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
> -    tcmsg = tc_make_request(index, RTM_GETTFILTER, NLM_F_DUMP, &request);
> -    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
> -                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
> -    tcmsg->tcm_info = TC_H_UNSPEC;
> -    tcmsg->tcm_handle = 0;
>  
> +    request_from_tc_id(id, 0, RTM_GETTFILTER, NLM_F_DUMP, &request);
>      nl_dump_start(dump, NETLINK_ROUTE, &request);
>      ofpbuf_uninit(&request);
>  
> @@ -1492,68 +1500,28 @@ tc_dump_flower_start(int ifindex, struct nl_dump *dump, uint32_t block_id,
>  }
>  
>  int
> -tc_flush(int ifindex, uint32_t block_id, enum tc_qdisc_hook hook)
> +tc_del_filter(struct tc_id *id)
>  {
>      struct ofpbuf request;
> -    struct tcmsg *tcmsg;
> -    int index;
> -
> -    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
> -    tcmsg = tc_make_request(index, RTM_DELTFILTER, NLM_F_ACK, &request);
> -    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
> -                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
> -    tcmsg->tcm_info = TC_H_UNSPEC;
>  
> +    request_from_tc_id(id, 0, RTM_DELTFILTER, NLM_F_ACK, &request);
>      return tc_transact(&request, NULL);
>  }
>  
>  int
> -tc_del_filter(int ifindex, int prio, int handle, uint32_t block_id,
> -              enum tc_qdisc_hook hook)
> -{
> -    struct ofpbuf request;
> -    struct tcmsg *tcmsg;
> -    struct ofpbuf *reply;
> -    int error;
> -    int index;
> -
> -    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
> -    tcmsg = tc_make_request(index, RTM_DELTFILTER, NLM_F_ECHO, &request);
> -    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
> -                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
> -    tcmsg->tcm_info = tc_make_handle(prio, 0);
> -    tcmsg->tcm_handle = handle;
> -
> -    error = tc_transact(&request, &reply);
> -    if (!error) {
> -        ofpbuf_delete(reply);
> -    }
> -    return error;
> -}
> -
> -int
> -tc_get_flower(int ifindex, int prio, int handle, struct tc_flower *flower,
> -              uint32_t block_id, enum tc_qdisc_hook hook)
> +tc_get_flower(struct tc_id *id, struct tc_flower *flower)
>  {
>      struct ofpbuf request;
> -    struct tcmsg *tcmsg;
>      struct ofpbuf *reply;
>      int error;
> -    int index;
> -
> -    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
> -    tcmsg = tc_make_request(index, RTM_GETTFILTER, NLM_F_ECHO, &request);
> -    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
> -                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
> -    tcmsg->tcm_info = tc_make_handle(prio, 0);
> -    tcmsg->tcm_handle = handle;
>  
> +    request_from_tc_id(id, 0, RTM_GETTFILTER, NLM_F_ECHO, &request);
>      error = tc_transact(&request, &reply);
>      if (error) {
>          return error;
>      }
>  
> -    error = parse_netlink_to_tc_flower(reply, flower);
> +    error = parse_netlink_to_tc_flower(reply, id, flower);
>      ofpbuf_delete(reply);
>      return error;
>  }
> @@ -2299,26 +2267,30 @@ nl_msg_put_flower_options(struct ofpbuf *request, struct tc_flower *flower)
>      return 0;
>  }
>  
> +struct tc_id make_tc_id(int ifindex, uint32_t block_id, uint16_t prio,
> +                        enum tc_qdisc_hook hook)
> +{
> +    struct tc_id id = { .handle = 0, };

Zeroing id does not seem strictly necessary if every field is set,
which seems to be the case.

> +
> +    id.ifindex = ifindex;
> +    id.block_id = block_id;
> +    id.prio = prio;
> +    id.hook = hook;
> +
> +    return id;

This is a very nice 'pure' function.
However I am concerned about any performance / stack overhead
of returning a structure by value. Did you give this some
consideration?

Perhaps this concern could be alleviated by making make_tc_id()
a static inline function. Just an idea.

> +}
> +
>  int
> -tc_replace_flower(int ifindex, uint16_t prio, uint32_t handle,
> -                  struct tc_flower *flower, uint32_t block_id,
> -                  enum tc_qdisc_hook hook)
> +tc_replace_flower(struct tc_id *id, struct tc_flower *flower)
>  {
>      struct ofpbuf request;
> -    struct tcmsg *tcmsg;
>      struct ofpbuf *reply;
>      int error = 0;
>      size_t basic_offset;
>      uint16_t eth_type = (OVS_FORCE uint16_t) flower->key.eth_type;
> -    int index;
>  
> -    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
> -    tcmsg = tc_make_request(index, RTM_NEWTFILTER, NLM_F_CREATE | NLM_F_ECHO,
> -                            &request);
> -    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
> -                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
> -    tcmsg->tcm_info = tc_make_handle(prio, eth_type);
> -    tcmsg->tcm_handle = handle;
> +    request_from_tc_id(id, eth_type, RTM_NEWTFILTER,
> +                       NLM_F_CREATE | NLM_F_ECHO, &request);
>  
>      nl_msg_put_string(&request, TCA_KIND, "flower");
>      basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS);
> @@ -2337,8 +2309,8 @@ tc_replace_flower(int ifindex, uint16_t prio, uint32_t handle,
>          struct tcmsg *tc =
>              ofpbuf_at_assert(reply, NLMSG_HDRLEN, sizeof *tc);
>  
> -        flower->prio = tc_get_major(tc->tcm_info);
> -        flower->handle = tc->tcm_handle;
> +        id->prio = tc_get_major(tc->tcm_info);
> +        id->handle = tc->tcm_handle;
>          ofpbuf_delete(reply);
>      }
>  
> diff --git a/lib/tc.h b/lib/tc.h
> index 2e0f5e3..00acc41 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -197,10 +197,18 @@ enum tc_offloaded_state {
>      TC_OFFLOADED_STATE_NOT_IN_HW,
>  };
>  
> -struct tc_flower {
> +struct tc_id {
> +    enum tc_qdisc_hook hook;
> +    uint32_t block_id;
> +    int ifindex;
> +    uint16_t prio;
>      uint32_t handle;
> -    uint32_t prio;
> +};

At a glance the ordering of the fields of struct tc_id
appears to result in a structure with holes on both 32bit and 64bit
architectures.

As this is an internal structure I think it would be worth
ordering the fields in a way that minimises holes.

>  
> +struct tc_id make_tc_id(int ifindex, uint32_t block_id, uint16_t prio,
> +                        enum tc_qdisc_hook hook);
> +
> +struct tc_flower {
>      struct tc_flower_key key;
>      struct tc_flower_key mask;
>  
> @@ -234,18 +242,12 @@ BUILD_ASSERT_DECL(offsetof(struct tc_flower, rewrite)
>                    + MEMBER_SIZEOF(struct tc_flower, rewrite)
>                    + sizeof(uint32_t) - 2 < sizeof(struct tc_flower));
>  
> -int tc_replace_flower(int ifindex, uint16_t prio, uint32_t handle,
> -                      struct tc_flower *flower, uint32_t block_id,
> -                      enum tc_qdisc_hook hook);
> -int tc_del_filter(int ifindex, int prio, int handle, uint32_t block_id,
> -                  enum tc_qdisc_hook hook);
> -int tc_get_flower(int ifindex, int prio, int handle,
> -                  struct tc_flower *flower, uint32_t block_id,
> -                  enum tc_qdisc_hook hook);
> -int tc_flush(int ifindex, uint32_t block_id, enum tc_qdisc_hook hook);
> -int tc_dump_flower_start(int ifindex, struct nl_dump *dump, uint32_t block_id,
> -                         enum tc_qdisc_hook hook);
> +int tc_replace_flower(struct tc_id *id, struct tc_flower *flower);
> +int tc_del_filter(struct tc_id *id);
> +int tc_get_flower(struct tc_id *id, struct tc_flower *flower);
> +int tc_dump_flower_start(struct tc_id *id, struct nl_dump *dump);
>  int parse_netlink_to_tc_flower(struct ofpbuf *reply,
> +                               struct tc_id *id,
>                                 struct tc_flower *flower);
>  void tc_set_policy(const char *policy);
>  
> -- 
> 1.8.3.1
>

On 7/26/2019 12:47 PM, Simon Horman wrote:

Hi Paul,

On Thu, Jul 04, 2019 at 05:28:22PM +0300, Paul Blakey wrote:


Move all that is needed to identify a tc filter to a
new structure, tc_id. This removes a lot of duplication
in accessing/creating tc filters.

Signed-off-by: Paul Blakey <paulb@mellanox.com><mailto:paulb@mellanox.com>



This is a rather large patch.
I'm wondering if you could comment on how thoroughly it has
been tested.

For V1 not much, but I'm adding V2 now to our regression before submitting it here as I'm waiting anyway to submit

the skb extension kernel side patchset that is in internal review. This is needed  to support tc <-> ovs recirc_id sharing this RFC is based on.

The tests we run in regression among the rest, can be found here:  https://github.com/roidayan/ovs-tests

If anyone wants to run this RFC, you will need 3 extra kernel side patches on top of linux net-next (which already has net/sched/act_ct.c):

1) net: Add tc chain extension to sk_buff

2) net/sched: rcu-ify ingress block and chain for datapath usage …

3) net: openvswitch: Set OvS recirc_id from tc skb extension chain index …

can be found here: https://github.com/vbuslov/linux/commits/conntrack3

Checking out this kernel branch also fine.


Thanks,

Paul.






---
 lib/netdev-linux.c      |   6 +-
 lib/netdev-offload-tc.c | 207 ++++++++++++++++++++----------------------------
 lib/tc.c                | 122 +++++++++++-----------------
 lib/tc.h                |  28 ++++---
 4 files changed, 151 insertions(+), 212 deletions(-)

diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index e4ea94c..69e0982 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -2421,6 +2421,8 @@ static int
 tc_del_matchall_policer(struct netdev *netdev)
 {
     uint32_t block_id = 0;
+    int prio = TC_RESERVED_PRIORITY_POLICE;
+    struct tc_id id;
     int ifindex;
     int err;

@@ -2429,8 +2431,8 @@ tc_del_matchall_policer(struct netdev *netdev)
         return err;
     }

-    err = tc_del_filter(ifindex, TC_RESERVED_PRIORITY_POLICE, 1, block_id,
-                        TC_INGRESS);
+    id = make_tc_id(ifindex, block_id, prio, TC_INGRESS);
+    err = tc_del_filter(&id);
     if (err) {
         return err;
     }
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 2af0f10..e37049c 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -145,20 +145,16 @@ static struct ovs_mutex ufid_lock = OVS_MUTEX_INITIALIZER;
 /**
  * struct ufid_tc_data - data entry for ufid_tc hmap.
  * @ufid_node: Element in @ufid_tc hash table by ufid key.
- * @tc_node: Element in @ufid_tc hash table by prio/handle/ifindex key.
+ * @tc_node: Element in @ufid_tc hash table by tc_id key.
  * @ufid: ufid assigned to the flow
- * @prio: tc priority
- * @handle: tc handle
- * @ifindex: netdev ifindex.
+ * @id: tc id
  * @netdev: netdev associated with the tc rule
  */
 struct ufid_tc_data {
     struct hmap_node ufid_node;
     struct hmap_node tc_node;
     ovs_u128 ufid;
-    uint16_t prio;
-    uint32_t handle;
-    int ifindex;
+    struct tc_id id;
     struct netdev *netdev;
 };

@@ -190,32 +186,27 @@ del_ufid_tc_mapping(const ovs_u128 *ufid)

 /* Wrapper function to delete filter and ufid tc mapping */
 static int
-del_filter_and_ufid_mapping(int ifindex, int prio, int handle,
-                            uint32_t block_id, const ovs_u128 *ufid,
-                            enum tc_qdisc_hook hook)
+del_filter_and_ufid_mapping(struct tc_id *id, const ovs_u128 *ufid)
 {
     int err;

-    err = tc_del_filter(ifindex, prio, handle, block_id, hook);
+    err = tc_del_filter(id);
     del_ufid_tc_mapping(ufid);
-
     return err;
 }

 /* Add ufid entry to ufid_tc hashmap. */
 static void
-add_ufid_tc_mapping(const ovs_u128 *ufid, int prio, int handle,
-                    struct netdev *netdev, int ifindex)
+add_ufid_tc_mapping(struct netdev *netdev, const ovs_u128 *ufid,
+                    struct tc_id *id)



Adding the id parameter after the ufid parameter does not
seem consistent with some other functions, f.e. find_ufid(),
while consistent with others, f.e. get_ufid_tc_mapping().

Could a more consistent parameter ordering be used throughout
this patch?



 {
     size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0);
-    size_t tc_hash = hash_int(hash_int(prio, handle), ifindex);
+    size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex);
     struct ufid_tc_data *new_data = xzalloc(sizeof *new_data);

     new_data->ufid = *ufid;
-    new_data->prio = prio;
-    new_data->handle = handle;
+    new_data->id = *id;
     new_data->netdev = netdev_ref(netdev);
-    new_data->ifindex = ifindex;

     ovs_mutex_lock(&ufid_lock);
     hmap_insert(&ufid_tc, &new_data->ufid_node, ufid_hash);
@@ -223,56 +214,49 @@ add_ufid_tc_mapping(const ovs_u128 *ufid, int prio, int handle,
     ovs_mutex_unlock(&ufid_lock);
 }

-/* Get ufid from ufid_tc hashmap.
+/* Get tc id from ufid_tc hashmap.
  *
- * If netdev output param is not NULL then the function will return
- * associated netdev on success and a refcount is taken on that netdev.
- * The caller is then responsible to close the netdev.
- *
- * Returns handle if successful and fill prio and netdev for that ufid.
- * Otherwise returns 0.
+ * Returns 0 if successful and fills id.
+ * Otherwise returns the error.
  */
 static int
-get_ufid_tc_mapping(const ovs_u128 *ufid, int *prio, struct netdev **netdev)
+get_ufid_tc_mapping(const ovs_u128 *ufid, struct tc_id *id)
 {
     size_t ufid_hash = hash_bytes(ufid, sizeof *ufid, 0);
     struct ufid_tc_data *data;
-    int handle = 0;

     ovs_mutex_lock(&ufid_lock);
     HMAP_FOR_EACH_WITH_HASH(data, ufid_node, ufid_hash, &ufid_tc) {
         if (ovs_u128_equals(*ufid, data->ufid)) {
-            if (prio) {
-                *prio = data->prio;
-            }
-            if (netdev) {
-                *netdev = netdev_ref(data->netdev);
-            }
-            handle = data->handle;
-            break;
+            *id = data->id;



Did you consider returning &data-id (or NULL on error)?

As this function now only returns one value (id) rather than two
(prio and netdev) returning a reference to the id may be cleaner.



+            ovs_mutex_unlock(&ufid_lock);
+            return 0;
         }
     }
     ovs_mutex_unlock(&ufid_lock);

-    return handle;
+    return ENOENT;
 }

-/* Find ufid entry in ufid_tc hashmap using prio, handle and netdev.
+/* Find ufid entry in ufid_tc hashmap using tc_id id.
  * The result is saved in ufid.
  *
  * Returns true on success.
  */
 static bool
-find_ufid(int prio, int handle, struct netdev *netdev, ovs_u128 *ufid)
+find_ufid(struct netdev *netdev, struct tc_id *id, ovs_u128 *ufid)



I think this could also return a reference to id.
But by that reasoning it could have also returned a reference before the id
change.  So perhaps I am missing something here.



 {
-    int ifindex = netdev_get_ifindex(netdev);
+    size_t tc_hash = hash_int(hash_int(id->prio, id->handle), id->ifindex);
     struct ufid_tc_data *data;
-    size_t tc_hash = hash_int(hash_int(prio, handle), ifindex);

     ovs_mutex_lock(&ufid_lock);
     HMAP_FOR_EACH_WITH_HASH(data, tc_node, tc_hash,  &ufid_tc) {
-        if (data->prio == prio && data->handle == handle
-            && data->ifindex == ifindex) {
+        if (netdev == data->netdev
+            && data->id.prio == id->prio
+            && data->id.handle == id->handle
+            && data->id.hook == id->hook
+            && data->id.block_id == id->block_id
+            && data->id.ifindex == id->ifindex) {
             *ufid = data->ufid;
             break;
         }
@@ -356,6 +340,8 @@ netdev_tc_flow_flush(struct netdev *netdev)
     enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
     int ifindex = netdev_get_ifindex(netdev);
     uint32_t block_id = 0;
+    struct tc_id id;
+    int prio = 0;

     if (ifindex < 0) {
         VLOG_ERR_RL(&error_rl, "flow_flush: failed to get ifindex for %s: %s",
@@ -364,8 +350,8 @@ netdev_tc_flow_flush(struct netdev *netdev)
     }

     block_id = get_block_id_from_netdev(netdev);
-
-    return tc_flush(ifindex, block_id, hook);
+    id = make_tc_id(ifindex, block_id, prio, hook);
+    return tc_del_filter(&id);
 }

 static int
@@ -375,6 +361,8 @@ netdev_tc_flow_dump_create(struct netdev *netdev,
     enum tc_qdisc_hook hook = get_tc_qdisc_hook(netdev);
     struct netdev_flow_dump *dump;
     uint32_t block_id = 0;
+    struct tc_id id;
+    int prio = 0;
     int ifindex;

     ifindex = netdev_get_ifindex(netdev);
@@ -388,7 +376,9 @@ netdev_tc_flow_dump_create(struct netdev *netdev,
     dump = xzalloc(sizeof *dump);
     dump->nl_dump = xzalloc(sizeof *dump->nl_dump);
     dump->netdev = netdev_ref(netdev);
-    tc_dump_flower_start(ifindex, dump->nl_dump, block_id, hook);
+
+    id = make_tc_id(ifindex, block_id, prio, hook);
+    tc_dump_flower_start(&id, dump->nl_dump);

     *dump_out = dump;

@@ -739,13 +729,18 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,
                          struct ofpbuf *rbuffer,
                          struct ofpbuf *wbuffer)
 {
+    struct netdev *netdev = dump->netdev;
     struct ofpbuf nl_flow;
+    struct tc_id id;
+
+    id.hook = get_tc_qdisc_hook(netdev);
+    id.block_id = get_block_id_from_netdev(netdev);
+    id.ifindex = netdev_get_ifindex(netdev);

     while (nl_dump_next(dump->nl_dump, &nl_flow, rbuffer)) {
         struct tc_flower flower;
-        struct netdev *netdev = dump->netdev;

-        if (parse_netlink_to_tc_flower(&nl_flow, &flower)) {
+        if (parse_netlink_to_tc_flower(&nl_flow, &id, &flower)) {
             continue;
         }

@@ -756,7 +751,7 @@ netdev_tc_flow_dump_next(struct netdev_flow_dump *dump,

         if (flower.act_cookie.len) {
             *ufid = *((ovs_u128 *) flower.act_cookie.data);
-        } else if (!find_ufid(flower.prio, flower.handle, netdev, ufid)) {
+        } else if (!find_ufid(netdev, &id, ufid)) {
             continue;
         }

@@ -1098,9 +1093,9 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
     struct tc_action *action;
     uint32_t block_id = 0;
     struct nlattr *nla;
+    struct tc_id id;
     size_t left;
     int prio = 0;
-    int handle;
     int ifindex;
     int err;

@@ -1352,35 +1347,32 @@ netdev_tc_flow_put(struct netdev *netdev, struct match *match,
         }
     }

-    block_id = get_block_id_from_netdev(netdev);
-    handle = get_ufid_tc_mapping(ufid, &prio, NULL);
-    if (handle && prio) {
-        VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d", handle, prio);
-        del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
-                                    hook);
-    }
-
-    if (!prio) {
-        prio = get_prio_for_tc_flower(&flower);
-        if (prio == 0) {
-            VLOG_ERR_RL(&rl, "couldn't get tc prio: %s", ovs_strerror(ENOSPC));
-            return ENOSPC;
-        }
+    if (!get_ufid_tc_mapping(ufid, &id)) {
+        VLOG_DBG_RL(&rl, "updating old handle: %d prio: %d",
+                    id.handle, id.prio);
+        del_filter_and_ufid_mapping(&id, ufid);
+    }
+
+    prio = get_prio_for_tc_flower(&flower);
+    if (prio == 0) {
+        VLOG_ERR_RL(&rl, "couldn't get tc prio: %s", ovs_strerror(ENOSPC));
+        return ENOSPC;
     }

     flower.act_cookie.data = ufid;
     flower.act_cookie.len = sizeof *ufid;

-    err = tc_replace_flower(ifindex, prio, handle, &flower, block_id, hook);
+    id = make_tc_id(ifindex, block_id, prio, hook);
+    err = tc_replace_flower(&id, &flower);
     if (!err) {
-        add_ufid_tc_mapping(ufid, flower.prio, flower.handle, netdev, ifindex);
+        add_ufid_tc_mapping(netdev, ufid, &id);
     }

     return err;
 }

 static int
-netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
+netdev_tc_flow_get(struct netdev *netdev,
                    struct match *match,
                    struct nlattr **actions,
                    const ovs_u128 *ufid,
@@ -1389,43 +1381,27 @@ netdev_tc_flow_get(struct netdev *netdev OVS_UNUSED,
                    struct ofpbuf *buf)
 {
     static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20);
-    struct netdev *dev;
     struct tc_flower flower;
-    enum tc_qdisc_hook hook;
-    uint32_t block_id = 0;
     odp_port_t in_port;
-    int prio = 0;
-    int ifindex;
-    int handle;
+    struct tc_id id;
     int err;

-    handle = get_ufid_tc_mapping(ufid, &prio, &dev);
-    if (!handle) {
-        return ENOENT;
-    }
-
-    hook = get_tc_qdisc_hook(dev);
-
-    ifindex = netdev_get_ifindex(dev);
-    if (ifindex < 0) {
-        VLOG_ERR_RL(&error_rl, "flow_get: failed to get ifindex for %s: %s",
-                    netdev_get_name(dev), ovs_strerror(-ifindex));
-        netdev_close(dev);
-        return -ifindex;
+    err = get_ufid_tc_mapping(ufid, &id);
+    if (err) {
+        return err;
     }

-    block_id = get_block_id_from_netdev(dev);
     VLOG_DBG_RL(&rl, "flow get (dev %s prio %d handle %d block_id %d)",
-                netdev_get_name(dev), prio, handle, block_id);
-    err = tc_get_flower(ifindex, prio, handle, &flower, block_id, hook);
-    netdev_close(dev);
+                netdev_get_name(netdev), id.prio, id.handle, id.block_id);
+
+    err = tc_get_flower(&id, &flower);
     if (err) {
         VLOG_ERR_RL(&error_rl, "flow get failed (dev %s prio %d handle %d): %s",
-                    netdev_get_name(dev), prio, handle, ovs_strerror(err));
+                    netdev_get_name(netdev), id.prio, id.handle, ovs_strerror(err));
         return err;
     }

-    in_port = netdev_ifindex_to_odp_port(ifindex);
+    in_port = netdev_ifindex_to_odp_port(id.ifindex);
     parse_tc_flower_to_match(&flower, match, actions, stats, attrs, buf);

     match->wc.masks.in_port.odp_port = u32_to_odp(UINT32_MAX);
@@ -1440,44 +1416,24 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED,
                    struct dpif_flow_stats *stats)
 {
     struct tc_flower flower;
-    enum tc_qdisc_hook hook;
-    uint32_t block_id = 0;
-    struct netdev *dev;
-    int prio = 0;
-    int ifindex;
-    int handle;
+    struct tc_id id;
     int error;

-    handle = get_ufid_tc_mapping(ufid, &prio, &dev);
-    if (!handle) {
-        return ENOENT;
-    }
-
-    hook = get_tc_qdisc_hook(dev);
-
-    ifindex = netdev_get_ifindex(dev);
-    if (ifindex < 0) {
-        VLOG_ERR_RL(&error_rl, "flow_del: failed to get ifindex for %s: %s",
-                    netdev_get_name(dev), ovs_strerror(-ifindex));
-        netdev_close(dev);
-        return -ifindex;
+    error = get_ufid_tc_mapping(ufid, &id);
+    if (error) {
+        return error;
     }

-    block_id = get_block_id_from_netdev(dev);
-
     if (stats) {
         memset(stats, 0, sizeof *stats);
-        if (!tc_get_flower(ifindex, prio, handle, &flower, block_id, hook)) {
+        if (!tc_get_flower(&id, &flower)) {
             stats->n_packets = get_32aligned_u64(&flower.stats.n_packets);
             stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes);
             stats->used = flower.lastused;
         }
     }

-    error = del_filter_and_ufid_mapping(ifindex, prio, handle, block_id, ufid,
-                                        hook);
-
-    netdev_close(dev);
+    error = del_filter_and_ufid_mapping(&id, ufid);

     return error;
 }
@@ -1486,7 +1442,9 @@ static void
 probe_multi_mask_per_prio(int ifindex)
 {
     struct tc_flower flower;
+    struct tc_id id1, id2;
     int block_id = 0;
+    int prio = 1;
     int error;

     error = tc_add_del_qdisc(ifindex, true, block_id, TC_INGRESS);
@@ -1501,7 +1459,8 @@ probe_multi_mask_per_prio(int ifindex)
     memset(&flower.key.dst_mac, 0x11, sizeof flower.key.dst_mac);
     memset(&flower.mask.dst_mac, 0xff, sizeof flower.mask.dst_mac);

-    error = tc_replace_flower(ifindex, 1, 1, &flower, block_id, TC_INGRESS);
+    id1 = make_tc_id(ifindex, block_id, prio, TC_INGRESS);
+    error = tc_replace_flower(&id1, &flower);
     if (error) {
         goto out;
     }
@@ -1509,14 +1468,15 @@ probe_multi_mask_per_prio(int ifindex)
     memset(&flower.key.src_mac, 0x11, sizeof flower.key.src_mac);
     memset(&flower.mask.src_mac, 0xff, sizeof flower.mask.src_mac);

-    error = tc_replace_flower(ifindex, 1, 2, &flower, block_id, TC_INGRESS);
-    tc_del_filter(ifindex, 1, 1, block_id, TC_INGRESS);
+    id2 = make_tc_id(ifindex, block_id, prio, TC_INGRESS);
+    error = tc_replace_flower(&id2, &flower);
+    tc_del_filter(&id1);

     if (error) {
         goto out;
     }

-    tc_del_filter(ifindex, 1, 2, block_id, TC_INGRESS);
+    tc_del_filter(&id2);

     multi_mask_per_prio = true;
     VLOG_INFO("probe tc: multiple masks on single tc prio is supported.");
@@ -1530,6 +1490,8 @@ probe_tc_block_support(int ifindex)
 {
     struct tc_flower flower;
     uint32_t block_id = 1;
+    struct tc_id id;
+    int prio = 0;
     int error;

     error = tc_add_del_qdisc(ifindex, true, block_id, TC_INGRESS);
@@ -1544,7 +1506,8 @@ probe_tc_block_support(int ifindex)
     memset(&flower.key.dst_mac, 0x11, sizeof flower.key.dst_mac);
     memset(&flower.mask.dst_mac, 0xff, sizeof flower.mask.dst_mac);

-    error = tc_replace_flower(ifindex, 1, 1, &flower, block_id, TC_INGRESS);
+    id = make_tc_id(ifindex, block_id, prio, TC_INGRESS);
+    error = tc_replace_flower(&id, &flower);

     tc_add_del_qdisc(ifindex, false, block_id, TC_INGRESS);

diff --git a/lib/tc.c b/lib/tc.c
index 1eca356..d7cb43e 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -193,6 +193,21 @@ tc_make_request(int ifindex, int type, unsigned int flags,
     return tcmsg;
 }

+static void request_from_tc_id(struct tc_id *id, uint16_t eth_type,
+                               int type, unsigned int flags,
+                               struct ofpbuf *request)
+{
+    int ifindex = id->block_id ? TCM_IFINDEX_MAGIC_BLOCK : id->ifindex;
+    struct tcmsg *tcmsg;
+
+    tcmsg = tc_make_request(ifindex, type, flags, request);
+    tcmsg->tcm_parent = (id->hook == TC_EGRESS) ?
+                        TC_EGRESS_PARENT : (id->block_id ? : TC_INGRESS_PARENT);
+    tcmsg->tcm_info = tc_make_handle(id->prio, eth_type);
+    tcmsg->tcm_handle = id->handle;
+}




The above helper seems nice, leading to a good reduction in code
duplication. But I wonder if it would be nicer to add it in a earlier
patch in this series - for one it would make this patch shorter and
easier to review. But I do not feel strongly about this.



+
+
 int
 tc_transact(struct ofpbuf *request, struct ofpbuf **replyp)
 {
@@ -1429,7 +1444,8 @@ nl_parse_flower_options(struct nlattr *nl_options, struct tc_flower *flower)
 }

 int
-parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tc_flower *flower)
+parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tc_id *id,
+                           struct tc_flower *flower)
 {
     struct tcmsg *tc;
     struct nlattr *ta[ARRAY_SIZE(tca_policy)];
@@ -1442,16 +1458,17 @@ parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tc_flower *flower)
     memset(flower, 0, sizeof *flower);

     tc = ofpbuf_at_assert(reply, NLMSG_HDRLEN, sizeof *tc);
-    flower->handle = tc->tcm_handle;
+
     flower->key.eth_type = (OVS_FORCE ovs_be16) tc_get_minor(tc->tcm_info);
     flower->mask.eth_type = OVS_BE16_MAX;
-    flower->prio = tc_get_major(tc->tcm_info);
+    id->prio = tc_get_major(tc->tcm_info);
+    id->handle = tc->tcm_handle;

-    if (flower->prio == TC_RESERVED_PRIORITY_POLICE) {
+    if (id->prio == TC_RESERVED_PRIORITY_POLICE) {
         return 0;
     }

-    if (!flower->handle) {
+    if (!id->handle) {
         return EAGAIN;
     }

@@ -1471,20 +1488,11 @@ parse_netlink_to_tc_flower(struct ofpbuf *reply, struct tc_flower *flower)
 }

 int
-tc_dump_flower_start(int ifindex, struct nl_dump *dump, uint32_t block_id,
-                     enum tc_qdisc_hook hook)
+tc_dump_flower_start(struct tc_id *id, struct nl_dump *dump)
 {
     struct ofpbuf request;
-    struct tcmsg *tcmsg;
-    int index;
-
-    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
-    tcmsg = tc_make_request(index, RTM_GETTFILTER, NLM_F_DUMP, &request);
-    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
-                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
-    tcmsg->tcm_info = TC_H_UNSPEC;
-    tcmsg->tcm_handle = 0;

+    request_from_tc_id(id, 0, RTM_GETTFILTER, NLM_F_DUMP, &request);
     nl_dump_start(dump, NETLINK_ROUTE, &request);
     ofpbuf_uninit(&request);

@@ -1492,68 +1500,28 @@ tc_dump_flower_start(int ifindex, struct nl_dump *dump, uint32_t block_id,
 }

 int
-tc_flush(int ifindex, uint32_t block_id, enum tc_qdisc_hook hook)
+tc_del_filter(struct tc_id *id)
 {
     struct ofpbuf request;
-    struct tcmsg *tcmsg;
-    int index;
-
-    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
-    tcmsg = tc_make_request(index, RTM_DELTFILTER, NLM_F_ACK, &request);
-    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
-                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
-    tcmsg->tcm_info = TC_H_UNSPEC;

+    request_from_tc_id(id, 0, RTM_DELTFILTER, NLM_F_ACK, &request);
     return tc_transact(&request, NULL);
 }

 int
-tc_del_filter(int ifindex, int prio, int handle, uint32_t block_id,
-              enum tc_qdisc_hook hook)
-{
-    struct ofpbuf request;
-    struct tcmsg *tcmsg;
-    struct ofpbuf *reply;
-    int error;
-    int index;
-
-    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
-    tcmsg = tc_make_request(index, RTM_DELTFILTER, NLM_F_ECHO, &request);
-    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
-                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
-    tcmsg->tcm_info = tc_make_handle(prio, 0);
-    tcmsg->tcm_handle = handle;
-
-    error = tc_transact(&request, &reply);
-    if (!error) {
-        ofpbuf_delete(reply);
-    }
-    return error;
-}
-
-int
-tc_get_flower(int ifindex, int prio, int handle, struct tc_flower *flower,
-              uint32_t block_id, enum tc_qdisc_hook hook)
+tc_get_flower(struct tc_id *id, struct tc_flower *flower)
 {
     struct ofpbuf request;
-    struct tcmsg *tcmsg;
     struct ofpbuf *reply;
     int error;
-    int index;
-
-    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
-    tcmsg = tc_make_request(index, RTM_GETTFILTER, NLM_F_ECHO, &request);
-    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
-                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
-    tcmsg->tcm_info = tc_make_handle(prio, 0);
-    tcmsg->tcm_handle = handle;

+    request_from_tc_id(id, 0, RTM_GETTFILTER, NLM_F_ECHO, &request);
     error = tc_transact(&request, &reply);
     if (error) {
         return error;
     }

-    error = parse_netlink_to_tc_flower(reply, flower);
+    error = parse_netlink_to_tc_flower(reply, id, flower);
     ofpbuf_delete(reply);
     return error;
 }
@@ -2299,26 +2267,30 @@ nl_msg_put_flower_options(struct ofpbuf *request, struct tc_flower *flower)
     return 0;
 }

+struct tc_id make_tc_id(int ifindex, uint32_t block_id, uint16_t prio,
+                        enum tc_qdisc_hook hook)
+{
+    struct tc_id id = { .handle = 0, };



Zeroing id does not seem strictly necessary if every field is set,
which seems to be the case.



+
+    id.ifindex = ifindex;
+    id.block_id = block_id;
+    id.prio = prio;
+    id.hook = hook;
+
+    return id;



This is a very nice 'pure' function.
However I am concerned about any performance / stack overhead
of returning a structure by value. Did you give this some
consideration?

Perhaps this concern could be alleviated by making make_tc_id()
a static inline function. Just an idea.



+}
+
 int
-tc_replace_flower(int ifindex, uint16_t prio, uint32_t handle,
-                  struct tc_flower *flower, uint32_t block_id,
-                  enum tc_qdisc_hook hook)
+tc_replace_flower(struct tc_id *id, struct tc_flower *flower)
 {
     struct ofpbuf request;
-    struct tcmsg *tcmsg;
     struct ofpbuf *reply;
     int error = 0;
     size_t basic_offset;
     uint16_t eth_type = (OVS_FORCE uint16_t) flower->key.eth_type;
-    int index;

-    index = block_id ? TCM_IFINDEX_MAGIC_BLOCK : ifindex;
-    tcmsg = tc_make_request(index, RTM_NEWTFILTER, NLM_F_CREATE | NLM_F_ECHO,
-                            &request);
-    tcmsg->tcm_parent = (hook == TC_EGRESS) ?
-                        TC_EGRESS_PARENT : (block_id ? : TC_INGRESS_PARENT);
-    tcmsg->tcm_info = tc_make_handle(prio, eth_type);
-    tcmsg->tcm_handle = handle;
+    request_from_tc_id(id, eth_type, RTM_NEWTFILTER,
+                       NLM_F_CREATE | NLM_F_ECHO, &request);

     nl_msg_put_string(&request, TCA_KIND, "flower");
     basic_offset = nl_msg_start_nested(&request, TCA_OPTIONS);
@@ -2337,8 +2309,8 @@ tc_replace_flower(int ifindex, uint16_t prio, uint32_t handle,
         struct tcmsg *tc =
             ofpbuf_at_assert(reply, NLMSG_HDRLEN, sizeof *tc);

-        flower->prio = tc_get_major(tc->tcm_info);
-        flower->handle = tc->tcm_handle;
+        id->prio = tc_get_major(tc->tcm_info);
+        id->handle = tc->tcm_handle;
         ofpbuf_delete(reply);
     }

diff --git a/lib/tc.h b/lib/tc.h
index 2e0f5e3..00acc41 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -197,10 +197,18 @@ enum tc_offloaded_state {
     TC_OFFLOADED_STATE_NOT_IN_HW,
 };

-struct tc_flower {
+struct tc_id {
+    enum tc_qdisc_hook hook;
+    uint32_t block_id;
+    int ifindex;
+    uint16_t prio;
     uint32_t handle;
-    uint32_t prio;
+};



At a glance the ordering of the fields of struct tc_id
appears to result in a structure with holes on both 32bit and 64bit
architectures.

As this is an internal structure I think it would be worth
ordering the fields in a way that minimises holes.




+struct tc_id make_tc_id(int ifindex, uint32_t block_id, uint16_t prio,
+                        enum tc_qdisc_hook hook);
+
+struct tc_flower {
     struct tc_flower_key key;
     struct tc_flower_key mask;

@@ -234,18 +242,12 @@ BUILD_ASSERT_DECL(offsetof(struct tc_flower, rewrite)
                   + MEMBER_SIZEOF(struct tc_flower, rewrite)
                   + sizeof(uint32_t) - 2 < sizeof(struct tc_flower));

-int tc_replace_flower(int ifindex, uint16_t prio, uint32_t handle,
-                      struct tc_flower *flower, uint32_t block_id,
-                      enum tc_qdisc_hook hook);
-int tc_del_filter(int ifindex, int prio, int handle, uint32_t block_id,
-                  enum tc_qdisc_hook hook);
-int tc_get_flower(int ifindex, int prio, int handle,
-                  struct tc_flower *flower, uint32_t block_id,
-                  enum tc_qdisc_hook hook);
-int tc_flush(int ifindex, uint32_t block_id, enum tc_qdisc_hook hook);
-int tc_dump_flower_start(int ifindex, struct nl_dump *dump, uint32_t block_id,
-                         enum tc_qdisc_hook hook);
+int tc_replace_flower(struct tc_id *id, struct tc_flower *flower);
+int tc_del_filter(struct tc_id *id);
+int tc_get_flower(struct tc_id *id, struct tc_flower *flower);
+int tc_dump_flower_start(struct tc_id *id, struct nl_dump *dump);
 int parse_netlink_to_tc_flower(struct ofpbuf *reply,
+                               struct tc_id *id,
                                struct tc_flower *flower);
 void tc_set_policy(const char *policy);

--
1.8.3.1