| Message ID | 20231218020249.3447973-4-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Commit | 0ef3ebb0cfef60a0f395ee7e2cd556c1dceab125 |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | ovsdb: transaction: Bug Fixes + Reuse of column diffs. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets <i.maximets@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.maximets@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? Otherwise, looks good! Acked-by: Mike Pattrick <mkp@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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 12/29/23 17:28, Mike Pattrick wrote: > On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets <i.maximets@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.maximets@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 <mkp@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
On Fri, Jan 5, 2024 at 9:30 AM Ilya Maximets <i.maximets@ovn.org> wrote: > > On 12/29/23 17:28, Mike Pattrick wrote: > > On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets <i.maximets@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.maximets@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. -M > > > > > Otherwise, looks good! > > > > Acked-by: Mike Pattrick <mkp@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 >
On 1/5/24 16:09, Mike Pattrick wrote: > On Fri, Jan 5, 2024 at 9:30 AM Ilya Maximets <i.maximets@ovn.org> wrote: >> >> On 12/29/23 17:28, Mike Pattrick wrote: >>> On Sun, Dec 17, 2023 at 9:03 PM Ilya Maximets <i.maximets@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.maximets@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 <mkp@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 >> >
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); + 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. */
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.maximets@ovn.org> --- ovsdb/transaction.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)