On Wed, May 4, 2022 at 3:44 PM Numan Siddique <num...@ovn.org> wrote:
>
> On Tue, May 3, 2022 at 7:32 PM Han Zhou <hz...@ovn.org> wrote:
> >
> > On Thu, Apr 28, 2022 at 9:36 AM Dumitru Ceara <dce...@redhat.com> wrote:
> > >
> > > This uses the newly added ovsdb_idl_set_write_changed_only_all() to
> > > explicitly disable atomicity checks for read/write columns and to
enable
> > > optimizing the process of building the SB transaction.
> > >
> > > Bump OVS submodule to pick up d94cd0d3eec3 ("ovsdb-idl: Support
> > > write-only-changed IDL monitor mode.").
> > >
> > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2069623
> > > Signed-off-by: Dumitru Ceara <dce...@redhat.com>
> > > ---
> > > NOTE: this bumps the OVS submodule to a revision that's not yet part
of
> > > a stable branch.
> > > ---
> > > northd/ovn-northd.c | 10 +++++++++-
> > > ovs | 2 +-
> > > 2 files changed, 10 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index 45b120697..0a0f85010 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -723,10 +723,18 @@ main(int argc, char *argv[])
> > > unixctl_command_register("nb-connection-status", "", 0, 0,
> > > ovn_conn_show, ovnnb_idl_loop.idl);
> > >
> > > - /* We want to detect all changes to the ovn-sb db. */
> > > + /* We want to detect all changes to the ovn-sb db so enable
change
> > > + * tracking but, for performance reasons, and because northd
> > > + * reconciles all database changes, also configure the IDL to
only
> > > + * write columns that actually change value.
> > > + */
> > > struct ovsdb_idl_loop ovnsb_idl_loop =
OVSDB_IDL_LOOP_INITIALIZER(
> > > ovsdb_idl_create(ovnsb_db, &sbrec_idl_class, true, true));
> > > ovsdb_idl_track_add_all(ovnsb_idl_loop.idl);
> > > + ovsdb_idl_set_write_changed_only_all(ovnsb_idl_loop.idl, true);
> > > +
> > > + /* Disable alerting for pure write-only columns. */
> > > + ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
> > &sbrec_sb_global_col_nb_cfg);
> > >
> > > unixctl_command_register("sb-connection-status", "", 0, 0,
> > > ovn_conn_show, ovnsb_idl_loop.idl);
> > > diff --git a/ovs b/ovs
> > > index ba159ee0f..d94cd0d3e 160000
> > > --- a/ovs
> > > +++ b/ovs
> > > @@ -1 +1 @@
> > > -Subproject commit ba159ee0f97ed770c244cd6710d34fe20595541d
> > > +Subproject commit d94cd0d3eec33e4290d7ca81918f5ac61444886e
> > > --
> > > 2.27.0
> > >
> >
> > Thanks Dumitru. I applied this to main.
> > I tested with a scale test and the extra SB writing cost is gone. Also,
the
> > recompute triggered by handling SB SB_Global:nb_cfg notification is
gone.
> > However, compared with the version before "ovn-northd: Enable change
> > tracking for all SB tables." (but after I-P northd), it still has an
extra
> > recompute triggered by change notifications for the SB changes made by
> > ovn-northd itself. I think we can probably still avoid this cost by
> > carefully evaluating and omitting (without tracking) columns that are
> > really "write-only". This can be separate patches of improvements.
> >
> > Since this patch is a fix for a performance regression, I am wondering
if
> > we should backport to 22.03 (or even older branches). It may be tricky
> > since this depends on the latest OVS change (not on any branch). Any
> > thoughts on this, Mark, Numan?
>
>
> +1 from me for the backport to 22.03 and 21.12. I think it's not
> applicable to 21.09 and older.
>
> Numan
Thanks Numan. There is one tricky point here. I remember we have the
practice that stable branches of OVN should have the OVS submodule point to
a stable branch of OVS repo. So, to backport this to 22.03, we need:
1. backport the OVS change to branch-2.17 first (@Ilya may help?)
2. bump the version of OVS submodule to the tip of branch-2.17. However,
branch-2.17 already contains the xxx_FOR_EACH_SAFE change. Although it is
backward compatible, it did mention in the NEWS that the API users "need to
double-check the use of such loop macros before compiling with a new
version" because "the user-provided pointer is now set to NULL after the
loop". I am wondering if we want to backport the patches in OVN that are
related to the xxx_FOR_EACH_SAFE macro changes just so that we will be safe
to use the OVS 2.17 with OVN 22.03 (hopefully not necessary).
Similar considerations needed for backporting to OVN 21.12.
Thanks,
Han
>
> >
> > Thanks,
> > Han
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
