On Fri, Dec 20, 2019 at 1:09 PM Dumitru Ceara <[email protected]> wrote: > > On Fri, Dec 6, 2019 at 11:29 PM Han Zhou <[email protected]> wrote: > > > > A meter/group can be used by multiple logical flows. However, current > > code didn't handle this properly. Each table_info item has a lflow_uuid > > field, which can keep track of only a single lflow. > > > > In most cases this doesn't create problems because multiple table_info > > entries are created even for same "name". > > > > However, when multiple lflows are added in the same main loop iteration > > using the same "name" (i.e. when the new_table_id == true), the function > > ovn_extend_table_assign_id() will return the old id without creating a > > new entry, and the reference by the second lflow is untracked. Later > > with incremental processing, if the old lflow is deleted, the table_info > > will be deleted, which results in the deletion of group/meter in OVS, > > even when it is still used by the second lflow. > > > > This patch fixes the problem by adding a hmap in each desired table_info > > item to keep track of multiple lflow references. A test case is added. > > The test case would fail without this fix. > > > > At the same time, this patch adds an index that maps from lflow_uuid to > > a list of desired table_info items used by the lflow, so that the > > ovn_extend_table_remove_desired() is more efficient, without having > > to do a O(N) iteration every time. > > > > Fixes: ca278d98a4f5 ("ovn-controller: Initial use of incremental engine - quiet mode.") > > Signed-off-by: Han Zhou <[email protected]> > > Nice catch! Looks good to me. > > Acked-by: Dumitru Ceara <[email protected]> > > > --- > > lib/extend-table.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++------- > > lib/extend-table.h | 31 ++++++++- > > tests/ovn.at | 55 ++++++++++++++++ > > 3 files changed, 241 insertions(+), 25 deletions(-) > > > > diff --git a/lib/extend-table.c b/lib/extend-table.c > > index 82dfcfa..b102e44 100644 > > --- a/lib/extend-table.c > > +++ b/lib/extend-table.c > > @@ -31,9 +31,37 @@ ovn_extend_table_init(struct ovn_extend_table *table) > > 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); > > } > > > > +static struct ovn_extend_table_info * > > +ovn_extend_table_info_alloc(const char *name, uint32_t id, bool is_new_id, > > + uint32_t hash) > > +{ > > + struct ovn_extend_table_info *e = xmalloc(sizeof *e); > > + e->name = xstrdup(name); > > + e->table_id = id; > > + e->new_table_id = is_new_id; > > + e->hmap_node.hash = hash; > > + hmap_init(&e->references); > > + return e; > > +} > > + > > +static void > > +ovn_extend_table_info_destroy(struct ovn_extend_table_info *e) > > +{ > > + free(e->name); > > + struct ovn_extend_table_lflow_ref *r, *r_next; > > + HMAP_FOR_EACH_SAFE (r, r_next, hmap_node, &e->references) { > > + hmap_remove(&e->references, &r->hmap_node); > > + ovs_list_remove(&r->list_node); > > + free(r); > > + } > > + hmap_destroy(&e->references); > > + free(e); > > +} > > + > > /* Finds and returns a group_info in 'existing' whose key is identical > > * to 'target''s key, or NULL if there is none. */ > > struct ovn_extend_table_info * > > @@ -51,6 +79,89 @@ ovn_extend_table_lookup(struct hmap *exisiting, > > return NULL; > > } > > > > +static struct ovn_extend_table_lflow_to_desired * > > +ovn_extend_table_find_desired_by_lflow(struct ovn_extend_table *table, > > + const struct uuid *lflow_uuid) > > +{ > > + struct ovn_extend_table_lflow_to_desired *l; > > + HMAP_FOR_EACH_WITH_HASH (l, hmap_node, uuid_hash(lflow_uuid), > > + &table->lflow_to_desired) { > > + if (uuid_equals(&l->lflow_uuid, lflow_uuid)) { > > + return l; > > + } > > + } > > + return NULL; > > +} > > + > > +/* Add a reference to the list of items that <lflow_uuid> uses. > > + * If the <lflow_uuid> entry doesn't exist in lflow_to_desired mapping, add > > + * the <lflow_uuid> entry first. */ > > +static void > > +ovn_extend_table_add_desired_to_lflow(struct ovn_extend_table *table, > > + const struct uuid *lflow_uuid, > > + struct ovn_extend_table_lflow_ref *r) > > +{ > > + struct ovn_extend_table_lflow_to_desired *l = > > + ovn_extend_table_find_desired_by_lflow(table, lflow_uuid); > > + if (!l) { > > + l = xmalloc(sizeof *l); > > + l->lflow_uuid = *lflow_uuid; > > + 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)); > > + } > > + > > + 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, > > + r->desired->table_id); > > +} > > + > > +static struct ovn_extend_table_lflow_ref * > > +ovn_extend_info_find_lflow_ref(struct ovn_extend_table_info *e, > > + const struct uuid *lflow_uuid) > > +{ > > + struct ovn_extend_table_lflow_ref *r; > > + HMAP_FOR_EACH_WITH_HASH (r, hmap_node, uuid_hash(lflow_uuid), > > + &e->references) { > > + if (uuid_equals(&r->lflow_uuid, lflow_uuid)) { > > + return r; > > + } > > + } > > + return NULL; > > +} > > + > > +/* Create the cross reference between <e> and <lflow_uuid> */ > > +static void > > +ovn_extend_info_add_lflow_ref(struct ovn_extend_table *table, > > + struct ovn_extend_table_info *e, > > + const struct uuid *lflow_uuid) > > +{ > > + struct ovn_extend_table_lflow_ref *r = > > + ovn_extend_info_find_lflow_ref(e, lflow_uuid); > > + if (!r) { > > + r = xmalloc(sizeof *r); > > + r->lflow_uuid = *lflow_uuid; > > + r->desired = e; > > + hmap_insert(&e->references, &r->hmap_node, uuid_hash(lflow_uuid)); > > + > > + ovn_extend_table_add_desired_to_lflow(table, lflow_uuid, r); > > + } > > +} > > + > > +static void > > +ovn_extend_info_del_lflow_ref(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), > > + hmap_count(&r->desired->references)); > > + hmap_remove(&r->desired->references, &r->hmap_node); > > + ovs_list_remove(&r->list_node); > > + free(r); > > +} > > + > > /* Clear either desired or existing in ovn_extend_table. */ > > void > > ovn_extend_table_clear(struct ovn_extend_table *table, bool existing) > > @@ -58,6 +169,16 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing) > > struct ovn_extend_table_info *g, *next; > > struct hmap *target = existing ? &table->existing : &table->desired; > > > > + /* Clear lflow_to_desired index, if the target is desired table. */ > > + if (!existing) { > > + struct ovn_extend_table_lflow_to_desired *l, *l_next; > > + HMAP_FOR_EACH_SAFE (l, l_next, hmap_node, &table->lflow_to_desired) { > > + hmap_remove(&table->lflow_to_desired, &l->hmap_node); > > + free(l); > > + } > > + } > > + > > + /* Clear the target table. */ > > HMAP_FOR_EACH_SAFE (g, next, hmap_node, target) { > > hmap_remove(target, &g->hmap_node); > > /* Don't unset bitmap for desired group_info if the group_id > > @@ -65,8 +186,7 @@ ovn_extend_table_clear(struct ovn_extend_table *table, bool existing) > > if (existing || g->new_table_id) { > > bitmap_set0(table->table_ids, g->table_id); > > } > > - free(g->name); > > - free(g); > > + ovn_extend_table_info_destroy(g); > > } > > } > > > > @@ -75,6 +195,7 @@ ovn_extend_table_destroy(struct ovn_extend_table *table) > > { > > ovn_extend_table_clear(table, false); > > hmap_destroy(&table->desired); > > + hmap_destroy(&table->lflow_to_desired); > > ovn_extend_table_clear(table, true); > > hmap_destroy(&table->existing); > > bitmap_free(table->table_ids); > > @@ -87,11 +208,10 @@ ovn_extend_table_remove_existing(struct ovn_extend_table *table, > > { > > /* Remove 'existing' from 'groups->existing' */ > > hmap_remove(&table->existing, &existing->hmap_node); > > - free(existing->name); > > > > /* Dealloc group_id. */ > > bitmap_set0(table->table_ids, existing->table_id); > > - free(existing); > > + ovn_extend_table_info_destroy(existing); > > } > > > > /* Remove entries in desired table that are created by the lflow_uuid */ > > @@ -99,29 +219,39 @@ void > > ovn_extend_table_remove_desired(struct ovn_extend_table *table, > > const struct uuid *lflow_uuid) > > { > > - struct ovn_extend_table_info *e, *next_e; > > - HMAP_FOR_EACH_SAFE (e, next_e, hmap_node, &table->desired) { > > - if (uuid_equals(&e->lflow_uuid, lflow_uuid)) { > > + struct ovn_extend_table_lflow_to_desired *l = > > + ovn_extend_table_find_desired_by_lflow(table, lflow_uuid); > > + > > + if (!l) { > > + return; > > + } > > + > > + hmap_remove(&table->lflow_to_desired, &l->hmap_node); > > + struct ovn_extend_table_lflow_ref *r, *next_r; > > + LIST_FOR_EACH_SAFE (r, next_r, list_node, &l->desired) { > > + struct ovn_extend_table_info *e = r->desired; > > + ovn_extend_info_del_lflow_ref(r); > > + if (hmap_is_empty(&e->references)) { > > + VLOG_DBG("%s: %s, "UUID_FMT, __func__, > > + e->name, UUID_ARGS(lflow_uuid)); > > hmap_remove(&table->desired, &e->hmap_node); > > - free(e->name); > > if (e->new_table_id) { > > bitmap_set0(table->table_ids, e->table_id); > > } > > - free(e); > > + ovn_extend_table_info_destroy(e); > > } > > } > > - > > + free(l); > > } > > > > static struct ovn_extend_table_info* > > ovn_extend_info_clone(struct ovn_extend_table_info *source) > > { > > - struct ovn_extend_table_info *clone = xmalloc(sizeof *clone); > > - clone->name = xstrdup(source->name); > > - clone->table_id = source->table_id; > > - clone->new_table_id = source->new_table_id; > > - clone->hmap_node.hash = source->hmap_node.hash; > > - clone->lflow_uuid = source->lflow_uuid; > > + struct ovn_extend_table_info *clone = > > + ovn_extend_table_info_alloc(source->name, > > + source->table_id, > > + source->new_table_id, > > + source->hmap_node.hash); > > return clone; > > } > > > > @@ -155,8 +285,12 @@ 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) && > > - table_info->new_table_id) { > > + 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; > > } > > } > > @@ -183,15 +317,13 @@ ovn_extend_table_assign_id(struct ovn_extend_table *table, const char *name, > > } > > bitmap_set1(table->table_ids, table_id); > > > > - table_info = xmalloc(sizeof *table_info); > > - table_info->name = xstrdup(name); > > - table_info->table_id = table_id; > > - table_info->hmap_node.hash = hash; > > - table_info->new_table_id = new_table_id; > > - table_info->lflow_uuid = lflow_uuid; > > + 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); > > > > + ovn_extend_info_add_lflow_ref(table, table_info, &lflow_uuid); > > + > > return table_id; > > } > > diff --git a/lib/extend-table.h b/lib/extend-table.h > > index 833dced..4d80cfd 100644 > > --- a/lib/extend-table.h > > +++ b/lib/extend-table.h > > @@ -31,16 +31,45 @@ struct ovn_extend_table { > > * for allocated group ids in either > > * desired or existing. */ > > struct hmap desired; > > + struct hmap lflow_to_desired; /* Index for looking up desired table > > + * items from given lflow uuid, with > > + * ovn_extend_table_lflow_to_desired nodes. > > + */ > > struct hmap existing; > > }; > > > > +struct ovn_extend_table_lflow_to_desired { > > + struct hmap_node hmap_node; /* In ovn_extend_table.lflow_to_desired. */ > > + struct uuid lflow_uuid; > > + struct ovs_list desired; /* List of desired items used by the lflow. */ > > +}; > > + > > struct ovn_extend_table_info { > > struct hmap_node hmap_node; > > char *name; /* Name for the table entity. */ > > - struct uuid lflow_uuid; > > uint32_t table_id; > > bool new_table_id; /* 'True' if 'table_id' was reserved from > > * ovn_extend_table's 'table_ids' bitmap. */ > > + struct hmap references; /* The lflows that are using this item, with > > + * ovn_extend_table_lflow_ref nodes. Only useful > > + * for items in ovn_extend_table.desired. */ > > +}; > > + > > +/* Maintains the link between a lflow and an ovn_extend_table_info item in > > + * ovn_extend_table.desired, indexed by both > > + * ovn_extend_table_lflow_to_desired.desired and > > + * ovn_extend_table_info.references. > > + * > > + * The struct is allocated whenever a new reference happens. > > + * It destroyed when a lflow is deleted (for all the desired table_info > > + * used by it), or when the lflow_to_desired table is being cleared. > > + * */ > > +struct ovn_extend_table_lflow_ref { > > + struct hmap_node hmap_node; /* In ovn_extend_table_info.references. */ > > + struct ovs_list list_node; /* In ovn_extend_table_lflow_to_desired.desired. > > + */ > > + struct uuid lflow_uuid; > > + struct ovn_extend_table_info *desired; > > }; > > > > void ovn_extend_table_init(struct ovn_extend_table *); > > diff --git a/tests/ovn.at b/tests/ovn.at > > index 5dc7af2..7bb97ed 100644 > > --- a/tests/ovn.at > > +++ b/tests/ovn.at > > @@ -7380,6 +7380,61 @@ OVN_CLEANUP([hv]) > > AT_CLEANUP > > > > > > +AT_SETUP([ovn -- same meter used by multiple logical flows]) > > +AT_KEYWORDS([ovn]) > > +ovn_start > > + > > +net_add n1 > > + > > +sim_add hv > > +as hv > > +ovs-vsctl add-br br-phys > > +ovn_attach n1 br-phys 192.168.0.1 > > +for i in lp1 lp2; do > > + ovs-vsctl -- add-port br-int $i -- \ > > + set interface $i external-ids:iface-id=$i \ > > + options:tx_pcap=hv/$i-tx.pcap \ > > + options:rxq_pcap=hv/$i-rx.pcap > > +done > > + > > +lp1_mac="f0:00:00:00:00:01" > > +lp1_ip="192.168.1.2" > > + > > +lp2_mac="f0:00:00:00:00:02" > > +lp2_ip="192.168.1.3" > > + > > +ovn-nbctl ls-add lsw0 > > +ovn-nbctl --wait=sb lsp-add lsw0 lp1 > > +ovn-nbctl --wait=sb lsp-add lsw0 lp2 > > +ovn-nbctl lsp-set-addresses lp1 $lp1_mac > > +ovn-nbctl lsp-set-addresses lp2 $lp2_mac > > +ovn-nbctl --wait=sb sync > > + > > +ovn-appctl -t ovn-controller vlog/set file:dbg > > + > > +# Add acl1 and acl2 using same meter. > > +ovn-nbctl meter-add http-rl1 drop 10 pktps > > +ovn-nbctl --log --meter=http-rl1 acl-add lsw0 to-lport 1000 'tcp.dst==80' drop \ > > + -- --log --meter=http-rl1 acl-add lsw0 to-lport 1000 'tcp.dst==81' allow > > + > > +ovn-nbctl --wait=hv sync > > + > > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [0], [ignore], [ignore]) > > + > > +# Delete acl1, meter should be kept in OVS > > +ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==80' > > +ovn-nbctl --wait=hv sync > > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [0], [ignore], [ignore]) > > + > > +# Delete acl2, meter should be deleted in OVS > > +ovn-nbctl acl-del lsw0 to-lport 1000 'tcp.dst==81' > > +ovn-nbctl --wait=hv sync > > +AT_CHECK([ovs-ofctl -O OpenFlow13 dump-meters br-int | grep meter], [1]) > > + > > +OVN_CLEANUP([hv]) > > +AT_CLEANUP > > + > > + > > AT_SETUP([ovn -- DSCP marking and meter check]) > > AT_KEYWORDS([ovn]) > > ovn_start > > -- > > 2.1.0 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > >
Thanks for the review. I applied the series to master. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
