On Fri, Jan 5, 2024 at 9:30 AM Ilya Maximets <[email protected]> wrote: > > On 12/29/23 17:28, Mike Pattrick wrote: > > On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets <[email protected]> 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 <[email protected]> > >> --- > >> 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. -M > > > > > Otherwise, looks good! > > > > Acked-by: Mike Pattrick <[email protected]> > > > >> + 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 [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
