On 12/29/23 17:28, Mike Pattrick wrote:
> On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
>>
>> While reassessing weak references the code attempts to collect added
>> and removed atoms, even if the column didn't change. In case the
>> column contains a large set, it may take significant amount of time
>> to process.
>>
>> Add a check for the column actually being changed either by removing
>> references to deleted rows or by direct removal.
>>
>> For example, rows in OVN Port_Group tables frequently have two large
>> sets - 'ports' and 'acls'. In case a new ACL is added to the set
>> without changing the ports, ports don't need to be reassessed.
>>
>> Fixes: 4dbff9f0a685 ("ovsdb: transaction: Incremental reassessment of weak
>> refs.")
>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org>
>> ---
>> ovsdb/transaction.c | 18 +++++++++++-------
>> 1 file changed, 11 insertions(+), 7 deletions(-)
>>
>> diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
>> index bbe4cddc1..a482588a0 100644
>> --- a/ovsdb/transaction.c
>> +++ b/ovsdb/transaction.c
>> @@ -745,13 +745,17 @@ assess_weak_refs(struct ovsdb_txn *txn, struct
>> ovsdb_txn_row *txn_row)
>> ovsdb_datum_destroy(&deleted_refs, &column->type);
>>
>> /* Generating the difference between old and new data. */
>> - if (txn_row->old) {
>> - ovsdb_datum_added_removed(&added, &removed,
>> - &txn_row->old->fields[column->index],
>> - datum, &column->type);
>> - } else {
>> - ovsdb_datum_init_empty(&removed);
>> - ovsdb_datum_clone(&added, datum);
>> + ovsdb_datum_init_empty(&added);
>> + ovsdb_datum_init_empty(&removed);
>
> Nit: Why init these here if they will be overwritten in
> ovsdb_datum_added_removed and ovsdb_datum_clone? Couldn't this be
> included in an else?
We could do that, it just seems a little less error-prone this way.
We'll need to initialize both in the common 'else' and also initialize
'removed' in the clone case, i.e.:
if (datum->n != orig_n
|| bitmap_is_set(txn_row->changed, column->index)) {
if (txn_row->old) {
ovsdb_datum_added_removed(&added, &removed,
&txn_row->old->fields[column->index],
datum, &column->type);
} else {
ovsdb_datum_init_empty(&removed);
ovsdb_datum_clone(&added, datum);
}
} else {
ovsdb_datum_init_empty(&added);
ovsdb_datum_init_empty(&removed);
}
I don't think the initialization here is performance-critical, so
I went with a slightly easier to read solution, that also saves
a couple lines of code. But I have no real objections for the
'else' variant above, if you think it is better.
What's your opinion?
>
> Otherwise, looks good!
>
> Acked-by: Mike Pattrick <m...@redhat.com>
>
>> + if (datum->n != orig_n
>> + || bitmap_is_set(txn_row->changed, column->index)) {
>> + if (txn_row->old) {
>> + ovsdb_datum_added_removed(&added, &removed,
>> +
>> &txn_row->old->fields[column->index],
>> + datum, &column->type);
>> + } else {
>> + ovsdb_datum_clone(&added, datum);
>> + }
>> }
>>
>> /* Checking added data and creating new references. */
>> --
>> 2.43.0
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
