From: Yifan Li <yifan...@corigine.com>
Allow revalidator for hardware offload of meters via OVS-TC.
Without revalidator, tc meter action can not be deleted while
flow exists. The revalidator fix this bug by continuously
checking existing meters and delete the unneeded ones. The
autotest cases of revalidator are also added.
Signed-off-by: Yifan Li <yifan...@corigine.com>
Signed-off-by: Simon Horman <simon.hor...@corigine.com>
---
lib/dpif-netdev.c | 1 +
lib/dpif-netlink.c | 201 +++++++++++++++++++++++++++++++
lib/dpif-netlink.h | 2 +
lib/dpif-provider.h | 4 +
lib/dpif.c | 7 ++
lib/dpif.h | 2 +
lib/id-pool.c | 13 ++
lib/id-pool.h | 3 +
lib/netdev-linux.c | 6 +
lib/netdev-offload-tc.c | 11 +-
lib/tc.c | 5 -
lib/tc.h | 9 ++
ofproto/ofproto-dpif-upcall.c | 5 +
tests/system-offloads-traffic.at | 4 +
14 files changed, 267 insertions(+), 6 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a45b460145c6..365aacadb03a 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -9583,6 +9583,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 a620a6ec52dd..fac3535e1748 100644
--- a/lib/dpif-netlink.c
+++ b/lib/dpif-netlink.c
@@ -25,6 +25,7 @@
#include <net/if.h>
#include <linux/types.h>
#include <linux/pkt_sched.h>
+#include <linux/rtnetlink.h>
#include <poll.h>
#include <stdlib.h>
#include <strings.h>
@@ -61,6 +62,7 @@
#include "packets.h"
#include "random.h"
#include "sset.h"
+#include "tc.h"
#include "timeval.h"
#include "unaligned.h"
#include "util.h"
@@ -4161,6 +4163,191 @@ dpif_netlink_meter_get_features(const struct dpif
*dpif_,
ofpbuf_delete(msg);
}
+static bool dpif_netlink_meter_should_revalidate(struct id_pool *meter_ids,
+ uint32_t meter_id)
+{
+ return !id_pool_id_exist(meter_ids, meter_id);
+}
+
+static void
+dpif_tc_meter_revalidate(struct dpif *dpif_ OVS_UNUSED,
+ struct id_pool *meter_ids, 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);
+ 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 },
+ };
+ struct nlattr *action_root_attrs[ARRAY_SIZE(tca_root_policy)];
+ static const struct nl_policy police_policy[] = {
+ [TCA_POLICE_TBF] = { .type = NL_A_UNSPEC,
+ .min_len = sizeof(struct tc_police),
+ .optional = false},
+ };
+ struct nlattr *action_police_tab[ARRAY_SIZE(police_policy)];
+ 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, },
+ };
+ 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;
+ ofproto_meter_id meter_id;
+ size_t revalidate_num;
+ size_t act_count;
+ uint32_t index;
+ int i;
+
+ if (!reply) {
+ VLOG_ERR_RL(&rl, "Null reply message during meter revalidation");
+ return;
+ }
+
+ if (reply->size <= NLMSG_ALIGNTO + NLMSG_HDRLEN) {
+ VLOG_DBG_RL(&rl, "No meters present in tc during meter "
+ "revalidation");
+ 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 during meter "
+ "revalidation");
+ return;
+ }
+
+ act_count = nl_attr_get_u32(action_root_attrs[TCA_ROOT_COUNT]);
+ if (!act_count) {
+ VLOG_ERR_RL(&rl, "No police action returned in message during "
+ "meter revalidation");
+ 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 during meter "
+ "revalidation of 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 during meter "
+ "revalidation");
+ return;
+ }
+
+ if (strcmp(nl_attr_get_string(action_police_attrs[TCA_KIND]),
+ "police")) {
+ VLOG_EMER("Non-police action found during meter revalidation");
+ 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 "
+ "during meter revalidation");
+ 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 during meter "
+ "revalidation");
+ continue;
+ }
+ index = tc_police->index;
+ /* The range of meter index is 0x10000000 to 0x1fffffff
+ * If not meter, continue */
+ if (!tc_is_meter_index(index)) {
+ VLOG_DBG_RL(&rl, "Meter index : %d is not meter:"
+ "action = %d, rate = %d, burst = %d, mtu = %d",
+ index, tc_police->action, tc_police->rate.rate,
+ tc_police->burst, tc_police->mtu);
+ continue;
+ }
+
+ /* transform police index to meter id */
+ index = POLICY_INDEX_TO_METER_ID(index);
+ if (dpif_netlink_meter_should_revalidate(meter_ids, index)) {
+ meter_id.uint32 = index;
+ VLOG_DBG_RL(&rl, "Revalidate meter id %u for police index "
+ "%08x", index, tc_police->index);
+ meter_offload_del(meter_id, NULL);
+ }
+ }
+}
+
+static int
+dpif_netlink_meter_revalidate__(struct dpif *dpif_ OVS_UNUSED,
+ struct id_pool *meter_ids)
+{
+ 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 0;
+ }
+ /* Make tc action request */
+ ofpbuf_init(&request, 16384);
+ nl_msg_put_nlmsghdr(&request, sizeof *tcmsg, RTM_GETACTION,
+ NLM_F_REQUEST | NLM_F_DUMP);
+ tcmsg = ofpbuf_put_zeros(&request, sizeof *tcmsg);
+ tcmsg->tca_family = AF_UNSPEC;
+ if (!tcmsg) {
+ return ENODEV;
+ }
+
+ /* Data path interface netlink police start */
+ total_offset = nl_msg_start_nested(&request, TCA_ACT_TAB);
+ act_offset = nl_msg_start_nested(&request, ++prio);
+ /* Send request */
+ nl_msg_put_string(&request, TCA_KIND, "police");
+
+ /* Data path interface netlink police end */
+ nl_msg_end_nested(&request, act_offset);
+ nl_msg_end_nested(&request, total_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", error);
+ return error;
+ }
+ dpif_tc_meter_revalidate(dpif_, meter_ids, reply);
+ ofpbuf_delete(reply);
+ return 0;
+}
+
static int
dpif_netlink_meter_set__(struct dpif *dpif_, ofproto_meter_id meter_id,
struct ofputil_meter_config *config)
@@ -4370,6 +4557,19 @@ dpif_netlink_meter_del(struct dpif *dpif,
ofproto_meter_id meter_id,
return err;
}
+static int
+dpif_netlink_meter_revalidate(struct dpif *dpif_, struct id_pool *meter_ids)
+{
+ int err;
+
+ if (probe_broken_meters(dpif_)) {
+ return ENOMEM;
+ }
+ err = dpif_netlink_meter_revalidate__(dpif_, meter_ids);
+
+ return err;
+}
+
static bool
probe_broken_meters__(struct dpif *dpif)
{
@@ -4589,6 +4789,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-netlink.h b/lib/dpif-netlink.h
index 24294bc42dc3..b3b113dc407c 100644
--- a/lib/dpif-netlink.h
+++ b/lib/dpif-netlink.h
@@ -17,6 +17,8 @@
#ifndef DPIF_NETLINK_H
#define DPIF_NETLINK_H 1
+#include <linux/pkt_cls.h>
+
#include <stdbool.h>
#include <stddef.h>
#include <stdint.h>
diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
index 12477a24feee..5eb252104ee8 100644
--- a/lib/dpif-provider.h
+++ b/lib/dpif-provider.h
@@ -631,6 +631,10 @@ struct dpif_class {
int (*meter_del)(struct dpif *, ofproto_meter_id meter_id,
struct ofputil_meter_stats *, uint16_t n_bands);
+ /* Checks unneeded meters from 'dpif' and removes them. They may
+ * be caused by deleting in-use meters. */
+ int (*meter_revalidate)(struct dpif *, struct id_pool *);
+
/* 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 40f5fe44606e..dedb33f71f59 100644
--- a/lib/dpif.c
+++ b/lib/dpif.c
@@ -2028,6 +2028,13 @@ dpif_meter_del(struct dpif *dpif, ofproto_meter_id
meter_id,
return error;
}
+int
+dpif_meter_revalidate(struct dpif *dpif, struct id_pool *meter_ids){
+ return dpif->dpif_class->meter_revalidate
+ ? dpif->dpif_class->meter_revalidate(dpif, meter_ids)
+ : EOPNOTSUPP;
+}
+
int
dpif_bond_add(struct dpif *dpif, uint32_t bond_id, odp_port_t *member_map)
{
diff --git a/lib/dpif.h b/lib/dpif.h
index 6cb4dae6d8d7..dedb97f008f2 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -384,6 +384,7 @@
#include "ovs-numa.h"
#include "packets.h"
#include "util.h"
+#include "id-pool.h"
#ifdef __cplusplus
extern "C" {
@@ -905,6 +906,7 @@ int dpif_meter_get(const struct dpif *, ofproto_meter_id
meter_id,
struct ofputil_meter_stats *, uint16_t n_bands);
int dpif_meter_del(struct dpif *, ofproto_meter_id meter_id,
struct ofputil_meter_stats *, uint16_t n_bands);
+int dpif_meter_revalidate(struct dpif *dpif, struct id_pool *meter_ids);
/* Bonding. */
diff --git a/lib/id-pool.c b/lib/id-pool.c
index 69910ad08e83..64096a184563 100644
--- a/lib/id-pool.c
+++ b/lib/id-pool.c
@@ -155,3 +155,16 @@ 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);
+}
+
+uint32_t id_pool_base(struct id_pool *pool){
+ return pool->base;
+}
+uint32_t id_pool_n(struct id_pool *pool){
+ return pool->n_ids;
+}
diff --git a/lib/id-pool.h b/lib/id-pool.h
index 8721f87934bb..c9b1903d8c74 100644
--- a/lib/id-pool.h
+++ b/lib/id-pool.h
@@ -30,6 +30,9 @@ 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);
+uint32_t id_pool_base(struct id_pool *pool);
+uint32_t id_pool_n(struct id_pool *pool);
/*
* ID pool.
* ========
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index cdc66246ced3..2bb0eda9d581 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -5823,6 +5823,12 @@ tc_del_policer_action(uint32_t index, struct
ofputil_meter_stats *stats)
error = tc_transact(&request, &replyp);
if (error) {
+ if (error == EPERM || error == ENOENT) {
+ /* EPERM means flow exists, it is right that meter deletion is
+ * not permited.
+ * ENOENT means meter has already been deleted. */
+ return error;
+ }
VLOG_ERR_RL(&rl, "Failed to delete police action (index: %u), err=%d",
index, error);
return error;
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index f6f90a741fde..df0247dc264c 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -2953,10 +2953,19 @@ meter_tc_del_policer(ofproto_meter_id meter_id,
if (!meter_id_lookup(meter_id.uint32, &police_index)) {
err = tc_del_policer_action(police_index, stats);
if (err && err != ENOENT) {
+ if (err == EPERM) {
+ /* Flow exists, it is right that meter deletion is not
+ * permited. */
+ return err;
+ }
VLOG_ERR_RL(&error_rl,
- "Failed to del police %u for meter %u: %s",
+ "Deletion of police %u for meter %u failed: %s",
police_index, meter_id.uint32, ovs_strerror(err));
+ return err;
} else {
+ VLOG_DBG("Deletion of police %u for meter %u succeeded: %s",
+ police_index, meter_id.uint32, ovs_strerror(err));
+ err = 0;
meter_free_police_index(police_index);
}
meter_id_remove(meter_id.uint32);
diff --git a/lib/tc.c b/lib/tc.c
index 94044cde6060..c7d539562fc6 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -51,9 +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
@@ -1987,8 +1984,6 @@ tc_parse_action_stats(struct nlattr *action, struct
ovs_flow_stats *stats_sw,
stats_hw, stats_dropped);
}
-#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 2e64ad372592..fcadc6d8c8a4 100644
--- a/lib/tc.h
+++ b/lib/tc.h
@@ -60,6 +60,10 @@ enum tc_qdisc_hook {
#define METER_POLICE_IDS_BASE 0x10000000
#define METER_POLICE_IDS_MAX 0x1FFFFFFF
+/* Mapping meter_id.uint32 into a 32-bit integer */
+#define METER_ID_TO_POLICY_INDEX(meter_id) (meter_id | METER_POLICE_IDS_BASE)
+/* Mapping policy_index to meter_id */
+#define POLICY_INDEX_TO_METER_ID(index) (index & ~METER_POLICE_IDS_BASE)
static inline bool
tc_is_meter_index(uint32_t index) {
@@ -301,7 +305,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 7ad728adffdb..35f898ca25f7 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2658,6 +2658,7 @@ revalidate(struct revalidator *revalidator)
struct udpif *udpif = revalidator->udpif;
struct dpif_flow_dump_thread *dump_thread;
+ bool flow_delete_exist = false;
uint64_t dump_seq, reval_seq;
bool kill_warn_print = true;
unsigned int flow_limit;
@@ -2790,6 +2791,7 @@ revalidate(struct revalidator *revalidator)
if (result != UKEY_KEEP) {
/* Takes ownership of 'recircs'. */
+ flow_delete_exist = true;
reval_op_init(&ops[n_ops++], result, udpif, ukey, &recircs,
&odp_actions);
}
@@ -2803,6 +2805,9 @@ revalidate(struct revalidator *revalidator)
ovsrcu_quiesce();
}
dpif_flow_dump_thread_destroy(dump_thread);
+ if (flow_delete_exist) {
+ dpif_meter_revalidate(udpif->dpif, udpif->backer->meter_ids);
+ }
ofpbuf_uninit(&odp_actions);
}
diff --git a/tests/system-offloads-traffic.at b/tests/system-offloads-traffic.at
index 1a60570801e1..0ca3897a721f 100644
--- a/tests/system-offloads-traffic.at
+++ b/tests/system-offloads-traffic.at
@@ -273,6 +273,10 @@ meter:1 flow_count:1 packet_in_count:11 byte_in_count:377
duration:0.001s bands:
0: packet_count:9 byte_count:0
])
+AT_CHECK([ovs-ofctl -O Openflow13 del-meter br0])
+sleep 10
+AT_CHECK([tc actions ls action police], [0], [])
+
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
--
2.30.2
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
