[ovs-dev,03/10] tc: Introduce tc_id to specify a tc filter

From: Paul Blakey <paulb@mellanox.com>

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>
Reviewed-by: Roi Dayan <roid@mellanox.com>
---
 lib/netdev-linux.c      |   6 +-
 lib/netdev-offload-tc.c | 208 ++++++++++++++++++++----------------------------
 lib/tc.c                | 122 +++++++++++-----------------
 lib/tc.h                |  28 ++++---
 4 files changed, 152 insertions(+), 212 deletions(-)

On Wed, Oct 30, 2019 at 03:37:17PM +0200, Roi Dayan wrote:
> From: Paul Blakey <paulb@mellanox.com>
> 
> 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>
> Reviewed-by: Roi Dayan <roid@mellanox.com>

Overall this change seems nice, though there is a log going
on in this patch, which perhaps does make review more difficult
than it might otherwise be.

I've added a few comments inline.

> ---
>  lib/netdev-linux.c      |   6 +-
>  lib/netdev-offload-tc.c | 208 ++++++++++++++++++++----------------------------
>  lib/tc.c                | 122 +++++++++++-----------------
>  lib/tc.h                |  28 ++++---
>  4 files changed, 152 insertions(+), 212 deletions(-)
> 
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index f4819237370a..1be161aecb92 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -2348,6 +2348,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;

nit: I think it would be good to preserve the reverse xmas tree
formation for the local variables in this function.

>      int ifindex;
>      int err;
>  
> @@ -2356,8 +2358,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 f6d1abb2e695..bb62398b6157 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)
>  {
>      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);
> -            }

It is not clear to me why callers no longer need the above netdev logic,/

> -            handle = data->handle;
> -            break;
> +            *id = data->id;
> +            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)
>  {
> -    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) {

Perhaps a helper for comparing struct tc_id would be appropriate.

>              *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;
>  
> @@ -776,13 +766,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);

The above seems to leave part of id uninitialised.
Is that intentional? Perhaps using make_tc_id() is appropriate here?

>  
>      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;
>          }
>  
> @@ -793,7 +788,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;
>          }
>  
> @@ -1155,9 +1150,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;
>  
> @@ -1427,35 +1422,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);
> +    }

Is it intentional that id.prio is not checked as the equivalent prio
seems to have been in the old code?

> +
> +    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);

Is it intentional that the handle of an existing mapping is not
stored in id?

> +    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,
> @@ -1464,43 +1456,28 @@ 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);
> @@ -1515,44 +1492,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;
>  }
> @@ -1561,7 +1518,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);
> @@ -1576,7 +1535,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;
>      }
> @@ -1584,14 +1544,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.");
> @@ -1605,6 +1566,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);
> @@ -1619,7 +1582,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 d5487097d8ec..157f54d61a04 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -194,6 +194,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;
> +    uint32_t ingress_parent = id->block_id ? : TC_INGRESS_PARENT;
> +    struct tcmsg *tcmsg;
> +
> +    tcmsg = tc_make_request(ifindex, type, flags, request);
> +    tcmsg->tcm_parent = (id->hook == TC_EGRESS) ?
> +                        TC_EGRESS_PARENT : ingress_parent;
> +    tcmsg->tcm_info = tc_make_handle(id->prio, eth_type);
> +    tcmsg->tcm_handle = id->handle;
> +}
> +
>  int
>  tc_transact(struct ofpbuf *request, struct ofpbuf **replyp)
>  {
> @@ -1525,7 +1540,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)];
> @@ -1538,16 +1554,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;
>      }
>  
> @@ -1567,20 +1584,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);
>  
> @@ -1588,68 +1596,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)
> +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_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)
> -{
> -    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;
>  }
> @@ -2477,26 +2445,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, };

I believe the above will effectively memset id to all-zeros.
But all fields, other than handle are set below.
So perhaps this memset is best avoided?

> +
> +    id.ifindex = ifindex;
> +    id.block_id = block_id;
> +    id.prio = prio;
> +    id.hook = hook;
> +
> +    return id;
> +}
> +
>  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);
> @@ -2515,8 +2487,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 f4073c6c47e6..845a66e62f3b 100644
> --- a/lib/tc.h
> +++ b/lib/tc.h
> @@ -210,10 +210,18 @@ enum tc_offloaded_state {
>  
>  #define TCA_ACT_MAX_NUM 16
>  
> -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;
> +};
>  
> +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;
>  
> @@ -247,18 +255,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);
>  
> -- 
> 2.8.4
>

On 11/20/2019 2:04 PM, Simon Horman wrote:
> On Wed, Oct 30, 2019 at 03:37:17PM +0200, Roi Dayan wrote:
>> From: Paul Blakey <paulb@mellanox.com>
>>
>> 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>
>> Reviewed-by: Roi Dayan <roid@mellanox.com>
> Overall this change seems nice, though there is a log going
> on in this patch, which perhaps does make review more difficult
> than it might otherwise be.
>
> I've added a few comments inline.
>
>> ---
>>   lib/netdev-linux.c      |   6 +-
>>   lib/netdev-offload-tc.c | 208 ++++++++++++++++++++----------------------------
>>   lib/tc.c                | 122 +++++++++++-----------------
>>   lib/tc.h                |  28 ++++---
>>   4 files changed, 152 insertions(+), 212 deletions(-)
>>
>> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
>> index f4819237370a..1be161aecb92 100644
>> --- a/lib/netdev-linux.c
>> +++ b/lib/netdev-linux.c
>> @@ -2348,6 +2348,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;
> nit: I think it would be good to preserve the reverse xmas tree
> formation for the local variables in this function.
Sure thanks.
>
>>       int ifindex;
>>       int err;
>>   
>> @@ -2356,8 +2358,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 f6d1abb2e695..bb62398b6157 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)
>>   {
>>       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);
>> -            }
> It is not clear to me why callers no longer need the above netdev logic,/

The returned id contains the tc hook (egress/ingress), and the prio.

The returned id->prio is used instead of *prio param, and for the netdev 
it was only

used for getting the hook, so instead we use id->hook (when it's passed 
to tc functions directly).

>
>> -            handle = data->handle;
>> -            break;
>> +            *id = data->id;
>> +            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)
>>   {
>> -    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) {
> Perhaps a helper for comparing struct tc_id would be appropriate.
Good idea, will do.
>
>>               *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;
>>   
>> @@ -776,13 +766,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);
> The above seems to leave part of id uninitialised.
> Is that intentional? Perhaps using make_tc_id() is appropriate here?

Yes it is intentional, it is used as a filter here.

Will convert it to make_tc_id(hook, block, 0, ifindex)

Thanks.


>
>>   
>>       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;
>>           }
>>   
>> @@ -793,7 +788,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;
>>           }
>>   
>> @@ -1155,9 +1150,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;
>>   
>> @@ -1427,35 +1422,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);
>> +    }
> Is it intentional that id.prio is not checked as the equivalent prio
> seems to have been in the old code?


The code behaves the same.

Old code did (handle  && prio) but it functioned the same as just handle 
(since any existing filter has a prio and handle) , which  means the 
ufid exists and return the (handle,prio).

Now new code does the same (!get_ufid_tc_mapping(ufid, &id)) - returns 0 
if a ufid exists, but returns all the details for that existing filter 
(handle, prio, qdisc....). (I will change this to < 0, so it will be 
clearer 0 means success and not failed to get...).

The (!prio) get_prio... is here below.


>
>> +
>> +    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);
> Is it intentional that the handle of an existing mapping is not
> stored in id?

The handle is filled out in id by tc_replace_flower

id is used as input/output, same as flower before (flower.prio, and 
flower.handle after success).

>
>> +    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,
>> @@ -1464,43 +1456,28 @@ 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);
>> @@ -1515,44 +1492,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;
>>   }
>> @@ -1561,7 +1518,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);
>> @@ -1576,7 +1535,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;
>>       }
>> @@ -1584,14 +1544,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.");
>> @@ -1605,6 +1566,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);
>> @@ -1619,7 +1582,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 d5487097d8ec..157f54d61a04 100644
>> --- a/lib/tc.c
>> +++ b/lib/tc.c
>> @@ -194,6 +194,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;
>> +    uint32_t ingress_parent = id->block_id ? : TC_INGRESS_PARENT;
>> +    struct tcmsg *tcmsg;
>> +
>> +    tcmsg = tc_make_request(ifindex, type, flags, request);
>> +    tcmsg->tcm_parent = (id->hook == TC_EGRESS) ?
>> +                        TC_EGRESS_PARENT : ingress_parent;
>> +    tcmsg->tcm_info = tc_make_handle(id->prio, eth_type);
>> +    tcmsg->tcm_handle = id->handle;
>> +}
>> +
>>   int
>>   tc_transact(struct ofpbuf *request, struct ofpbuf **replyp)
>>   {
>> @@ -1525,7 +1540,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)];
>> @@ -1538,16 +1554,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;
>>       }
>>   
>> @@ -1567,20 +1584,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);
>>   
>> @@ -1588,68 +1596,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)
>> +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_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)
>> -{
>> -    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;
>>   }
>> @@ -2477,26 +2445,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, };
> I believe the above will effectively memset id to all-zeros.

Yes I used it this way because iirc it's supported by most compilers, 
compared to {}, and sometimes {0} is picky.

> But all fields, other than handle are set below.
> So perhaps this memset is best avoided?
Yes I will explicitly set all fields.
>
>> +
>> +    id.ifindex = ifindex;
>> +    id.block_id = block_id;
>> +    id.prio = prio;
>> +    id.hook = hook;
>> +
>> +    return id;
>> +}
>> +
>>   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);
>> @@ -2515,8 +2487,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 f4073c6c47e6..845a66e62f3b 100644
>> --- a/lib/tc.h
>> +++ b/lib/tc.h
>> @@ -210,10 +210,18 @@ enum tc_offloaded_state {
>>   
>>   #define TCA_ACT_MAX_NUM 16
>>   
>> -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;
>> +};
>>   
>> +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;
>>   
>> @@ -247,18 +255,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);
>>   
>> -- 
>> 2.8.4


Thanks for reviewing, sorry for late reply.