On Wed, May 4, 2022 at 4:22 PM Han Zhou <hz...@ovn.org> wrote:
>
>
>
> 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?)
As discussed in today's OVN meeting, it is ok to point OVS submodule to a
point in the master branch.
> 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).
>
The branch-22.03 is already pointing to the master branch that has the new
version of xxx_FOR_EACH_SAFE macros, and the required changes to OVN are
already backported to branch-22.03.
So, I just backported this patch to branch-22.03.
> Similar considerations needed for backporting to OVN 21.12.
branch-21.12 doesn't have the xxx_FOR_EACH_SAFE related patches backported,
and it is not a LTS branch, so I leave it untouched for now. It can be
backported together with the above mentioned changes later if necessary.
Thanks,
Han
>
> 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
