[ovs-dev,v2] dpif-netlink: add revalidator for offload of meters

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.

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               | 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(-)

References:  <20220930111259.724462-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: 525, 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 30 Sep 2022, at 13:12, 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.

Hi Yifan,

I understand the problem, but I’m wondering why you decided to revalidate all the meters. Would it not be better to just keep track of a list of meters where deletion failed, and only handle those? Or maybe even better, check if the meter needs cleaning up when a flow with a meter action gets deleted. Maybe there are better/other ways but I need to give it some thought. Jainbo?

As mentioned before I think we should avoid doing all this extra work when there can be a simpler solution to the problem.


Some comments are below, which was not because I did a full review, but just noticed them when glancing over it.

> 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               | 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)

Maybe call this id_pool_id_allocated() to be more in line with the id_pool_alloc_id() API.

> +{
> +    return !!id_pool_find(pool, id);
> +}
> +
> +uint32_t id_pool_base(struct id_pool *pool){

Don’t think this function is used, so you could remove it.

> +    return pool->base;
> +}

Add newline

> +uint32_t id_pool_n(struct id_pool *pool){

uint32_t and { on it’s own line to be consistent with the other functions in this file.

Maybe call it id_pool_size() to be inline with other APIs?

> +    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);
>
No new line needed I guess.

> +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);
Newline should be here.
>  /*
>   * 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)

These macro’s are not valid, as the policy ID is allocated with id_pool_alloc_id() which will always give the lowest pool id.
Some values could also be reserved as they could not be deleted at OVS startup.

>  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

Do we need such a long delay? It will be a shame to wait 10 seconds if it’s done in 5. Maybe you can use OVS_WAIT_UNTIL() here.


> +AT_CHECK([tc actions ls action police], [0], [])
> +
>  OVS_TRAFFIC_VSWITCHD_STOP
>  AT_CLEANUP
>
> -- 
> 2.30.2