On 1/5/24 16:09, Mike Pattrick wrote:
> On Fri, Jan 5, 2024 at 9:30 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
>>
>> 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?
>
> I think you're right, your way is much more straightforward.
Thanks! I applied the fixes from this set to appropriate baranches.
Will re-spin the last two patches as v2 addressing the comments.
Best regards, Ilya Maximets.
>
> -M
>
>>
>>>
>>> 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
