From: Baowen Zheng <[email protected]> When meter deleted the deletion of the corresponding TC police action, used to facilitate offload via OVS-TC, may fail. This is occurs when the TC police action instance still in use by a TC flower classifiers (flows added to the TC datapath).
In this commit, i add a revalidate process to clean the police action that offloaded relatewith a meter. Signed-off-by: Baowen Zheng <[email protected]> Signed-off-by: Tianyu Yuan <[email protected]> Signed-off-by: Simon Horman <[email protected]> --- lib/dpif-netdev.c | 1 + lib/dpif-netlink.c | 172 ++++++++++++++++++++++++++++++++++ lib/dpif-provider.h | 4 + lib/dpif.c | 6 ++ lib/id-pool.c | 6 ++ lib/id-pool.h | 1 + lib/tc.c | 6 -- lib/tc.h | 7 ++ ofproto/ofproto-dpif-upcall.c | 1 + ofproto/ofproto-dpif.h | 2 + 10 files changed, 200 insertions(+), 6 deletions(-) diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index bf4de3cf4..0c0a0249e 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -8809,6 +8809,7 @@ const struct dpif_class dpif_netdev_class = { dpif_netdev_meter_set, dpif_netdev_meter_get, dpif_netdev_meter_del, + NULL, /* meter_revalidate */ dpif_netdev_bond_add, dpif_netdev_bond_del, dpif_netdev_bond_stats_get, diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index d270dbbd2..aff78abcc 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -49,6 +49,7 @@ #include "netlink.h" #include "netnsid.h" #include "odp-util.h" +#include "ofproto/ofproto-dpif.h" #include "openvswitch/dynamic-string.h" #include "openvswitch/flow.h" #include "openvswitch/hmap.h" @@ -4035,6 +4036,12 @@ dpif_netlink_meter_transact(struct ofpbuf *request, struct ofpbuf **replyp, return 0; } +static const struct nl_policy police_policy[] = { + [TCA_POLICE_TBF] = { .type = NL_A_UNSPEC, + .min_len = sizeof(struct tc_police), + .optional = false, }, +}; + static void dpif_netlink_meter_get_features(const struct dpif *dpif_, struct ofputil_meter_features *features) @@ -4294,6 +4301,170 @@ dpif_netlink_meter_set(struct dpif *dpif_, ofproto_meter_id meter_id, return dpif_netlink_meter_set__(dpif_, meter_id, add, config); } +static const struct nl_policy tca_root_policy[] = { + [TCA_ACT_TAB] = { .type = NL_A_NESTED, .optional = false }, + [TCA_ROOT_COUNT] = { .type = NL_A_U32, .optional = false }, +}; + +static const struct nl_policy act_policy[] = { + [TCA_ACT_KIND] = { .type = NL_A_STRING, .optional = false, }, + [TCA_ACT_COOKIE] = { .type = NL_A_UNSPEC, .optional = true, }, + [TCA_ACT_OPTIONS] = { .type = NL_A_NESTED, .optional = true, }, + [TCA_ACT_STATS] = { .type = NL_A_NESTED, .optional = false, }, +}; + +static bool dpif_netlink_meter_should_revalidate(struct dpif_backer *backer, + uint32_t meter_id) +{ + return !id_pool_id_exist(backer->meter_ids, meter_id); +} + +static void +dpif_tc_meter_revalidate(struct dpif *dpif_ OVS_UNUSED, + struct dpif_backer *backer, struct ofpbuf *reply) +{ + static struct nl_policy actions_orders_policy[ACT_MAX_NUM + 1] = {}; + struct nlattr *actions_orders[ARRAY_SIZE(actions_orders_policy)]; + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); + struct nlattr *action_root_attrs[ARRAY_SIZE(tca_root_policy)]; + struct nlattr *action_police_tab[ARRAY_SIZE(police_policy)]; + struct nlattr *action_police_attrs[ARRAY_SIZE(act_policy)]; + const int max_size = ARRAY_SIZE(actions_orders_policy); + const struct tc_police *tc_police = NULL; + struct ofputil_meter_stats stats; + ofproto_meter_id meter_id; + size_t revalidate_num; + size_t act_count; + uint32_t index; + int i; + + if (!reply) { + VLOG_ERR_RL(&rl, "get null reply message when meter revalidate \n"); + return; + } + + if (!nl_policy_parse(reply, NLMSG_HDRLEN + sizeof(struct tcamsg), + tca_root_policy, action_root_attrs, + ARRAY_SIZE(action_root_attrs))) { + VLOG_ERR_RL(&rl, "failed to parse reply message when meter " + "revalidate \n"); + return; + } + + act_count = nl_attr_get_u32(action_root_attrs[TCA_ROOT_COUNT]); + if (!act_count) { + VLOG_ERR_RL(&rl, "there is no police action returned in message when " + "meter revalidate \n"); + return; + } + + for (i = 0; i < max_size; i++) { + actions_orders_policy[i].type = NL_A_NESTED; + actions_orders_policy[i].optional = true; + } + + revalidate_num = act_count > ACT_MAX_NUM ? + (ACT_MAX_NUM + 1) : (act_count + 1); + + if (!nl_parse_nested(action_root_attrs[TCA_ACT_TAB], actions_orders_policy, + actions_orders, revalidate_num) ) { + VLOG_ERR_RL(&rl, "failed to parse TCA_ACT_TAB when meter revalidate " + "for act_count %lu", act_count); + return; + } + + for (i = 0; i < revalidate_num; i++) { + if (!actions_orders[i]) { + continue; + } + + if (!nl_parse_nested(actions_orders[i], act_policy, + action_police_attrs, ARRAY_SIZE(act_policy))) { + VLOG_ERR_RL(&rl, "failed to parse police action when meter " + "revalidate\n"); + return; + } + if (strcmp(nl_attr_get_string(action_police_attrs[TCA_KIND]), + "police")) { + VLOG_EMER("get none police action when meter revalidate\n"); + continue; + } + if (!nl_parse_nested(action_police_attrs[TCA_ACT_OPTIONS], + police_policy, action_police_tab, + ARRAY_SIZE(action_police_tab))) { + VLOG_ERR_RL(&rl, "failed to parse the single police action when " + "meter revalidate\n"); + return; + } + tc_police = nl_attr_get_unspec(action_police_tab[TCA_POLICE_TBF], + sizeof *tc_police); + if (!tc_police) { + VLOG_ERR_RL(&rl, "can not get police struct in police order %u " + "when meter revalidate\n", i); + continue; + } + index = tc_police->index; + if (UNLIKELY_METER_ACTION(index)) { + continue; + } + + index = POLICY_INDEX_TO_METER_ID(index); + if (dpif_netlink_meter_should_revalidate(backer, index)) { + meter_id.uint32 = index; + VLOG_ERR_RL(&rl, "revalidate the meter id %u for police index " + "%08x\n", index, tc_police->index); + dpif_netlink_meter_del_police(meter_id, &stats, 1); + } + } +} + +static void +dpif_netlink_meter_revalidate__(struct dpif *dpif_ OVS_UNUSED, + struct dpif_backer *backer) +{ + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 20); + struct nla_bitfield32 dump_flags = { TCA_DUMP_FLAGS_TERSE, + TCA_DUMP_FLAGS_TERSE }; + struct ofpbuf request; + struct ofpbuf *reply; + struct tcamsg *tcmsg; + size_t total_offset; + size_t act_offset; + int prio = 0; + int error; + + if (!netdev_is_flow_api_enabled()) { + return; + } + tcmsg = tc_act_make_request(RTM_GETACTION, NLM_F_DUMP, &request); + if (!tcmsg) { + return; + } + dpif_netlink_police_start_nested(&request, &prio, &total_offset, + &act_offset); + nl_msg_put_string(&request, TCA_KIND, "police"); + dpif_netlink_police_end_nested(&request, &total_offset, &act_offset); + nl_msg_put_unspec(&request, TCA_ROOT_FLAGS, &dump_flags, + sizeof dump_flags); + error = tc_transact(&request, &reply); + if (error) { + VLOG_ERR_RL(&rl, "failed to send dump netlink msg for revalidate " + "error %d\n", error); + return; + } + dpif_tc_meter_revalidate(dpif_, backer, reply); + ofpbuf_delete(reply); +} + +static void +dpif_netlink_meter_revalidate(struct dpif *dpif_, struct dpif_backer *backer) +{ + if (probe_broken_meters(dpif_)) { + return; + } + dpif_netlink_meter_revalidate__(dpif_, backer); +} + /* Retrieve statistics and/or delete meter 'meter_id'. Statistics are * stored in 'stats', if it is not null. If 'command' is * OVS_METER_CMD_DEL, the meter is deleted and statistics are optionally @@ -4617,6 +4788,7 @@ const struct dpif_class dpif_netlink_class = { dpif_netlink_meter_set, dpif_netlink_meter_get, dpif_netlink_meter_del, + dpif_netlink_meter_revalidate, NULL, /* bond_add */ NULL, /* bond_del */ NULL, /* bond_stats_get */ diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index 0d092de9b..f3893b2d1 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -25,6 +25,7 @@ #include "openflow/openflow.h" #include "dpif.h" #include "util.h" +#include "ofproto/ofproto-dpif.h" #ifdef __cplusplus extern "C" { @@ -624,6 +625,9 @@ struct dpif_class { int (*meter_del)(struct dpif *, ofproto_meter_id meter_id, struct ofputil_meter_stats *, uint16_t n_bands); + /* revalidate meter entries in offload cases */ + void (*meter_revalidate)(struct dpif *, struct dpif_backer *); + /* Adds a bond with 'bond_id' and the member-map to 'dpif'. */ int (*bond_add)(struct dpif *dpif, uint32_t bond_id, odp_port_t *member_map); diff --git a/lib/dpif.c b/lib/dpif.c index 3d4eae1a6..984b20fef 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -2090,3 +2090,9 @@ dpif_cache_set_size(struct dpif *dpif, uint32_t level, uint32_t size) ? dpif->dpif_class->cache_set_size(dpif, level, size) : EOPNOTSUPP; } + +void +dpif_meter_revalidate(struct dpif *dpif, struct dpif_backer *backer) +{ + dpif->dpif_class->meter_revalidate(dpif, backer); +} diff --git a/lib/id-pool.c b/lib/id-pool.c index 69910ad08..50999a096 100644 --- a/lib/id-pool.c +++ b/lib/id-pool.c @@ -155,3 +155,9 @@ id_pool_free_id(struct id_pool *pool, uint32_t id) } } } + +bool +id_pool_id_exist(struct id_pool *pool, uint32_t id) +{ + return !!id_pool_find(pool, id); +} diff --git a/lib/id-pool.h b/lib/id-pool.h index 8721f8793..08fba6d43 100644 --- a/lib/id-pool.h +++ b/lib/id-pool.h @@ -29,6 +29,7 @@ void id_pool_destroy(struct id_pool *); bool id_pool_alloc_id(struct id_pool *, uint32_t *id); void id_pool_free_id(struct id_pool *, uint32_t id); void id_pool_add(struct id_pool *, uint32_t id); +bool id_pool_id_exist(struct id_pool *pool, uint32_t id); /* * ID pool. diff --git a/lib/tc.c b/lib/tc.c index 3993498b0..5d89560f8 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -51,10 +51,6 @@ #define TCM_IFINDEX_MAGIC_BLOCK (0xFFFFFFFFU) #endif -#ifndef TCA_DUMP_FLAGS_TERSE -#define TCA_DUMP_FLAGS_TERSE (1 << 0) -#endif - #if TCA_MAX < 15 #define TCA_CHAIN 11 #define TCA_INGRESS_BLOCK 13 @@ -1821,8 +1817,6 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, return 0; } -#define TCA_ACT_MIN_PRIO 1 - static int nl_parse_flower_actions(struct nlattr **attrs, struct tc_flower *flower, bool terse) diff --git a/lib/tc.h b/lib/tc.h index 0f7de8677..5b1c934d8 100644 --- a/lib/tc.h +++ b/lib/tc.h @@ -58,6 +58,8 @@ enum tc_flower_reserved_prio { #define METER_ID_TO_POLICY_INDEX(meter_id) 0xff << 24 | (meter_id + 1) << 8 /* Mapping policy_index to meter_id */ #define POLICY_INDEX_TO_METER_ID(index) (((index >> 8) & 0xffff) - 1) +/* Ckeck if given policy action is meter action*/ +#define UNLIKELY_METER_ACTION(index) !(index & (0xff << 24)) || !((index >> 8) & 0xffff) enum tc_qdisc_hook { TC_INGRESS, @@ -284,7 +286,12 @@ enum tc_offloaded_state { TC_OFFLOADED_STATE_NOT_IN_HW, }; +#ifndef TCA_DUMP_FLAGS_TERSE +#define TCA_DUMP_FLAGS_TERSE (1 << 0) +#endif +#define ACT_MAX_NUM 1024 #define TCA_ACT_MAX_NUM 16 +#define TCA_ACT_MIN_PRIO 1 struct tcf_id { enum tc_qdisc_hook hook; diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 1c9c720f0..091b0a626 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -2794,6 +2794,7 @@ revalidate(struct revalidator *revalidator) ovsrcu_quiesce(); } dpif_flow_dump_thread_destroy(dump_thread); + dpif_meter_revalidate(udpif->dpif, udpif->backer); ofpbuf_uninit(&odp_actions); } diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index 191cfcb0d..cf7ebc29d 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -401,4 +401,6 @@ bool ofproto_dpif_ct_zone_timeout_policy_get_name( bool ovs_explicit_drop_action_supported(struct ofproto_dpif *); +void dpif_meter_revalidate(struct dpif *dpif, struct dpif_backer *backer); + #endif /* ofproto-dpif.h */ -- 2.20.1 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
