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 <[email protected]>
> ---
> 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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
