diff mbox series

[ovs-dev,v3,3/3] dpif-netlink: add revalidator for offload of meters

Message ID 20230130164256.764534-4-simon.horman@corigine.com
State Changes Requested
Headers show
Series dpif-netlink: add revalidator for offload of meters | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test success github build: passed

Commit Message

Simon Horman Jan. 30, 2023, 4:42 p.m. UTC 
From: Tianyu Yuan <tianyu.yuan@corigine.com>

Allow revalidator to continuously delete police in kernel
tc datapath until it is deleted.

In current implementation, polices in tc datapath will not
deleted when they are being used and these remaining polices
will be cleared when the openvswitch restarts.

This patch supports revalidator to delete remaining polices
in tc datapath by storing meter_id deleted unsuccessfully in
a hmap and continuously deleting them in revalidator until
success.

Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com>
Signed-off-by: Simon Horman <simon.horman@corigine.com>

---
 lib/dpif-netdev.c             |  1 +
 lib/dpif-netlink.c            | 26 +++++++++++++++++++++++++-
 lib/dpif-provider.h           |  4 ++++
 lib/dpif.c                    |  7 +++++++
 lib/dpif.h                    |  2 ++
 lib/netdev-offload-tc.c       |  5 ++++-
 lib/netdev-offload.c          |  8 +++++---
 ofproto/ofproto-dpif-upcall.c |  6 ++++++
 ofproto/ofproto-dpif.c        | 10 +++++++++-
 ofproto/ofproto-dpif.h        |  3 +++
 10 files changed, 66 insertions(+), 6 deletions(-)

Comments

0-day Robot Jan. 30, 2023, 5:03 p.m. UTC | #1 
References:  <20230130164256.764534-4-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: 284, 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
Eelco Chaudron Feb. 3, 2023, 12:34 p.m. UTC | #2 
On 30 Jan 2023, at 17:42, Simon Horman wrote:

> From: Tianyu Yuan <tianyu.yuan@corigine.com>
>
> Allow revalidator to continuously delete police in kernel
> tc datapath until it is deleted.
>
> In current implementation, polices in tc datapath will not
> deleted when they are being used and these remaining polices
> will be cleared when the openvswitch restarts.
>
> This patch supports revalidator to delete remaining polices
> in tc datapath by storing meter_id deleted unsuccessfully in
> a hmap and continuously deleting them in revalidator until
> success.

Hi Tianyu,

Thanks for the new revision of the patch...

Were you able to figure out if we really need to do this during revalidating flows?

I was wondering if there would be a different way, i.e. on flow deletion see if it was the last flow using this meter and if so remove it then. I guess this can be done with the meter having a flag that it has been deleted.

This would be way more elegant than doing this in the revalidator thread.

See some inline comments below. This was a quick review/tests, as the above might require a different approach.

Cheers,

Eelco

> Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com>
> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>
> ---
>  lib/dpif-netdev.c             |  1 +
>  lib/dpif-netlink.c            | 26 +++++++++++++++++++++++++-
>  lib/dpif-provider.h           |  4 ++++
>  lib/dpif.c                    |  7 +++++++
>  lib/dpif.h                    |  2 ++
>  lib/netdev-offload-tc.c       |  5 ++++-
>  lib/netdev-offload.c          |  8 +++++---
>  ofproto/ofproto-dpif-upcall.c |  6 ++++++
>  ofproto/ofproto-dpif.c        | 10 +++++++++-
>  ofproto/ofproto-dpif.h        |  3 +++
>  10 files changed, 66 insertions(+), 6 deletions(-)
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index c9f7179c3b4c..878fe5933d1a 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -9689,6 +9689,7 @@ const struct dpif_class dpif_netdev_class = {
>      dpif_netdev_meter_set,
>      dpif_netdev_meter_get,
>      dpif_netdev_meter_del,
> +    NULL,
>      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 026b0daa8d83..8b95912aea97 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>
> @@ -37,6 +38,7 @@
>  #include "dpif-provider.h"
>  #include "fat-rwlock.h"
>  #include "flow.h"
> +#include "id-pool.h"
>  #include "netdev-linux.h"
>  #include "netdev-offload.h"
>  #include "netdev-provider.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"
> @@ -4363,12 +4366,32 @@ dpif_netlink_meter_del(struct dpif *dpif, ofproto_meter_id meter_id,
>      err  = dpif_netlink_meter_get_stats(dpif, meter_id, stats,
>                                          max_bands, OVS_METER_CMD_DEL);
>      if (!err && netdev_is_flow_api_enabled()) {
> -        meter_offload_del(meter_id, stats);
> +        return meter_offload_del(meter_id, stats);
>      }
>
>      return err;
>  }
>
> +static void
> +dpif_netlink_meter_revalidate(struct dpif *dpif_ OVS_UNUSED,
> +                              struct hmap *meter_map)
> +{
> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> +    ofproto_meter_id meter_id;
> +    struct id_node *id_node;
> +
> +    HMAP_FOR_EACH_POP (id_node, node, meter_map) {
> +        meter_id.uint32 = id_node->id;
> +
> +        if (meter_offload_del(meter_id, NULL) == EPERM) {
> +            id_hmap_add(meter_map, meter_id.uint32);
> +        } else {
> +            VLOG_DBG_RL(&rl, "Delete meter %u in datapath success",
> +                        meter_id.uint32);
> +        }
> +    }
> +}
> +
>  static bool
>  probe_broken_meters__(struct dpif *dpif)
>  {
> @@ -4588,6 +4611,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 12477a24feee..794fbc21686a 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. */
> +    void (*meter_revalidate)(struct dpif *, struct hmap *meter_map);
> +

I guess this is not really a revalidate action, it's more of a cleanup.
So maybe also call it something like meter_cleanup(), or meter_garbage_collection.

Or maybe even better, why not use the existing periodic 'bool (*run)(struct dpif *dpif)' callback and handle it there?

>      /* 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 fe4db83fbfee..c8718239117d 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;
>  }
>
> +void
> +dpif_meter_revalidate(struct dpif *dpif, struct hmap *meter_map){
> +    if (dpif->dpif_class->meter_revalidate) {
> +        dpif->dpif_class->meter_revalidate(dpif, meter_map);
> +    }
> +}
> +
>  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..e181e4ee1d6c 100644
> --- a/lib/dpif.h
> +++ b/lib/dpif.h
> @@ -378,6 +378,7 @@
>
>  #include "dpdk.h"
>  #include "dp-packet.h"
> +#include "id-pool.h"
>  #include "netdev.h"
>  #include "openflow/openflow.h"
>  #include "openvswitch/ofp-meter.h"
> @@ -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);
> +void dpif_meter_revalidate(struct dpif *dpif, struct hmap *meter_map);
>
>  /* Bonding. */
>
> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
> index 15d1c36aa04e..f87a5e05e202 100644
> --- a/lib/netdev-offload-tc.c
> +++ b/lib/netdev-offload-tc.c
> @@ -2980,8 +2980,11 @@ meter_tc_del_policer(ofproto_meter_id meter_id,
>                          police_index, meter_id.uint32, ovs_strerror(err));
>          } else {
>              meter_free_police_index(police_index);
> +            /* Do not remove the mapping between meter_id and police_index
> +             * until this meter is deleted successfully in datapath. This
> +             * mapping will be used in meter deletion in revalidator. */
> +            meter_id_remove(meter_id.uint32);

This approach might have problem, what if the policy gets deleted, and re-added with a new configuration?
The datapath flows are not cleared, and the add will fail as the meter_id is still in use.

Guess we need to keep the police index as this is tc specific, the meter_id is global.

>          }
> -        meter_id_remove(meter_id.uint32);
>      }
>
>      return err;
> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
> index 4592262bd34e..0769f796e5b7 100644
> --- a/lib/netdev-offload.c
> +++ b/lib/netdev-offload.c
> @@ -241,19 +241,21 @@ int
>  meter_offload_del(ofproto_meter_id meter_id, struct ofputil_meter_stats *stats)
>  {
>      struct netdev_registered_flow_api *rfa;
> +    int ret = 0;
>
>      CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
>          if (rfa->flow_api->meter_del) {
> -            int ret = rfa->flow_api->meter_del(meter_id, stats);
> -            if (ret) {
> +            int err = rfa->flow_api->meter_del(meter_id, stats);
> +            if (err) {
>                  VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow api %s, "
>                              "error %d", meter_id.uint32, rfa->flow_api->type,
>                              ret);
> +                ret = err;
>              }
>          }
>      }
>
> -    return 0;
> +    return ret;
>  }
>
>  int
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 31ac02d116fc..b802c9c7f13a 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -2828,6 +2828,12 @@ revalidate(struct revalidator *revalidator)
>          }
>          ovsrcu_quiesce();
>      }
> +    /* When fail_meter_hmap is not empty, revalidate to delete meters in this
> +     * hashmap. */
> +    if (hmap_count(&udpif->backer->fail_meter_hmap)) {
> +        dpif_meter_revalidate(udpif->dpif, &udpif->backer->fail_meter_hmap);

This could be removed with the general run() implementation. But in general we should not have this hmap, just call the callback. See below.

> +    }
> +
>      dpif_flow_dump_thread_destroy(dump_thread);
>      ofpbuf_uninit(&odp_actions);
>  }
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index f87e27a8cd7f..591af5503765 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -835,6 +835,7 @@ open_dpif_backer(const char *type, struct dpif_backer **backerp)
>      dpif_meter_get_features(backer->dpif, &features);
>      if (features.max_meters) {
>          backer->meter_ids = id_pool_create(0, features.max_meters);
> +        hmap_init(&backer->fail_meter_hmap);
>      } else {
>          backer->meter_ids = NULL;
>      }
> @@ -6790,8 +6791,15 @@ static void
>  free_meter_id(struct free_meter_id_args *args)
>  {
>      struct ofproto_dpif *ofproto = args->ofproto;
> +    int err;
> +
> +    err = dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
> +    /* Add fail deleted meter to fail_meter_hmap and waiting to be deleted in
> +     * revalidator. */
> +    if (err == EPERM) {
> +        id_hmap_add(&ofproto->backer->fail_meter_hmap, args->meter_id.uint32);
> +    }

I think we should not track any of this in the ofproto layer. As this is a dpif problem, it should be completely hidden in the dpif layer. This should be easy if you remove the dependency on meter_id as suggested above.

>
> -    dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
>      id_pool_free_id(ofproto->backer->meter_ids, args->meter_id.uint32);
>      free(args);
>  }
> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
> index d8e0cd37ac5b..befc68d13baa 100644
> --- a/ofproto/ofproto-dpif.h
> +++ b/ofproto/ofproto-dpif.h
> @@ -61,6 +61,7 @@ struct ofproto_dpif;
>  struct uuid;
>  struct xlate_cache;
>  struct xlate_ctx;
> +struct id_pool;
>
>  /* Number of implemented OpenFlow tables. */
>  enum { N_TABLES = 255 };
> @@ -260,6 +261,8 @@ struct dpif_backer {
>
>      /* Meter. */
>      struct id_pool *meter_ids;     /* Datapath meter allocation. */
> +    struct hmap fail_meter_hmap;   /* Hashmap for meter with failed deletion
> +                                    * in datapath */
>
>      /* Connection tracking. */
>      struct id_pool *tp_ids;             /* Datapath timeout policy id
> -- 
> 2.30.2

Please add a test to 'make check-offloads'.
Tianyu Yuan Feb. 6, 2023, 7:48 a.m. UTC | #3 
On Fri 2/3/2023 8:35 PM, Eelco Chaudron wrote:
> 
> On 30 Jan 2023, at 17:42, Simon Horman wrote:
> 
> > From: Tianyu Yuan <tianyu.yuan@corigine.com>
> >
> > Allow revalidator to continuously delete police in kernel tc datapath
> > until it is deleted.
> >
> > In current implementation, polices in tc datapath will not deleted
> > when they are being used and these remaining polices will be cleared
> > when the openvswitch restarts.
> >
> > This patch supports revalidator to delete remaining polices in tc
> > datapath by storing meter_id deleted unsuccessfully in a hmap and
> > continuously deleting them in revalidator until success.
> 
> Hi Tianyu,
> 
> Thanks for the new revision of the patch...
> 
> Were you able to figure out if we really need to do this during revalidating
> flows?
> 
> I was wondering if there would be a different way, i.e. on flow deletion see if
> it was the last flow using this meter and if so remove it then. I guess this can
> be done with the meter having a flag that it has been deleted.
> 
> This would be way more elegant than doing this in the revalidator thread.
> 
> See some inline comments below. This was a quick review/tests, as the
> above might require a different approach.
> 
> Cheers,
> 
> Eelco
> 

Hi Eelco,

Thanks for your review and comments. I've got your point of view:
1. Another approach that trigger tc police deletion only when the last flow using this
meter.
2. depend the deletion on fail deletion of police_index rather than that of meter_id,
keep the operation in dpif layer and this should be invisible from ofproto layer.

Please point me out if I misunderstood.

The AT test of system-offload is attached at the end of this mail.
The error "9: offloads - check_pkt_len action - offloads disabled" also happens on
Upstream master branch so it is unrelated to this patch

Cheers
Tianyu

> > Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com>
> > Signed-off-by: Simon Horman <simon.horman@corigine.com>
> >
> > ---
> >  lib/dpif-netdev.c             |  1 +
> >  lib/dpif-netlink.c            | 26 +++++++++++++++++++++++++-
> >  lib/dpif-provider.h           |  4 ++++
> >  lib/dpif.c                    |  7 +++++++
> >  lib/dpif.h                    |  2 ++
> >  lib/netdev-offload-tc.c       |  5 ++++-
> >  lib/netdev-offload.c          |  8 +++++---
> >  ofproto/ofproto-dpif-upcall.c |  6 ++++++
> >  ofproto/ofproto-dpif.c        | 10 +++++++++-
> >  ofproto/ofproto-dpif.h        |  3 +++
> >  10 files changed, 66 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > c9f7179c3b4c..878fe5933d1a 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -9689,6 +9689,7 @@ const struct dpif_class dpif_netdev_class = {
> >      dpif_netdev_meter_set,
> >      dpif_netdev_meter_get,
> >      dpif_netdev_meter_del,
> > +    NULL,
> >      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
> > 026b0daa8d83..8b95912aea97 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>
> > @@ -37,6 +38,7 @@
> >  #include "dpif-provider.h"
> >  #include "fat-rwlock.h"
> >  #include "flow.h"
> > +#include "id-pool.h"
> >  #include "netdev-linux.h"
> >  #include "netdev-offload.h"
> >  #include "netdev-provider.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"
> > @@ -4363,12 +4366,32 @@ dpif_netlink_meter_del(struct dpif *dpif,
> ofproto_meter_id meter_id,
> >      err  = dpif_netlink_meter_get_stats(dpif, meter_id, stats,
> >                                          max_bands, OVS_METER_CMD_DEL);
> >      if (!err && netdev_is_flow_api_enabled()) {
> > -        meter_offload_del(meter_id, stats);
> > +        return meter_offload_del(meter_id, stats);
> >      }
> >
> >      return err;
> >  }
> >
> > +static void
> > +dpif_netlink_meter_revalidate(struct dpif *dpif_ OVS_UNUSED,
> > +                              struct hmap *meter_map) {
> > +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > +    ofproto_meter_id meter_id;
> > +    struct id_node *id_node;
> > +
> > +    HMAP_FOR_EACH_POP (id_node, node, meter_map) {
> > +        meter_id.uint32 = id_node->id;
> > +
> > +        if (meter_offload_del(meter_id, NULL) == EPERM) {
> > +            id_hmap_add(meter_map, meter_id.uint32);
> > +        } else {
> > +            VLOG_DBG_RL(&rl, "Delete meter %u in datapath success",
> > +                        meter_id.uint32);
> > +        }
> > +    }
> > +}
> > +
> >  static bool
> >  probe_broken_meters__(struct dpif *dpif)  { @@ -4588,6 +4611,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
> > 12477a24feee..794fbc21686a 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. */
> > +    void (*meter_revalidate)(struct dpif *, struct hmap *meter_map);
> > +
> 
> I guess this is not really a revalidate action, it's more of a cleanup.
> So maybe also call it something like meter_cleanup(), or
> meter_garbage_collection.
> 
> Or maybe even better, why not use the existing periodic 'bool (*run)(struct
> dpif *dpif)' callback and handle it there?
> 
> >      /* 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 fe4db83fbfee..c8718239117d 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;
> >  }
> >
> > +void
> > +dpif_meter_revalidate(struct dpif *dpif, struct hmap *meter_map){
> > +    if (dpif->dpif_class->meter_revalidate) {
> > +        dpif->dpif_class->meter_revalidate(dpif, meter_map);
> > +    }
> > +}
> > +
> >  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..e181e4ee1d6c 100644
> > --- a/lib/dpif.h
> > +++ b/lib/dpif.h
> > @@ -378,6 +378,7 @@
> >
> >  #include "dpdk.h"
> >  #include "dp-packet.h"
> > +#include "id-pool.h"
> >  #include "netdev.h"
> >  #include "openflow/openflow.h"
> >  #include "openvswitch/ofp-meter.h"
> > @@ -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);
> > +void dpif_meter_revalidate(struct dpif *dpif, struct hmap
> > +*meter_map);
> >
> >  /* Bonding. */
> >
> > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index
> > 15d1c36aa04e..f87a5e05e202 100644
> > --- a/lib/netdev-offload-tc.c
> > +++ b/lib/netdev-offload-tc.c
> > @@ -2980,8 +2980,11 @@ meter_tc_del_policer(ofproto_meter_id
> meter_id,
> >                          police_index, meter_id.uint32, ovs_strerror(err));
> >          } else {
> >              meter_free_police_index(police_index);
> > +            /* Do not remove the mapping between meter_id and police_index
> > +             * until this meter is deleted successfully in datapath. This
> > +             * mapping will be used in meter deletion in revalidator. */
> > +            meter_id_remove(meter_id.uint32);
> 
> This approach might have problem, what if the policy gets deleted, and re-
> added with a new configuration?
> The datapath flows are not cleared, and the add will fail as the meter_id is
> still in use.
> 
> Guess we need to keep the police index as this is tc specific, the meter_id is
> global.
> 
> >          }
> > -        meter_id_remove(meter_id.uint32);
> >      }
> >
> >      return err;
> > diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index
> > 4592262bd34e..0769f796e5b7 100644
> > --- a/lib/netdev-offload.c
> > +++ b/lib/netdev-offload.c
> > @@ -241,19 +241,21 @@ int
> >  meter_offload_del(ofproto_meter_id meter_id, struct
> > ofputil_meter_stats *stats)  {
> >      struct netdev_registered_flow_api *rfa;
> > +    int ret = 0;
> >
> >      CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
> >          if (rfa->flow_api->meter_del) {
> > -            int ret = rfa->flow_api->meter_del(meter_id, stats);
> > -            if (ret) {
> > +            int err = rfa->flow_api->meter_del(meter_id, stats);
> > +            if (err) {
> >                  VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow api %s, "
> >                              "error %d", meter_id.uint32, rfa->flow_api->type,
> >                              ret);
> > +                ret = err;
> >              }
> >          }
> >      }
> >
> > -    return 0;
> > +    return ret;
> >  }
> >
> >  int
> > diff --git a/ofproto/ofproto-dpif-upcall.c
> > b/ofproto/ofproto-dpif-upcall.c index 31ac02d116fc..b802c9c7f13a
> > 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -2828,6 +2828,12 @@ revalidate(struct revalidator *revalidator)
> >          }
> >          ovsrcu_quiesce();
> >      }
> > +    /* When fail_meter_hmap is not empty, revalidate to delete meters in
> this
> > +     * hashmap. */
> > +    if (hmap_count(&udpif->backer->fail_meter_hmap)) {
> > +        dpif_meter_revalidate(udpif->dpif,
> > + &udpif->backer->fail_meter_hmap);
> 
> This could be removed with the general run() implementation. But in general
> we should not have this hmap, just call the callback. See below.
> 
> > +    }
> > +
> >      dpif_flow_dump_thread_destroy(dump_thread);
> >      ofpbuf_uninit(&odp_actions);
> >  }
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index
> > f87e27a8cd7f..591af5503765 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -835,6 +835,7 @@ open_dpif_backer(const char *type, struct
> dpif_backer **backerp)
> >      dpif_meter_get_features(backer->dpif, &features);
> >      if (features.max_meters) {
> >          backer->meter_ids = id_pool_create(0, features.max_meters);
> > +        hmap_init(&backer->fail_meter_hmap);
> >      } else {
> >          backer->meter_ids = NULL;
> >      }
> > @@ -6790,8 +6791,15 @@ static void
> >  free_meter_id(struct free_meter_id_args *args)  {
> >      struct ofproto_dpif *ofproto = args->ofproto;
> > +    int err;
> > +
> > +    err = dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
> > +    /* Add fail deleted meter to fail_meter_hmap and waiting to be
> deleted in
> > +     * revalidator. */
> > +    if (err == EPERM) {
> > +        id_hmap_add(&ofproto->backer->fail_meter_hmap, args-
> >meter_id.uint32);
> > +    }
> 
> I think we should not track any of this in the ofproto layer. As this is a dpif
> problem, it should be completely hidden in the dpif layer. This should be easy
> if you remove the dependency on meter_id as suggested above.
> 
> >
> > -    dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
> >      id_pool_free_id(ofproto->backer->meter_ids, args->meter_id.uint32);
> >      free(args);
> >  }
> > diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index
> > d8e0cd37ac5b..befc68d13baa 100644
> > --- a/ofproto/ofproto-dpif.h
> > +++ b/ofproto/ofproto-dpif.h
> > @@ -61,6 +61,7 @@ struct ofproto_dpif;  struct uuid;  struct
> > xlate_cache;  struct xlate_ctx;
> > +struct id_pool;
> >
> >  /* Number of implemented OpenFlow tables. */  enum { N_TABLES = 255
> > }; @@ -260,6 +261,8 @@ struct dpif_backer {
> >
> >      /* Meter. */
> >      struct id_pool *meter_ids;     /* Datapath meter allocation. */
> > +    struct hmap fail_meter_hmap;   /* Hashmap for meter with failed
> deletion
> > +                                    * in datapath */
> >
> >      /* Connection tracking. */
> >      struct id_pool *tp_ids;             /* Datapath timeout policy id
> > --
> > 2.30.2
> 
> Please add a test to 'make check-offloads'.

``````
## ------------------------------ ##
## openvswitch 3.1.90 test suite. ##
## ------------------------------ ##

datapath offloads

  1: offloads - ping between two ports - offloads disabled ok
  2: offloads - ping between two ports - offloads enabled ok
  3: offloads - set ingress_policing_rate and ingress_policing_burst - offloads disabled ok
  4: offloads - set ingress_policing_rate and ingress_policing_burst - offloads enabled ok
  5: offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads disabled ok
  6: offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads enabled ok
  7: offloads - check interface meter offloading -  offloads disabled ok
  8: offloads - check interface meter offloading -  offloads enabled ok
  9: offloads - check_pkt_len action - offloads disabled FAILED (system-offloads-traffic.at:328)
 10: offloads - check_pkt_len action - offloads enabled ok

## ------------- ##
## Test results. ##
## ------------- ##

ERROR: All 10 tests were run,
1 failed unexpectedly.
## ------------------------------------------ ##
## system-offloads-testsuite.log was created. ##
## ------------------------------------------ ##

Please send `tests/system-offloads-testsuite.log' and all information you think might help:

   To: <bugs@openvswitch.org>
   Subject: [openvswitch 3.1.90] system-offloads-testsuite: 9 failed

You may investigate any problem if you feel able to do so, in which
case the test suite provides a good starting point.  Its output may
be found below `tests/system-offloads-testsuite.dir'.

make: *** [Makefile:7034: check-offloads] Error 1
``````
Eelco Chaudron Feb. 6, 2023, 11:35 a.m. UTC | #4 
On 6 Feb 2023, at 8:48, Tianyu Yuan wrote:

> On Fri 2/3/2023 8:35 PM, Eelco Chaudron wrote:
>>
>> On 30 Jan 2023, at 17:42, Simon Horman wrote:
>>
>>> From: Tianyu Yuan <tianyu.yuan@corigine.com>
>>>
>>> Allow revalidator to continuously delete police in kernel tc datapath
>>> until it is deleted.
>>>
>>> In current implementation, polices in tc datapath will not deleted
>>> when they are being used and these remaining polices will be cleared
>>> when the openvswitch restarts.
>>>
>>> This patch supports revalidator to delete remaining polices in tc
>>> datapath by storing meter_id deleted unsuccessfully in a hmap and
>>> continuously deleting them in revalidator until success.
>>
>> Hi Tianyu,
>>
>> Thanks for the new revision of the patch...
>>
>> Were you able to figure out if we really need to do this during revalidating
>> flows?
>>
>> I was wondering if there would be a different way, i.e. on flow deletion see if
>> it was the last flow using this meter and if so remove it then. I guess this can
>> be done with the meter having a flag that it has been deleted.
>>
>> This would be way more elegant than doing this in the revalidator thread.
>>
>> See some inline comments below. This was a quick review/tests, as the
>> above might require a different approach.
>>
>> Cheers,
>>
>> Eelco
>>
>
> Hi Eelco,
>
> Thanks for your review and comments. I've got your point of view:
> 1. Another approach that trigger tc police deletion only when the last flow using this
> meter.

Not sure if this is possible, but if, it will be more clean and hidden in the tc layers.

> 2. depend the deletion on fail deletion of police_index rather than that of meter_id,
> keep the operation in dpif layer and this should be invisible from ofproto layer.

Yes, if the above is not possible, we should keep all this handling in the offload layer for TC.

> Please point me out if I misunderstood.
>
> The AT test of system-offload is attached at the end of this mail.
> The error "9: offloads - check_pkt_len action - offloads disabled" also happens on
> Upstream master branch so it is unrelated to this patch

I’m asking for a new test case that shows this problem. i.e. it will fail without your patch applied but will pass with it.

Thanks,

Eelco

> Cheers
> Tianyu
>
>>> Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com>
>>> Signed-off-by: Simon Horman <simon.horman@corigine.com>
>>>
>>> ---
>>>  lib/dpif-netdev.c             |  1 +
>>>  lib/dpif-netlink.c            | 26 +++++++++++++++++++++++++-
>>>  lib/dpif-provider.h           |  4 ++++
>>>  lib/dpif.c                    |  7 +++++++
>>>  lib/dpif.h                    |  2 ++
>>>  lib/netdev-offload-tc.c       |  5 ++++-
>>>  lib/netdev-offload.c          |  8 +++++---
>>>  ofproto/ofproto-dpif-upcall.c |  6 ++++++
>>>  ofproto/ofproto-dpif.c        | 10 +++++++++-
>>>  ofproto/ofproto-dpif.h        |  3 +++
>>>  10 files changed, 66 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
>>> c9f7179c3b4c..878fe5933d1a 100644
>>> --- a/lib/dpif-netdev.c
>>> +++ b/lib/dpif-netdev.c
>>> @@ -9689,6 +9689,7 @@ const struct dpif_class dpif_netdev_class = {
>>>      dpif_netdev_meter_set,
>>>      dpif_netdev_meter_get,
>>>      dpif_netdev_meter_del,
>>> +    NULL,
>>>      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
>>> 026b0daa8d83..8b95912aea97 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>
>>> @@ -37,6 +38,7 @@
>>>  #include "dpif-provider.h"
>>>  #include "fat-rwlock.h"
>>>  #include "flow.h"
>>> +#include "id-pool.h"
>>>  #include "netdev-linux.h"
>>>  #include "netdev-offload.h"
>>>  #include "netdev-provider.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"
>>> @@ -4363,12 +4366,32 @@ dpif_netlink_meter_del(struct dpif *dpif,
>> ofproto_meter_id meter_id,
>>>      err  = dpif_netlink_meter_get_stats(dpif, meter_id, stats,
>>>                                          max_bands, OVS_METER_CMD_DEL);
>>>      if (!err && netdev_is_flow_api_enabled()) {
>>> -        meter_offload_del(meter_id, stats);
>>> +        return meter_offload_del(meter_id, stats);
>>>      }
>>>
>>>      return err;
>>>  }
>>>
>>> +static void
>>> +dpif_netlink_meter_revalidate(struct dpif *dpif_ OVS_UNUSED,
>>> +                              struct hmap *meter_map) {
>>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>>> +    ofproto_meter_id meter_id;
>>> +    struct id_node *id_node;
>>> +
>>> +    HMAP_FOR_EACH_POP (id_node, node, meter_map) {
>>> +        meter_id.uint32 = id_node->id;
>>> +
>>> +        if (meter_offload_del(meter_id, NULL) == EPERM) {
>>> +            id_hmap_add(meter_map, meter_id.uint32);
>>> +        } else {
>>> +            VLOG_DBG_RL(&rl, "Delete meter %u in datapath success",
>>> +                        meter_id.uint32);
>>> +        }
>>> +    }
>>> +}
>>> +
>>>  static bool
>>>  probe_broken_meters__(struct dpif *dpif)  { @@ -4588,6 +4611,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
>>> 12477a24feee..794fbc21686a 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. */
>>> +    void (*meter_revalidate)(struct dpif *, struct hmap *meter_map);
>>> +
>>
>> I guess this is not really a revalidate action, it's more of a cleanup.
>> So maybe also call it something like meter_cleanup(), or
>> meter_garbage_collection.
>>
>> Or maybe even better, why not use the existing periodic 'bool (*run)(struct
>> dpif *dpif)' callback and handle it there?
>>
>>>      /* 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 fe4db83fbfee..c8718239117d 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;
>>>  }
>>>
>>> +void
>>> +dpif_meter_revalidate(struct dpif *dpif, struct hmap *meter_map){
>>> +    if (dpif->dpif_class->meter_revalidate) {
>>> +        dpif->dpif_class->meter_revalidate(dpif, meter_map);
>>> +    }
>>> +}
>>> +
>>>  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..e181e4ee1d6c 100644
>>> --- a/lib/dpif.h
>>> +++ b/lib/dpif.h
>>> @@ -378,6 +378,7 @@
>>>
>>>  #include "dpdk.h"
>>>  #include "dp-packet.h"
>>> +#include "id-pool.h"
>>>  #include "netdev.h"
>>>  #include "openflow/openflow.h"
>>>  #include "openvswitch/ofp-meter.h"
>>> @@ -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);
>>> +void dpif_meter_revalidate(struct dpif *dpif, struct hmap
>>> +*meter_map);
>>>
>>>  /* Bonding. */
>>>
>>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index
>>> 15d1c36aa04e..f87a5e05e202 100644
>>> --- a/lib/netdev-offload-tc.c
>>> +++ b/lib/netdev-offload-tc.c
>>> @@ -2980,8 +2980,11 @@ meter_tc_del_policer(ofproto_meter_id
>> meter_id,
>>>                          police_index, meter_id.uint32, ovs_strerror(err));
>>>          } else {
>>>              meter_free_police_index(police_index);
>>> +            /* Do not remove the mapping between meter_id and police_index
>>> +             * until this meter is deleted successfully in datapath. This
>>> +             * mapping will be used in meter deletion in revalidator. */
>>> +            meter_id_remove(meter_id.uint32);
>>
>> This approach might have problem, what if the policy gets deleted, and re-
>> added with a new configuration?
>> The datapath flows are not cleared, and the add will fail as the meter_id is
>> still in use.
>>
>> Guess we need to keep the police index as this is tc specific, the meter_id is
>> global.
>>
>>>          }
>>> -        meter_id_remove(meter_id.uint32);
>>>      }
>>>
>>>      return err;
>>> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index
>>> 4592262bd34e..0769f796e5b7 100644
>>> --- a/lib/netdev-offload.c
>>> +++ b/lib/netdev-offload.c
>>> @@ -241,19 +241,21 @@ int
>>>  meter_offload_del(ofproto_meter_id meter_id, struct
>>> ofputil_meter_stats *stats)  {
>>>      struct netdev_registered_flow_api *rfa;
>>> +    int ret = 0;
>>>
>>>      CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
>>>          if (rfa->flow_api->meter_del) {
>>> -            int ret = rfa->flow_api->meter_del(meter_id, stats);
>>> -            if (ret) {
>>> +            int err = rfa->flow_api->meter_del(meter_id, stats);
>>> +            if (err) {
>>>                  VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow api %s, "
>>>                              "error %d", meter_id.uint32, rfa->flow_api->type,
>>>                              ret);
>>> +                ret = err;
>>>              }
>>>          }
>>>      }
>>>
>>> -    return 0;
>>> +    return ret;
>>>  }
>>>
>>>  int
>>> diff --git a/ofproto/ofproto-dpif-upcall.c
>>> b/ofproto/ofproto-dpif-upcall.c index 31ac02d116fc..b802c9c7f13a
>>> 100644
>>> --- a/ofproto/ofproto-dpif-upcall.c
>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>> @@ -2828,6 +2828,12 @@ revalidate(struct revalidator *revalidator)
>>>          }
>>>          ovsrcu_quiesce();
>>>      }
>>> +    /* When fail_meter_hmap is not empty, revalidate to delete meters in
>> this
>>> +     * hashmap. */
>>> +    if (hmap_count(&udpif->backer->fail_meter_hmap)) {
>>> +        dpif_meter_revalidate(udpif->dpif,
>>> + &udpif->backer->fail_meter_hmap);
>>
>> This could be removed with the general run() implementation. But in general
>> we should not have this hmap, just call the callback. See below.
>>
>>> +    }
>>> +
>>>      dpif_flow_dump_thread_destroy(dump_thread);
>>>      ofpbuf_uninit(&odp_actions);
>>>  }
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index
>>> f87e27a8cd7f..591af5503765 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -835,6 +835,7 @@ open_dpif_backer(const char *type, struct
>> dpif_backer **backerp)
>>>      dpif_meter_get_features(backer->dpif, &features);
>>>      if (features.max_meters) {
>>>          backer->meter_ids = id_pool_create(0, features.max_meters);
>>> +        hmap_init(&backer->fail_meter_hmap);
>>>      } else {
>>>          backer->meter_ids = NULL;
>>>      }
>>> @@ -6790,8 +6791,15 @@ static void
>>>  free_meter_id(struct free_meter_id_args *args)  {
>>>      struct ofproto_dpif *ofproto = args->ofproto;
>>> +    int err;
>>> +
>>> +    err = dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
>>> +    /* Add fail deleted meter to fail_meter_hmap and waiting to be
>> deleted in
>>> +     * revalidator. */
>>> +    if (err == EPERM) {
>>> +        id_hmap_add(&ofproto->backer->fail_meter_hmap, args-
>>> meter_id.uint32);
>>> +    }
>>
>> I think we should not track any of this in the ofproto layer. As this is a dpif
>> problem, it should be completely hidden in the dpif layer. This should be easy
>> if you remove the dependency on meter_id as suggested above.
>>
>>>
>>> -    dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
>>>      id_pool_free_id(ofproto->backer->meter_ids, args->meter_id.uint32);
>>>      free(args);
>>>  }
>>> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index
>>> d8e0cd37ac5b..befc68d13baa 100644
>>> --- a/ofproto/ofproto-dpif.h
>>> +++ b/ofproto/ofproto-dpif.h
>>> @@ -61,6 +61,7 @@ struct ofproto_dpif;  struct uuid;  struct
>>> xlate_cache;  struct xlate_ctx;
>>> +struct id_pool;
>>>
>>>  /* Number of implemented OpenFlow tables. */  enum { N_TABLES = 255
>>> }; @@ -260,6 +261,8 @@ struct dpif_backer {
>>>
>>>      /* Meter. */
>>>      struct id_pool *meter_ids;     /* Datapath meter allocation. */
>>> +    struct hmap fail_meter_hmap;   /* Hashmap for meter with failed
>> deletion
>>> +                                    * in datapath */
>>>
>>>      /* Connection tracking. */
>>>      struct id_pool *tp_ids;             /* Datapath timeout policy id
>>> --
>>> 2.30.2
>>
>> Please add a test to 'make check-offloads'.
>
> ``````
> ## ------------------------------ ##
> ## openvswitch 3.1.90 test suite. ##
> ## ------------------------------ ##
>
> datapath offloads
>
>   1: offloads - ping between two ports - offloads disabled ok
>   2: offloads - ping between two ports - offloads enabled ok
>   3: offloads - set ingress_policing_rate and ingress_policing_burst - offloads disabled ok
>   4: offloads - set ingress_policing_rate and ingress_policing_burst - offloads enabled ok
>   5: offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads disabled ok
>   6: offloads - set ingress_policing_kpkts_rate and ingress_policing_kpkts_burst - offloads enabled ok
>   7: offloads - check interface meter offloading -  offloads disabled ok
>   8: offloads - check interface meter offloading -  offloads enabled ok
>   9: offloads - check_pkt_len action - offloads disabled FAILED (system-offloads-traffic.at:328)
>  10: offloads - check_pkt_len action - offloads enabled ok
>
> ## ------------- ##
> ## Test results. ##
> ## ------------- ##
>
> ERROR: All 10 tests were run,
> 1 failed unexpectedly.
> ## ------------------------------------------ ##
> ## system-offloads-testsuite.log was created. ##
> ## ------------------------------------------ ##
>
> Please send `tests/system-offloads-testsuite.log' and all information you think might help:
>
>    To: <bugs@openvswitch.org>
>    Subject: [openvswitch 3.1.90] system-offloads-testsuite: 9 failed
>
> You may investigate any problem if you feel able to do so, in which
> case the test suite provides a good starting point.  Its output may
> be found below `tests/system-offloads-testsuite.dir'.
>
> make: *** [Makefile:7034: check-offloads] Error 1
> ``````
Tianyu Yuan Feb. 7, 2023, 1:56 a.m. UTC | #5 
On Mon 2/6/2023 7:35 PM, Eelco Chaudron wrote:
> 
> On 6 Feb 2023, at 8:48, Tianyu Yuan wrote:
> 
> > On Fri 2/3/2023 8:35 PM, Eelco Chaudron wrote:
> >>
> >> On 30 Jan 2023, at 17:42, Simon Horman wrote:
> >>
> >>> From: Tianyu Yuan <tianyu.yuan@corigine.com>
> >>>
> >>> Allow revalidator to continuously delete police in kernel tc
> >>> datapath until it is deleted.
> >>>
> >>> In current implementation, polices in tc datapath will not deleted
> >>> when they are being used and these remaining polices will be cleared
> >>> when the openvswitch restarts.
> >>>
> >>> This patch supports revalidator to delete remaining polices in tc
> >>> datapath by storing meter_id deleted unsuccessfully in a hmap and
> >>> continuously deleting them in revalidator until success.
> >>
> >> Hi Tianyu,
> >>
> >> Thanks for the new revision of the patch...
> >>
> >> Were you able to figure out if we really need to do this during
> >> revalidating flows?
> >>
> >> I was wondering if there would be a different way, i.e. on flow
> >> deletion see if it was the last flow using this meter and if so
> >> remove it then. I guess this can be done with the meter having a flag that
> it has been deleted.
> >>
> >> This would be way more elegant than doing this in the revalidator thread.
> >>
> >> See some inline comments below. This was a quick review/tests, as the
> >> above might require a different approach.
> >>
> >> Cheers,
> >>
> >> Eelco
> >>
> >
> > Hi Eelco,
> >
> > Thanks for your review and comments. I've got your point of view:
> > 1. Another approach that trigger tc police deletion only when the last
> > flow using this meter.
> 
> Not sure if this is possible, but if, it will be more clean and hidden in the tc
> layers.
> 
> > 2. depend the deletion on fail deletion of police_index rather than
> > that of meter_id, keep the operation in dpif layer and this should be
> invisible from ofproto layer.
> 
> Yes, if the above is not possible, we should keep all this handling in the
> offload layer for TC.
> 
> > Please point me out if I misunderstood.
> >
> > The AT test of system-offload is attached at the end of this mail.
> > The error "9: offloads - check_pkt_len action - offloads disabled"
> > also happens on Upstream master branch so it is unrelated to this
> > patch
> 
> I’m asking for a new test case that shows this problem. i.e. it will fail without
> your patch applied but will pass with it.

Ok, I will consider your suggestions and attach the conresponding AT test script in
V4 patch.

Thanks,

Tianyu
> 
> Thanks,
> 
> Eelco
> 
> > Cheers
> > Tianyu
> >
> >>> Signed-off-by: Tianyu Yuan <tianyu.yuan@corigine.com>
> >>> Signed-off-by: Simon Horman <simon.horman@corigine.com>
> >>>
> >>> ---
> >>>  lib/dpif-netdev.c             |  1 +
> >>>  lib/dpif-netlink.c            | 26 +++++++++++++++++++++++++-
> >>>  lib/dpif-provider.h           |  4 ++++
> >>>  lib/dpif.c                    |  7 +++++++
> >>>  lib/dpif.h                    |  2 ++
> >>>  lib/netdev-offload-tc.c       |  5 ++++-
> >>>  lib/netdev-offload.c          |  8 +++++---
> >>>  ofproto/ofproto-dpif-upcall.c |  6 ++++++
> >>>  ofproto/ofproto-dpif.c        | 10 +++++++++-
> >>>  ofproto/ofproto-dpif.h        |  3 +++
> >>>  10 files changed, 66 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> >>> c9f7179c3b4c..878fe5933d1a 100644
> >>> --- a/lib/dpif-netdev.c
> >>> +++ b/lib/dpif-netdev.c
> >>> @@ -9689,6 +9689,7 @@ const struct dpif_class dpif_netdev_class = {
> >>>      dpif_netdev_meter_set,
> >>>      dpif_netdev_meter_get,
> >>>      dpif_netdev_meter_del,
> >>> +    NULL,
> >>>      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
> >>> 026b0daa8d83..8b95912aea97 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>
> >>> @@ -37,6 +38,7 @@
> >>>  #include "dpif-provider.h"
> >>>  #include "fat-rwlock.h"
> >>>  #include "flow.h"
> >>> +#include "id-pool.h"
> >>>  #include "netdev-linux.h"
> >>>  #include "netdev-offload.h"
> >>>  #include "netdev-provider.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"
> >>> @@ -4363,12 +4366,32 @@ dpif_netlink_meter_del(struct dpif *dpif,
> >> ofproto_meter_id meter_id,
> >>>      err  = dpif_netlink_meter_get_stats(dpif, meter_id, stats,
> >>>                                          max_bands, OVS_METER_CMD_DEL);
> >>>      if (!err && netdev_is_flow_api_enabled()) {
> >>> -        meter_offload_del(meter_id, stats);
> >>> +        return meter_offload_del(meter_id, stats);
> >>>      }
> >>>
> >>>      return err;
> >>>  }
> >>>
> >>> +static void
> >>> +dpif_netlink_meter_revalidate(struct dpif *dpif_ OVS_UNUSED,
> >>> +                              struct hmap *meter_map) {
> >>> +    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> >>> +    ofproto_meter_id meter_id;
> >>> +    struct id_node *id_node;
> >>> +
> >>> +    HMAP_FOR_EACH_POP (id_node, node, meter_map) {
> >>> +        meter_id.uint32 = id_node->id;
> >>> +
> >>> +        if (meter_offload_del(meter_id, NULL) == EPERM) {
> >>> +            id_hmap_add(meter_map, meter_id.uint32);
> >>> +        } else {
> >>> +            VLOG_DBG_RL(&rl, "Delete meter %u in datapath success",
> >>> +                        meter_id.uint32);
> >>> +        }
> >>> +    }
> >>> +}
> >>> +
> >>>  static bool
> >>>  probe_broken_meters__(struct dpif *dpif)  { @@ -4588,6 +4611,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
> >>> 12477a24feee..794fbc21686a 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. */
> >>> +    void (*meter_revalidate)(struct dpif *, struct hmap
> >>> + *meter_map);
> >>> +
> >>
> >> I guess this is not really a revalidate action, it's more of a cleanup.
> >> So maybe also call it something like meter_cleanup(), or
> >> meter_garbage_collection.
> >>
> >> Or maybe even better, why not use the existing periodic 'bool
> >> (*run)(struct dpif *dpif)' callback and handle it there?
> >>
> >>>      /* 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 fe4db83fbfee..c8718239117d 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;
> >>>  }
> >>>
> >>> +void
> >>> +dpif_meter_revalidate(struct dpif *dpif, struct hmap *meter_map){
> >>> +    if (dpif->dpif_class->meter_revalidate) {
> >>> +        dpif->dpif_class->meter_revalidate(dpif, meter_map);
> >>> +    }
> >>> +}
> >>> +
> >>>  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..e181e4ee1d6c 100644
> >>> --- a/lib/dpif.h
> >>> +++ b/lib/dpif.h
> >>> @@ -378,6 +378,7 @@
> >>>
> >>>  #include "dpdk.h"
> >>>  #include "dp-packet.h"
> >>> +#include "id-pool.h"
> >>>  #include "netdev.h"
> >>>  #include "openflow/openflow.h"
> >>>  #include "openvswitch/ofp-meter.h"
> >>> @@ -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);
> >>> +void dpif_meter_revalidate(struct dpif *dpif, struct hmap
> >>> +*meter_map);
> >>>
> >>>  /* Bonding. */
> >>>
> >>> diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c index
> >>> 15d1c36aa04e..f87a5e05e202 100644
> >>> --- a/lib/netdev-offload-tc.c
> >>> +++ b/lib/netdev-offload-tc.c
> >>> @@ -2980,8 +2980,11 @@ meter_tc_del_policer(ofproto_meter_id
> >> meter_id,
> >>>                          police_index, meter_id.uint32, ovs_strerror(err));
> >>>          } else {
> >>>              meter_free_police_index(police_index);
> >>> +            /* Do not remove the mapping between meter_id and
> police_index
> >>> +             * until this meter is deleted successfully in datapath. This
> >>> +             * mapping will be used in meter deletion in revalidator. */
> >>> +            meter_id_remove(meter_id.uint32);
> >>
> >> This approach might have problem, what if the policy gets deleted,
> >> and re- added with a new configuration?
> >> The datapath flows are not cleared, and the add will fail as the
> >> meter_id is still in use.
> >>
> >> Guess we need to keep the police index as this is tc specific, the
> >> meter_id is global.
> >>
> >>>          }
> >>> -        meter_id_remove(meter_id.uint32);
> >>>      }
> >>>
> >>>      return err;
> >>> diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c index
> >>> 4592262bd34e..0769f796e5b7 100644
> >>> --- a/lib/netdev-offload.c
> >>> +++ b/lib/netdev-offload.c
> >>> @@ -241,19 +241,21 @@ int
> >>>  meter_offload_del(ofproto_meter_id meter_id, struct
> >>> ofputil_meter_stats *stats)  {
> >>>      struct netdev_registered_flow_api *rfa;
> >>> +    int ret = 0;
> >>>
> >>>      CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
> >>>          if (rfa->flow_api->meter_del) {
> >>> -            int ret = rfa->flow_api->meter_del(meter_id, stats);
> >>> -            if (ret) {
> >>> +            int err = rfa->flow_api->meter_del(meter_id, stats);
> >>> +            if (err) {
> >>>                  VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow api %s, "
> >>>                              "error %d", meter_id.uint32, rfa->flow_api->type,
> >>>                              ret);
> >>> +                ret = err;
> >>>              }
> >>>          }
> >>>      }
> >>>
> >>> -    return 0;
> >>> +    return ret;
> >>>  }
> >>>
> >>>  int
> >>> diff --git a/ofproto/ofproto-dpif-upcall.c
> >>> b/ofproto/ofproto-dpif-upcall.c index 31ac02d116fc..b802c9c7f13a
> >>> 100644
> >>> --- a/ofproto/ofproto-dpif-upcall.c
> >>> +++ b/ofproto/ofproto-dpif-upcall.c
> >>> @@ -2828,6 +2828,12 @@ revalidate(struct revalidator *revalidator)
> >>>          }
> >>>          ovsrcu_quiesce();
> >>>      }
> >>> +    /* When fail_meter_hmap is not empty, revalidate to delete
> >>> + meters in
> >> this
> >>> +     * hashmap. */
> >>> +    if (hmap_count(&udpif->backer->fail_meter_hmap)) {
> >>> +        dpif_meter_revalidate(udpif->dpif,
> >>> + &udpif->backer->fail_meter_hmap);
> >>
> >> This could be removed with the general run() implementation. But in
> >> general we should not have this hmap, just call the callback. See below.
> >>
> >>> +    }
> >>> +
> >>>      dpif_flow_dump_thread_destroy(dump_thread);
> >>>      ofpbuf_uninit(&odp_actions);
> >>>  }
> >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index
> >>> f87e27a8cd7f..591af5503765 100644
> >>> --- a/ofproto/ofproto-dpif.c
> >>> +++ b/ofproto/ofproto-dpif.c
> >>> @@ -835,6 +835,7 @@ open_dpif_backer(const char *type, struct
> >> dpif_backer **backerp)
> >>>      dpif_meter_get_features(backer->dpif, &features);
> >>>      if (features.max_meters) {
> >>>          backer->meter_ids = id_pool_create(0, features.max_meters);
> >>> +        hmap_init(&backer->fail_meter_hmap);
> >>>      } else {
> >>>          backer->meter_ids = NULL;
> >>>      }
> >>> @@ -6790,8 +6791,15 @@ static void
> >>>  free_meter_id(struct free_meter_id_args *args)  {
> >>>      struct ofproto_dpif *ofproto = args->ofproto;
> >>> +    int err;
> >>> +
> >>> +    err = dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL,
> 0);
> >>> +    /* Add fail deleted meter to fail_meter_hmap and waiting to be
> >> deleted in
> >>> +     * revalidator. */
> >>> +    if (err == EPERM) {
> >>> +        id_hmap_add(&ofproto->backer->fail_meter_hmap, args-
> >>> meter_id.uint32);
> >>> +    }
> >>
> >> I think we should not track any of this in the ofproto layer. As this
> >> is a dpif problem, it should be completely hidden in the dpif layer.
> >> This should be easy if you remove the dependency on meter_id as
> suggested above.
> >>
> >>>
> >>> -    dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
> >>>      id_pool_free_id(ofproto->backer->meter_ids, args-
> >meter_id.uint32);
> >>>      free(args);
> >>>  }
> >>> diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h index
> >>> d8e0cd37ac5b..befc68d13baa 100644
> >>> --- a/ofproto/ofproto-dpif.h
> >>> +++ b/ofproto/ofproto-dpif.h
> >>> @@ -61,6 +61,7 @@ struct ofproto_dpif;  struct uuid;  struct
> >>> xlate_cache;  struct xlate_ctx;
> >>> +struct id_pool;
> >>>
> >>>  /* Number of implemented OpenFlow tables. */  enum { N_TABLES =
> 255
> >>> }; @@ -260,6 +261,8 @@ struct dpif_backer {
> >>>
> >>>      /* Meter. */
> >>>      struct id_pool *meter_ids;     /* Datapath meter allocation. */
> >>> +    struct hmap fail_meter_hmap;   /* Hashmap for meter with failed
> >> deletion
> >>> +                                    * in datapath */
> >>>
> >>>      /* Connection tracking. */
> >>>      struct id_pool *tp_ids;             /* Datapath timeout policy id
> >>> --
> >>> 2.30.2
> >>
> >> Please add a test to 'make check-offloads'.
> >
> > ``````
> > ## ------------------------------ ##
> > ## openvswitch 3.1.90 test suite. ##
> > ## ------------------------------ ##
> >
> > datapath offloads
> >
> >   1: offloads - ping between two ports - offloads disabled ok
> >   2: offloads - ping between two ports - offloads enabled ok
> >   3: offloads - set ingress_policing_rate and ingress_policing_burst -
> offloads disabled ok
> >   4: offloads - set ingress_policing_rate and ingress_policing_burst -
> offloads enabled ok
> >   5: offloads - set ingress_policing_kpkts_rate and
> ingress_policing_kpkts_burst - offloads disabled ok
> >   6: offloads - set ingress_policing_kpkts_rate and
> ingress_policing_kpkts_burst - offloads enabled ok
> >   7: offloads - check interface meter offloading -  offloads disabled ok
> >   8: offloads - check interface meter offloading -  offloads enabled ok
> >   9: offloads - check_pkt_len action - offloads disabled FAILED
> > (system-offloads-traffic.at:328)
> >  10: offloads - check_pkt_len action - offloads enabled ok
> >
> > ## ------------- ##
> > ## Test results. ##
> > ## ------------- ##
> >
> > ERROR: All 10 tests were run,
> > 1 failed unexpectedly.
> > ## ------------------------------------------ ## ##
> > system-offloads-testsuite.log was created. ## ##
> > ------------------------------------------ ##
> >
> > Please send `tests/system-offloads-testsuite.log' and all information you
> think might help:
> >
> >    To: <bugs@openvswitch.org>
> >    Subject: [openvswitch 3.1.90] system-offloads-testsuite: 9 failed
> >
> > You may investigate any problem if you feel able to do so, in which
> > case the test suite provides a good starting point.  Its output may be
> > found below `tests/system-offloads-testsuite.dir'.
> >
> > make: *** [Makefile:7034: check-offloads] Error 1 ``````
diff mbox series

Patch

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index c9f7179c3b4c..878fe5933d1a 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -9689,6 +9689,7 @@  const struct dpif_class dpif_netdev_class = {
     dpif_netdev_meter_set,
     dpif_netdev_meter_get,
     dpif_netdev_meter_del,
+    NULL,
     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 026b0daa8d83..8b95912aea97 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>
@@ -37,6 +38,7 @@ 
 #include "dpif-provider.h"
 #include "fat-rwlock.h"
 #include "flow.h"
+#include "id-pool.h"
 #include "netdev-linux.h"
 #include "netdev-offload.h"
 #include "netdev-provider.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"
@@ -4363,12 +4366,32 @@  dpif_netlink_meter_del(struct dpif *dpif, ofproto_meter_id meter_id,
     err  = dpif_netlink_meter_get_stats(dpif, meter_id, stats,
                                         max_bands, OVS_METER_CMD_DEL);
     if (!err && netdev_is_flow_api_enabled()) {
-        meter_offload_del(meter_id, stats);
+        return meter_offload_del(meter_id, stats);
     }
 
     return err;
 }
 
+static void
+dpif_netlink_meter_revalidate(struct dpif *dpif_ OVS_UNUSED,
+                              struct hmap *meter_map)
+{
+    static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
+    ofproto_meter_id meter_id;
+    struct id_node *id_node;
+
+    HMAP_FOR_EACH_POP (id_node, node, meter_map) {
+        meter_id.uint32 = id_node->id;
+
+        if (meter_offload_del(meter_id, NULL) == EPERM) {
+            id_hmap_add(meter_map, meter_id.uint32);
+        } else {
+            VLOG_DBG_RL(&rl, "Delete meter %u in datapath success",
+                        meter_id.uint32);
+        }
+    }
+}
+
 static bool
 probe_broken_meters__(struct dpif *dpif)
 {
@@ -4588,6 +4611,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 12477a24feee..794fbc21686a 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. */
+    void (*meter_revalidate)(struct dpif *, struct hmap *meter_map);
+
     /* 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 fe4db83fbfee..c8718239117d 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;
 }
 
+void
+dpif_meter_revalidate(struct dpif *dpif, struct hmap *meter_map){
+    if (dpif->dpif_class->meter_revalidate) {
+        dpif->dpif_class->meter_revalidate(dpif, meter_map);
+    }
+}
+
 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..e181e4ee1d6c 100644
--- a/lib/dpif.h
+++ b/lib/dpif.h
@@ -378,6 +378,7 @@ 
 
 #include "dpdk.h"
 #include "dp-packet.h"
+#include "id-pool.h"
 #include "netdev.h"
 #include "openflow/openflow.h"
 #include "openvswitch/ofp-meter.h"
@@ -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);
+void dpif_meter_revalidate(struct dpif *dpif, struct hmap *meter_map);
 
 /* Bonding. */
 
diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c
index 15d1c36aa04e..f87a5e05e202 100644
--- a/lib/netdev-offload-tc.c
+++ b/lib/netdev-offload-tc.c
@@ -2980,8 +2980,11 @@  meter_tc_del_policer(ofproto_meter_id meter_id,
                         police_index, meter_id.uint32, ovs_strerror(err));
         } else {
             meter_free_police_index(police_index);
+            /* Do not remove the mapping between meter_id and police_index
+             * until this meter is deleted successfully in datapath. This
+             * mapping will be used in meter deletion in revalidator. */
+            meter_id_remove(meter_id.uint32);
         }
