[ovs-dev,v4] controller: Don't artificially limit group and meter IDs to 16bit.

OVS actually supports way more.  Detect the real number of groups and
meters instead. To avoid preallocating huge bitmaps for the IDs,
switch to id-pool instead (as suggested by Ilya).

Reported-at: https://issues.redhat.com/browse/FDP-70
Suggested-by: Ilya Maximets <i.maximets@ovn.org>
Signed-off-by: Dumitru Ceara <dceara@redhat.com>
Acked-by: Numan Siddique <numans@ovn.org>
---
V4:
- Addressed Numan's comment and added his ack:
  - copy the table name argument
  - kept Numan's ack as the other changes since v3 were not
    functionality related.
- Addressed Ilya's comments:
  - Removed _clear() from ovn_extend_table_reinit().
  - Fixed indentation.
  - Use better name for extend_table number of IDs.
  - Omit name in prototype when possible.
  - Changed "datapath capabilities" comment to "OpenFlow switch
    capabilities".
V3:
- Addressed Ilya's comments:
  - removed references to 'bitmap' in comments
  - fixed indentation & typos
  - moved "id-pool.h" include to C file
  - extended the OVS feature detection component to also check for the
    max number of groups.
V2:
- Addressed Ilya's comment: fixed the way id_pool_create() is called.
- renamed max_size to max_id, it's more accurate.
---
 controller/ofctrl.c         |  2 +
 controller/ovn-controller.c |  6 +--
 include/ovn/features.h      |  3 ++
 lib/extend-table.c          | 81 ++++++++++++++++++++--------------
 lib/extend-table.h          | 13 ++++--
 lib/features.c              | 88 ++++++++++++++++++++++++++++++-------
 tests/test-ovn.c            |  4 +-
 7 files changed, 139 insertions(+), 58 deletions(-)

On 10/31/23 18:00, Dumitru Ceara wrote:
> OVS actually supports way more.  Detect the real number of groups and
> meters instead. To avoid preallocating huge bitmaps for the IDs,
> switch to id-pool instead (as suggested by Ilya).
> 
> Reported-at: https://issues.redhat.com/browse/FDP-70
> Suggested-by: Ilya Maximets <i.maximets@ovn.org>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> Acked-by: Numan Siddique <numans@ovn.org>
> ---
> V4:
> - Addressed Numan's comment and added his ack:
>   - copy the table name argument
>   - kept Numan's ack as the other changes since v3 were not
>     functionality related.
> - Addressed Ilya's comments:
>   - Removed _clear() from ovn_extend_table_reinit().
>   - Fixed indentation.
>   - Use better name for extend_table number of IDs.
>   - Omit name in prototype when possible.
>   - Changed "datapath capabilities" comment to "OpenFlow switch
>     capabilities".
> V3:
> - Addressed Ilya's comments:
>   - removed references to 'bitmap' in comments
>   - fixed indentation & typos
>   - moved "id-pool.h" include to C file
>   - extended the OVS feature detection component to also check for the
>     max number of groups.
> V2:
> - Addressed Ilya's comment: fixed the way id_pool_create() is called.
> - renamed max_size to max_id, it's more accurate.
> ---
>  controller/ofctrl.c         |  2 +
>  controller/ovn-controller.c |  6 +--
>  include/ovn/features.h      |  3 ++
>  lib/extend-table.c          | 81 ++++++++++++++++++++--------------
>  lib/extend-table.h          | 13 ++++--
>  lib/features.c              | 88 ++++++++++++++++++++++++++++++-------
>  tests/test-ovn.c            |  4 +-
>  7 files changed, 139 insertions(+), 58 deletions(-)

Thanks for v4!  I didn't test it, so I'm not going to Ack.
But this version visually looks good to me.

Couple of tiny nitpicks below.

Best regards, Ilya Maximets.

