Message ID | 20191127125516.5135-4-roid@mellanox.com |
---|---|
State | Superseded |
Headers | show |
Series | Add support for offloading CT datapath rules to TC | expand |
Bleep bloop. Greetings Roi Dayan, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: ERROR: Author Paul Blakey <paulb@mellanox.com> needs to sign off. Lines checked: 745, Warnings: 0, Errors: 1 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On Wed, Nov 27, 2019 at 02:55:09PM +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. Ok, gotta say, 'tc_id' is a bit confusing here. Every time I read it, I try to find a correlating id in the kernel land but it doesn't exist, because 'tc' itself refers to a much bigger thing and not just filters. That said, I was thinking, what about 'tcf_id', or even a longer one, 'tc_filter_id'? But before answering, please read below. :-) > > --- > > Changelog: > V1->V2: > In tc_del_matchall_policer - reverse xmas param order > Added and used helper is_tc_id_eq(id1, id2) in find_ufid > In netdev_tc_flow_dump_next - use make_tc_id() instead of manualy filling id ^^^^^^^^^^ > In netdev_tc_flow_put - use if (get_ufid_tc_mapping(ufid, &id) == 0) to be mor explict we found the mapping not failed to get > In make_tc_id - fill id explictily and removed memset. > Moved tc_make_id to be static inline in tc.h ^^^^^^^^^^ The patch is currently using make_tc_id. I see other functions already using the latter one, tc_make_*, like tc_make_handle and tc_make_request. Though it also already has make_tc_id_chain. Which then, with this last one, it gets easier to understand what a 'tc_id' means. I don't have a strong opinion here, so I'm just highlighting the topic. > > Signed-off-by: Paul Blakey <paulb@mellanox.com> > --- > lib/netdev-linux.c | 6 +- > lib/netdev-offload-tc.c | 204 +++++++++++++++++++----------------------------- > lib/tc.c | 109 ++++++++------------------ > lib/tc.h | 51 +++++++++--- > 4 files changed, 158 insertions(+), 212 deletions(-) > > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c > index f4819237370a..dd431f05ef2c 100644 > --- a/lib/netdev-linux.c > +++ b/lib/netdev-linux.c > @@ -2347,7 +2347,9 @@ tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate, > static int > tc_del_matchall_policer(struct netdev *netdev) > { > + int prio = TC_RESERVED_PRIORITY_POLICE; > uint32_t block_id = 0; > + struct tc_id id; > 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 1adbb325b94f..6354ad59676a 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,44 @@ 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; > + 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 && is_tc_id_eq(&data->id, id)) { > *ufid = data->ufid; > break; > } > @@ -356,6 +335,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 +345,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 +356,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 +371,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; > > @@ -777,13 +762,19 @@ 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 = make_tc_id(netdev_get_ifindex(netdev), > + get_block_id_from_netdev(netdev), > + 0, /* prio */ > + get_tc_qdisc_hook(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; > } > > @@ -794,7 +785,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; > } > > @@ -1156,9 +1147,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; > > @@ -1428,38 +1419,35 @@ 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) == 0) { > + 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) { > if (stats) { > memset(stats, 0, sizeof *stats); > } > - 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, > @@ -1468,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); > @@ -1519,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; > } > @@ -1565,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); > @@ -1580,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; > } > @@ -1588,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."); > @@ -1609,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); > @@ -1623,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 ba3e9d09dabe..36a93b164158 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) > -{ > - 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; > } > @@ -2493,25 +2461,16 @@ nl_msg_put_flower_options(struct ofpbuf *request, struct tc_flower *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) > +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); > @@ -2530,8 +2489,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..b48e10c352ab 100644 > --- a/lib/tc.h > +++ b/lib/tc.h > @@ -210,10 +210,41 @@ 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; > +}; > + > +static inline struct > +tc_id make_tc_id(int ifindex, uint32_t block_id, uint16_t prio, > + enum tc_qdisc_hook hook) > +{ > + struct tc_id id; > + > + id.block_id = block_id; > + id.ifindex = ifindex; > + id.prio = prio; > + id.hook = hook; > + id.handle = 0; > > + return id; > +} > + > +static inline bool > +is_tc_id_eq(struct tc_id *id1, struct tc_id *id2) > +{ > + return id1->prio == id2->prio > + && id1->handle == id2->handle > + && id1->handle == id2->handle > + && id1->hook == id2->hook > + && id1->block_id == id2->block_id > + && id1->ifindex == id2->ifindex; > +} > + > +struct tc_flower { > struct tc_flower_key key; > struct tc_flower_key mask; > > @@ -247,18 +278,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/27/2019 4:07 PM, Marcelo Ricardo Leitner wrote: > On Wed, Nov 27, 2019 at 02:55:09PM +0200, Roi Dayan wrote: >> From: Paul Blakey <paulb@mellanox.com><mailto: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. > > Ok, gotta say, 'tc_id' is a bit confusing here. Every time I read > it, I try to find a correlating id in the kernel land but it doesn't > exist, because 'tc' itself refers to a much bigger thing and not > just filters. > > That said, I was thinking, what about 'tcf_id', or even a longer > one, 'tc_filter_id'? But before answering, please read below. :-) I don't mind either way, but from this I'd rater have the short one tcf_id. > >> >> --- >> >> Changelog: V1->V2: In tc_del_matchall_policer - reverse xmas param >> order Added and used helper is_tc_id_eq(id1, id2) in find_ufid In >> netdev_tc_flow_dump_next - use make_tc_id() instead of manualy >> filling id > ^^^^^^^^^^ > >> In netdev_tc_flow_put - use if (get_ufid_tc_mapping(ufid, &id) == >> 0) to be mor explict we found the mapping not failed to get In >> make_tc_id - fill id explictily and removed memset. Moved >> tc_make_id to be static inline in tc.h > ^^^^^^^^^^ > > The patch is currently using make_tc_id. I see other functions > already using the latter one, tc_make_*, like tc_make_handle and > tc_make_request. Though it also already has make_tc_id_chain. Which > then, with this last one, it gets easier to understand what a 'tc_id' > means. > > I don't have a strong opinion here, so I'm just highlighting the > topic. Not sure what you meant here. Would you rather we have tc_make_tcf_id() and tc_make_tcf_id_chain() (in later patch) to be more consistent?
On Sun, Dec 01, 2019 at 07:38:35AM +0000, Paul Blakey wrote: > > On 11/27/2019 4:07 PM, Marcelo Ricardo Leitner wrote: > > On Wed, Nov 27, 2019 at 02:55:09PM +0200, Roi Dayan wrote: >> From: Paul Blakey <paulb@mellanox.com><mailto: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. > > Ok, gotta say, 'tc_id' is a bit confusing here. Every time I read > it, I try to find a correlating id in the kernel land but it doesn't > exist, because 'tc' itself refers to a much bigger thing and not > just filters. > > That said, I was thinking, what about 'tcf_id', or even a longer > one, 'tc_filter_id'? But before answering, please read below. :-) > I don't mind either way, but from this I'd rater have the short one tcf_id. > > > >> >> --- >> >> Changelog: V1->V2: In tc_del_matchall_policer - reverse xmas param >> order Added and used helper is_tc_id_eq(id1, id2) in find_ufid In >> netdev_tc_flow_dump_next - use make_tc_id() instead of manualy >> filling id > ^^^^^^^^^^ > >> In netdev_tc_flow_put - use if (get_ufid_tc_mapping(ufid, &id) == >> 0) to be mor explict we found the mapping not failed to get In >> make_tc_id - fill id explictily and removed memset. Moved >> tc_make_id to be static inline in tc.h > ^^^^^^^^^^ > > The patch is currently using make_tc_id. I see other functions > already using the latter one, tc_make_*, like tc_make_handle and > tc_make_request. Though it also already has make_tc_id_chain. Which > then, with this last one, it gets easier to understand what a 'tc_id' > means. > > I don't have a strong opinion here, so I'm just highlighting the > topic. > Not sure what you meant here. > > > Would you rather we have tc_make_tcf_id() and tc_make_tcf_id_chain() (in later patch) to be more consistent? Yes. That seems the best to me. Thanks, Marcelo
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c index f4819237370a..dd431f05ef2c 100644 --- a/lib/netdev-linux.c +++ b/lib/netdev-linux.c @@ -2347,7 +2347,9 @@ tc_add_matchall_policer(struct netdev *netdev, uint32_t kbits_rate, static int tc_del_matchall_policer(struct netdev *netdev) { + int prio = TC_RESERVED_PRIORITY_POLICE; uint32_t block_id = 0; + struct tc_id id; 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 1adbb325b94f..6354ad59676a 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,44 @@ 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; + 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 && is_tc_id_eq(&data->id, id)) { *ufid = data->ufid; break; } @@ -356,6 +335,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 +345,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 +356,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 +371,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; @@ -777,13 +762,19 @@ 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 = make_tc_id(netdev_get_ifindex(netdev), + get_block_id_from_netdev(netdev), + 0, /* prio */ + get_tc_qdisc_hook(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; } @@ -794,7 +785,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; } @@ -1156,9 +1147,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; @@ -1428,38 +1419,35 @@ 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) == 0) { + 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) { if (stats) { memset(stats, 0, sizeof *stats); } - 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, @@ -1468,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); @@ -1519,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; } @@ -1565,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); @@ -1580,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; } @@ -1588,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."); @@ -1609,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); @@ -1623,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 ba3e9d09dabe..36a93b164158 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) -{ - 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; } @@ -2493,25 +2461,16 @@ nl_msg_put_flower_options(struct ofpbuf *request, struct tc_flower *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) +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); @@ -2530,8 +2489,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..b48e10c352ab 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -210,10 +210,41 @@ 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; +}; + +static inline struct +tc_id make_tc_id(int ifindex, uint32_t block_id, uint16_t prio, + enum tc_qdisc_hook hook) +{ + struct tc_id id; + + id.block_id = block_id; + id.ifindex = ifindex; + id.prio = prio; + id.hook = hook; + id.handle = 0; + return id; +} + +static inline bool +is_tc_id_eq(struct tc_id *id1, struct tc_id *id2) +{ + return id1->prio == id2->prio + && id1->handle == id2->handle + && id1->handle == id2->handle + && id1->hook == id2->hook + && id1->block_id == id2->block_id + && id1->ifindex == id2->ifindex; +} + +struct tc_flower { struct tc_flower_key key; struct tc_flower_key mask; @@ -247,18 +278,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);