-        meter_id_remove(meter_id.uint32);
     }
 
     return err;
diff --git a/lib/netdev-offload.c b/lib/netdev-offload.c
index 4592262bd34e..0769f796e5b7 100644
--- a/lib/netdev-offload.c
+++ b/lib/netdev-offload.c
@@ -241,19 +241,21 @@  int
 meter_offload_del(ofproto_meter_id meter_id, struct ofputil_meter_stats *stats)
 {
     struct netdev_registered_flow_api *rfa;
+    int ret = 0;
 
     CMAP_FOR_EACH (rfa, cmap_node, &netdev_flow_apis) {
         if (rfa->flow_api->meter_del) {
-            int ret = rfa->flow_api->meter_del(meter_id, stats);
-            if (ret) {
+            int err = rfa->flow_api->meter_del(meter_id, stats);
+            if (err) {
                 VLOG_DBG_RL(&rl, "Failed deleting meter %u for flow api %s, "
                             "error %d", meter_id.uint32, rfa->flow_api->type,
                             ret);
+                ret = err;
             }
         }
     }
 
-    return 0;
+    return ret;
 }
 
 int
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 31ac02d116fc..b802c9c7f13a 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -2828,6 +2828,12 @@  revalidate(struct revalidator *revalidator)
         }
         ovsrcu_quiesce();
     }
