Hi Ben,

I gave these patches another look, and I can say that for patches 1-10, and patch 27:

Acked-by: Mark Michelson <[email protected]>

For patch 11, I had suggested previously a configure-time check to detect if the correct DDLog version was installed, and you replied with an example that you would roll into the next version of the series. But I don't see it here. Is it in a different patch series?

For patches 12-26, I don't feel qualified to definitively state that it's the "best" way to optimize things, but the changes all make sense to me and don't seem to be incorrect, so for patches 12-26:

Acked-by: Mark Michelson <[email protected]>

but I wouldn't be averse to others having a look to be certain.

Thanks!

On 5/7/21 12:06 AM, Ben Pfaff wrote:
This series of patches greatly improves the performance of
ovn-northd-ddlog with the benchmark added in the final patch.  The
first three patches improve both the benchmark for both versions of
ovn-northd.

Here are the timings that I measure in each case.  All of them
include the benefit of the first three patches.  Without those
patches, the C version takes over 500 seconds and the other take much
longer too; the relative timings aren't affected much, it's just all
slower:

C:                                 106.8s (0.135s ... 1.043s)
ddlog before optimization patches: 176.0s (0.128s ... 1.804s)
ddlog after optimization patches:   35.2s (0.129s ... 0.147s)

(I haven't re-measured for v3.)

v1->v2:
   - Don't remove --output-only-table option from ovsdb2ddlog2c
     in "ovn-northd-ddlog: Intern selected input relations.".
   - New patches "ovn-nbctl: Daemon mode is no longer experimental."
     and "ovn-nbctl: Recommend ovn-appctl instead of ovs-appctl."
     and make similar changes to new ovn-sbctl manpage.
   - Update ovn-sbctl and ovn-nbctl manpages to reference ovn-appctl
     manpage.
   - Various trivial changes suggested by checkpatch.
   - New patches "ovn-nbctl: Fix memory leak in client mode."
     and "ovn-northd-ddlog: Fix two memory leaks." fix memory leaks
     reported by Numan and found by Address Sanitizer.
   - Fix bug introduced into ovsdb2ddlog2c in "ovn-northd-ddlog: Intern
     selected input relations."

v2->v3:
   - Rebase.
   - Apply Mark Michelson's comments for "ovn-sbctl: Add daemon support."
     and to "tests: Miscellaneous debuggability improvements.".
   - New patch "ovn-nbctl: Don't replicate entire database unnecessarily."

Ben Pfaff (12):
   ovn-northd-ddlog: Fix two memory leaks.
   ovn-nbctl: Fix memory leak in client mode.
   ovn-nbctl: Improve manpage.
   ovn-nbctl: Recommend ovn-appctl instead of ovs-appctl.
   ovn-nbctl: Daemon mode is no longer experimental.
   ovn-nbctl: Refactor into infrastructure and northbound details.
   ovn-dbctl: Fix memory leak in client mode.
   ovn-sbctl: Add daemon support.
   ovn-nbctl: Don't replicate entire database unnecessarily.
   tests: Miscellaneous debuggability improvements.
   ovn-northd-ddlog: Preserve NB_Global more carefully.
   tutorial: Add benchmarking test script to run within sandbox.

Leonid Ryzhyk (15):
   ovn-northd-ddlog: Upgrade to ddlog 0.38.
   ovn-northd-ddlog: Remove `lr` field from `Router`.
   ovn-northd-ddlog: Intern the `Router` table.
   ovn-northd-ddlog: Workaround for slow group_by.
   ovn-northd-ddlog: Intern the Switch table.
   ovn-northd-ddlog: Remove `ls` field from `Switch`.
   ovn-northd-ddlog: Intern the SwitchPort table.
   ovn-northd-ddlog: Intern the RouterPort table.
   ovn-northd-ddlog: Remove unused function.
   ovn-northd-ddlog: Eliminate remaining Ref's.
   ovn-northd-ddlog: Eliminate redundant dereferences.
   ovn-northd-ddlog: Intern selected input relations.
   ovn-northd-ddlog: Intern nb::Logical_Router_Port.
   ovn-northd-ddlog: Intern nb::Logical_Switch_Port.
   ovn-northd-ddlog: Remove Router.static_routes.

  NEWS                          |    5 +-
  manpages.mk                   |   17 -
  northd/helpers.dl             |   40 +-
  northd/ipam.dl                |   61 +-
  northd/lrouter.dl             |  188 +--
  northd/lswitch.dl             |  243 ++--
  northd/multicast.dl           |   77 +-
  northd/ovn-nb.dlopts          |   10 +
  northd/ovn-northd-ddlog.c     |   23 +-
  northd/ovn-sb.dlopts          |    1 +
  northd/ovn_northd.dl          | 1045 ++++++++-------
  northd/ovsdb2ddlog2c          |    4 +-
  tests/ovn-sbctl.at            |   76 +-
  tests/ovn.at                  |   51 +-
  tutorial/automake.mk          |    3 +-
  tutorial/northd_ddlog_test.sh |   81 ++
  utilities/automake.mk         |   12 +-
  utilities/ovn-dbctl.c         | 1230 +++++++++++++++++
  utilities/ovn-dbctl.h         |   61 +
  utilities/ovn-nbctl.8.xml     |  667 +++++----
  utilities/ovn-nbctl.c         | 2385 ++++++++++++++-------------------
  utilities/ovn-sbctl.8.in      |  317 -----
  utilities/ovn-sbctl.8.xml     |  580 ++++++++
  utilities/ovn-sbctl.c         |  669 ++-------
  24 files changed, 4464 insertions(+), 3382 deletions(-)
  create mode 100755 tutorial/northd_ddlog_test.sh
  create mode 100644 utilities/ovn-dbctl.c
  create mode 100644 utilities/ovn-dbctl.h
  delete mode 100644 utilities/ovn-sbctl.8.in
  create mode 100644 utilities/ovn-sbctl.8.xml


_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to