On 10/4/23 18:33, Dumitru Ceara wrote:
> OVS actually supports way more. Use the real numbers instead.
> To avoid preallocating huge bitmaps for the group/meter IDs, switch to
> id-pool instead (as suggested by Ilya).
>
> Reported-at: https://issues.redhat.com/browse/FDP-70
> Suggested-by: Ilya Maximets <i.maxim...@ovn.org>
> Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> ---
> V2:
> - Addressed Ilya's comment: fixed the way id_pool_create() is called.
> - renamed max_size to max_id, it's more accurate.
> ---
> controller/ovn-controller.c | 4 ++--
> lib/extend-table.c | 28 ++++++++++++----------------
> lib/extend-table.h | 7 +++----
> tests/test-ovn.c | 4 ++--
> 4 files changed, 19 insertions(+), 24 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index 9a81f1a80f..50a101f0e8 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -3936,8 +3936,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, OFPG_MAX);
> + ovn_extend_table_init(&data->meter_table, OFPM13_MAX);
> objdep_mgr_init(&data->lflow_deps_mgr);
> lflow_conj_ids_init(&data->conj_ids);
> uuidset_init(&data->objs_processed);
> diff --git a/lib/extend-table.c b/lib/extend-table.c
> index ebb1a054cb..90a1cf92b2 100644
> --- a/lib/extend-table.c
> +++ b/lib/extend-table.c
> @@ -17,7 +17,6 @@
> #include <config.h>
> #include <string.h>
>
> -#include "bitmap.h"
> #include "extend-table.h"
> #include "hash.h"
> #include "lib/uuid.h"
> @@ -30,10 +29,10 @@ 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, uint32_t max_id)
> {
> - table->table_ids = bitmap_allocate(MAX_EXT_TABLE_ID);
> - bitmap_set1(table->table_ids, 0); /* table id 0 is invalid. */
> + /* Table id 0 is invalid, set id-pool base to 1. */
> + table->table_ids = id_pool_create(1, max_id);
> hmap_init(&table->desired);
> hmap_init(&table->lflow_to_desired);
> hmap_init(&table->existing);
> @@ -192,7 +191,7 @@ ovn_extend_table_clear(struct ovn_extend_table *table,
> bool existing)
> g->peer->peer = NULL;
> } else {
> /* Unset the bitmap because the peer is deleted already. */
bitmap?
> - bitmap_set0(table->table_ids, g->table_id);
> + id_pool_free_id(table->table_ids, g->table_id);
Just to double check. This is called only once per id?
Asking because it's fine to clear the same bit multiple
times, but it's not OK to free the same id-pool ID twice.
> }
> ovn_extend_table_info_destroy(g);
> }
> @@ -206,7 +205,7 @@ 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);
> }
>
> /* Remove an entry from existing table */
> @@ -221,7 +220,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);
> }
> @@ -242,7 +241,7 @@ ovn_extend_table_delete_desired(struct ovn_extend_table
> *table,
> 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);
> }
> @@ -320,15 +319,12 @@ ovn_extend_table_assign_id(struct ovn_extend_table
> *table, const char *name,
Comment for this function is also talking about bitmap.
>
> if (!existing_info) {
> /* Reserve a new id. */
> - table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID + 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;
> + if (!id_pool_alloc_id(table->table_ids, &table_id)) {
> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1);
Maybe an empty line here?
> + VLOG_ERR_RL(&rl, "%"PRIu32" out of table ids.", table_id);
Table ID is always zero at this point, so the error message
is not very informative. Not that it was particularly useful
before...
> + 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);
So, we're creating a new hash map entry here with pre-existing ID.
Wouldn't this cause a double free for that ID later?
> diff --git a/lib/extend-table.h b/lib/extend-table.h
> index b43a146b4f..56d54afe93 100644
> --- a/lib/extend-table.h
> +++ b/lib/extend-table.h
> @@ -17,18 +17,17 @@
> #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"
> +#include "id-pool.h"
I'd say the include should be in the .c file. In the header we should
only have:
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
> + struct id_pool *table_ids; /* Used to allocated ids in either desired or
s/allocated/allocate/
> * existing (or both). If the same "name"
> * exists in both desired and existing tables,
> * they must share the same ID. The "peer"
> @@ -81,7 +80,7 @@ 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 *, uint32_t max_id);
>
> void ovn_extend_table_destroy(struct ovn_extend_table *);
>
> diff --git a/tests/test-ovn.c b/tests/test-ovn.c
> index 1f1e27b51f..e4f03806a3 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, 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, OFPM13_MAX);
I did a bit more digging in this topic and instead of using these
constants we should technically ask OVS about real limits
with OFPMP13_METER_FEATURES_REQUEST and OFPST12_GROUP_FEATURES_REQUEST
respectively. They will report how many meters and groups are actually
supported.
At the same time OVN may not do that at the expense of getting and
OpenFlow error reply on attempt to create more meters/groups than
the implementation supports. For example, userspace datapath supprts
2^18 meters, and kernel allows creating meters up to filling 3% of
physical memory. :)
Currently OVS always reports OFPG_MAX as the maximum number of groups
though anyway.
Does it matter if we fail a table ID allocation vs the OF request
failure later?
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
