On 10/9/23 17:39, Ilya Maximets wrote:
> 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.
Hmm. This is not a problem for id-pool. I mixed up things with id-fpool.
Basic id-pool handles double free case just fine.
>
>> }
>> 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?
Again, not a problem for id-pool.
>
>> 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
