| Message ID | 1212633d1a3d342e087416bd9f93b0cef572e011.1642182763.git.lorenzo.bianconi@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | ovn metering incremental processing | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
Hi Lorenzo, I think this and patch 3 should be combined. See notes below for why. On 1/14/22 13:01, Lorenzo Bianconi wrote: > Introduce pending_id map used to store meter_id allocated but not yet > inserted in the desired_table. This is a preliminary patch to add > metering IP engine. > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > --- > controller/ofctrl.c | 9 +++++ > controller/ofctrl.h | 1 + > lib/extend-table.c | 82 +++++++++++++++++++++++++++++++++------------ > lib/extend-table.h | 7 ++++ > 4 files changed, 78 insertions(+), 21 deletions(-) > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > index 08fcfed8b..bf715787e 100644 > --- a/controller/ofctrl.c > +++ b/controller/ofctrl.c > @@ -1801,6 +1801,15 @@ add_meter_string(struct ovn_extend_table_info *m_desired, > free(meter_string); > } > > +uint32_t ofctrl_get_meter_id(const char *name, bool new_id) ofctrl_get_meter_id() is not used anywhere in this patch, so trying to analyze its purpose on its own is difficult (e.g. what are the circumstances for setting new_id to true or false?). To properly review this, I had to look at patch 3 as well. > +{ > + uint32_t id; > + bool val; > + > + ovn_extend_table_get_id(meters, name, &id, NULL, &val, new_id); > + return id; > +} > + > static void > add_meter(struct ovn_extend_table_info *m_desired, > const struct sbrec_meter_table *meter_table, > diff --git a/controller/ofctrl.h b/controller/ofctrl.h > index 014de210d..0efcb2ef5 100644 > --- a/controller/ofctrl.h > +++ b/controller/ofctrl.h > @@ -129,5 +129,6 @@ void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *, > bool ofctrl_is_connected(void); > void ofctrl_set_probe_interval(int probe_interval); > void ofctrl_get_memory_usage(struct simap *usage); > +uint32_t ofctrl_get_meter_id(const char *name, bool new_id); > > #endif /* controller/ofctrl.h */ > diff --git a/lib/extend-table.c b/lib/extend-table.c > index c708e24b9..52c8cd62e 100644 > --- a/lib/extend-table.c > +++ b/lib/extend-table.c > @@ -37,6 +37,7 @@ ovn_extend_table_init(struct ovn_extend_table *table) > hmap_init(&table->desired); > hmap_init(&table->lflow_to_desired); > hmap_init(&table->existing); > + shash_init(&table->pending_ids); > } > > static struct ovn_extend_table_info * > @@ -191,6 +192,14 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing) > } > ovn_extend_table_info_destroy(g); > } > + > + struct shash_node *node, *tmp; > + SHASH_FOR_EACH_SAFE (node, tmp, &table->pending_ids) { > + uint32_t *id = node->data; > + shash_delete(&table->pending_ids, node); > + free(id); > + } > + shash_destroy(&table->pending_ids); You can replace all of these lines with shash_destroy_free_data(&table->pending_ids) > } > > void > @@ -282,54 +291,85 @@ ovn_extend_table_sync(struct ovn_extend_table *table) > } > } > > -/* Assign a new table ID for the table information from the bitmap. > - * If it already exists, return the old ID. */ > -uint32_t > -ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name, > - struct uuid lflow_uuid) > +bool > +ovn_extend_table_get_id(struct ovn_extend_table *table, const char *name, > + uint32_t *meter_id, struct uuid *lflow_uuid, > + bool *new_id, bool pending) The return value of this function needs to be documented. It's not clear at all what a true or false return means. I *think* that a true return means that a new ID was allocated for I think the meter_id parameter should have its name changed. Extend tables are used for groups, meters, and other purposes. "id" is a better choice. > { > - uint32_t table_id = 0, hash; > struct ovn_extend_table_info *table_info; > - > - hash = hash_string(name, 0); > + uint32_t hash = hash_string(name, 0); > > /* 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 Nit: This is in a different function now, so it should be "ovn_extend_table_get_id" in this debug message. > - " for %s, used by lflow "UUID_FMT, > - 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; > + " for %s", table_info->table_id, table_info->name); > + if (lflow_uuid) { > + ovn_extend_info_add_lflow_ref(table, table_info, lflow_uuid); > + } > + *meter_id = table_info->table_id; > + return false; > } > } > > + *new_id = false; > /* Check whether we already have an installed entry for this > * combination. */ > HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->existing) { > if (!strcmp(table_info->name, name)) { > - table_id = table_info->table_id; > + *meter_id = table_info->table_id; > + return true; > } > } > > - bool new_table_id = false; > - if (!table_id) { > - /* Reserve a new group_id. */ > - table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID + 1); > - new_table_id = true; > + /* Check if a ids has been already allocated for this flow. */ > + uint32_t *id = shash_find_and_delete(&table->pending_ids, name); It took me quite a while to understand what the pending_ids were being used for. I *think* the idea is that the incremental engine is generating IDs but storing them in table->pending_ids instead of creating extend_table_info structures. Then later, when ovn_extend_table_assign_id() is called by the flow_engine, we can pull the ID from here instead of using another bit in table->table_ids. This way, the table info is generated by lflow processing like it always has, and the lflow_uuid can be assigned at that point. The problem is, this is an unorthodox use of the incremental engine, and it's the reason why it took me so long to understand. The IDs created by the meter engine node are directly required by the lflow processing engine node. So presumably, the meter engine node should be an input to the lflow processing engine node. This way, the lflow change handler can be called in the case the meter information has changed. As it stands, the two separate engines are using a third-party structure for their data, and it just happens to work out because the meter_engine is run before the flow_engine. I also think this was harder to analyze since it required me to look at patch 3 to understand the concept fully. This is another reason I think patch 2 and patch 3 should be combined. > + if (id) { > + *meter_id = *id; > + free(id); > + return true; > } > > + /* Reserve a new group_id. */ > + uint32_t 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; > + *meter_id = EXT_TABLE_ID_INVALID; > + return false; > } > bitmap_set1(table->table_ids, table_id); > + *meter_id = table_id; > + > + *new_id = true; > + if (pending) { > + /* Add the id to pedning map. */ > + id = xmalloc(sizeof *id); > + *id = table_id; > + shash_add(&table->pending_ids, name, id); > + } > + > + return true; > +} > + > +/* Assign a new table ID for the table information from the bitmap. > + * If it already exists, return the old ID. */ > +uint32_t > +ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name, > + struct uuid lflow_uuid) > +{ > + uint32_t table_id, hash = hash_string(name, 0); > + struct ovn_extend_table_info *table_info; > + bool new_table_id; > + > + if (!ovn_extend_table_get_id(table, name, &table_id, > + &lflow_uuid, &new_table_id, false)) { > + return table_id; > + } > > table_info = ovn_extend_table_info_alloc(name, table_id, new_table_id, > hash); > - > hmap_insert(&table->desired, > &table_info->hmap_node, table_info->hmap_node.hash); > > diff --git a/lib/extend-table.h b/lib/extend-table.h > index 4d80cfd80..5c5f65695 100644 > --- a/lib/extend-table.h > +++ b/lib/extend-table.h > @@ -21,6 +21,7 @@ > #define EXT_TABLE_ID_INVALID 0 > > #include "openvswitch/hmap.h" > +#include "openvswitch/shash.h" > #include "openvswitch/list.h" > #include "openvswitch/uuid.h" > > @@ -30,6 +31,8 @@ struct ovn_extend_table { > unsigned long *table_ids; /* Used as a bitmap with value set > * for allocated group ids in either > * desired or existing. */ > + struct shash pending_ids; > + > struct hmap desired; > struct hmap lflow_to_desired; /* Index for looking up desired table > * items from given lflow uuid, with > @@ -93,6 +96,10 @@ void ovn_extend_table_sync(struct ovn_extend_table *); > uint32_t ovn_extend_table_assign_id(struct ovn_extend_table *, > const char *name, > struct uuid lflow_uuid); > +bool > +ovn_extend_table_get_id(struct ovn_extend_table *table, const char *name, > + uint32_t *meter_id, struct uuid *lflow_uuid, > + bool *new_id, bool pending); > > /* Iterates 'DESIRED' through all of the 'ovn_extend_table_info's in > * 'TABLE'->desired that are not in 'TABLE'->existing. (The loop body >
> Hi Lorenzo, > > I think this and patch 3 should be combined. See notes below for why. > > On 1/14/22 13:01, Lorenzo Bianconi wrote: > > Introduce pending_id map used to store meter_id allocated but not yet > > inserted in the desired_table. This is a preliminary patch to add > > metering IP engine. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> > > --- > > controller/ofctrl.c | 9 +++++ > > controller/ofctrl.h | 1 + > > lib/extend-table.c | 82 +++++++++++++++++++++++++++++++++------------ > > lib/extend-table.h | 7 ++++ > > 4 files changed, 78 insertions(+), 21 deletions(-) > > > > diff --git a/controller/ofctrl.c b/controller/ofctrl.c > > index 08fcfed8b..bf715787e 100644 > > --- a/controller/ofctrl.c > > +++ b/controller/ofctrl.c > > @@ -1801,6 +1801,15 @@ add_meter_string(struct ovn_extend_table_info *m_desired, > > free(meter_string); > > } > > +uint32_t ofctrl_get_meter_id(const char *name, bool new_id) > > ofctrl_get_meter_id() is not used anywhere in this patch, so trying to > analyze its purpose on its own is difficult (e.g. what are the circumstances > for setting new_id to true or false?). To properly review this, I had to > look at patch 3 as well. ack, I am fine with it. > > > +{ > > + uint32_t id; > > + bool val; > > + > > + ovn_extend_table_get_id(meters, name, &id, NULL, &val, new_id); > > + return id; > > +} > > + > > static void > > add_meter(struct ovn_extend_table_info *m_desired, > > const struct sbrec_meter_table *meter_table, > > diff --git a/controller/ofctrl.h b/controller/ofctrl.h > > index 014de210d..0efcb2ef5 100644 > > --- a/controller/ofctrl.h > > +++ b/controller/ofctrl.h > > @@ -129,5 +129,6 @@ void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *, > > bool ofctrl_is_connected(void); > > void ofctrl_set_probe_interval(int probe_interval); > > void ofctrl_get_memory_usage(struct simap *usage); > > +uint32_t ofctrl_get_meter_id(const char *name, bool new_id); > > #endif /* controller/ofctrl.h */ > > diff --git a/lib/extend-table.c b/lib/extend-table.c > > index c708e24b9..52c8cd62e 100644 > > --- a/lib/extend-table.c > > +++ b/lib/extend-table.c > > @@ -37,6 +37,7 @@ ovn_extend_table_init(struct ovn_extend_table *table) > > hmap_init(&table->desired); > > hmap_init(&table->lflow_to_desired); > > hmap_init(&table->existing); > > + shash_init(&table->pending_ids); > > } > > static struct ovn_extend_table_info * > > @@ -191,6 +192,14 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing) > > } > > ovn_extend_table_info_destroy(g); > > } > > + > > + struct shash_node *node, *tmp; > > + SHASH_FOR_EACH_SAFE (node, tmp, &table->pending_ids) { > > + uint32_t *id = node->data; > > + shash_delete(&table->pending_ids, node); > > + free(id); > > + } > > + shash_destroy(&table->pending_ids); > > You can replace all of these lines with > shash_destroy_free_data(&table->pending_ids) ack, I will fix it > > > } > > void > > @@ -282,54 +291,85 @@ ovn_extend_table_sync(struct ovn_extend_table *table) > > } > > } > > -/* Assign a new table ID for the table information from the bitmap. > > - * If it already exists, return the old ID. */ > > -uint32_t > > -ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name, > > - struct uuid lflow_uuid) > > +bool > > +ovn_extend_table_get_id(struct ovn_extend_table *table, const char *name, > > + uint32_t *meter_id, struct uuid *lflow_uuid, > > + bool *new_id, bool pending) > > The return value of this function needs to be documented. It's not clear at > all what a true or false return means. I *think* that a true return means > that a new ID was allocated for > > I think the meter_id parameter should have its name changed. Extend tables > are used for groups, meters, and other purposes. "id" is a better choice. ack, I will fix it > > > { > > - uint32_t table_id = 0, hash; > > struct ovn_extend_table_info *table_info; > > - > > - hash = hash_string(name, 0); > > + uint32_t hash = hash_string(name, 0); > > /* 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 > > Nit: This is in a different function now, so it should be > "ovn_extend_table_get_id" in this debug message. > > > - " for %s, used by lflow "UUID_FMT, > > - 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; > > + " for %s", table_info->table_id, table_info->name); > > + if (lflow_uuid) { > > + ovn_extend_info_add_lflow_ref(table, table_info, lflow_uuid); > > + } > > + *meter_id = table_info->table_id; > > + return false; > > } > > } > > + *new_id = false; > > /* Check whether we already have an installed entry for this > > * combination. */ > > HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->existing) { > > if (!strcmp(table_info->name, name)) { > > - table_id = table_info->table_id; > > + *meter_id = table_info->table_id; > > + return true; > > } > > } > > - bool new_table_id = false; > > - if (!table_id) { > > - /* Reserve a new group_id. */ > > - table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID + 1); > > - new_table_id = true; > > + /* Check if a ids has been already allocated for this flow. */ > > + uint32_t *id = shash_find_and_delete(&table->pending_ids, name); > > It took me quite a while to understand what the pending_ids were being used > for. I *think* the idea is that the incremental engine is generating IDs but > storing them in table->pending_ids instead of creating extend_table_info > structures. Then later, when ovn_extend_table_assign_id() is called by the > flow_engine, we can pull the ID from here instead of using another bit in > table->table_ids. This way, the table info is generated by lflow processing > like it always has, and the lflow_uuid can be assigned at that point. ack, correct. > > The problem is, this is an unorthodox use of the incremental engine, and > it's the reason why it took me so long to understand. The IDs created by the > meter engine node are directly required by the lflow processing engine node. > So presumably, the meter engine node should be an input to the lflow > processing engine node. This way, the lflow change handler can be called in > the case the meter information has changed. As it stands, the two separate > engines are using a third-party structure for their data, and it just > happens to work out because the meter_engine is run before the flow_engine. I think for meter-band table is not 1:1 with all other SB tables since changing a meter band will not change the related lflow since in logical_flow table we just reference the UUID not the band value. I guess we can try to hook it into the lflow engine, but I guess we need to understand how to manage the full-recompute and how to send configuration changes to OVS. > > I also think this was harder to analyze since it required me to look at > patch 3 to understand the concept fully. This is another reason I think > patch 2 and patch 3 should be combined. ack, fine with it. Regards, Lorenzo > > > + if (id) { > > + *meter_id = *id; > > + free(id); > > + return true; > > } > > + /* Reserve a new group_id. */ > > + uint32_t 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; > > + *meter_id = EXT_TABLE_ID_INVALID; > > + return false; > > } > > bitmap_set1(table->table_ids, table_id); > > + *meter_id = table_id; > > + > > + *new_id = true; > > + if (pending) { > > + /* Add the id to pedning map. */ > > + id = xmalloc(sizeof *id); > > + *id = table_id; > > + shash_add(&table->pending_ids, name, id); > > + } > > + > > + return true; > > +} > > + > > +/* Assign a new table ID for the table information from the bitmap. > > + * If it already exists, return the old ID. */ > > +uint32_t > > +ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name, > > + struct uuid lflow_uuid) > > +{ > > + uint32_t table_id, hash = hash_string(name, 0); > > + struct ovn_extend_table_info *table_info; > > + bool new_table_id; > > + > > + if (!ovn_extend_table_get_id(table, name, &table_id, > > + &lflow_uuid, &new_table_id, false)) { > > + return table_id; > > + } > > table_info = ovn_extend_table_info_alloc(name, table_id, new_table_id, > > hash); > > - > > hmap_insert(&table->desired, > > &table_info->hmap_node, table_info->hmap_node.hash); > > diff --git a/lib/extend-table.h b/lib/extend-table.h > > index 4d80cfd80..5c5f65695 100644 > > --- a/lib/extend-table.h > > +++ b/lib/extend-table.h > > @@ -21,6 +21,7 @@ > > #define EXT_TABLE_ID_INVALID 0 > > #include "openvswitch/hmap.h" > > +#include "openvswitch/shash.h" > > #include "openvswitch/list.h" > > #include "openvswitch/uuid.h" > > @@ -30,6 +31,8 @@ struct ovn_extend_table { > > unsigned long *table_ids; /* Used as a bitmap with value set > > * for allocated group ids in either > > * desired or existing. */ > > + struct shash pending_ids; > > + > > > > struct hmap desired; > > struct hmap lflow_to_desired; /* Index for looking up desired table > > * items from given lflow uuid, with > > @@ -93,6 +96,10 @@ void ovn_extend_table_sync(struct ovn_extend_table *); > > uint32_t ovn_extend_table_assign_id(struct ovn_extend_table *, > > const char *name, > > struct uuid lflow_uuid); > > +bool > > +ovn_extend_table_get_id(struct ovn_extend_table *table, const char *name, > > + uint32_t *meter_id, struct uuid *lflow_uuid, > > + bool *new_id, bool pending); > > /* Iterates 'DESIRED' through all of the 'ovn_extend_table_info's in > > * 'TABLE'->desired that are not in 'TABLE'->existing. (The loop body > > >
diff --git a/controller/ofctrl.c b/controller/ofctrl.c index 08fcfed8b..bf715787e 100644 --- a/controller/ofctrl.c +++ b/controller/ofctrl.c @@ -1801,6 +1801,15 @@ add_meter_string(struct ovn_extend_table_info *m_desired, free(meter_string); } +uint32_t ofctrl_get_meter_id(const char *name, bool new_id) +{ + uint32_t id; + bool val; + + ovn_extend_table_get_id(meters, name, &id, NULL, &val, new_id); + return id; +} + static void add_meter(struct ovn_extend_table_info *m_desired, const struct sbrec_meter_table *meter_table, diff --git a/controller/ofctrl.h b/controller/ofctrl.h index 014de210d..0efcb2ef5 100644 --- a/controller/ofctrl.h +++ b/controller/ofctrl.h @@ -129,5 +129,6 @@ void ofctrl_check_and_add_flow_metered(struct ovn_desired_flow_table *, bool ofctrl_is_connected(void); void ofctrl_set_probe_interval(int probe_interval); void ofctrl_get_memory_usage(struct simap *usage); +uint32_t ofctrl_get_meter_id(const char *name, bool new_id); #endif /* controller/ofctrl.h */ diff --git a/lib/extend-table.c b/lib/extend-table.c index c708e24b9..52c8cd62e 100644 --- a/lib/extend-table.c +++ b/lib/extend-table.c @@ -37,6 +37,7 @@ ovn_extend_table_init(struct ovn_extend_table *table) hmap_init(&table->desired); hmap_init(&table->lflow_to_desired); hmap_init(&table->existing); + shash_init(&table->pending_ids); } static struct ovn_extend_table_info * @@ -191,6 +192,14 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing) } ovn_extend_table_info_destroy(g); } + + struct shash_node *node, *tmp; + SHASH_FOR_EACH_SAFE (node, tmp, &table->pending_ids) { + uint32_t *id = node->data; + shash_delete(&table->pending_ids, node); + free(id); + } + shash_destroy(&table->pending_ids); } void @@ -282,54 +291,85 @@ ovn_extend_table_sync(struct ovn_extend_table *table) } } -/* Assign a new table ID for the table information from the bitmap. - * If it already exists, return the old ID. */ -uint32_t -ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name, - struct uuid lflow_uuid) +bool +ovn_extend_table_get_id(struct ovn_extend_table *table, const char *name, + uint32_t *meter_id, struct uuid *lflow_uuid, + bool *new_id, bool pending) { - uint32_t table_id = 0, hash; struct ovn_extend_table_info *table_info; - - hash = hash_string(name, 0); + uint32_t hash = hash_string(name, 0); /* 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, - UUID_ARGS(&lflow_uuid)); - ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid); - return table_info->table_id; + " for %s", table_info->table_id, table_info->name); + if (lflow_uuid) { + ovn_extend_info_add_lflow_ref(table, table_info, lflow_uuid); + } + *meter_id = table_info->table_id; + return false; } } + *new_id = false; /* Check whether we already have an installed entry for this * combination. */ HMAP_FOR_EACH_WITH_HASH (table_info, hmap_node, hash, &table->existing) { if (!strcmp(table_info->name, name)) { - table_id = table_info->table_id; + *meter_id = table_info->table_id; + return true; } } - bool new_table_id = false; - if (!table_id) { - /* Reserve a new group_id. */ - table_id = bitmap_scan(table->table_ids, 0, 1, MAX_EXT_TABLE_ID + 1); - new_table_id = true; + /* Check if a ids has been already allocated for this flow. */ + uint32_t *id = shash_find_and_delete(&table->pending_ids, name); + if (id) { + *meter_id = *id; + free(id); + return true; } + /* Reserve a new group_id. */ + uint32_t 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; + *meter_id = EXT_TABLE_ID_INVALID; + return false; } bitmap_set1(table->table_ids, table_id); + *meter_id = table_id; + + *new_id = true; + if (pending) { + /* Add the id to pedning map. */ + id = xmalloc(sizeof *id); + *id = table_id; + shash_add(&table->pending_ids, name, id); + } + + return true; +} + +/* Assign a new table ID for the table information from the bitmap. + * If it already exists, return the old ID. */ +uint32_t +ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name, + struct uuid lflow_uuid) +{ + uint32_t table_id, hash = hash_string(name, 0); + struct ovn_extend_table_info *table_info; + bool new_table_id; + + if (!ovn_extend_table_get_id(table, name, &table_id, + &lflow_uuid, &new_table_id, false)) { + return table_id; + } table_info = ovn_extend_table_info_alloc(name, table_id, new_table_id, hash); - hmap_insert(&table->desired, &table_info->hmap_node, table_info->hmap_node.hash); diff --git a/lib/extend-table.h b/lib/extend-table.h index 4d80cfd80..5c5f65695 100644 --- a/lib/extend-table.h +++ b/lib/extend-table.h @@ -21,6 +21,7 @@ #define EXT_TABLE_ID_INVALID 0 #include "openvswitch/hmap.h" +#include "openvswitch/shash.h" #include "openvswitch/list.h" #include "openvswitch/uuid.h" @@ -30,6 +31,8 @@ struct ovn_extend_table { unsigned long *table_ids; /* Used as a bitmap with value set * for allocated group ids in either * desired or existing. */ + struct shash pending_ids; + struct hmap desired; struct hmap lflow_to_desired; /* Index for looking up desired table * items from given lflow uuid, with @@ -93,6 +96,10 @@ void ovn_extend_table_sync(struct ovn_extend_table *); uint32_t ovn_extend_table_assign_id(struct ovn_extend_table *, const char *name, struct uuid lflow_uuid); +bool +ovn_extend_table_get_id(struct ovn_extend_table *table, const char *name, + uint32_t *meter_id, struct uuid *lflow_uuid, + bool *new_id, bool pending); /* Iterates 'DESIRED' through all of the 'ovn_extend_table_info's in * 'TABLE'->desired that are not in 'TABLE'->existing. (The loop body
Introduce pending_id map used to store meter_id allocated but not yet inserted in the desired_table. This is a preliminary patch to add metering IP engine. Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@redhat.com> --- controller/ofctrl.c | 9 +++++ controller/ofctrl.h | 1 + lib/extend-table.c | 82 +++++++++++++++++++++++++++++++++------------ lib/extend-table.h | 7 ++++ 4 files changed, 78 insertions(+), 21 deletions(-)