[ovs-dev] [PATCH] ovsdb: transaction: Incremental reassessment of weak refs.

Ilya Maximets i.maximets at ovn.org
Thu Nov 4 22:59:35 UTC 2021


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.maximets at 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 <dceara at 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.


More information about the dev mailing list