> 
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 63b0aa975b..06f8b33232 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -677,11 +677,13 @@ run_S_CLEAR_FLOWS(void)
>      /* Clear existing groups, to match the state of the switch. */
>      if (groups) {
>          ovn_extend_table_clear(groups, true);
> +        ovn_extend_table_reinit(groups, ovs_feature_max_select_groups_get());
>      }
>  
>      /* Clear existing meters, to match the state of the switch. */
>      if (meters) {
>          ovn_extend_table_clear(meters, true);
> +        ovn_extend_table_reinit(meters, ovs_feature_max_meters_get());
>          ofctrl_meter_bands_clear();
>      }
>  
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index da7d145ed3..7a8ca38286 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -3948,8 +3948,8 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED,
>  {
>      struct ed_type_lflow_output *data = xzalloc(sizeof *data);
>      ovn_desired_flow_table_init(&data->flow_table);
> -    ovn_extend_table_init(&data->group_table);
> -    ovn_extend_table_init(&data->meter_table);
> +    ovn_extend_table_init(&data->group_table, "group-table", 0);
> +    ovn_extend_table_init(&data->meter_table, "meter-table", 0);
>      objdep_mgr_init(&data->lflow_deps_mgr);
>      lflow_conj_ids_init(&data->conj_ids);
>      uuidset_init(&data->objs_processed);
> @@ -5656,7 +5656,7 @@ main(int argc, char *argv[])
>                  engine_set_force_recompute(true);
>              }
>  
> -            if (br_int) {
> +            if (br_int && ovs_feature_set_discovered()) {
>                  ct_zones_data = engine_get_data(&en_ct_zones);
>                  if (ct_zones_data && ofctrl_run(br_int, ovs_table,
>                                                  &ct_zones_data->pending)) {
> diff --git a/include/ovn/features.h b/include/ovn/features.h
> index 7c3004b271..2c47ab766e 100644
> --- a/include/ovn/features.h
> +++ b/include/ovn/features.h
> @@ -49,5 +49,8 @@ void ovs_feature_support_destroy(void);
>  bool ovs_feature_is_supported(enum ovs_feature_value feature);
>  bool ovs_feature_support_run(const struct smap *ovs_capabilities,
>                               const char *br_name);
> +bool ovs_feature_set_discovered(void);
> +uint32_t ovs_feature_max_meters_get(void);
> +uint32_t ovs_feature_max_select_groups_get(void);
>  
>  #endif
> diff --git a/lib/extend-table.c b/lib/extend-table.c
> index ebb1a054cb..7f23587784 100644
> --- a/lib/extend-table.c
> +++ b/lib/extend-table.c
> @@ -17,9 +17,9 @@
>  #include <config.h>
>  #include <string.h>
>  
> -#include "bitmap.h"
>  #include "extend-table.h"
>  #include "hash.h"
> +#include "id-pool.h"
>  #include "lib/uuid.h"
>  #include "openvswitch/vlog.h"
>  
> @@ -30,13 +30,28 @@ ovn_extend_table_delete_desired(struct ovn_extend_table *table,
>                                  struct ovn_extend_table_lflow_to_desired *l);
>  
>  void
> -ovn_extend_table_init(struct ovn_extend_table *table)
> +ovn_extend_table_init(struct ovn_extend_table *table, const char *table_name,
> +                      uint32_t n_ids)
>  {
> -    table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
> -    bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
> -    hmap_init(&table->desired);
> -    hmap_init(&table->lflow_to_desired);
> -    hmap_init(&table->existing);
> +    *table = (struct ovn_extend_table) {
> +        .name = xstrdup(table_name),
> +        .n_ids = n_ids,
> +        /* Table id 0 is invalid, set id-pool base to 1. */
> +        .table_ids = id_pool_create(1, n_ids),
> +        .desired = HMAP_INITIALIZER(&table->desired),
> +        .lflow_to_desired = HMAP_INITIALIZER(&table->lflow_to_desired),
> +        .existing = HMAP_INITIALIZER(&table->existing),
> +    };
> +}
> +
> +void
> +ovn_extend_table_reinit(struct ovn_extend_table *table, uint32_t n_ids)
> +{
> +    if (n_ids != table->n_ids) {
> +        id_pool_destroy(table->table_ids);
> +        table->table_ids = id_pool_create(1, n_ids);
> +        table->n_ids = n_ids;
> +    }
>  }
>  
>  static struct ovn_extend_table_info *
> @@ -117,13 +132,13 @@ ovn_extend_table_add_desired_to_lflow(struct ovn_extend_table *table,
>          ovs_list_init(&l->desired);
>          hmap_insert(&table->lflow_to_desired, &l->hmap_node,
>                      uuid_hash(lflow_uuid));
> -        VLOG_DBG("%s: add new lflow_to_desired entry "UUID_FMT,
> -                 __func__, UUID_ARGS(lflow_uuid));
> +        VLOG_DBG("%s: table %s: add new lflow_to_desired entry "UUID_FMT,
> +                 __func__, table->name, UUID_ARGS(lflow_uuid));
>      }
>  
>      ovs_list_insert(&l->desired, &r->list_node);
> -    VLOG_DBG("%s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
> -             __func__, UUID_ARGS(lflow_uuid), r->desired->name,
> +    VLOG_DBG("%s: table %s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
> +             __func__, table->name, UUID_ARGS(lflow_uuid), r->desired->name,
>               r->desired->table_id);
>  }
>  
> @@ -160,10 +175,11 @@ ovn_extend_info_add_lflow_ref(struct ovn_extend_table *table,
>  }
>  
>  static void
> -ovn_extend_info_del_lflow_ref(struct ovn_extend_table_lflow_ref *r)
> +ovn_extend_info_del_lflow_ref(struct ovn_extend_table *table,
> +                              struct ovn_extend_table_lflow_ref *r)
>  {
> -    VLOG_DBG("%s: name %s, lflow "UUID_FMT" n %"PRIuSIZE, __func__,
> -             r->desired->name, UUID_ARGS(&r->lflow_uuid),
> +    VLOG_DBG("%s: table %s: name %s, lflow "UUID_FMT" n %"PRIuSIZE, __func__,
> +             table->name, r->desired->name, UUID_ARGS(&r->lflow_uuid),
>               hmap_count(&r->desired->references));
>      hmap_remove(&r->desired->references, &r->hmap_node);
>      ovs_list_remove(&r->list_node);
> @@ -191,8 +207,8 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing)
>          if (g->peer) {
>              g->peer->peer = NULL;
>          } else {
> -            /* Unset the bitmap because the peer is deleted already. */
> -            bitmap_set0(table->table_ids, g->table_id);
> +            /* Unset the id because the peer is deleted already. */
> +            id_pool_free_id(table->table_ids, g->table_id);
>          }
>          ovn_extend_table_info_destroy(g);
>      }
> @@ -206,7 +222,8 @@ ovn_extend_table_destroy(struct ovn_extend_table *table)
>      hmap_destroy(&table->lflow_to_desired);
>      ovn_extend_table_clear(table, true);
>      hmap_destroy(&table->existing);
> -    bitmap_free(table->table_ids);
> +    id_pool_destroy(table->table_ids);
> +    free(table->name);
>  }
>  
>  /* Remove an entry from existing table */
> @@ -221,7 +238,7 @@ ovn_extend_table_remove_existing(struct ovn_extend_table *table,
>          existing->peer->peer = NULL;
>      } else {
>          /* Dealloc the ID. */
> -        bitmap_set0(table->table_ids, existing->table_id);
> +        id_pool_free_id(table->table_ids, existing->table_id);
>      }
>      ovn_extend_table_info_destroy(existing);
>  }
> @@ -234,15 +251,15 @@ ovn_extend_table_delete_desired(struct ovn_extend_table *table,
>      struct ovn_extend_table_lflow_ref *r;
>      LIST_FOR_EACH_SAFE (r, list_node, &l->desired) {
>          struct ovn_extend_table_info *e = r->desired;
> -        ovn_extend_info_del_lflow_ref(r);
> +        ovn_extend_info_del_lflow_ref(table, r);
>          if (hmap_is_empty(&e->references)) {
> -            VLOG_DBG("%s: %s, "UUID_FMT, __func__,
> -                     e->name, UUID_ARGS(&l->lflow_uuid));
> +            VLOG_DBG("%s: table %s: %s, "UUID_FMT, __func__,
> +                     table->name, e->name, UUID_ARGS(&l->lflow_uuid));
>              hmap_remove(&table->desired, &e->hmap_node);
>              if (e->peer) {
>                  e->peer->peer = NULL;
>              } else {
> -                bitmap_set0(table->table_ids, e->table_id);
> +                id_pool_free_id(table->table_ids, e->table_id);
>              }
>              ovn_extend_table_info_destroy(e);
>          }
> @@ -284,7 +301,7 @@ ovn_extend_table_sync(struct ovn_extend_table *table)
>      }
>  }
>  
> -/* Assign a new table ID for the table information from the bitmap.
> +/* Assign a new table ID for the table information from the ID pool.
>   * If it already exists, return the old ID. */
>  uint32_t
>  ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
> @@ -298,9 +315,9 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
>      /* Check whether we have non installed but allocated group_id. */
>      HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->desired) {
>          if (!strcmp(table_info->name, name)) {
> -            VLOG_DBG("ovn_externd_table_assign_id: reuse old id %"PRIu32
> -                     " for %s, used by lflow "UUID_FMT,
> -                     table_info->table_id, table_info->name,
> +            VLOG_DBG("ovn_extend_table_assign_id: table %s: "
> +                     "reuse old id %"PRIu32" for %s, used by lflow "UUID_FMT,
> +                     table->name, table_info->table_id, table_info->name,
>                       UUID_ARGS(&lflow_uuid));
>              ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid);
>              return table_info->table_id;
> @@ -320,15 +337,13 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
>  
>      if (!existing_info) {
>          /* Reserve a new id. */
> -        table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID + 1);
> -    }
> +        if (!id_pool_alloc_id(table->table_ids, &table_id)) {
> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>  
> -    if (table_id == MAX_EXT_TABLE_ID + 1) {
> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> -        VLOG_ERR_RL(&rl, "%"PRIu32" out of table ids.", table_id);
> -        return EXT_TABLE_ID_INVALID;
> +            VLOG_ERR_RL(&rl, "table %s: out of table ids.", table->name);
> +            return EXT_TABLE_ID_INVALID;
> +        }
>      }
> -    bitmap_set1(table->table_ids, table_id);
>  
>      table_info = ovn_extend_table_info_alloc(name, table_id, existing_info,
>                                               hash);
> diff --git a/lib/extend-table.h b/lib/extend-table.h
> index b43a146b4f..90e6e470d0 100644
> --- a/lib/extend-table.h
> +++ b/lib/extend-table.h
> @@ -17,18 +17,21 @@
>  #ifndef EXTEND_TABLE_H
>  #define EXTEND_TABLE_H 1
>  
> -#define MAX_EXT_TABLE_ID 65535
>  #define EXT_TABLE_ID_INVALID 0
>  
>  #include "openvswitch/hmap.h"
>  #include "openvswitch/list.h"
>  #include "openvswitch/uuid.h"
>  
> +struct id_pool;
> +
>  /* Used to manage expansion tables associated with Flow table,
>   * such as the Group Table or Meter Table. */
>  struct ovn_extend_table {
> -    unsigned long *table_ids;  /* Used as a bitmap with value set
> -                                * for allocated ids in either desired or
> +    char *name; /* Used to identify this table in a user friendly way,
> +                 * e.g., for logging. */
> +    uint32_t n_ids;
> +    struct id_pool *table_ids; /* Used to allocate ids in either desired or
>                                  * existing (or both).  If the same "name"
>                                  * exists in both desired and existing tables,
>                                  * they must share the same ID.  The "peer"
> @@ -81,7 +84,9 @@ struct ovn_extend_table_lflow_ref {
>      struct ovn_extend_table_info *desired;
>  };
>  
> -void ovn_extend_table_init(struct ovn_extend_table *);
> +void ovn_extend_table_init(struct ovn_extend_table *, const char *table_name,
> +                           uint32_t n_ids);
> +void ovn_extend_table_reinit(struct ovn_extend_table *, uint32_t n_ids);
>  
>  void ovn_extend_table_destroy(struct ovn_extend_table *);
>  
> diff --git a/lib/features.c b/lib/features.c
> index d24e8f6c5c..914f52d39c 100644
> --- a/lib/features.c
> +++ b/lib/features.c
> @@ -27,6 +27,7 @@
>  #include "openvswitch/rconn.h"
>  #include "openvswitch/ofp-msgs.h"
>  #include "openvswitch/ofp-meter.h"
> +#include "openvswitch/ofp-group.h"
>  #include "openvswitch/ofp-util.h"
>  #include "ovn/features.h"
>  
> @@ -81,6 +82,18 @@ static struct ovs_feature all_ovs_features[] = {
>  /* A bitmap of OVS features that have been detected as 'supported'. */
>  static uint32_t supported_ovs_features;
>  
> +/* Last set of received feature replies. */
> +static struct ofputil_meter_features ovs_meter_features_reply;
> +static struct ofputil_group_features ovs_group_features_reply;
> +
> +/* Currently discovered set of features. */
> +static struct ofputil_meter_features ovs_meter_features;
> +static struct ofputil_group_features ovs_group_features;
> +
> +/* Number of features replies still expected to receive for the requests
> + * we sent already. */
> +static uint32_t features_reply_expected;
> +
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>  
>  /* ovs-vswitchd connection. */
> @@ -145,11 +158,19 @@ ovs_feature_get_openflow_cap(const char *br_name)
>  
>      /* send new requests just after reconnect. */
>      if (conn_seq_no != rconn_get_connection_seqno(swconn)) {
> -        /* dump datapath meter capabilities. */
> +        features_reply_expected = 0;
> +
> +        /* Dump OpenFlow switch meter capabilities. */
>          msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
>                             rconn_get_version(swconn), 0);
>          rconn_send(swconn, msg, NULL);
> +        features_reply_expected++;
> +        /* Dump OpenFlow switch group capabilities. */
> +        msg = ofputil_encode_group_features_request(rconn_get_version(swconn));
> +        rconn_send(swconn, msg, NULL);
> +        features_reply_expected++;
>      }
> +    conn_seq_no = rconn_get_connection_seqno(swconn);
>  
>      bool ret = false;
>      for (int i = 0; i < 50; i++) {
> @@ -163,21 +184,13 @@ ovs_feature_get_openflow_cap(const char *br_name)
>          ofptype_decode(&type, oh);
>  
>          if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) {
> -            struct ofputil_meter_features mf;
> -            ofputil_decode_meter_features(oh, &mf);
> -
> -            bool old_state = supported_ovs_features & OVS_DP_METER_SUPPORT;
> -            bool new_state = mf.max_meters > 0;
> -
> -            if (old_state != new_state) {
> -                ret = true;
> -                if (new_state) {
> -                    supported_ovs_features |= OVS_DP_METER_SUPPORT;
> -                } else {
> -                    supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
> -                }
> -            }
> -            conn_seq_no = rconn_get_connection_seqno(swconn);
> +            ofputil_decode_meter_features(oh, &ovs_meter_features_reply);
> +            ovs_assert(features_reply_expected);
> +            features_reply_expected--;
> +        } else if (type == OFPTYPE_GROUP_FEATURES_STATS_REPLY) {
> +            ofputil_decode_group_features_reply(oh, &ovs_group_features_reply);
> +            ovs_assert(features_reply_expected);
> +            features_reply_expected--;
>          } else if (type == OFPTYPE_ECHO_REQUEST) {
>              rconn_send(swconn, ofputil_encode_echo_reply(oh), NULL);
>          }
> @@ -186,6 +199,26 @@ ovs_feature_get_openflow_cap(const char *br_name)
>      rconn_run_wait(swconn);
>      rconn_recv_wait(swconn);
>  
> +    /* If all feature replies were received, update the set of supported
> +     * features. */
> +    if (!features_reply_expected) {
> +        if (memcmp(&ovs_meter_features, &ovs_meter_features_reply,
> +                   sizeof ovs_meter_features_reply)) {
> +            ovs_meter_features = ovs_meter_features_reply;
> +            if (ovs_meter_features.max_meters) {
> +                supported_ovs_features |= OVS_DP_METER_SUPPORT;
> +            } else {
> +                supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
> +            }
> +            ret = true;
> +        }
> +        if (memcmp(&ovs_group_features, &ovs_group_features_reply,
> +                   sizeof ovs_group_features_reply)) {
> +            ovs_group_features = ovs_group_features_reply;
> +            ret = true;
> +        }
> +    }
> +
>      return ret;
>  }
>  
> @@ -229,3 +262,26 @@ ovs_feature_support_run(const struct smap *ovs_capabilities,
>      }
>      return updated;
>  }
> +
> +bool
> +ovs_feature_set_discovered(void)
> +{
> +    /* The supported feature set has been discovered if we're connected
> +     * to OvS and OVS replied to all our feature request messages. */

OvS and OVS ?

> +    return swconn && rconn_is_connected(swconn) &&
> +        features_reply_expected == 0;

Maybe indent this line to the 'swconn' ?

> +}
> +
> +/* Returns the number of meters the OVS datapath supports. */
> +uint32_t
> +ovs_feature_max_meters_get(void)
> +{
> +    return ovs_meter_features.max_meters;
> +}
> +
> +/* Returns the number of select groups the OVS datapath supports. */
> +uint32_t
> +ovs_feature_max_select_groups_get(void)
> +{
> +    return ovs_group_features.max_groups[OFPGT11_SELECT];
> +}
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 1f1e27b51f..aaf2825edc 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1300,11 +1300,11 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
>  
>      /* Initialize group ids. */
>      struct ovn_extend_table group_table;
> -    ovn_extend_table_init(&group_table);
> +    ovn_extend_table_init(&group_table, "group-table", OFPG_MAX);
>  
>      /* Initialize meter ids for QoS. */
>      struct ovn_extend_table meter_table;
> -    ovn_extend_table_init(&meter_table);
> +    ovn_extend_table_init(&meter_table, "meter-table", OFPM13_MAX);
>  
>      /* Initialize collector sets. */
>      struct flow_collector_ids collector_ids;

