On 5/6/22 07:17, Han Zhou wrote: > On Wed, May 4, 2022 at 4:22 PM Han Zhou <[email protected]> wrote: >> >> >> >> On Wed, May 4, 2022 at 3:44 PM Numan Siddique <[email protected]> wrote: >>> >>> On Tue, May 3, 2022 at 7:32 PM Han Zhou <[email protected]> wrote: >>>> >>>> On Thu, Apr 28, 2022 at 9:36 AM Dumitru Ceara <[email protected]> > 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 <[email protected]> >>>>> --- >>>>> 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. >
Thanks Han! >> 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. > Sounds good to me. We can revisit this later if needed. > Thanks, > Han > Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
