On 11/2/21 17:18, Dumitru Ceara wrote: > 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 <i.maxim...@ovn.org> >> --- > > 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 <dce...@redhat.com> > > 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.
OK. > >> + 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. > I moved the definition below the ovsdb_row_find_weak_ref() in a header file and moved ovsdb_weak_ref_destroy() in a .c file accordingly. With that, applied. Thanks! Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev