On Wed, Oct 30, 2019 at 03:37:17PM +0200, Roi Dayan wrote:
> From: Paul Blakey <pa...@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 <pa...@mellanox.com>
> Reviewed-by: Roi Dayan <r...@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
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
