Skip to content

Commit

Permalink
controller: Don't artificially limit group and meter IDs to 16bit.
Browse files Browse the repository at this point in the history
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 <[email protected]>
Signed-off-by: Dumitru Ceara <[email protected]>
---
V2:
- Addressed Ilya's comment: fixed the way id_pool_create() is called.
- renamed max_size to max_id, it's more accurate.
  • Loading branch information
dceara committed Oct 11, 2023
1 parent 5efdf01 commit be51349
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 24 deletions.
4 changes: 2 additions & 2 deletions controller/ovn-controller.c
Original file line number Diff line number Diff line change
Expand Up @@ -3949,8 +3949,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);
Expand Down
28 changes: 12 additions & 16 deletions lib/extend-table.c
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
#include <config.h>
#include <string.h>

#include "bitmap.h"
#include "extend-table.h"
#include "hash.h"
#include "lib/uuid.h"
Expand All @@ -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);
Expand Down Expand Up @@ -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_set0(table->table_ids, g->table_id);
id_pool_free_id(table->table_ids, g->table_id);
}
ovn_extend_table_info_destroy(g);
}
Expand All @@ -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 */
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand Down Expand Up @@ -320,15 +319,12 @@ 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 (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);
VLOG_ERR_RL(&rl, "%"PRIu32" out of table ids.", table_id);
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);
Expand Down
7 changes: 3 additions & 4 deletions lib/extend-table.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"

/* 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
* existing (or both). If the same "name"
* exists in both desired and existing tables,
* they must share the same ID. The "peer"
Expand Down Expand Up @@ -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 *);

Expand Down
4 changes: 2 additions & 2 deletions tests/test-ovn.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);

/* Initialize collector sets. */
struct flow_collector_ids collector_ids;
Expand Down

0 comments on commit be51349

Please sign in to comment.