+    /* When fail_meter_hmap is not empty, revalidate to delete meters in this
+     * hashmap. */
+    if (hmap_count(&udpif->backer->fail_meter_hmap)) {
+        dpif_meter_revalidate(udpif->dpif, &udpif->backer->fail_meter_hmap);
+    }
+
     dpif_flow_dump_thread_destroy(dump_thread);
     ofpbuf_uninit(&odp_actions);
 }
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index f87e27a8cd7f..591af5503765 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -835,6 +835,7 @@  open_dpif_backer(const char *type, struct dpif_backer **backerp)
     dpif_meter_get_features(backer->dpif, &features);
     if (features.max_meters) {
         backer->meter_ids = id_pool_create(0, features.max_meters);
+        hmap_init(&backer->fail_meter_hmap);
     } else {
         backer->meter_ids = NULL;
     }
@@ -6790,8 +6791,15 @@  static void
 free_meter_id(struct free_meter_id_args *args)
 {
     struct ofproto_dpif *ofproto = args->ofproto;
+    int err;
+
+    err = dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
+    /* Add fail deleted meter to fail_meter_hmap and waiting to be deleted in
+     * revalidator. */
+    if (err == EPERM) {
+        id_hmap_add(&ofproto->backer->fail_meter_hmap, args->meter_id.uint32);
+    }
 
-    dpif_meter_del(ofproto->backer->dpif, args->meter_id, NULL, 0);
     id_pool_free_id(ofproto->backer->meter_ids, args->meter_id.uint32);
     free(args);
 }
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index d8e0cd37ac5b..befc68d13baa 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -61,6 +61,7 @@  struct ofproto_dpif;
 struct uuid;
 struct xlate_cache;
 struct xlate_ctx;
+struct id_pool;
 
 /* Number of implemented OpenFlow tables. */
 enum { N_TABLES = 255 };
@@ -260,6 +261,8 @@  struct dpif_backer {
 
     /* Meter. */
     struct id_pool *meter_ids;     /* Datapath meter allocation. */
+    struct hmap fail_meter_hmap;   /* Hashmap for meter with failed deletion
+                                    * in datapath */
 
     /* Connection tracking. */
     struct id_pool *tp_ids;             /* Datapath timeout policy id