On Tue, Oct 31, 2023 at 6:01 PM Dumitru Ceara <dceara@redhat.com> wrote:

> OVS actually supports way more.  Detect the real number of groups and
> meters instead. To avoid preallocating huge bitmaps for the IDs,
> switch to id-pool instead (as suggested by Ilya).
>
> Reported-at: https://issues.redhat.com/browse/FDP-70
> Suggested-by: Ilya Maximets <i.maximets@ovn.org>
> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
> Acked-by: Numan Siddique <numans@ovn.org>
> ---
>

Hi Dumitru,
I have two small comments below that could be addressed during merge.


> V4:
> - Addressed Numan's comment and added his ack:
>   - copy the table name argument
>   - kept Numan's ack as the other changes since v3 were not
>     functionality related.
> - Addressed Ilya's comments:
>   - Removed _clear() from ovn_extend_table_reinit().
>   - Fixed indentation.
>   - Use better name for extend_table number of IDs.
>   - Omit name in prototype when possible.
>   - Changed "datapath capabilities" comment to "OpenFlow switch
>     capabilities".
> V3:
> - Addressed Ilya's comments:
>   - removed references to 'bitmap' in comments
>   - fixed indentation & typos
>   - moved "id-pool.h" include to C file
>   - extended the OVS feature detection component to also check for the
>     max number of groups.
> V2:
> - Addressed Ilya's comment: fixed the way id_pool_create() is called.
> - renamed max_size to max_id, it's more accurate.
> ---
>  controller/ofctrl.c         |  2 +
>  controller/ovn-controller.c |  6 +--
>  include/ovn/features.h      |  3 ++
>  lib/extend-table.c          | 81 ++++++++++++++++++++--------------
>  lib/extend-table.h          | 13 ++++--
>  lib/features.c              | 88 ++++++++++++++++++++++++++++++-------
>  tests/test-ovn.c            |  4 +-
>  7 files changed, 139 insertions(+), 58 deletions(-)
>
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 63b0aa975b..06f8b33232 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -677,11 +677,13 @@ run_S_CLEAR_FLOWS(void)
>      /* Clear existing groups, to match the state of the switch. */
>      if (groups) {
>          ovn_extend_table_clear(groups, true);
> +        ovn_extend_table_reinit(groups,
> ovs_feature_max_select_groups_get());
>      }
>
>      /* Clear existing meters, to match the state of the switch. */
>      if (meters) {
>          ovn_extend_table_clear(meters, true);
> +        ovn_extend_table_reinit(meters, ovs_feature_max_meters_get());
>          ofctrl_meter_bands_clear();
>      }
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index da7d145ed3..7a8ca38286 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -3948,8 +3948,8 @@ en_lflow_output_init(struct engine_node *node
> OVS_UNUSED,
>  {
>      struct ed_type_lflow_output *data = xzalloc(sizeof *data);
>      ovn_desired_flow_table_init(&data->flow_table);
> -    ovn_extend_table_init(&data->group_table);
> -    ovn_extend_table_init(&data->meter_table);
> +    ovn_extend_table_init(&data->group_table, "group-table", 0);
> +    ovn_extend_table_init(&data->meter_table, "meter-table", 0);
>      objdep_mgr_init(&data->lflow_deps_mgr);
>      lflow_conj_ids_init(&data->conj_ids);
>      uuidset_init(&data->objs_processed);
> @@ -5656,7 +5656,7 @@ main(int argc, char *argv[])
>                  engine_set_force_recompute(true);
>              }
>
> -            if (br_int) {
> +            if (br_int && ovs_feature_set_discovered()) {
>                  ct_zones_data = engine_get_data(&en_ct_zones);
>                  if (ct_zones_data && ofctrl_run(br_int, ovs_table,
>                                                  &ct_zones_data->pending))
> {
> diff --git a/include/ovn/features.h b/include/ovn/features.h
> index 7c3004b271..2c47ab766e 100644
> --- a/include/ovn/features.h
> +++ b/include/ovn/features.h
> @@ -49,5 +49,8 @@ void ovs_feature_support_destroy(void);
>  bool ovs_feature_is_supported(enum ovs_feature_value feature);
>  bool ovs_feature_support_run(const struct smap *ovs_capabilities,
>                               const char *br_name);
> +bool ovs_feature_set_discovered(void);
> +uint32_t ovs_feature_max_meters_get(void);
> +uint32_t ovs_feature_max_select_groups_get(void);
>
>  #endif
> diff --git a/lib/extend-table.c b/lib/extend-table.c
> index ebb1a054cb..7f23587784 100644
> --- a/lib/extend-table.c
> +++ b/lib/extend-table.c
> @@ -17,9 +17,9 @@
>  #include <config.h>
>  #include <string.h>
>
> -#include "bitmap.h"
>  #include "extend-table.h"
>  #include "hash.h"
> +#include "id-pool.h"
>  #include "lib/uuid.h"
>  #include "openvswitch/vlog.h"
>
> @@ -30,13 +30,28 @@ ovn_extend_table_delete_desired(struct
> ovn_extend_table *table,
>                                  struct ovn_extend_table_lflow_to_desired
> *l);
>
>  void
> -ovn_extend_table_init(struct ovn_extend_table *table)
> +ovn_extend_table_init(struct ovn_extend_table *table, const char
> *table_name,
> +                      uint32_t n_ids)
>  {
> -    table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
> -    bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
> -    hmap_init(&table->desired);
> -    hmap_init(&table->lflow_to_desired);
> -    hmap_init(&table->existing);
> +    *table = (struct ovn_extend_table) {
> +        .name = xstrdup(table_name),
> +        .n_ids = n_ids,
> +        /* Table id 0 is invalid, set id-pool base to 1. */
> +        .table_ids = id_pool_create(1, n_ids),
> +        .desired = HMAP_INITIALIZER(&table->desired),
> +        .lflow_to_desired = HMAP_INITIALIZER(&table->lflow_to_desired),
> +        .existing = HMAP_INITIALIZER(&table->existing),
> +    };
> +}
> +
> +void
> +ovn_extend_table_reinit(struct ovn_extend_table *table, uint32_t n_ids)
> +{
> +    if (n_ids != table->n_ids) {
>

nit: That's the only place where we use the n_ids, could we use the pool's
n_ids instead? It seems redundant to store twice the same thing.


> +        id_pool_destroy(table->table_ids);
> +        table->table_ids = id_pool_create(1, n_ids);
> +        table->n_ids = n_ids;
> +    }
>  }
>
>  static struct ovn_extend_table_info *
> @@ -117,13 +132,13 @@ ovn_extend_table_add_desired_to_lflow(struct
> ovn_extend_table *table,
>          ovs_list_init(&l->desired);
>          hmap_insert(&table->lflow_to_desired, &l->hmap_node,
>                      uuid_hash(lflow_uuid));
> -        VLOG_DBG("%s: add new lflow_to_desired entry "UUID_FMT,
> -                 __func__, UUID_ARGS(lflow_uuid));
> +        VLOG_DBG("%s: table %s: add new lflow_to_desired entry "UUID_FMT,
> +                 __func__, table->name, UUID_ARGS(lflow_uuid));
>      }
>
>      ovs_list_insert(&l->desired, &r->list_node);
> -    VLOG_DBG("%s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
> -             __func__, UUID_ARGS(lflow_uuid), r->desired->name,
> +    VLOG_DBG("%s: table %s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
> +             __func__, table->name, UUID_ARGS(lflow_uuid),
> r->desired->name,
>               r->desired->table_id);
>  }
>
> @@ -160,10 +175,11 @@ ovn_extend_info_add_lflow_ref(struct
> ovn_extend_table *table,
>  }
>
>  static void
> -ovn_extend_info_del_lflow_ref(struct ovn_extend_table_lflow_ref *r)
> +ovn_extend_info_del_lflow_ref(struct ovn_extend_table *table,
> +                              struct ovn_extend_table_lflow_ref *r)
>  {
> -    VLOG_DBG("%s: name %s, lflow "UUID_FMT" n %"PRIuSIZE, __func__,
> -             r->desired->name, UUID_ARGS(&r->lflow_uuid),
> +    VLOG_DBG("%s: table %s: name %s, lflow "UUID_FMT" n %"PRIuSIZE,
> __func__,
> +             table->name, r->desired->name, UUID_ARGS(&r->lflow_uuid),
>               hmap_count(&r->desired->references));
>      hmap_remove(&r->desired->references, &r->hmap_node);
>      ovs_list_remove(&r->list_node);
> @@ -191,8 +207,8 @@ ovn_extend_table_clear(struct ovn_extend_table *table,
> bool existing)
>          if (g->peer) {
>              g->peer->peer = NULL;
>          } else {
> -            /* Unset the bitmap because the peer is deleted already. */
> -            bitmap_set0(table->table_ids, g->table_id);
> +            /* Unset the id because the peer is deleted already. */
> +            id_pool_free_id(table->table_ids, g->table_id);
>          }
>          ovn_extend_table_info_destroy(g);
>      }
> @@ -206,7 +222,8 @@ ovn_extend_table_destroy(struct ovn_extend_table
> *table)
>      hmap_destroy(&table->lflow_to_desired);
>      ovn_extend_table_clear(table, true);
>      hmap_destroy(&table->existing);
> -    bitmap_free(table->table_ids);
> +    id_pool_destroy(table->table_ids);
> +    free(table->name);
>  }
>
>  /* Remove an entry from existing table */
> @@ -221,7 +238,7 @@ ovn_extend_table_remove_existing(struct
> ovn_extend_table *table,
>          existing->peer->peer = NULL;
>      } else {
>          /* Dealloc the ID. */
> -        bitmap_set0(table->table_ids, existing->table_id);
> +        id_pool_free_id(table->table_ids, existing->table_id);
>      }
>      ovn_extend_table_info_destroy(existing);
>  }
> @@ -234,15 +251,15 @@ ovn_extend_table_delete_desired(struct
> ovn_extend_table *table,
>      struct ovn_extend_table_lflow_ref *r;
>      LIST_FOR_EACH_SAFE (r, list_node, &l->desired) {
>          struct ovn_extend_table_info *e = r->desired;
> -        ovn_extend_info_del_lflow_ref(r);
> +        ovn_extend_info_del_lflow_ref(table, r);
>          if (hmap_is_empty(&e->references)) {
> -            VLOG_DBG("%s: %s, "UUID_FMT, __func__,
> -                     e->name, UUID_ARGS(&l->lflow_uuid));
> +            VLOG_DBG("%s: table %s: %s, "UUID_FMT, __func__,
> +                     table->name, e->name, UUID_ARGS(&l->lflow_uuid));
>              hmap_remove(&table->desired, &e->hmap_node);
>              if (e->peer) {
>                  e->peer->peer = NULL;
>              } else {
> -                bitmap_set0(table->table_ids, e->table_id);
> +                id_pool_free_id(table->table_ids, e->table_id);
>              }
>              ovn_extend_table_info_destroy(e);
>          }
> @@ -284,7 +301,7 @@ ovn_extend_table_sync(struct ovn_extend_table *table)
>      }
>  }
>
> -/* Assign a new table ID for the table information from the bitmap.
> +/* Assign a new table ID for the table information from the ID pool.
>   * If it already exists, return the old ID. */
>  uint32_t
>  ovn_extend_table_assign_id(struct ovn_extend_table *table, const char
> *name,
> @@ -298,9 +315,9 @@ ovn_extend_table_assign_id(struct ovn_extend_table
> *table, const char *name,
>      /* Check whether we have non installed but allocated group_id. */
>      HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash,
> &table->desired) {
>          if (!strcmp(table_info->name, name)) {
> -            VLOG_DBG("ovn_externd_table_assign_id: reuse old id %"PRIu32
> -                     " for %s, used by lflow "UUID_FMT,
> -                     table_info->table_id, table_info->name,
> +            VLOG_DBG("ovn_extend_table_assign_id: table %s: "
> +                     "reuse old id %"PRIu32" for %s, used by lflow
> "UUID_FMT,
> +                     table->name, table_info->table_id, table_info->name,
>                       UUID_ARGS(&lflow_uuid));
>              ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid);
>              return table_info->table_id;
> @@ -320,15 +337,13 @@ ovn_extend_table_assign_id(struct ovn_extend_table
> *table, const char *name,
>
>      if (!existing_info) {
>          /* Reserve a new id. */
> -        table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID +
> 1);
> -    }
> +        if (!id_pool_alloc_id(table->table_ids, &table_id)) {
> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>
> -    if (table_id == MAX_EXT_TABLE_ID + 1) {
> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
> -        VLOG_ERR_RL(&rl, "%"PRIu32" out of table ids.", table_id);
> -        return EXT_TABLE_ID_INVALID;
> +            VLOG_ERR_RL(&rl, "table %s: out of table ids.", table->name);
> +            return EXT_TABLE_ID_INVALID;
> +        }
>      }
> -    bitmap_set1(table->table_ids, table_id);
>
>      table_info = ovn_extend_table_info_alloc(name, table_id,
> existing_info,
>                                               hash);
> diff --git a/lib/extend-table.h b/lib/extend-table.h
> index b43a146b4f..90e6e470d0 100644
> --- a/lib/extend-table.h
> +++ b/lib/extend-table.h
> @@ -17,18 +17,21 @@
>  #ifndef EXTEND_TABLE_H
>  #define EXTEND_TABLE_H 1
>
> -#define MAX_EXT_TABLE_ID 65535
>  #define EXT_TABLE_ID_INVALID 0
>
>  #include "openvswitch/hmap.h"
>  #include "openvswitch/list.h"
>  #include "openvswitch/uuid.h"
>
> +struct id_pool;
> +
>  /* Used to manage expansion tables associated with Flow table,
>   * such as the Group Table or Meter Table. */
>  struct ovn_extend_table {
> -    unsigned long *table_ids;  /* Used as a bitmap with value set
> -                                * for allocated ids in either desired or
> +    char *name; /* Used to identify this table in a user friendly way,
> +                 * e.g., for logging. */
> +    uint32_t n_ids;
> +    struct id_pool *table_ids; /* Used to allocate ids in either desired
> or
>                                  * existing (or both).  If the same "name"
>                                  * exists in both desired and existing
> tables,
>                                  * they must share the same ID.  The "peer"
> @@ -81,7 +84,9 @@ struct ovn_extend_table_lflow_ref {
>      struct ovn_extend_table_info *desired;
>  };
>
> -void ovn_extend_table_init(struct ovn_extend_table *);
> +void ovn_extend_table_init(struct ovn_extend_table *, const char
> *table_name,
> +                           uint32_t n_ids);
> +void ovn_extend_table_reinit(struct ovn_extend_table *, uint32_t n_ids);
>
>  void ovn_extend_table_destroy(struct ovn_extend_table *);
>
> diff --git a/lib/features.c b/lib/features.c
> index d24e8f6c5c..914f52d39c 100644
> --- a/lib/features.c
> +++ b/lib/features.c
> @@ -27,6 +27,7 @@
>  #include "openvswitch/rconn.h"
>  #include "openvswitch/ofp-msgs.h"
>  #include "openvswitch/ofp-meter.h"
> +#include "openvswitch/ofp-group.h"
>  #include "openvswitch/ofp-util.h"
>  #include "ovn/features.h"
>
> @@ -81,6 +82,18 @@ static struct ovs_feature all_ovs_features[] = {
>  /* A bitmap of OVS features that have been detected as 'supported'. */
>  static uint32_t supported_ovs_features;
>
> +/* Last set of received feature replies. */
> +static struct ofputil_meter_features ovs_meter_features_reply;
> +static struct ofputil_group_features ovs_group_features_reply;
> +
> +/* Currently discovered set of features. */
> +static struct ofputil_meter_features ovs_meter_features;
> +static struct ofputil_group_features ovs_group_features;
> +
> +/* Number of features replies still expected to receive for the requests
> + * we sent already. */
> +static uint32_t features_reply_expected;
>

nit: n_features_reply_expected? The current name implies more bool and not
uint IMO.


> +
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>
>  /* ovs-vswitchd connection. */
> @@ -145,11 +158,19 @@ ovs_feature_get_openflow_cap(const char *br_name)
>
>      /* send new requests just after reconnect. */
>      if (conn_seq_no != rconn_get_connection_seqno(swconn)) {
> -        /* dump datapath meter capabilities. */
> +        features_reply_expected = 0;
> +
> +        /* Dump OpenFlow switch meter capabilities. */
>          msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
>                             rconn_get_version(swconn), 0);
>          rconn_send(swconn, msg, NULL);
> +        features_reply_expected++;
> +        /* Dump OpenFlow switch group capabilities. */
> +        msg =
> ofputil_encode_group_features_request(rconn_get_version(swconn));
> +        rconn_send(swconn, msg, NULL);
> +        features_reply_expected++;
>      }
> +    conn_seq_no = rconn_get_connection_seqno(swconn);
>
>      bool ret = false;
>      for (int i = 0; i < 50; i++) {
> @@ -163,21 +184,13 @@ ovs_feature_get_openflow_cap(const char *br_name)
>          ofptype_decode(&type, oh);
>
>          if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) {
> -            struct ofputil_meter_features mf;
> -            ofputil_decode_meter_features(oh, &mf);
> -
> -            bool old_state = supported_ovs_features &
> OVS_DP_METER_SUPPORT;
> -            bool new_state = mf.max_meters > 0;
> -
> -            if (old_state != new_state) {
> -                ret = true;
> -                if (new_state) {
> -                    supported_ovs_features |= OVS_DP_METER_SUPPORT;
> -                } else {
> -                    supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
> -                }
> -            }
> -            conn_seq_no = rconn_get_connection_seqno(swconn);
> +            ofputil_decode_meter_features(oh, &ovs_meter_features_reply);
> +            ovs_assert(features_reply_expected);
> +            features_reply_expected--;
> +        } else if (type == OFPTYPE_GROUP_FEATURES_STATS_REPLY) {
> +            ofputil_decode_group_features_reply(oh,
> &ovs_group_features_reply);
> +            ovs_assert(features_reply_expected);
> +            features_reply_expected--;
>          } else if (type == OFPTYPE_ECHO_REQUEST) {
>              rconn_send(swconn, ofputil_encode_echo_reply(oh), NULL);
>          }
> @@ -186,6 +199,26 @@ ovs_feature_get_openflow_cap(const char *br_name)
>      rconn_run_wait(swconn);
>      rconn_recv_wait(swconn);
>
> +    /* If all feature replies were received, update the set of supported
> +     * features. */
> +    if (!features_reply_expected) {
> +        if (memcmp(&ovs_meter_features, &ovs_meter_features_reply,
> +                   sizeof ovs_meter_features_reply)) {
> +            ovs_meter_features = ovs_meter_features_reply;
> +            if (ovs_meter_features.max_meters) {
> +                supported_ovs_features |= OVS_DP_METER_SUPPORT;
> +            } else {
> +                supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
> +            }
> +            ret = true;
> +        }
> +        if (memcmp(&ovs_group_features, &ovs_group_features_reply,
> +                   sizeof ovs_group_features_reply)) {
> +            ovs_group_features = ovs_group_features_reply;
> +            ret = true;
> +        }
> +    }
> +
>      return ret;
>  }
>
> @@ -229,3 +262,26 @@ ovs_feature_support_run(const struct smap
> *ovs_capabilities,
>      }
>      return updated;
>  }
> +
> +bool
> +ovs_feature_set_discovered(void)
> +{
> +    /* The supported feature set has been discovered if we're connected
> +     * to OvS and OVS replied to all our feature request messages. */
> +    return swconn && rconn_is_connected(swconn) &&
> +        features_reply_expected == 0;
> +}
> +
> +/* Returns the number of meters the OVS datapath supports. */
> +uint32_t
> +ovs_feature_max_meters_get(void)
> +{
> +    return ovs_meter_features.max_meters;
> +}
> +
> +/* Returns the number of select groups the OVS datapath supports. */
> +uint32_t
> +ovs_feature_max_select_groups_get(void)
> +{
> +    return ovs_group_features.max_groups[OFPGT11_SELECT];
> +}
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 1f1e27b51f..aaf2825edc 100644
> --- a/tests/test-ovn.c
> +++ b/tests/test-ovn.c
> @@ -1300,11 +1300,11 @@ test_parse_actions(struct ovs_cmdl_context *ctx
> OVS_UNUSED)
>
>      /* Initialize group ids. */
>      struct ovn_extend_table group_table;
> -    ovn_extend_table_init(&group_table);
> +    ovn_extend_table_init(&group_table, "group-table", OFPG_MAX);
>
>      /* Initialize meter ids for QoS. */
>      struct ovn_extend_table meter_table;
> -    ovn_extend_table_init(&meter_table);
> +    ovn_extend_table_init(&meter_table, "meter-table", OFPM13_MAX);
>
>      /* Initialize collector sets. */
>      struct flow_collector_ids collector_ids;
> --
> 2.39.3
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Other than that it looks good.

Acked-by: Ales Musil <amusil@redhat.com>

Thanks,
Ales

On 11/17/23 22:56, Ilya Maximets wrote:
> On 10/31/23 18:00, Dumitru Ceara wrote:
>> OVS actually supports way more.  Detect the real number of groups and
>> meters instead. To avoid preallocating huge bitmaps for the IDs,
>> switch to id-pool instead (as suggested by Ilya).
>>
>> Reported-at: https://issues.redhat.com/browse/FDP-70
>> Suggested-by: Ilya Maximets <i.maximets@ovn.org>
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>> Acked-by: Numan Siddique <numans@ovn.org>
>> ---
>> V4:
>> - Addressed Numan's comment and added his ack:
>>   - copy the table name argument
>>   - kept Numan's ack as the other changes since v3 were not
>>     functionality related.
>> - Addressed Ilya's comments:
>>   - Removed _clear() from ovn_extend_table_reinit().
>>   - Fixed indentation.
>>   - Use better name for extend_table number of IDs.
>>   - Omit name in prototype when possible.
>>   - Changed "datapath capabilities" comment to "OpenFlow switch
>>     capabilities".
>> V3:
>> - Addressed Ilya's comments:
>>   - removed references to 'bitmap' in comments
>>   - fixed indentation & typos
>>   - moved "id-pool.h" include to C file
>>   - extended the OVS feature detection component to also check for the
>>     max number of groups.
>> V2:
>> - Addressed Ilya's comment: fixed the way id_pool_create() is called.
>> - renamed max_size to max_id, it's more accurate.
>> ---
>>  controller/ofctrl.c         |  2 +
>>  controller/ovn-controller.c |  6 +--
>>  include/ovn/features.h      |  3 ++
>>  lib/extend-table.c          | 81 ++++++++++++++++++++--------------
>>  lib/extend-table.h          | 13 ++++--
>>  lib/features.c              | 88 ++++++++++++++++++++++++++++++-------
>>  tests/test-ovn.c            |  4 +-
>>  7 files changed, 139 insertions(+), 58 deletions(-)
> 
> Thanks for v4!  I didn't test it, so I'm not going to Ack.
> But this version visually looks good to me.
> 
> Couple of tiny nitpicks below.
> 

Thanks for having a look!

> Best regards, Ilya Maximets.
> 
>>
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index 63b0aa975b..06f8b33232 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -677,11 +677,13 @@ run_S_CLEAR_FLOWS(void)
>>      /* Clear existing groups, to match the state of the switch. */
>>      if (groups) {
>>          ovn_extend_table_clear(groups, true);
>> +        ovn_extend_table_reinit(groups, ovs_feature_max_select_groups_get());
>>      }
>>  
>>      /* Clear existing meters, to match the state of the switch. */
>>      if (meters) {
>>          ovn_extend_table_clear(meters, true);
>> +        ovn_extend_table_reinit(meters, ovs_feature_max_meters_get());
>>          ofctrl_meter_bands_clear();
>>      }
>>  
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index da7d145ed3..7a8ca38286 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -3948,8 +3948,8 @@ en_lflow_output_init(struct engine_node *node OVS_UNUSED,
>>  {
>>      struct ed_type_lflow_output *data = xzalloc(sizeof *data);
>>      ovn_desired_flow_table_init(&data->flow_table);
>> -    ovn_extend_table_init(&data->group_table);
>> -    ovn_extend_table_init(&data->meter_table);
>> +    ovn_extend_table_init(&data->group_table, "group-table", 0);
>> +    ovn_extend_table_init(&data->meter_table, "meter-table", 0);
>>      objdep_mgr_init(&data->lflow_deps_mgr);
>>      lflow_conj_ids_init(&data->conj_ids);
>>      uuidset_init(&data->objs_processed);
>> @@ -5656,7 +5656,7 @@ main(int argc, char *argv[])
>>                  engine_set_force_recompute(true);
>>              }
>>  
>> -            if (br_int) {
>> +            if (br_int && ovs_feature_set_discovered()) {
>>                  ct_zones_data = engine_get_data(&en_ct_zones);
>>                  if (ct_zones_data && ofctrl_run(br_int, ovs_table,
>>                                                  &ct_zones_data->pending)) {
>> diff --git a/include/ovn/features.h b/include/ovn/features.h
>> index 7c3004b271..2c47ab766e 100644
>> --- a/include/ovn/features.h
>> +++ b/include/ovn/features.h
>> @@ -49,5 +49,8 @@ void ovs_feature_support_destroy(void);
>>  bool ovs_feature_is_supported(enum ovs_feature_value feature);
>>  bool ovs_feature_support_run(const struct smap *ovs_capabilities,
>>                               const char *br_name);
>> +bool ovs_feature_set_discovered(void);
>> +uint32_t ovs_feature_max_meters_get(void);
>> +uint32_t ovs_feature_max_select_groups_get(void);
>>  
>>  #endif
>> diff --git a/lib/extend-table.c b/lib/extend-table.c
>> index ebb1a054cb..7f23587784 100644
>> --- a/lib/extend-table.c
>> +++ b/lib/extend-table.c
>> @@ -17,9 +17,9 @@
>>  #include <config.h>
>>  #include <string.h>
>>  
>> -#include "bitmap.h"
>>  #include "extend-table.h"
>>  #include "hash.h"
>> +#include "id-pool.h"
>>  #include "lib/uuid.h"
>>  #include "openvswitch/vlog.h"
>>  
>> @@ -30,13 +30,28 @@ ovn_extend_table_delete_desired(struct ovn_extend_table *table,
>>                                  struct ovn_extend_table_lflow_to_desired *l);
>>  
>>  void
>> -ovn_extend_table_init(struct ovn_extend_table *table)
>> +ovn_extend_table_init(struct ovn_extend_table *table, const char *table_name,
>> +                      uint32_t n_ids)
>>  {
>> -    table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
>> -    bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
>> -    hmap_init(&table->desired);
>> -    hmap_init(&table->lflow_to_desired);
>> -    hmap_init(&table->existing);
>> +    *table = (struct ovn_extend_table) {
>> +        .name = xstrdup(table_name),
>> +        .n_ids = n_ids,
>> +        /* Table id 0 is invalid, set id-pool base to 1. */
>> +        .table_ids = id_pool_create(1, n_ids),
>> +        .desired = HMAP_INITIALIZER(&table->desired),
>> +        .lflow_to_desired = HMAP_INITIALIZER(&table->lflow_to_desired),
>> +        .existing = HMAP_INITIALIZER(&table->existing),
>> +    };
>> +}
>> +
>> +void
>> +ovn_extend_table_reinit(struct ovn_extend_table *table, uint32_t n_ids)
>> +{
>> +    if (n_ids != table->n_ids) {
>> +        id_pool_destroy(table->table_ids);
>> +        table->table_ids = id_pool_create(1, n_ids);
>> +        table->n_ids = n_ids;
>> +    }
>>  }
>>  
>>  static struct ovn_extend_table_info *
>> @@ -117,13 +132,13 @@ ovn_extend_table_add_desired_to_lflow(struct ovn_extend_table *table,
>>          ovs_list_init(&l->desired);
>>          hmap_insert(&table->lflow_to_desired, &l->hmap_node,
>>                      uuid_hash(lflow_uuid));
>> -        VLOG_DBG("%s: add new lflow_to_desired entry "UUID_FMT,
>> -                 __func__, UUID_ARGS(lflow_uuid));
>> +        VLOG_DBG("%s: table %s: add new lflow_to_desired entry "UUID_FMT,
>> +                 __func__, table->name, UUID_ARGS(lflow_uuid));
>>      }
>>  
>>      ovs_list_insert(&l->desired, &r->list_node);
>> -    VLOG_DBG("%s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
>> -             __func__, UUID_ARGS(lflow_uuid), r->desired->name,
>> +    VLOG_DBG("%s: table %s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
>> +             __func__, table->name, UUID_ARGS(lflow_uuid), r->desired->name,
>>               r->desired->table_id);
>>  }
>>  
>> @@ -160,10 +175,11 @@ ovn_extend_info_add_lflow_ref(struct ovn_extend_table *table,
>>  }
>>  
>>  static void
>> -ovn_extend_info_del_lflow_ref(struct ovn_extend_table_lflow_ref *r)
>> +ovn_extend_info_del_lflow_ref(struct ovn_extend_table *table,
>> +                              struct ovn_extend_table_lflow_ref *r)
>>  {
>> -    VLOG_DBG("%s: name %s, lflow "UUID_FMT" n %"PRIuSIZE, __func__,
>> -             r->desired->name, UUID_ARGS(&r->lflow_uuid),
>> +    VLOG_DBG("%s: table %s: name %s, lflow "UUID_FMT" n %"PRIuSIZE, __func__,
>> +             table->name, r->desired->name, UUID_ARGS(&r->lflow_uuid),
>>               hmap_count(&r->desired->references));
>>      hmap_remove(&r->desired->references, &r->hmap_node);
>>      ovs_list_remove(&r->list_node);
>> @@ -191,8 +207,8 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing)
>>          if (g->peer) {
>>              g->peer->peer = NULL;
>>          } else {
>> -            /* Unset the bitmap because the peer is deleted already. */
>> -            bitmap_set0(table->table_ids, g->table_id);
>> +            /* Unset the id because the peer is deleted already. */
>> +            id_pool_free_id(table->table_ids, g->table_id);
>>          }
>>          ovn_extend_table_info_destroy(g);
>>      }
>> @@ -206,7 +222,8 @@ ovn_extend_table_destroy(struct ovn_extend_table *table)
>>      hmap_destroy(&table->lflow_to_desired);
>>      ovn_extend_table_clear(table, true);
>>      hmap_destroy(&table->existing);
>> -    bitmap_free(table->table_ids);
>> +    id_pool_destroy(table->table_ids);
>> +    free(table->name);
>>  }
>>  
>>  /* Remove an entry from existing table */
>> @@ -221,7 +238,7 @@ ovn_extend_table_remove_existing(struct ovn_extend_table *table,
>>          existing->peer->peer = NULL;
>>      } else {
>>          /* Dealloc the ID. */
>> -        bitmap_set0(table->table_ids, existing->table_id);
>> +        id_pool_free_id(table->table_ids, existing->table_id);
>>      }
>>      ovn_extend_table_info_destroy(existing);
>>  }
>> @@ -234,15 +251,15 @@ ovn_extend_table_delete_desired(struct ovn_extend_table *table,
>>      struct ovn_extend_table_lflow_ref *r;
>>      LIST_FOR_EACH_SAFE (r, list_node, &l->desired) {
>>          struct ovn_extend_table_info *e = r->desired;
>> -        ovn_extend_info_del_lflow_ref(r);
>> +        ovn_extend_info_del_lflow_ref(table, r);
>>          if (hmap_is_empty(&e->references)) {
>> -            VLOG_DBG("%s: %s, "UUID_FMT, __func__,
>> -                     e->name, UUID_ARGS(&l->lflow_uuid));
>> +            VLOG_DBG("%s: table %s: %s, "UUID_FMT, __func__,
>> +                     table->name, e->name, UUID_ARGS(&l->lflow_uuid));
>>              hmap_remove(&table->desired, &e->hmap_node);
>>              if (e->peer) {
>>                  e->peer->peer = NULL;
>>              } else {
>> -                bitmap_set0(table->table_ids, e->table_id);
>> +                id_pool_free_id(table->table_ids, e->table_id);
>>              }
>>              ovn_extend_table_info_destroy(e);
>>          }
>> @@ -284,7 +301,7 @@ ovn_extend_table_sync(struct ovn_extend_table *table)
>>      }
>>  }
>>  
>> -/* Assign a new table ID for the table information from the bitmap.
>> +/* Assign a new table ID for the table information from the ID pool.
>>   * If it already exists, return the old ID. */
>>  uint32_t
>>  ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
>> @@ -298,9 +315,9 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
>>      /* Check whether we have non installed but allocated group_id. */
>>      HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->desired) {
>>          if (!strcmp(table_info->name, name)) {
>> -            VLOG_DBG("ovn_externd_table_assign_id: reuse old id %"PRIu32
>> -                     " for %s, used by lflow "UUID_FMT,
>> -                     table_info->table_id, table_info->name,
>> +            VLOG_DBG("ovn_extend_table_assign_id: table %s: "
>> +                     "reuse old id %"PRIu32" for %s, used by lflow "UUID_FMT,
>> +                     table->name, table_info->table_id, table_info->name,
>>                       UUID_ARGS(&lflow_uuid));
>>              ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid);
>>              return table_info->table_id;
>> @@ -320,15 +337,13 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name,
>>  
>>      if (!existing_info) {
>>          /* Reserve a new id. */
>> -        table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID + 1);
>> -    }
>> +        if (!id_pool_alloc_id(table->table_ids, &table_id)) {
>> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>>  
>> -    if (table_id == MAX_EXT_TABLE_ID + 1) {
>> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> -        VLOG_ERR_RL(&rl, "%"PRIu32" out of table ids.", table_id);
>> -        return EXT_TABLE_ID_INVALID;
>> +            VLOG_ERR_RL(&rl, "table %s: out of table ids.", table->name);
>> +            return EXT_TABLE_ID_INVALID;
>> +        }
>>      }
>> -    bitmap_set1(table->table_ids, table_id);
>>  
>>      table_info = ovn_extend_table_info_alloc(name, table_id, existing_info,
>>                                               hash);
>> diff --git a/lib/extend-table.h b/lib/extend-table.h
>> index b43a146b4f..90e6e470d0 100644
>> --- a/lib/extend-table.h
>> +++ b/lib/extend-table.h
>> @@ -17,18 +17,21 @@
>>  #ifndef EXTEND_TABLE_H
>>  #define EXTEND_TABLE_H 1
>>  
>> -#define MAX_EXT_TABLE_ID 65535
>>  #define EXT_TABLE_ID_INVALID 0
>>  
>>  #include "openvswitch/hmap.h"
>>  #include "openvswitch/list.h"
>>  #include "openvswitch/uuid.h"
>>  
>> +struct id_pool;
>> +
>>  /* Used to manage expansion tables associated with Flow table,
>>   * such as the Group Table or Meter Table. */
>>  struct ovn_extend_table {
>> -    unsigned long *table_ids;  /* Used as a bitmap with value set
>> -                                * for allocated ids in either desired or
>> +    char *name; /* Used to identify this table in a user friendly way,
>> +                 * e.g., for logging. */
>> +    uint32_t n_ids;
>> +    struct id_pool *table_ids; /* Used to allocate ids in either desired or
>>                                  * existing (or both).  If the same "name"
>>                                  * exists in both desired and existing tables,
>>                                  * they must share the same ID.  The "peer"
>> @@ -81,7 +84,9 @@ struct ovn_extend_table_lflow_ref {
>>      struct ovn_extend_table_info *desired;
>>  };
>>  
>> -void ovn_extend_table_init(struct ovn_extend_table *);
>> +void ovn_extend_table_init(struct ovn_extend_table *, const char *table_name,
>> +                           uint32_t n_ids);
>> +void ovn_extend_table_reinit(struct ovn_extend_table *, uint32_t n_ids);
>>  
>>  void ovn_extend_table_destroy(struct ovn_extend_table *);
>>  
>> diff --git a/lib/features.c b/lib/features.c
>> index d24e8f6c5c..914f52d39c 100644
>> --- a/lib/features.c
>> +++ b/lib/features.c
>> @@ -27,6 +27,7 @@
>>  #include "openvswitch/rconn.h"
>>  #include "openvswitch/ofp-msgs.h"
>>  #include "openvswitch/ofp-meter.h"
>> +#include "openvswitch/ofp-group.h"
>>  #include "openvswitch/ofp-util.h"
>>  #include "ovn/features.h"
>>  
>> @@ -81,6 +82,18 @@ static struct ovs_feature all_ovs_features[] = {
>>  /* A bitmap of OVS features that have been detected as 'supported'. */
>>  static uint32_t supported_ovs_features;
>>  
>> +/* Last set of received feature replies. */
>> +static struct ofputil_meter_features ovs_meter_features_reply;
>> +static struct ofputil_group_features ovs_group_features_reply;
>> +
>> +/* Currently discovered set of features. */
>> +static struct ofputil_meter_features ovs_meter_features;
>> +static struct ofputil_group_features ovs_group_features;
>> +
>> +/* Number of features replies still expected to receive for the requests
>> + * we sent already. */
>> +static uint32_t features_reply_expected;
>> +
>>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>>  
>>  /* ovs-vswitchd connection. */
>> @@ -145,11 +158,19 @@ ovs_feature_get_openflow_cap(const char *br_name)
>>  
>>      /* send new requests just after reconnect. */
>>      if (conn_seq_no != rconn_get_connection_seqno(swconn)) {
>> -        /* dump datapath meter capabilities. */
>> +        features_reply_expected = 0;
>> +
>> +        /* Dump OpenFlow switch meter capabilities. */
>>          msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
>>                             rconn_get_version(swconn), 0);
>>          rconn_send(swconn, msg, NULL);
>> +        features_reply_expected++;
>> +        /* Dump OpenFlow switch group capabilities. */
>> +        msg = ofputil_encode_group_features_request(rconn_get_version(swconn));
>> +        rconn_send(swconn, msg, NULL);
>> +        features_reply_expected++;
>>      }
>> +    conn_seq_no = rconn_get_connection_seqno(swconn);
>>  
>>      bool ret = false;
>>      for (int i = 0; i < 50; i++) {
>> @@ -163,21 +184,13 @@ ovs_feature_get_openflow_cap(const char *br_name)
>>          ofptype_decode(&type, oh);
>>  
>>          if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) {
>> -            struct ofputil_meter_features mf;
>> -            ofputil_decode_meter_features(oh, &mf);
>> -
>> -            bool old_state = supported_ovs_features & OVS_DP_METER_SUPPORT;
>> -            bool new_state = mf.max_meters > 0;
>> -
>> -            if (old_state != new_state) {
>> -                ret = true;
>> -                if (new_state) {
>> -                    supported_ovs_features |= OVS_DP_METER_SUPPORT;
>> -                } else {
>> -                    supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
>> -                }
>> -            }
>> -            conn_seq_no = rconn_get_connection_seqno(swconn);
>> +            ofputil_decode_meter_features(oh, &ovs_meter_features_reply);
>> +            ovs_assert(features_reply_expected);
>> +            features_reply_expected--;
>> +        } else if (type == OFPTYPE_GROUP_FEATURES_STATS_REPLY) {
>> +            ofputil_decode_group_features_reply(oh, &ovs_group_features_reply);
>> +            ovs_assert(features_reply_expected);
>> +            features_reply_expected--;
>>          } else if (type == OFPTYPE_ECHO_REQUEST) {
>>              rconn_send(swconn, ofputil_encode_echo_reply(oh), NULL);
>>          }
>> @@ -186,6 +199,26 @@ ovs_feature_get_openflow_cap(const char *br_name)
>>      rconn_run_wait(swconn);
>>      rconn_recv_wait(swconn);
>>  
>> +    /* If all feature replies were received, update the set of supported
>> +     * features. */
>> +    if (!features_reply_expected) {
>> +        if (memcmp(&ovs_meter_features, &ovs_meter_features_reply,
>> +                   sizeof ovs_meter_features_reply)) {
>> +            ovs_meter_features = ovs_meter_features_reply;
>> +            if (ovs_meter_features.max_meters) {
>> +                supported_ovs_features |= OVS_DP_METER_SUPPORT;
>> +            } else {
>> +                supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
>> +            }
>> +            ret = true;
>> +        }
>> +        if (memcmp(&ovs_group_features, &ovs_group_features_reply,
>> +                   sizeof ovs_group_features_reply)) {
>> +            ovs_group_features = ovs_group_features_reply;
>> +            ret = true;
>> +        }
>> +    }
>> +
>>      return ret;
>>  }
>>  
>> @@ -229,3 +262,26 @@ ovs_feature_support_run(const struct smap *ovs_capabilities,
>>      }
>>      return updated;
>>  }
>> +
>> +bool
>> +ovs_feature_set_discovered(void)
>> +{
>> +    /* The supported feature set has been discovered if we're connected
>> +     * to OvS and OVS replied to all our feature request messages. */
> 
> OvS and OVS ?
> 

Ack.

>> +    return swconn && rconn_is_connected(swconn) &&
>> +        features_reply_expected == 0;
> 
> Maybe indent this line to the 'swconn' ?
> 

Sure, sounds good.

>> +}
>> +
>> +/* Returns the number of meters the OVS datapath supports. */
>> +uint32_t
>> +ovs_feature_max_meters_get(void)
>> +{
>> +    return ovs_meter_features.max_meters;
>> +}
>> +
>> +/* Returns the number of select groups the OVS datapath supports. */
>> +uint32_t
>> +ovs_feature_max_select_groups_get(void)
>> +{
>> +    return ovs_group_features.max_groups[OFPGT11_SELECT];
>> +}
>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
>> index 1f1e27b51f..aaf2825edc 100644
>> --- a/tests/test-ovn.c
>> +++ b/tests/test-ovn.c
>> @@ -1300,11 +1300,11 @@ test_parse_actions(struct ovs_cmdl_context *ctx OVS_UNUSED)
>>  
>>      /* Initialize group ids. */
>>      struct ovn_extend_table group_table;
>> -    ovn_extend_table_init(&group_table);
>> +    ovn_extend_table_init(&group_table, "group-table", OFPG_MAX);
>>  
>>      /* Initialize meter ids for QoS. */
>>      struct ovn_extend_table meter_table;
>> -    ovn_extend_table_init(&meter_table);
>> +    ovn_extend_table_init(&meter_table, "meter-table", OFPM13_MAX);
>>  
>>      /* Initialize collector sets. */
>>      struct flow_collector_ids collector_ids;
>

On 11/20/23 13:03, Ales Musil wrote:
> On Tue, Oct 31, 2023 at 6:01 PM Dumitru Ceara <dceara@redhat.com> wrote:
> 
>> OVS actually supports way more.  Detect the real number of groups and
>> meters instead. To avoid preallocating huge bitmaps for the IDs,
>> switch to id-pool instead (as suggested by Ilya).
>>
>> Reported-at: https://issues.redhat.com/browse/FDP-70
>> Suggested-by: Ilya Maximets <i.maximets@ovn.org>
>> Signed-off-by: Dumitru Ceara <dceara@redhat.com>
>> Acked-by: Numan Siddique <numans@ovn.org>
>> ---
>>
> 
> Hi Dumitru,
> I have two small comments below that could be addressed during merge.
> 

Thanks for your review!

> 
>> V4:
>> - Addressed Numan's comment and added his ack:
>>   - copy the table name argument
>>   - kept Numan's ack as the other changes since v3 were not
>>     functionality related.
>> - Addressed Ilya's comments:
>>   - Removed _clear() from ovn_extend_table_reinit().
>>   - Fixed indentation.
>>   - Use better name for extend_table number of IDs.
>>   - Omit name in prototype when possible.
>>   - Changed "datapath capabilities" comment to "OpenFlow switch
>>     capabilities".
>> V3:
>> - Addressed Ilya's comments:
>>   - removed references to 'bitmap' in comments
>>   - fixed indentation & typos
>>   - moved "id-pool.h" include to C file
>>   - extended the OVS feature detection component to also check for the
>>     max number of groups.
>> V2:
>> - Addressed Ilya's comment: fixed the way id_pool_create() is called.
>> - renamed max_size to max_id, it's more accurate.
>> ---
>>  controller/ofctrl.c         |  2 +
>>  controller/ovn-controller.c |  6 +--
>>  include/ovn/features.h      |  3 ++
>>  lib/extend-table.c          | 81 ++++++++++++++++++++--------------
>>  lib/extend-table.h          | 13 ++++--
>>  lib/features.c              | 88 ++++++++++++++++++++++++++++++-------
>>  tests/test-ovn.c            |  4 +-
>>  7 files changed, 139 insertions(+), 58 deletions(-)
>>
>> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> index 63b0aa975b..06f8b33232 100644
>> --- a/controller/ofctrl.c
>> +++ b/controller/ofctrl.c
>> @@ -677,11 +677,13 @@ run_S_CLEAR_FLOWS(void)
>>      /* Clear existing groups, to match the state of the switch. */
>>      if (groups) {
>>          ovn_extend_table_clear(groups, true);
>> +        ovn_extend_table_reinit(groups,
>> ovs_feature_max_select_groups_get());
>>      }
>>
>>      /* Clear existing meters, to match the state of the switch. */
>>      if (meters) {
>>          ovn_extend_table_clear(meters, true);
>> +        ovn_extend_table_reinit(meters, ovs_feature_max_meters_get());
>>          ofctrl_meter_bands_clear();
>>      }
>>
>> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
>> index da7d145ed3..7a8ca38286 100644
>> --- a/controller/ovn-controller.c
>> +++ b/controller/ovn-controller.c
>> @@ -3948,8 +3948,8 @@ en_lflow_output_init(struct engine_node *node
>> OVS_UNUSED,
>>  {
>>      struct ed_type_lflow_output *data = xzalloc(sizeof *data);
>>      ovn_desired_flow_table_init(&data->flow_table);
>> -    ovn_extend_table_init(&data->group_table);
>> -    ovn_extend_table_init(&data->meter_table);
>> +    ovn_extend_table_init(&data->group_table, "group-table", 0);
>> +    ovn_extend_table_init(&data->meter_table, "meter-table", 0);
>>      objdep_mgr_init(&data->lflow_deps_mgr);
>>      lflow_conj_ids_init(&data->conj_ids);
>>      uuidset_init(&data->objs_processed);
>> @@ -5656,7 +5656,7 @@ main(int argc, char *argv[])
>>                  engine_set_force_recompute(true);
>>              }
>>
>> -            if (br_int) {
>> +            if (br_int && ovs_feature_set_discovered()) {
>>                  ct_zones_data = engine_get_data(&en_ct_zones);
>>                  if (ct_zones_data && ofctrl_run(br_int, ovs_table,
>>                                                  &ct_zones_data->pending))
>> {
>> diff --git a/include/ovn/features.h b/include/ovn/features.h
>> index 7c3004b271..2c47ab766e 100644
>> --- a/include/ovn/features.h
>> +++ b/include/ovn/features.h
>> @@ -49,5 +49,8 @@ void ovs_feature_support_destroy(void);
>>  bool ovs_feature_is_supported(enum ovs_feature_value feature);
>>  bool ovs_feature_support_run(const struct smap *ovs_capabilities,
>>                               const char *br_name);
>> +bool ovs_feature_set_discovered(void);
>> +uint32_t ovs_feature_max_meters_get(void);
>> +uint32_t ovs_feature_max_select_groups_get(void);
>>
>>  #endif
>> diff --git a/lib/extend-table.c b/lib/extend-table.c
>> index ebb1a054cb..7f23587784 100644
>> --- a/lib/extend-table.c
>> +++ b/lib/extend-table.c
>> @@ -17,9 +17,9 @@
>>  #include <config.h>
>>  #include <string.h>
>>
>> -#include "bitmap.h"
>>  #include "extend-table.h"
>>  #include "hash.h"
>> +#include "id-pool.h"
>>  #include "lib/uuid.h"
>>  #include "openvswitch/vlog.h"
>>
>> @@ -30,13 +30,28 @@ ovn_extend_table_delete_desired(struct
>> ovn_extend_table *table,
>>                                  struct ovn_extend_table_lflow_to_desired
>> *l);
>>
>>  void
>> -ovn_extend_table_init(struct ovn_extend_table *table)
>> +ovn_extend_table_init(struct ovn_extend_table *table, const char
>> *table_name,
>> +                      uint32_t n_ids)
>>  {
>> -    table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
>> -    bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
>> -    hmap_init(&table->desired);
>> -    hmap_init(&table->lflow_to_desired);
>> -    hmap_init(&table->existing);
>> +    *table = (struct ovn_extend_table) {
>> +        .name = xstrdup(table_name),
>> +        .n_ids = n_ids,
>> +        /* Table id 0 is invalid, set id-pool base to 1. */
>> +        .table_ids = id_pool_create(1, n_ids),
>> +        .desired = HMAP_INITIALIZER(&table->desired),
>> +        .lflow_to_desired = HMAP_INITIALIZER(&table->lflow_to_desired),
>> +        .existing = HMAP_INITIALIZER(&table->existing),
>> +    };
>> +}
>> +
>> +void
>> +ovn_extend_table_reinit(struct ovn_extend_table *table, uint32_t n_ids)
>> +{
>> +    if (n_ids != table->n_ids) {
>>
> 
> nit: That's the only place where we use the n_ids, could we use the pool's
> n_ids instead? It seems redundant to store twice the same thing.
> 

Unfortunately struct id_pool is opaque, that's why I had to use a new
n_ids field.

> 
>> +        id_pool_destroy(table->table_ids);
>> +        table->table_ids = id_pool_create(1, n_ids);
>> +        table->n_ids = n_ids;
>> +    }
>>  }
>>
>>  static struct ovn_extend_table_info *
>> @@ -117,13 +132,13 @@ ovn_extend_table_add_desired_to_lflow(struct
>> ovn_extend_table *table,
>>          ovs_list_init(&l->desired);
>>          hmap_insert(&table->lflow_to_desired, &l->hmap_node,
>>                      uuid_hash(lflow_uuid));
>> -        VLOG_DBG("%s: add new lflow_to_desired entry "UUID_FMT,
>> -                 __func__, UUID_ARGS(lflow_uuid));
>> +        VLOG_DBG("%s: table %s: add new lflow_to_desired entry "UUID_FMT,
>> +                 __func__, table->name, UUID_ARGS(lflow_uuid));
>>      }
>>
>>      ovs_list_insert(&l->desired, &r->list_node);
>> -    VLOG_DBG("%s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
>> -             __func__, UUID_ARGS(lflow_uuid), r->desired->name,
>> +    VLOG_DBG("%s: table %s: lflow "UUID_FMT" use new item %s, id %"PRIu32,
>> +             __func__, table->name, UUID_ARGS(lflow_uuid),
>> r->desired->name,
>>               r->desired->table_id);
>>  }
>>
>> @@ -160,10 +175,11 @@ ovn_extend_info_add_lflow_ref(struct
>> ovn_extend_table *table,
>>  }
>>
>>  static void
>> -ovn_extend_info_del_lflow_ref(struct ovn_extend_table_lflow_ref *r)
>> +ovn_extend_info_del_lflow_ref(struct ovn_extend_table *table,
>> +                              struct ovn_extend_table_lflow_ref *r)
>>  {
>> -    VLOG_DBG("%s: name %s, lflow "UUID_FMT" n %"PRIuSIZE, __func__,
>> -             r->desired->name, UUID_ARGS(&r->lflow_uuid),
>> +    VLOG_DBG("%s: table %s: name %s, lflow "UUID_FMT" n %"PRIuSIZE,
>> __func__,
>> +             table->name, r->desired->name, UUID_ARGS(&r->lflow_uuid),
>>               hmap_count(&r->desired->references));
>>      hmap_remove(&r->desired->references, &r->hmap_node);
>>      ovs_list_remove(&r->list_node);
>> @@ -191,8 +207,8 @@ ovn_extend_table_clear(struct ovn_extend_table *table,
>> bool existing)
>>          if (g->peer) {
>>              g->peer->peer = NULL;
>>          } else {
>> -            /* Unset the bitmap because the peer is deleted already. */
>> -            bitmap_set0(table->table_ids, g->table_id);
>> +            /* Unset the id because the peer is deleted already. */
>> +            id_pool_free_id(table->table_ids, g->table_id);
>>          }
>>          ovn_extend_table_info_destroy(g);
>>      }
>> @@ -206,7 +222,8 @@ ovn_extend_table_destroy(struct ovn_extend_table
>> *table)
>>      hmap_destroy(&table->lflow_to_desired);
>>      ovn_extend_table_clear(table, true);
>>      hmap_destroy(&table->existing);
>> -    bitmap_free(table->table_ids);
>> +    id_pool_destroy(table->table_ids);
>> +    free(table->name);
>>  }
>>
>>  /* Remove an entry from existing table */
>> @@ -221,7 +238,7 @@ ovn_extend_table_remove_existing(struct
>> ovn_extend_table *table,
>>          existing->peer->peer = NULL;
>>      } else {
>>          /* Dealloc the ID. */
>> -        bitmap_set0(table->table_ids, existing->table_id);
>> +        id_pool_free_id(table->table_ids, existing->table_id);
>>      }
>>      ovn_extend_table_info_destroy(existing);
>>  }
>> @@ -234,15 +251,15 @@ ovn_extend_table_delete_desired(struct
>> ovn_extend_table *table,
>>      struct ovn_extend_table_lflow_ref *r;
>>      LIST_FOR_EACH_SAFE (r, list_node, &l->desired) {
>>          struct ovn_extend_table_info *e = r->desired;
>> -        ovn_extend_info_del_lflow_ref(r);
>> +        ovn_extend_info_del_lflow_ref(table, r);
>>          if (hmap_is_empty(&e->references)) {
>> -            VLOG_DBG("%s: %s, "UUID_FMT, __func__,
>> -                     e->name, UUID_ARGS(&l->lflow_uuid));
>> +            VLOG_DBG("%s: table %s: %s, "UUID_FMT, __func__,
>> +                     table->name, e->name, UUID_ARGS(&l->lflow_uuid));
>>              hmap_remove(&table->desired, &e->hmap_node);
>>              if (e->peer) {
>>                  e->peer->peer = NULL;
>>              } else {
>> -                bitmap_set0(table->table_ids, e->table_id);
>> +                id_pool_free_id(table->table_ids, e->table_id);
>>              }
>>              ovn_extend_table_info_destroy(e);
>>          }
>> @@ -284,7 +301,7 @@ ovn_extend_table_sync(struct ovn_extend_table *table)
>>      }
>>  }
>>
>> -/* Assign a new table ID for the table information from the bitmap.
>> +/* Assign a new table ID for the table information from the ID pool.
>>   * If it already exists, return the old ID. */
>>  uint32_t
>>  ovn_extend_table_assign_id(struct ovn_extend_table *table, const char
>> *name,
>> @@ -298,9 +315,9 @@ ovn_extend_table_assign_id(struct ovn_extend_table
>> *table, const char *name,
>>      /* Check whether we have non installed but allocated group_id. */
>>      HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash,
>> &table->desired) {
>>          if (!strcmp(table_info->name, name)) {
>> -            VLOG_DBG("ovn_externd_table_assign_id: reuse old id %"PRIu32
>> -                     " for %s, used by lflow "UUID_FMT,
>> -                     table_info->table_id, table_info->name,
>> +            VLOG_DBG("ovn_extend_table_assign_id: table %s: "
>> +                     "reuse old id %"PRIu32" for %s, used by lflow
>> "UUID_FMT,
>> +                     table->name, table_info->table_id, table_info->name,
>>                       UUID_ARGS(&lflow_uuid));
>>              ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid);
>>              return table_info->table_id;
>> @@ -320,15 +337,13 @@ ovn_extend_table_assign_id(struct ovn_extend_table
>> *table, const char *name,
>>
>>      if (!existing_info) {
>>          /* Reserve a new id. */
>> -        table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID +
>> 1);
>> -    }
>> +        if (!id_pool_alloc_id(table->table_ids, &table_id)) {
>> +            static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>>
>> -    if (table_id == MAX_EXT_TABLE_ID + 1) {
>> -        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
>> -        VLOG_ERR_RL(&rl, "%"PRIu32" out of table ids.", table_id);
>> -        return EXT_TABLE_ID_INVALID;
>> +            VLOG_ERR_RL(&rl, "table %s: out of table ids.", table->name);
>> +            return EXT_TABLE_ID_INVALID;
>> +        }
>>      }
>> -    bitmap_set1(table->table_ids, table_id);
>>
>>      table_info = ovn_extend_table_info_alloc(name, table_id,
>> existing_info,
>>                                               hash);
>> diff --git a/lib/extend-table.h b/lib/extend-table.h
>> index b43a146b4f..90e6e470d0 100644
>> --- a/lib/extend-table.h
>> +++ b/lib/extend-table.h
>> @@ -17,18 +17,21 @@
>>  #ifndef EXTEND_TABLE_H
>>  #define EXTEND_TABLE_H 1
>>
>> -#define MAX_EXT_TABLE_ID 65535
>>  #define EXT_TABLE_ID_INVALID 0
>>
>>  #include "openvswitch/hmap.h"
>>  #include "openvswitch/list.h"
>>  #include "openvswitch/uuid.h"
>>
>> +struct id_pool;
>> +
>>  /* Used to manage expansion tables associated with Flow table,
>>   * such as the Group Table or Meter Table. */
>>  struct ovn_extend_table {
>> -    unsigned long *table_ids;  /* Used as a bitmap with value set
>> -                                * for allocated ids in either desired or
>> +    char *name; /* Used to identify this table in a user friendly way,
>> +                 * e.g., for logging. */
>> +    uint32_t n_ids;
>> +    struct id_pool *table_ids; /* Used to allocate ids in either desired
>> or
>>                                  * existing (or both).  If the same "name"
>>                                  * exists in both desired and existing
>> tables,
>>                                  * they must share the same ID.  The "peer"
>> @@ -81,7 +84,9 @@ struct ovn_extend_table_lflow_ref {
>>      struct ovn_extend_table_info *desired;
>>  };
>>
>> -void ovn_extend_table_init(struct ovn_extend_table *);
>> +void ovn_extend_table_init(struct ovn_extend_table *, const char
>> *table_name,
>> +                           uint32_t n_ids);
>> +void ovn_extend_table_reinit(struct ovn_extend_table *, uint32_t n_ids);
>>
>>  void ovn_extend_table_destroy(struct ovn_extend_table *);
>>
>> diff --git a/lib/features.c b/lib/features.c
>> index d24e8f6c5c..914f52d39c 100644
>> --- a/lib/features.c
>> +++ b/lib/features.c
>> @@ -27,6 +27,7 @@
>>  #include "openvswitch/rconn.h"
>>  #include "openvswitch/ofp-msgs.h"
>>  #include "openvswitch/ofp-meter.h"
>> +#include "openvswitch/ofp-group.h"
>>  #include "openvswitch/ofp-util.h"
>>  #include "ovn/features.h"
>>
>> @@ -81,6 +82,18 @@ static struct ovs_feature all_ovs_features[] = {
>>  /* A bitmap of OVS features that have been detected as 'supported'. */
>>  static uint32_t supported_ovs_features;
>>
>> +/* Last set of received feature replies. */
>> +static struct ofputil_meter_features ovs_meter_features_reply;
>> +static struct ofputil_group_features ovs_group_features_reply;
>> +
>> +/* Currently discovered set of features. */
>> +static struct ofputil_meter_features ovs_meter_features;
>> +static struct ofputil_group_features ovs_group_features;
>> +
>> +/* Number of features replies still expected to receive for the requests
>> + * we sent already. */
>> +static uint32_t features_reply_expected;
>>
> 
> nit: n_features_reply_expected? The current name implies more bool and not
> uint IMO.
> 

You're right, it looks better, I changed it to n_features_reply_expected.

> 
>> +
>>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>>
>>  /* ovs-vswitchd connection. */
>> @@ -145,11 +158,19 @@ ovs_feature_get_openflow_cap(const char *br_name)
>>
>>      /* send new requests just after reconnect. */
>>      if (conn_seq_no != rconn_get_connection_seqno(swconn)) {
>> -        /* dump datapath meter capabilities. */
>> +        features_reply_expected = 0;
>> +
>> +        /* Dump OpenFlow switch meter capabilities. */
>>          msg = ofpraw_alloc(OFPRAW_OFPST13_METER_FEATURES_REQUEST,
>>                             rconn_get_version(swconn), 0);
>>          rconn_send(swconn, msg, NULL);
>> +        features_reply_expected++;
>> +        /* Dump OpenFlow switch group capabilities. */
>> +        msg =
>> ofputil_encode_group_features_request(rconn_get_version(swconn));
>> +        rconn_send(swconn, msg, NULL);
>> +        features_reply_expected++;
>>      }
>> +    conn_seq_no = rconn_get_connection_seqno(swconn);
>>
>>      bool ret = false;
>>      for (int i = 0; i < 50; i++) {
>> @@ -163,21 +184,13 @@ ovs_feature_get_openflow_cap(const char *br_name)
>>          ofptype_decode(&type, oh);
>>
>>          if (type == OFPTYPE_METER_FEATURES_STATS_REPLY) {
>> -            struct ofputil_meter_features mf;
>> -            ofputil_decode_meter_features(oh, &mf);
>> -
>> -            bool old_state = supported_ovs_features &
>> OVS_DP_METER_SUPPORT;
>> -            bool new_state = mf.max_meters > 0;
>> -
>> -            if (old_state != new_state) {
>> -                ret = true;
>> -                if (new_state) {
>> -                    supported_ovs_features |= OVS_DP_METER_SUPPORT;
>> -                } else {
>> -                    supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
>> -                }
>> -            }
>> -            conn_seq_no = rconn_get_connection_seqno(swconn);
>> +            ofputil_decode_meter_features(oh, &ovs_meter_features_reply);
>> +            ovs_assert(features_reply_expected);
>> +            features_reply_expected--;
>> +        } else if (type == OFPTYPE_GROUP_FEATURES_STATS_REPLY) {
>> +            ofputil_decode_group_features_reply(oh,
>> &ovs_group_features_reply);
>> +            ovs_assert(features_reply_expected);
>> +            features_reply_expected--;
>>          } else if (type == OFPTYPE_ECHO_REQUEST) {
>>              rconn_send(swconn, ofputil_encode_echo_reply(oh), NULL);
>>          }
>> @@ -186,6 +199,26 @@ ovs_feature_get_openflow_cap(const char *br_name)
>>      rconn_run_wait(swconn);
>>      rconn_recv_wait(swconn);
>>
>> +    /* If all feature replies were received, update the set of supported
>> +     * features. */
>> +    if (!features_reply_expected) {
>> +        if (memcmp(&ovs_meter_features, &ovs_meter_features_reply,
>> +                   sizeof ovs_meter_features_reply)) {
>> +            ovs_meter_features = ovs_meter_features_reply;
>> +            if (ovs_meter_features.max_meters) {
>> +                supported_ovs_features |= OVS_DP_METER_SUPPORT;
>> +            } else {
>> +                supported_ovs_features &= ~OVS_DP_METER_SUPPORT;
>> +            }
>> +            ret = true;
>> +        }
>> +        if (memcmp(&ovs_group_features, &ovs_group_features_reply,
>> +                   sizeof ovs_group_features_reply)) {
>> +            ovs_group_features = ovs_group_features_reply;
>> +            ret = true;
>> +        }
>> +    }
>> +
>>      return ret;
>>  }
>>
>> @@ -229,3 +262,26 @@ ovs_feature_support_run(const struct smap
>> *ovs_capabilities,
>>      }
>>      return updated;
>>  }
>> +
>> +bool
>> +ovs_feature_set_discovered(void)
>> +{
>> +    /* The supported feature set has been discovered if we're connected
>> +     * to OvS and OVS replied to all our feature request messages. */
>> +    return swconn && rconn_is_connected(swconn) &&
>> +        features_reply_expected == 0;
>> +}
>> +
>> +/* Returns the number of meters the OVS datapath supports. */
>> +uint32_t
>> +ovs_feature_max_meters_get(void)
>> +{
>> +    return ovs_meter_features.max_meters;
>> +}
>> +
>> +/* Returns the number of select groups the OVS datapath supports. */
>> +uint32_t
>> +ovs_feature_max_select_groups_get(void)
>> +{
>> +    return ovs_group_features.max_groups[OFPGT11_SELECT];
>> +}
>> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
>> index 1f1e27b51f..aaf2825edc 100644
>> --- a/tests/test-ovn.c
>> +++ b/tests/test-ovn.c
>> @@ -1300,11 +1300,11 @@ test_parse_actions(struct ovs_cmdl_context *ctx
>> OVS_UNUSED)
>>
>>      /* Initialize group ids. */
>>      struct ovn_extend_table group_table;
>> -    ovn_extend_table_init(&group_table);
>> +    ovn_extend_table_init(&group_table, "group-table", OFPG_MAX);
>>
>>      /* Initialize meter ids for QoS. */
>>      struct ovn_extend_table meter_table;
>> -    ovn_extend_table_init(&meter_table);
>> +    ovn_extend_table_init(&meter_table, "meter-table", OFPM13_MAX);
>>
>>      /* Initialize collector sets. */
>>      struct flow_collector_ids collector_ids;
>> --
>> 2.39.3
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
> Other than that it looks good.
> 
> Acked-by: Ales Musil <amusil@redhat.com>
> 

Thanks, applied to main and backported to all stable branches down to 22.03.

Regards,
Dumitru