| Message ID | 20220503030836.28783-7-jianbol@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for ovs metering with tc offload | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | fail | test: fail |
On 3 May 2022, at 5:08, Jianbo Liu via dev wrote: > As the police actions with indexes of 0x10000000-0x1fffffff are > reserved for meter offload, to provide a clean environment for OVS, > these reserved police actions should be deleted on startup. So dump > all the police actions, delete those actions with indexes in this > range. > > Signed-off-by: Jianbo Liu <jianbol@nvidia.com> > --- > lib/netdev-offload-tc.c | 75 +++++++++++++++++++++++++++++++++++++++++ > lib/tc.c | 61 +++++++++++++++++++++++++++++++++ > lib/tc.h | 2 ++ > 3 files changed, 138 insertions(+) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index b3c60c125..bded4bc8c 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -69,6 +69,11 @@ struct meter_id_to_police_idx_data { > uint32_t police_idx; > }; > > +struct policer_node { > + struct hmap_node node; > + uint32_t police_idx; > +}; > + > #define METER_POLICE_IDS_BASE 0x10000000 > #define METER_POLICE_IDS_MAX 0x1FFFFFFF > /* Protects below meter ids pool and hashmaps. */ > @@ -2252,6 +2257,74 @@ probe_tc_block_support(int ifindex) > } > } > > +static int > +tc_get_policer_action_ids(struct hmap *map) > +{ > + uint32_t police_idx[TCA_ACT_MAX_PRIO] = {}; > + struct policer_node *policer_node; > + struct netdev_flow_dump *dump; > + struct ofpbuf rbuffer, reply; > + size_t hash; > + int i, err; > + > + dump = xzalloc(sizeof *dump); > + dump->nl_dump = xzalloc(sizeof *dump->nl_dump); > + > + ofpbuf_init(&rbuffer, NL_DUMP_BUFSIZE); > + tc_dump_tc_action_start("police", dump->nl_dump); > + > + while (nl_dump_next(dump->nl_dump, &reply, &rbuffer)) { > + if (parse_netlink_to_tc_policer(&reply, police_idx)) { > + continue; > + } > + > + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { > + if (!police_idx[i]) { > + break; > + } > + policer_node = xzalloc(sizeof *policer_node); > + policer_node->police_idx = police_idx[i]; > + hash = hash_int(police_idx[i], 0); > + hmap_insert(map, &policer_node->node, hash); > + } > + memset(police_idx, 0, TCA_ACT_MAX_PRIO * sizeof(uint32_t)); > + } > + > + err = nl_dump_done(dump->nl_dump); > + ofpbuf_uninit(&rbuffer); > + free(dump->nl_dump); > + free(dump); > + > + return err; > +} > + > +static void > +tc_cleanup_policer_action(struct id_pool *police_ids, Should the function name not be tc_cleanup_policer_actions(), as it cleans up all configured actions? > + uint32_t id_min, uint32_t id_max) > +{ > + struct policer_node *policer_node; > + uint32_t police_idx; > + struct hmap map; > + int err; > + > + hmap_init(&map); > + tc_get_policer_action_ids(&map); > + > + HMAP_FOR_EACH_POP (policer_node, node, &map) { > + police_idx = policer_node->police_idx; > + if (police_idx >= id_min && police_idx <= id_max) { > + err = tc_del_policer_action(police_idx, NULL); > + if (err) { Are we ok to do this on all errors? Maybe ignore the error if the action can not be found, as it might have been cleaned up in the meanwhile? > + /* don't use this police any more */ Comments should start with a capital and end with a dot. > + id_pool_add(police_ids, police_idx); > + } > + } > + free(policer_node); > + } > + > + hmap_destroy(&map); > +} > + > static int > netdev_tc_init_flow_api(struct netdev *netdev) > { > @@ -2304,6 +2377,8 @@ netdev_tc_init_flow_api(struct netdev *netdev) > > meter_police_ids = id_pool_create(METER_POLICE_IDS_BASE, > METER_POLICE_IDS_MAX - METER_POLICE_IDS_BASE + 1); > + tc_cleanup_policer_action(meter_police_ids, METER_POLICE_IDS_BASE, > + METER_POLICE_IDS_MAX); > > ovsthread_once_done(&once); > } > diff --git a/lib/tc.c b/lib/tc.c > index ee16364ea..16d4ae3da 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -2067,6 +2067,67 @@ tc_dump_tc_chain_start(struct tcf_id *id, struct nl_dump *dump) > return 0; > } > > +int > +tc_dump_tc_action_start(char *name, struct nl_dump *dump) > +{ > + size_t offset, root_offset; > + struct ofpbuf request; > + uint32_t prio = 0; > + > + tc_make_action_request(RTM_GETACTION, NLM_F_DUMP, &request); > + root_offset = nl_msg_start_nested(&request, TCA_ACT_TAB); > + offset = nl_msg_start_nested(&request, ++prio); Same as earlier, why prio++? > + nl_msg_put_string(&request, TCA_ACT_KIND, name); > + nl_msg_end_nested(&request, offset); > + nl_msg_end_nested(&request, root_offset); > + > + nl_dump_start(dump, NETLINK_ROUTE, &request); > + ofpbuf_uninit(&request); > + > + return 0; > +} > + > +int > +parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]) > +{ > + static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO] = {}; > + struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; > + const int max_size = ARRAY_SIZE(actions_orders_policy); > + const struct nlattr *actions; > + struct tc_flower flower; > + struct tcamsg *tca; > + int i, cnt = 0; > + int err; > + > + for (i = 0; i < max_size; i++) { > + actions_orders_policy[i].type = NL_A_NESTED; > + actions_orders_policy[i].optional = true; > + } > + > + tca = ofpbuf_at_assert(reply, NLMSG_HDRLEN, sizeof *tca); > + actions = nl_attr_find(reply, NLMSG_HDRLEN + sizeof *tca, TCA_ACT_TAB); > + if (!actions || !nl_parse_nested(actions, actions_orders_policy, > + actions_orders, max_size)) { > + VLOG_ERR_RL(&error_rl, "failed to parse police actions"); "Failed..." > + return EPROTO; > + } > + > + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { > + if (actions_orders[i]) { > + memset(&flower, 0, sizeof(struct tc_flower)); > + err = tc_parse_single_action(actions_orders[i], &flower, false); > + if (err) { > + continue; > + } > + if (flower.actions[0].police.index) { This could be any kind of action, should we not explicitly check for policy and skip all other ones? > + police_idx[cnt++] = flower.actions[0].police.index; > + } > + } > + } > + > + return 0; > +} > + > int > tc_del_filter(struct tcf_id *id) > { > diff --git a/lib/tc.h b/lib/tc.h > index 96b9e6ccc..f6d1ed91c 100644 > --- a/lib/tc.h > +++ b/lib/tc.h > @@ -382,5 +382,7 @@ int parse_netlink_to_tc_chain(struct ofpbuf *reply, uint32_t *chain); > void tc_set_policy(const char *policy); > int tc_parse_single_action(struct nlattr *action, struct tc_flower *flower, > bool terse); > +int tc_dump_tc_action_start(char *name, struct nl_dump *dump); > +int parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]); > > #endif /* tc.h */ > -- > 2.26.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index b3c60c125..bded4bc8c 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -69,6 +69,11 @@ struct meter_id_to_police_idx_data { uint32_t police_idx; }; +struct policer_node { + struct hmap_node node; + uint32_t police_idx; +}; + #define METER_POLICE_IDS_BASE 0x10000000 #define METER_POLICE_IDS_MAX 0x1FFFFFFF /* Protects below meter ids pool and hashmaps. */ @@ -2252,6 +2257,74 @@ probe_tc_block_support(int ifindex) } } +static int +tc_get_policer_action_ids(struct hmap *map) +{ + uint32_t police_idx[TCA_ACT_MAX_PRIO] = {}; + struct policer_node *policer_node; + struct netdev_flow_dump *dump; + struct ofpbuf rbuffer, reply; + size_t hash; + int i, err; + + dump = xzalloc(sizeof *dump); + dump->nl_dump = xzalloc(sizeof *dump->nl_dump); + + ofpbuf_init(&rbuffer, NL_DUMP_BUFSIZE); + tc_dump_tc_action_start("police", dump->nl_dump); + + while (nl_dump_next(dump->nl_dump, &reply, &rbuffer)) { + if (parse_netlink_to_tc_policer(&reply, police_idx)) { + continue; + } + + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { + if (!police_idx[i]) { + break; + } + policer_node = xzalloc(sizeof *policer_node); + policer_node->police_idx = police_idx[i]; + hash = hash_int(police_idx[i], 0); + hmap_insert(map, &policer_node->node, hash); + } + memset(police_idx, 0, TCA_ACT_MAX_PRIO * sizeof(uint32_t)); + } + + err = nl_dump_done(dump->nl_dump); + ofpbuf_uninit(&rbuffer); + free(dump->nl_dump); + free(dump); + + return err; +} + +static void +tc_cleanup_policer_action(struct id_pool *police_ids, + uint32_t id_min, uint32_t id_max) +{ + struct policer_node *policer_node; + uint32_t police_idx; + struct hmap map; + int err; + + hmap_init(&map); + tc_get_policer_action_ids(&map); + + HMAP_FOR_EACH_POP (policer_node, node, &map) { + police_idx = policer_node->police_idx; + if (police_idx >= id_min && police_idx <= id_max) { + err = tc_del_policer_action(police_idx, NULL); + if (err) { + /* don't use this police any more */ + id_pool_add(police_ids, police_idx); + } + } + free(policer_node); + } + + hmap_destroy(&map); +} + static int netdev_tc_init_flow_api(struct netdev *netdev) { @@ -2304,6 +2377,8 @@ netdev_tc_init_flow_api(struct netdev *netdev) meter_police_ids = id_pool_create(METER_POLICE_IDS_BASE, METER_POLICE_IDS_MAX - METER_POLICE_IDS_BASE + 1); + tc_cleanup_policer_action(meter_police_ids, METER_POLICE_IDS_BASE, + METER_POLICE_IDS_MAX); ovsthread_once_done(&once); } diff --git a/lib/tc.c b/lib/tc.c index ee16364ea..16d4ae3da 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -2067,6 +2067,67 @@ tc_dump_tc_chain_start(struct tcf_id *id, struct nl_dump *dump) return 0; } +int +tc_dump_tc_action_start(char *name, struct nl_dump *dump) +{ + size_t offset, root_offset; + struct ofpbuf request; + uint32_t prio = 0; + + tc_make_action_request(RTM_GETACTION, NLM_F_DUMP, &request); + root_offset = nl_msg_start_nested(&request, TCA_ACT_TAB); + offset = nl_msg_start_nested(&request, ++prio); + nl_msg_put_string(&request, TCA_ACT_KIND, name); + nl_msg_end_nested(&request, offset); + nl_msg_end_nested(&request, root_offset); + + nl_dump_start(dump, NETLINK_ROUTE, &request); + ofpbuf_uninit(&request); + + return 0; +} + +int +parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]) +{ + static struct nl_policy actions_orders_policy[TCA_ACT_MAX_PRIO] = {}; + struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; + const int max_size = ARRAY_SIZE(actions_orders_policy); + const struct nlattr *actions; + struct tc_flower flower; + struct tcamsg *tca; + int i, cnt = 0; + int err; + + for (i = 0; i < max_size; i++) { + actions_orders_policy[i].type = NL_A_NESTED; + actions_orders_policy[i].optional = true; + } + + tca = ofpbuf_at_assert(reply, NLMSG_HDRLEN, sizeof *tca); + actions = nl_attr_find(reply, NLMSG_HDRLEN + sizeof *tca, TCA_ACT_TAB); + if (!actions || !nl_parse_nested(actions, actions_orders_policy, + actions_orders, max_size)) { + VLOG_ERR_RL(&error_rl, "failed to parse police actions"); + return EPROTO; + } + + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { + if (actions_orders[i]) { + memset(&flower, 0, sizeof(struct tc_flower)); + err = tc_parse_single_action(actions_orders[i], &flower, false); + if (err) { + continue; + } + if (flower.actions[0].police.index) { + police_idx[cnt++] = flower.actions[0].police.index; + } + } + } + + return 0; +} + int tc_del_filter(struct tcf_id *id) { diff --git a/lib/tc.h b/lib/tc.h index 96b9e6ccc..f6d1ed91c 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -382,5 +382,7 @@ int parse_netlink_to_tc_chain(struct ofpbuf *reply, uint32_t *chain); void tc_set_policy(const char *policy); int tc_parse_single_action(struct nlattr *action, struct tc_flower *flower, bool terse); +int tc_dump_tc_action_start(char *name, struct nl_dump *dump); +int parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]); #endif /* tc.h */
As the police actions with indexes of 0x10000000-0x1fffffff are reserved for meter offload, to provide a clean environment for OVS, these reserved police actions should be deleted on startup. So dump all the police actions, delete those actions with indexes in this range. Signed-off-by: Jianbo Liu <jianbol@nvidia.com> --- lib/netdev-offload-tc.c | 75 +++++++++++++++++++++++++++++++++++++++++ lib/tc.c | 61 +++++++++++++++++++++++++++++++++ lib/tc.h | 2 ++ 3 files changed, 138 insertions(+)