| Message ID | 20220923083514.296638-1-simon.horman@corigine.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] dpif-netlink: add revalidator for offload of meters | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | warning | apply and check: warning |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
References: <20220923083514.296638-1-simon.horman@corigine.com> Bleep bloop. Greetings Simon Horman, I am a robot and I have tried out your patch. Thanks for your contribution. I encountered some error that I wasn't expecting. See the details below. checkpatch: WARNING: Unexpected sign-offs from developers who are not authors or co-authors or committers: Simon Horman <simon.horman@corigine.com> Lines checked: 715, Warnings: 1, Errors: 0 Please check this out. If you feel there has been an error, please email aconole@redhat.com Thanks, 0-day Robot
On 23 Sep 2022, at 10:35, Simon Horman wrote: > From: Yifan Li <yifan.li@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. This is not a review, but just some quick observations/questions. - Please undo the n_handlers to _n_handlers renames, as it does not make sense for this patch. - New TC related code was added to dpif-netdev.c this is not the place where such code should live. - Log messages do not need a trailing \n, and please have them uniform, i.e. start with a capital for all new log messages. - Can you explain why we need this expensive revalidate process and isn’t there a better way to make sure they are cleaned up properly? Also including Jianbo who added the initial code. //Eelco > Signed-off-by: Yifan Li <yifan.li@corigine.com> > Signed-off-by: Simon Horman <simon.horman@corigine.com> > --- > lib/dpif-netdev.c | 1 + > lib/dpif-netlink.c | 257 +++++++++++++++++++++++++++---- > lib/dpif-netlink.h | 2 + > lib/dpif-provider.h | 5 + > lib/dpif.c | 15 +- > lib/id-pool.c | 13 ++ > lib/id-pool.h | 4 +- > lib/netdev-linux.c | 6 + > lib/netdev-offload-tc.c | 11 +- > lib/tc.c | 5 - > lib/tc.h | 10 +- > ofproto/ofproto-dpif-upcall.c | 5 + > ofproto/ofproto-dpif.h | 2 + > tests/system-offloads-traffic.at | 4 + > 14 files changed, 301 insertions(+), 39 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..4eee4761dc6b 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> > @@ -48,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" > @@ -61,6 +63,7 @@ > #include "packets.h" > #include "random.h" > #include "sset.h" > +#include "tc.h" > #include "timeval.h" > #include "unaligned.h" > #include "util.h" > @@ -2571,18 +2574,18 @@ static uint32_t > dpif_netlink_calculate_n_handlers(void) > { > uint32_t total_cores = count_total_cores(); > - uint32_t n_handlers = count_cpu_cores(); > + uint32_t _n_handlers = count_cpu_cores(); > uint32_t next_prime_num; > > /* If not all cores are available to OVS, create additional handler > * threads to ensure more fair distribution of load between them. > */ > - if (n_handlers < total_cores && total_cores > 2) { > - next_prime_num = next_prime(n_handlers + 1); > - n_handlers = MIN(next_prime_num, total_cores); > + if (_n_handlers < total_cores && total_cores > 2) { > + next_prime_num = next_prime(_n_handlers + 1); > + _n_handlers = MIN(next_prime_num, total_cores); > } > > - return n_handlers; > + return _n_handlers; > } > > static int > @@ -2591,17 +2594,17 @@ dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif) > { > int handler_id; > int error = 0; > - uint32_t n_handlers; > uint32_t *upcall_pids; > + uint32_t _n_handlers; > > - n_handlers = dpif_netlink_calculate_n_handlers(); > - if (dpif->n_handlers != n_handlers) { > + _n_handlers = dpif_netlink_calculate_n_handlers(); > + if (dpif->n_handlers != _n_handlers) { > VLOG_DBG("Dispatch mode(per-cpu): initializing %d handlers", > - n_handlers); > + _n_handlers); > destroy_all_handlers(dpif); > - upcall_pids = xzalloc(n_handlers * sizeof *upcall_pids); > - dpif->handlers = xzalloc(n_handlers * sizeof *dpif->handlers); > - for (handler_id = 0; handler_id < n_handlers; handler_id++) { > + upcall_pids = xzalloc(_n_handlers * sizeof *upcall_pids); > + dpif->handlers = xzalloc(_n_handlers * sizeof *dpif->handlers); > + for (handler_id = 0; handler_id < _n_handlers; handler_id++) { > struct dpif_handler *handler = &dpif->handlers[handler_id]; > error = create_nl_sock(dpif, &handler->sock); > if (error) { > @@ -2615,9 +2618,9 @@ dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif) > handler_id, upcall_pids[handler_id]); > } > > - dpif->n_handlers = n_handlers; > + dpif->n_handlers = _n_handlers; > error = dpif_netlink_set_handler_pids(&dpif->dpif, upcall_pids, > - n_handlers); > + _n_handlers); > free(upcall_pids); > } > return error; > @@ -2629,7 +2632,7 @@ dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif) > * backing kernel vports. */ > static int > dpif_netlink_refresh_handlers_vport_dispatch(struct dpif_netlink *dpif, > - uint32_t n_handlers) > + uint32_t _n_handlers) > OVS_REQ_WRLOCK(dpif->upcall_lock) > { > unsigned long int *keep_channels; > @@ -2641,13 +2644,13 @@ dpif_netlink_refresh_handlers_vport_dispatch(struct dpif_netlink *dpif, > int retval = 0; > size_t i; > > - ovs_assert(!WINDOWS || n_handlers <= 1); > + ovs_assert(!WINDOWS || _n_handlers <= 1); > ovs_assert(!WINDOWS || dpif->n_handlers <= 1); > > - if (dpif->n_handlers != n_handlers) { > + if (dpif->n_handlers != _n_handlers) { > destroy_all_channels(dpif); > - dpif->handlers = xzalloc(n_handlers * sizeof *dpif->handlers); > - for (i = 0; i < n_handlers; i++) { > + dpif->handlers = xzalloc(_n_handlers * sizeof *dpif->handlers); > + for (i = 0; i < _n_handlers; i++) { > int error; > struct dpif_handler *handler = &dpif->handlers[i]; > > @@ -2665,10 +2668,10 @@ dpif_netlink_refresh_handlers_vport_dispatch(struct dpif_netlink *dpif, > return error; > } > } > - dpif->n_handlers = n_handlers; > + dpif->n_handlers = _n_handlers; > } > > - for (i = 0; i < n_handlers; i++) { > + for (i = 0; i < _n_handlers; i++) { > struct dpif_handler *handler = &dpif->handlers[i]; > > handler->event_offset = handler->n_events = 0; > @@ -2801,7 +2804,7 @@ dpif_netlink_recv_set(struct dpif *dpif_, bool enable) > } > > static int > -dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t n_handlers) > +dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t _n_handlers) > { > struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); > int error = 0; > @@ -2809,7 +2812,7 @@ dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t n_handlers) > #ifdef _WIN32 > /* Multiple upcall handlers will be supported once kernel datapath supports > * it. */ > - if (n_handlers > 1) { > + if (_n_handlers > 1) { > return error; > } > #endif > @@ -2820,7 +2823,7 @@ dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t n_handlers) > error = dpif_netlink_refresh_handlers_cpu_dispatch(dpif); > } else { > error = dpif_netlink_refresh_handlers_vport_dispatch(dpif, > - n_handlers); > + _n_handlers); > } > } > fat_rwlock_unlock(&dpif->upcall_lock); > @@ -2829,12 +2832,13 @@ dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t n_handlers) > } > > static bool > -dpif_netlink_number_handlers_required(struct dpif *dpif_, uint32_t *n_handlers) > +dpif_netlink_number_handlers_required(struct dpif *dpif_, > + uint32_t *_n_handlers) > { > struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); > > if (dpif_netlink_upcall_per_cpu(dpif)) { > - *n_handlers = dpif_netlink_calculate_n_handlers(); > + *_n_handlers = dpif_netlink_calculate_n_handlers(); > return true; > } > > @@ -2843,7 +2847,7 @@ dpif_netlink_number_handlers_required(struct dpif *dpif_, uint32_t *n_handlers) > > static int > dpif_netlink_queue_to_priority(const struct dpif *dpif OVS_UNUSED, > - uint32_t queue_id, uint32_t *priority) > + uint32_t queue_id, uint32_t *priority) > { > if (queue_id < 0xf000) { > *priority = TC_H_MAKE(1 << 16, queue_id + 1); > @@ -4161,6 +4165,191 @@ dpif_netlink_meter_get_features(const struct dpif *dpif_, > ofpbuf_delete(msg); > } > > +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); > + 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\n"); > + return; > + } > + > + if (reply->size <= NLMSG_ALIGNTO + NLMSG_HDRLEN) { > + VLOG_DBG_RL(&rl, "no meters present in tc during meter " > + "revalidation\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 during meter " > + "revalidation\n"); > + 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\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 during meter " > + "revalidation of act_count\n"); > + 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\n"); > + return; > + } > + > + if (strcmp(nl_attr_get_string(action_police_attrs[TCA_KIND]), > + "police")) { > + VLOG_EMER("non-police action found during meter revalidation\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 " > + "during meter revalidation\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 during meter " > + "revalidation\n"); > + 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(backer, index)) { > + meter_id.uint32 = index; > + VLOG_DBG_RL(&rl, "revalidate meter id %u for police index " > + "%08x\n", index, tc_police->index); > + meter_offload_del(meter_id, NULL); > + } > + } > +} > + > +static int > +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 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\n", error); > + return error; > + } > + dpif_tc_meter_revalidate(dpif_, backer, 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 +4559,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 dpif_backer *backer) > +{ > + int err; > + > + if (probe_broken_meters(dpif_)) { > + return ENOMEM; > + } > + err = dpif_netlink_meter_revalidate__(dpif_, backer); > + > + return err; > +} > + > static bool > probe_broken_meters__(struct dpif *dpif) > { > @@ -4589,6 +4791,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..efc6ca989fa9 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" { > @@ -631,6 +632,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 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 40f5fe44606e..ceee076af195 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -1491,12 +1491,12 @@ dpif_recv_set(struct dpif *dpif, bool enable) > * > * Returns 0 if successful, otherwise a positive errno value. */ > int > -dpif_handlers_set(struct dpif *dpif, uint32_t n_handlers) > +dpif_handlers_set(struct dpif *dpif, uint32_t _n_handlers) > { > int error = 0; > > if (dpif->dpif_class->handlers_set) { > - error = dpif->dpif_class->handlers_set(dpif, n_handlers); > + error = dpif->dpif_class->handlers_set(dpif, _n_handlers); > log_operation(dpif, "handlers_set", error); > } > return error; > @@ -1510,10 +1510,10 @@ dpif_handlers_set(struct dpif *dpif, uint32_t n_handlers) > * If not, returns 'false' > */ > bool > -dpif_number_handlers_required(struct dpif *dpif, uint32_t *n_handlers) > +dpif_number_handlers_required(struct dpif *dpif, uint32_t *_n_handlers) > { > if (dpif->dpif_class->number_handlers_required) { > - return dpif->dpif_class->number_handlers_required(dpif, n_handlers); > + return dpif->dpif_class->number_handlers_required(dpif, _n_handlers); > } > return false; > } > @@ -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 dpif_backer *backer){ > + return dpif->dpif_class->meter_revalidate > + ? dpif->dpif_class->meter_revalidate(dpif, backer) > + : EOPNOTSUPP; > +} > + > int > dpif_bond_add(struct dpif *dpif, uint32_t bond_id, odp_port_t *member_map) > { > 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..b3bfd79386db 100644 > --- a/lib/id-pool.h > +++ b/lib/id-pool.h > @@ -29,7 +29,9 @@ 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); > +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..8e7107d1bd74 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) { > @@ -300,8 +304,12 @@ enum tc_offloaded_state { > TC_OFFLOADED_STATE_IN_HW, > 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..09c1c7aedc34 100644 > --- a/ofproto/ofproto-dpif-upcall.c > +++ b/ofproto/ofproto-dpif-upcall.c > @@ -2659,6 +2659,7 @@ revalidate(struct revalidator *revalidator) > struct udpif *udpif = revalidator->udpif; > struct dpif_flow_dump_thread *dump_thread; > uint64_t dump_seq, reval_seq; > + bool flow_delete_exist = false; > 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); > + } > ofpbuf_uninit(&odp_actions); > } > > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h > index d8e0cd37ac5b..ef0c7338e04e 100644 > --- a/ofproto/ofproto-dpif.h > +++ b/ofproto/ofproto-dpif.h > @@ -404,4 +404,6 @@ bool ofproto_dpif_ct_zone_timeout_policy_get_name( > > bool ovs_explicit_drop_action_supported(struct ofproto_dpif *); > > +int dpif_meter_revalidate(struct dpif *dpif, struct dpif_backer *backer); > + > #endif /* ofproto-dpif.h */ > 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
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..4eee4761dc6b 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> @@ -48,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" @@ -61,6 +63,7 @@ #include "packets.h" #include "random.h" #include "sset.h" +#include "tc.h" #include "timeval.h" #include "unaligned.h" #include "util.h" @@ -2571,18 +2574,18 @@ static uint32_t dpif_netlink_calculate_n_handlers(void) { uint32_t total_cores = count_total_cores(); - uint32_t n_handlers = count_cpu_cores(); + uint32_t _n_handlers = count_cpu_cores(); uint32_t next_prime_num; /* If not all cores are available to OVS, create additional handler * threads to ensure more fair distribution of load between them. */ - if (n_handlers < total_cores && total_cores > 2) { - next_prime_num = next_prime(n_handlers + 1); - n_handlers = MIN(next_prime_num, total_cores); + if (_n_handlers < total_cores && total_cores > 2) { + next_prime_num = next_prime(_n_handlers + 1); + _n_handlers = MIN(next_prime_num, total_cores); } - return n_handlers; + return _n_handlers; } static int @@ -2591,17 +2594,17 @@ dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif) { int handler_id; int error = 0; - uint32_t n_handlers; uint32_t *upcall_pids; + uint32_t _n_handlers; - n_handlers = dpif_netlink_calculate_n_handlers(); - if (dpif->n_handlers != n_handlers) { + _n_handlers = dpif_netlink_calculate_n_handlers(); + if (dpif->n_handlers != _n_handlers) { VLOG_DBG("Dispatch mode(per-cpu): initializing %d handlers", - n_handlers); + _n_handlers); destroy_all_handlers(dpif); - upcall_pids = xzalloc(n_handlers * sizeof *upcall_pids); - dpif->handlers = xzalloc(n_handlers * sizeof *dpif->handlers); - for (handler_id = 0; handler_id < n_handlers; handler_id++) { + upcall_pids = xzalloc(_n_handlers * sizeof *upcall_pids); + dpif->handlers = xzalloc(_n_handlers * sizeof *dpif->handlers); + for (handler_id = 0; handler_id < _n_handlers; handler_id++) { struct dpif_handler *handler = &dpif->handlers[handler_id]; error = create_nl_sock(dpif, &handler->sock); if (error) { @@ -2615,9 +2618,9 @@ dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif) handler_id, upcall_pids[handler_id]); } - dpif->n_handlers = n_handlers; + dpif->n_handlers = _n_handlers; error = dpif_netlink_set_handler_pids(&dpif->dpif, upcall_pids, - n_handlers); + _n_handlers); free(upcall_pids); } return error; @@ -2629,7 +2632,7 @@ dpif_netlink_refresh_handlers_cpu_dispatch(struct dpif_netlink *dpif) * backing kernel vports. */ static int dpif_netlink_refresh_handlers_vport_dispatch(struct dpif_netlink *dpif, - uint32_t n_handlers) + uint32_t _n_handlers) OVS_REQ_WRLOCK(dpif->upcall_lock) { unsigned long int *keep_channels; @@ -2641,13 +2644,13 @@ dpif_netlink_refresh_handlers_vport_dispatch(struct dpif_netlink *dpif, int retval = 0; size_t i; - ovs_assert(!WINDOWS || n_handlers <= 1); + ovs_assert(!WINDOWS || _n_handlers <= 1); ovs_assert(!WINDOWS || dpif->n_handlers <= 1); - if (dpif->n_handlers != n_handlers) { + if (dpif->n_handlers != _n_handlers) { destroy_all_channels(dpif); - dpif->handlers = xzalloc(n_handlers * sizeof *dpif->handlers); - for (i = 0; i < n_handlers; i++) { + dpif->handlers = xzalloc(_n_handlers * sizeof *dpif->handlers); + for (i = 0; i < _n_handlers; i++) { int error; struct dpif_handler *handler = &dpif->handlers[i]; @@ -2665,10 +2668,10 @@ dpif_netlink_refresh_handlers_vport_dispatch(struct dpif_netlink *dpif, return error; } } - dpif->n_handlers = n_handlers; + dpif->n_handlers = _n_handlers; } - for (i = 0; i < n_handlers; i++) { + for (i = 0; i < _n_handlers; i++) { struct dpif_handler *handler = &dpif->handlers[i]; handler->event_offset = handler->n_events = 0; @@ -2801,7 +2804,7 @@ dpif_netlink_recv_set(struct dpif *dpif_, bool enable) } static int -dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t n_handlers) +dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t _n_handlers) { struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); int error = 0; @@ -2809,7 +2812,7 @@ dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t n_handlers) #ifdef _WIN32 /* Multiple upcall handlers will be supported once kernel datapath supports * it. */ - if (n_handlers > 1) { + if (_n_handlers > 1) { return error; } #endif @@ -2820,7 +2823,7 @@ dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t n_handlers) error = dpif_netlink_refresh_handlers_cpu_dispatch(dpif); } else { error = dpif_netlink_refresh_handlers_vport_dispatch(dpif, - n_handlers); + _n_handlers); } } fat_rwlock_unlock(&dpif->upcall_lock); @@ -2829,12 +2832,13 @@ dpif_netlink_handlers_set(struct dpif *dpif_, uint32_t n_handlers) } static bool -dpif_netlink_number_handlers_required(struct dpif *dpif_, uint32_t *n_handlers) +dpif_netlink_number_handlers_required(struct dpif *dpif_, + uint32_t *_n_handlers) { struct dpif_netlink *dpif = dpif_netlink_cast(dpif_); if (dpif_netlink_upcall_per_cpu(dpif)) { - *n_handlers = dpif_netlink_calculate_n_handlers(); + *_n_handlers = dpif_netlink_calculate_n_handlers(); return true; } @@ -2843,7 +2847,7 @@ dpif_netlink_number_handlers_required(struct dpif *dpif_, uint32_t *n_handlers) static int dpif_netlink_queue_to_priority(const struct dpif *dpif OVS_UNUSED, - uint32_t queue_id, uint32_t *priority) + uint32_t queue_id, uint32_t *priority) { if (queue_id < 0xf000) { *priority = TC_H_MAKE(1 << 16, queue_id + 1); @@ -4161,6 +4165,191 @@ dpif_netlink_meter_get_features(const struct dpif *dpif_, ofpbuf_delete(msg); } +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); + 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\n"); + return; + } + + if (reply->size <= NLMSG_ALIGNTO + NLMSG_HDRLEN) { + VLOG_DBG_RL(&rl, "no meters present in tc during meter " + "revalidation\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 during meter " + "revalidation\n"); + 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\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 during meter " + "revalidation of act_count\n"); + 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\n"); + return; + } + + if (strcmp(nl_attr_get_string(action_police_attrs[TCA_KIND]), + "police")) { + VLOG_EMER("non-police action found during meter revalidation\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 " + "during meter revalidation\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 during meter " + "revalidation\n"); + 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(backer, index)) { + meter_id.uint32 = index; + VLOG_DBG_RL(&rl, "revalidate meter id %u for police index " + "%08x\n", index, tc_police->index); + meter_offload_del(meter_id, NULL); + } + } +} + +static int +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 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\n", error); + return error; + } + dpif_tc_meter_revalidate(dpif_, backer, 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 +4559,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 dpif_backer *backer) +{ + int err; + + if (probe_broken_meters(dpif_)) { + return ENOMEM; + } + err = dpif_netlink_meter_revalidate__(dpif_, backer); + + return err; +} + static bool probe_broken_meters__(struct dpif *dpif) { @@ -4589,6 +4791,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..efc6ca989fa9 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" { @@ -631,6 +632,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 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 40f5fe44606e..ceee076af195 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -1491,12 +1491,12 @@ dpif_recv_set(struct dpif *dpif, bool enable) * * Returns 0 if successful, otherwise a positive errno value. */ int -dpif_handlers_set(struct dpif *dpif, uint32_t n_handlers) +dpif_handlers_set(struct dpif *dpif, uint32_t _n_handlers) { int error = 0; if (dpif->dpif_class->handlers_set) { - error = dpif->dpif_class->handlers_set(dpif, n_handlers); + error = dpif->dpif_class->handlers_set(dpif, _n_handlers); log_operation(dpif, "handlers_set", error); } return error; @@ -1510,10 +1510,10 @@ dpif_handlers_set(struct dpif *dpif, uint32_t n_handlers) * If not, returns 'false' */ bool -dpif_number_handlers_required(struct dpif *dpif, uint32_t *n_handlers) +dpif_number_handlers_required(struct dpif *dpif, uint32_t *_n_handlers) { if (dpif->dpif_class->number_handlers_required) { - return dpif->dpif_class->number_handlers_required(dpif, n_handlers); + return dpif->dpif_class->number_handlers_required(dpif, _n_handlers); } return false; } @@ -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 dpif_backer *backer){ + return dpif->dpif_class->meter_revalidate + ? dpif->dpif_class->meter_revalidate(dpif, backer) + : EOPNOTSUPP; +} + int dpif_bond_add(struct dpif *dpif, uint32_t bond_id, odp_port_t *member_map) { 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..b3bfd79386db 100644 --- a/lib/id-pool.h +++ b/lib/id-pool.h @@ -29,7 +29,9 @@ 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); +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..8e7107d1bd74 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) { @@ -300,8 +304,12 @@ enum tc_offloaded_state { TC_OFFLOADED_STATE_IN_HW, 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..09c1c7aedc34 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -2659,6 +2659,7 @@ revalidate(struct revalidator *revalidator) struct udpif *udpif = revalidator->udpif; struct dpif_flow_dump_thread *dump_thread; uint64_t dump_seq, reval_seq; + bool flow_delete_exist = false; 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); + } ofpbuf_uninit(&odp_actions); } diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index d8e0cd37ac5b..ef0c7338e04e 100644 --- a/ofproto/ofproto-dpif.h +++ b/ofproto/ofproto-dpif.h @@ -404,4 +404,6 @@ bool ofproto_dpif_ct_zone_timeout_policy_get_name( bool ovs_explicit_drop_action_supported(struct ofproto_dpif *); +int dpif_meter_revalidate(struct dpif *dpif, struct dpif_backer *backer); + #endif /* ofproto-dpif.h */ 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