On 10/16/21 3:20 AM, Ilya Maximets wrote: > The main idea is to not store list of weak references in the source > row, so they all don't need to be re-checked/updated on every > modification of that source row. The point is that source row already > knows UUIDs of all destination rows stored in the data, so there is no > much profit in storing this information somewhere else. If needed, > destination row can be looked up and reference can be looked up in the > destination row. For the fast lookup, destination row now stores > references in a hash map. > > Weak reference structure now contains the table and uuid of a source > row instead of a direct pointer. This allows to replace/update the > source row without breaking any weak references stored in destination > rows. > > Structure also now contains the key-value pair of atoms that triggered > creation of this reference. These atoms can be used to quickly > subtract removed references from a source row. During reassessment, > ovsdb now only needs to care about new added or removed atoms, and > atoms that got removed due to removal of the destination rows, but > these are marked for reassessment by the destination row. > > ovsdb_datum_subtract() is used to remove atoms that points to removed > or incorrect rows, so there is no need to re-sort datum in the end. > > Results of an OVN load-balancer benchmark that adds 3K load-balancers > to each of 120 logical switches and 120 logical routers in the OVN > sandbox with clustered Northbound database and then removes them: > > Before: > > %CPU CPU Time CMD > 86.8 00:16:05 ovsdb-server nb1.db > 44.1 00:08:11 ovsdb-server nb2.db > 43.2 00:08:00 ovsdb-server nb3.db > > After: > > %CPU CPU Time CMD > 54.9 00:02:58 ovsdb-server nb1.db > 33.3 00:01:48 ovsdb-server nb2.db > 32.2 00:01:44 ovsdb-server nb3.db > > So, on a cluster leader the processing time dropped by 5.4x, on > followers - by 4.5x. More load-balancers - larger the performance > difference. There is a slight increase of memory usage, because new > reference structure is larger, but the difference is not significant. > > Signed-off-by: Ilya Maximets <[email protected]> > ---
Hi Ilya, We've been running quite a few scale tests with this change applied downstream and performance improvement is visible and we didn't detect any regressions AFAICT. The code looks good to me, I left a couple of tiny nits that can be fixed at apply time. Acked-by: Dumitru Ceara <[email protected]> Thanks, Dumitru > ovsdb/row.c | 97 +++++++++++++--- > ovsdb/row.h | 34 +++++- > ovsdb/transaction.c | 262 ++++++++++++++++++++++++++++++-------------- > 3 files changed, 293 insertions(+), 100 deletions(-) > > diff --git a/ovsdb/row.c b/ovsdb/row.c > index 65a054621..7c6914773 100644 > --- a/ovsdb/row.c > +++ b/ovsdb/row.c > @@ -38,8 +38,7 @@ allocate_row(const struct ovsdb_table *table) > struct ovsdb_row *row = xmalloc(row_size); > row->table = CONST_CAST(struct ovsdb_table *, table); > row->txn_row = NULL; > - ovs_list_init(&row->src_refs); > - ovs_list_init(&row->dst_refs); > + hmap_init(&row->dst_refs); > row->n_refs = 0; > return row; > } > @@ -61,6 +60,63 @@ ovsdb_row_create(const struct ovsdb_table *table) > return row; > } > > +static struct ovsdb_weak_ref * > +ovsdb_weak_ref_clone(struct ovsdb_weak_ref *src) > +{ > + struct ovsdb_weak_ref *weak = xzalloc(sizeof *weak); > + > + weak->src_table = src->src_table; > + weak->src = src->src; > + weak->dst_table = src->dst_table; > + weak->dst = src->dst; > + ovsdb_type_clone(&weak->type, &src->type); > + ovsdb_atom_clone(&weak->key, &src->key, src->type.key.type); > + if (src->type.value.type != OVSDB_TYPE_VOID) { > + ovsdb_atom_clone(&weak->value, &src->value, src->type.value.type); > + } > + weak->by_key = src->by_key; > + weak->column_idx = src->column_idx; > + ovs_list_init(&weak->src_node); > + hmap_node_nullify(&weak->dst_node); Nit: I think we can initialize 'weak''s fields in the same order as they're defined in the ovsdb_weak_ref structure. > + return weak; > +} > + > +uint32_t > +ovsdb_weak_ref_hash(const struct ovsdb_weak_ref *weak) > +{ > + return uuid_hash(&weak->src); > +} > + > +static bool > +ovsdb_weak_ref_equals(const struct ovsdb_weak_ref *a, > + const struct ovsdb_weak_ref *b) > +{ > + if (a == b) { > + return true; > + } > + return a->src_table == b->src_table > + && a->dst_table == b->dst_table > + && uuid_equals(&a->src, &b->src) > + && uuid_equals(&a->dst, &b->dst) > + && a->column_idx == b->column_idx > + && a->by_key == b->by_key > + && ovsdb_atom_equals(&a->key, &b->key, a->type.key.type); > +} > + > +struct ovsdb_weak_ref * > +ovsdb_row_find_weak_ref(const struct ovsdb_row *row, > + const struct ovsdb_weak_ref *ref) > +{ > + struct ovsdb_weak_ref *weak; > + HMAP_FOR_EACH_WITH_HASH (weak, dst_node, > + ovsdb_weak_ref_hash(ref), &row->dst_refs) { > + if (ovsdb_weak_ref_equals(weak, ref)) { > + return weak; > + } > + } > + return NULL; > +} > + > struct ovsdb_row * > ovsdb_row_clone(const struct ovsdb_row *old) > { > @@ -75,9 +131,31 @@ ovsdb_row_clone(const struct ovsdb_row *old) > &old->fields[column->index], > &column->type); > } > + > + struct ovsdb_weak_ref *weak, *clone; > + HMAP_FOR_EACH (weak, dst_node, &old->dst_refs) { > + clone = ovsdb_weak_ref_clone(weak); > + hmap_insert(&new->dst_refs, &clone->dst_node, > + ovsdb_weak_ref_hash(clone)); > + } > return new; > } > > +void > +ovsdb_weak_ref_destroy(struct ovsdb_weak_ref *weak) Nit: this could go just before the definition of ovsdb_row_find_weak_ref() to match the order from the header file. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
