Message ID | 20220527090021.22594-7-jianbol@nvidia.com |
---|---|
State | Superseded |
Headers | show |
Series | Add support for ovs metering with tc offload | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | warning | apply and check: warning |
ovsrobot/intel-ovs-compilation | success | test: success |
On 27 May 2022, at 11:00, Jianbo Liu 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 | 60 +++++++++++++++++++++++++++++++++ > lib/tc.h | 2 ++ > 3 files changed, 137 insertions(+) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index c88fb6a37..7e5f04bdf 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -69,6 +69,11 @@ struct meter_police_mapping_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 police ids pool. */ > @@ -2254,6 +2259,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] = {}; Maybe not initialize it here, but to the explicit memset, to avoid the extra memset() at the end? > + 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)) { memset(police_idx, 0, sizeof police_idx); In addition use sizeof policy_idx, instead of TCA_ACT_MAX_PRIO * sizeof(uint32_t) > + 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)); Remove memset above, see an earlier comment. > + } > + > + err = nl_dump_done(dump->nl_dump); > + ofpbuf_uninit(&rbuffer); > + free(dump->nl_dump); > + free(dump); > + > + return err; > +} > + > +static void > +tc_cleanup_policer_actions(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 && err != ENOENT) { > + /* Don't use this police any more. */ > + id_pool_add(police_ids, police_idx); > + } > + } There is no log message when this happens, and I think this should be logged. Maybe a general warning level one with the number of IDs we could not re-user, and some debug level messages with the actual ID? What about the following? @@ -2326,6 +2327,7 @@ tc_cleanup_policer_actions(struct id_pool *police_ids, uint32_t id_min, uint32_t id_max) { struct policer_node *policer_node; + unsigned int unusable_ids = 0; uint32_t police_idx; struct hmap map; int err; @@ -2340,11 +2342,19 @@ tc_cleanup_policer_actions(struct id_pool *police_ids, if (err && err != ENOENT) { /* Don't use this police any more. */ id_pool_add(police_ids, police_idx); + + unusable_ids++; + VLOG_DBG("Policer index %u could not be freed for OVS, " + "error %d", police_idx, err); } } free(policer_node); } + if (unusable_ids) { + VLOG_WARN("Full policer index pool allocation failed, " + "%u indexes are unavailable", unusable_ids); + } hmap_destroy(&map); } > + free(policer_node); > + } > + > + hmap_destroy(&map); > +} > + > static int > netdev_tc_init_flow_api(struct netdev *netdev) > { > @@ -2307,6 +2380,8 @@ netdev_tc_init_flow_api(struct netdev *netdev) > ovs_mutex_lock(&meter_police_ids_mutex); > meter_police_ids = id_pool_create(METER_POLICE_IDS_BASE, > METER_POLICE_IDS_MAX - METER_POLICE_IDS_BASE + 1); > + tc_cleanup_policer_actions(meter_police_ids, METER_POLICE_IDS_BASE, > + METER_POLICE_IDS_MAX); > ovs_mutex_unlock(&meter_police_ids_mutex); > > ovsthread_once_done(&once); > diff --git a/lib/tc.c b/lib/tc.c > index 77d0c9de7..ac53c56db 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -2087,6 +2087,66 @@ 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; > + > + 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, 1); > + 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"); These could be all kinds of actions, not only police ones, i.e. whatever is programmed. Maybe change this to "Failed to parse flower actions". > + return EPROTO; > + } > + > + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { > + if (actions_orders[i]) { > + memset(&flower, 0, sizeof(struct tc_flower)); > + err = nl_parse_single_action(actions_orders[i], &flower, false); > + if (err) { > + continue; > + } > + if (flower.actions[0].police.index) { You need to make sure this is the Police action (see the previous review), i.e., if (flower.action[0].type == TC_ACT_POLICE && 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 30cd2f7ff..4690dce4f 100644 > --- a/lib/tc.h > +++ b/lib/tc.h > @@ -382,5 +382,7 @@ int tc_parse_action_stats(struct nlattr *action, > struct ovs_flow_stats *stats_sw, > struct ovs_flow_stats *stats_hw, > struct ovs_flow_stats *stats_dropped); > +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
On Mon, 2022-06-20 at 12:16 +0200, Eelco Chaudron wrote: > On 27 May 2022, at 11:00, Jianbo Liu 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 | 60 +++++++++++++++++++++++++++++++++ > > lib/tc.h | 2 ++ > > 3 files changed, 137 insertions(+) > > > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > > index c88fb6a37..7e5f04bdf 100644 > > --- a/lib/netdev-offload-tc.c > > +++ b/lib/netdev-offload-tc.c > > @@ -69,6 +69,11 @@ struct meter_police_mapping_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 police ids pool. */ > > @@ -2254,6 +2259,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] = {}; > > Maybe not initialize it here, but to the explicit memset, to avoid > the extra memset() at the end? > OK, I will change. > > + 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)) { > > memset(police_idx, 0, sizeof police_idx); > > In addition use sizeof policy_idx, instead of TCA_ACT_MAX_PRIO * > sizeof(uint32_t) > > > + 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)); > > Remove memset above, see an earlier comment. > > > + } > > + > > + err = nl_dump_done(dump->nl_dump); > > + ofpbuf_uninit(&rbuffer); > > + free(dump->nl_dump); > > + free(dump); > > + > > + return err; > > +} > > + > > +static void > > +tc_cleanup_policer_actions(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 && err != ENOENT) { > > + /* Don't use this police any more. */ > > + id_pool_add(police_ids, police_idx); > > + } > > + } > > There is no log message when this happens, and I think this should be > logged. Maybe a general warning level one with the number of IDs we > could not re-user, and some debug level messages with the actual ID? > > What about the following? Good idea. Thanks! > > @@ -2326,6 +2327,7 @@ tc_cleanup_policer_actions(struct id_pool > *police_ids, > uint32_t id_min, uint32_t id_max) > { > struct policer_node *policer_node; > + unsigned int unusable_ids = 0; > uint32_t police_idx; > struct hmap map; > int err; > @@ -2340,11 +2342,19 @@ tc_cleanup_policer_actions(struct id_pool > *police_ids, > if (err && err != ENOENT) { > /* Don't use this police any more. */ > id_pool_add(police_ids, police_idx); > + > + unusable_ids++; > + VLOG_DBG("Policer index %u could not be freed for > OVS, " > + "error %d", police_idx, err); > } > } > free(policer_node); > } > > + if (unusable_ids) { > + VLOG_WARN("Full policer index pool allocation failed, " > + "%u indexes are unavailable", unusable_ids); > + } > hmap_destroy(&map); > } > > > > + free(policer_node); > > + } > > + > > + hmap_destroy(&map); > > +} > > + > > static int > > netdev_tc_init_flow_api(struct netdev *netdev) > > { > > @@ -2307,6 +2380,8 @@ netdev_tc_init_flow_api(struct netdev > > *netdev) > > ovs_mutex_lock(&meter_police_ids_mutex); > > meter_police_ids = id_pool_create(METER_POLICE_IDS_BASE, > > METER_POLICE_IDS_MAX - > > METER_POLICE_IDS_BASE + 1); > > + tc_cleanup_policer_actions(meter_police_ids, > > METER_POLICE_IDS_BASE, > > + METER_POLICE_IDS_MAX); > > ovs_mutex_unlock(&meter_police_ids_mutex); > > > > ovsthread_once_done(&once); > > diff --git a/lib/tc.c b/lib/tc.c > > index 77d0c9de7..ac53c56db 100644 > > --- a/lib/tc.c > > +++ b/lib/tc.c > > @@ -2087,6 +2087,66 @@ 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; > > + > > + 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, 1); > > + 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"); > > These could be all kinds of actions, not only police ones, i.e. > whatever is programmed. > Maybe change this to "Failed to parse flower actions". > But this function is intended for police only, and its name is ended with "_policer". Do you want to make it generel func for all actions? > > + return EPROTO; > > + } > > + > > + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { > > + if (actions_orders[i]) { > > + memset(&flower, 0, sizeof(struct tc_flower)); > > + err = nl_parse_single_action(actions_orders[i], > > &flower, false); > > + if (err) { > > + continue; > > + } > > + if (flower.actions[0].police.index) { > > You need to make sure this is the Police action (see the previous > review), i.e., > I remember your comment in previous version. Sorry I didn't change here becasue the same reason above. > if (flower.action[0].type == TC_ACT_POLICE > && 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 30cd2f7ff..4690dce4f 100644 > > --- a/lib/tc.h > > +++ b/lib/tc.h > > @@ -382,5 +382,7 @@ int tc_parse_action_stats(struct nlattr > > *action, > > struct ovs_flow_stats *stats_sw, > > struct ovs_flow_stats *stats_hw, > > struct ovs_flow_stats *stats_dropped); > > +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 >
On 21 Jun 2022, at 5:51, Jianbo Liu wrote: > On Mon, 2022-06-20 at 12:16 +0200, Eelco Chaudron wrote: >> On 27 May 2022, at 11:00, Jianbo Liu 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> >>> --- <SNIP> >>> +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"); >> >> These could be all kinds of actions, not only police ones, i.e. >> whatever is programmed. >> Maybe change this to "Failed to parse flower actions". >> > > But this function is intended for police only, and its name is ended > with "_policer". Do you want to make it generel func for all actions? As you could receive any set of actions from netlink (all that exists in the system), the error might not be specific to a police action, that’s why I think it should be removed. >>> + return EPROTO; >>> + } >>> + >>> + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { >>> + if (actions_orders[i]) { >>> + memset(&flower, 0, sizeof(struct tc_flower)); >>> + err = nl_parse_single_action(actions_orders[i], >>> &flower, false); >>> + if (err) { >>> + continue; >>> + } >>> + if (flower.actions[0].police.index) { >> >> You need to make sure this is the Police action (see the previous >> review), i.e., >> > > I remember your comment in previous version. Sorry I didn't change here > becasue the same reason above. Let me try to be more clear. Whatever you receive from netlink could be any kind of action, i.e. whatever is programmed in the system. So if someone manually added add list of actions, in the ID range, you think it’s Police but it could be something else. What about the following for the two comments above: --- a/lib/tc.c +++ b/lib/tc.c @@ -2127,7 +2127,8 @@ parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]) 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"); + VLOG_ERR_RL(&error_rl, + "Failed to parse actions in netlink to policer"); return EPROTO; } @@ -2135,7 +2136,7 @@ parse_netlink_to_tc_policer(struct ofpbuf *reply, uint32_t police_idx[]) if (actions_orders[i]) { memset(&flower, 0, sizeof(struct tc_flower)); err = nl_parse_single_action(actions_orders[i], &flower, false); - if (err) { + if (err || flower.actions[0].type != TC_ACT_POLICE) { continue; } if (flower.actions[0].police.index) {
On Mon, 2022-06-27 at 11:17 +0200, Eelco Chaudron wrote: > > > On 21 Jun 2022, at 5:51, Jianbo Liu wrote: > > > On Mon, 2022-06-20 at 12:16 +0200, Eelco Chaudron wrote: > > > On 27 May 2022, at 11:00, Jianbo Liu 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> > > > > --- > > <SNIP> > > > > > +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"); > > > > > > These could be all kinds of actions, not only police ones, i.e. > > > whatever is programmed. > > > Maybe change this to "Failed to parse flower actions". > > > > > > > But this function is intended for police only, and its name is > > ended > > with "_policer". Do you want to make it generel func for all > > actions? > > As you could receive any set of actions from netlink (all that exists > in the system), the error might not be specific to a police action, > that’s why I think it should be removed. > > > > > + return EPROTO; > > > > + } > > > > + > > > > + for (i = 0; i < TCA_ACT_MAX_PRIO; i++) { > > > > + if (actions_orders[i]) { > > > > + memset(&flower, 0, sizeof(struct tc_flower)); > > > > + err = nl_parse_single_action(actions_orders[i], > > > > &flower, false); > > > > + if (err) { > > > > + continue; > > > > + } > > > > + if (flower.actions[0].police.index) { > > > > > > You need to make sure this is the Police action (see the previous > > > review), i.e., > > > > > > > I remember your comment in previous version. Sorry I didn't change > > here > > becasue the same reason above. > > Let me try to be more clear. Whatever you receive from netlink could > be any kind of action, i.e. whatever is programmed in the system. > So if someone manually added add list of actions, in the ID range, > you think it’s Police but it could be something else. > > What about the following for the two comments above: I don't know if someone will use these functions. But currently they are only for police, as I dump police actions by RTM_GETACTION request. Anyway, I will update as you suggested. > > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -2127,7 +2127,8 @@ parse_netlink_to_tc_policer(struct ofpbuf > *reply, uint32_t police_idx[]) > 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"); > + VLOG_ERR_RL(&error_rl, > + "Failed to parse actions in netlink to > policer"); > return EPROTO; > } > > @@ -2135,7 +2136,7 @@ parse_netlink_to_tc_policer(struct ofpbuf > *reply, uint32_t police_idx[]) > if (actions_orders[i]) { > memset(&flower, 0, sizeof(struct tc_flower)); > err = nl_parse_single_action(actions_orders[i], &flower, > false); > - if (err) { > + if (err || flower.actions[0].type != TC_ACT_POLICE) { > continue; > } > if (flower.actions[0].police.index) { >
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index c88fb6a37..7e5f04bdf 100644 --- a/lib/netdev-offload-tc.c +++ b/lib/netdev-offload-tc.c @@ -69,6 +69,11 @@ struct meter_police_mapping_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 police ids pool. */ @@ -2254,6 +2259,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_actions(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 && err != ENOENT) { + /* 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) { @@ -2307,6 +2380,8 @@ netdev_tc_init_flow_api(struct netdev *netdev) ovs_mutex_lock(&meter_police_ids_mutex); meter_police_ids = id_pool_create(METER_POLICE_IDS_BASE, METER_POLICE_IDS_MAX - METER_POLICE_IDS_BASE + 1); + tc_cleanup_policer_actions(meter_police_ids, METER_POLICE_IDS_BASE, + METER_POLICE_IDS_MAX); ovs_mutex_unlock(&meter_police_ids_mutex); ovsthread_once_done(&once); diff --git a/lib/tc.c b/lib/tc.c index 77d0c9de7..ac53c56db 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -2087,6 +2087,66 @@ 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; + + 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, 1); + 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 = nl_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 30cd2f7ff..4690dce4f 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -382,5 +382,7 @@ int tc_parse_action_stats(struct nlattr *action, struct ovs_flow_stats *stats_sw, struct ovs_flow_stats *stats_hw, struct ovs_flow_stats *stats_dropped); +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 | 60 +++++++++++++++++++++++++++++++++ lib/tc.h | 2 ++ 3 files changed, 137 insertions(+)