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