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