On Wed, Jun 8, 2022 at 1:19 AM Han Zhou <[email protected]> wrote:
>
> On Tue, Jun 7, 2022 at 8:20 AM Dumitru Ceara <[email protected]> wrote:
> >
> > In large scale scenarios this option hugely reduces the size of the
> > Southbound database positively affecting end to end performance. In
> > such scenarios there's no real reason to ever disable datapath groups.
> >
> > In lower scale scenarios any potential overhead due to logical datapath
> > groups is, very likely, negligible.
> >
> > Aside from potential scalability concerns, the
> > NB.NB_Global.options:use_logical_dp_group knob was kept until now to
> > ensure that in case of a bug in the logical datapath groups code a CMS
> > may turn it off and fall back to the mode in which logical flows are not
> > grouped together. As far as I know, this has never happened until now.
> >
> > Moreover, datpath_groups are enabled by default since v21.09.0 (4 stable
> > releases ago), via 90daa7ce18dc ("northd: Enable logical dp groups by
> > default.").
> >
> > From a testing perspective removing this knob will halve the CI matrix.
> > This is desirable, especially in the context of more tests being added,
> > e.g.:
> >
> >
> https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/
> >
> > Signed-off-by: Dumitru Ceara <[email protected]>
>
> Thanks Dumitru! I haven't reviewed the patch in detail, but the idea sounds
> good to me.
>
+1 from me to move this patch from RFC to a formal one.
Numan
> Han
>
> > ---
> > NEWS | 1 +
> > northd/northd.c | 77 +++++++++++--------------------------
> > ovn-nb.xml | 19 +++-------
> > tests/ovn-macros.at | 23 ++----------
> > tests/ovn-northd.at | 92 ++++-----------------------------------------
> > tests/ovs-macros.at | 4 +-
> > 6 files changed, 41 insertions(+), 175 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index e015ae8e7..20b4c5d91 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -4,6 +4,7 @@ Post v22.06.0
> > "ovn-encap-df_default" to enable or disable tunnel DF flag.
> > - Add option "localnet_learn_fdb" to LSP that will allow localnet
> > ports to learn MAC addresses and store them in FDB table.
> > + - Removed possibility of disabling logical datapath groups.
> >
> > OVN v22.06.0 - XX XXX XXXX
> > --------------------------
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 0207f6ce1..a97a321cd 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -4813,10 +4813,6 @@ ovn_lflow_equal(const struct ovn_lflow *a, const
> struct ovn_datapath *od,
> > && nullable_string_is_equal(a->ctrl_meter, ctrl_meter));
> > }
> >
> > -/* If this option is 'true' northd will combine logical flows that
> differ by
> > - * logical datapath only by creating a datapath group. */
> > -static bool use_logical_dp_groups = false;
> > -
> > enum {
> > STATE_NULL, /* parallelization is off */
> > STATE_INIT_HASH_SIZES, /* parallelization is on; hashes sizing
> needed */
> > @@ -4841,8 +4837,7 @@ ovn_lflow_init(struct ovn_lflow *lflow, struct
> ovn_datapath *od,
> > lflow->ctrl_meter = ctrl_meter;
> > lflow->dpg = NULL;
> > lflow->where = where;
> > - if ((parallelization_state != STATE_NULL)
> > - && use_logical_dp_groups) {
> > + if (parallelization_state != STATE_NULL) {
> > ovs_mutex_init(&lflow->odg_lock);
> > }
> > }
> > @@ -4852,7 +4847,7 @@ ovn_dp_group_add_with_reference(struct ovn_lflow
> *lflow_ref,
> > struct ovn_datapath *od)
> > OVS_NO_THREAD_SAFETY_ANALYSIS
> > {
> > - if (!use_logical_dp_groups || !lflow_ref) {
> > + if (!lflow_ref) {
> > return false;
> > }
> >
> > @@ -4931,13 +4926,11 @@ do_ovn_lflow_add(struct hmap *lflow_map, struct
> ovn_datapath *od,
> > struct ovn_lflow *old_lflow;
> > struct ovn_lflow *lflow;
> >
> > - if (use_logical_dp_groups) {
> > - old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority,
> match,
> > - actions, ctrl_meter, hash);
> > - if (old_lflow) {
> > - ovn_dp_group_add_with_reference(old_lflow, od);
> > - return old_lflow;
> > - }
> > + old_lflow = ovn_lflow_find(lflow_map, NULL, stage, priority, match,
> > + actions, ctrl_meter, hash);
> > + if (old_lflow) {
> > + ovn_dp_group_add_with_reference(old_lflow, od);
> > + return old_lflow;
> > }
> >
> > lflow = xmalloc(sizeof *lflow);
> > @@ -4993,8 +4986,7 @@ ovn_lflow_add_at_with_hash(struct hmap *lflow_map,
> struct ovn_datapath *od,
> > struct ovn_lflow *lflow;
> >
> > ovs_assert(ovn_stage_to_datapath_type(stage) ==
> ovn_datapath_get_type(od));
> > - if (use_logical_dp_groups
> > - && (parallelization_state == STATE_USE_PARALLELIZATION)) {
> > + if (parallelization_state == STATE_USE_PARALLELIZATION) {
> > lflow = do_ovn_lflow_add_pd(lflow_map, od, hash, stage, priority,
> > match, actions, io_port, stage_hint,
> where,
> > ctrl_meter);
> > @@ -5080,8 +5072,7 @@ static void
> > ovn_lflow_destroy(struct hmap *lflows, struct ovn_lflow *lflow)
> > {
> > if (lflow) {
> > - if ((parallelization_state != STATE_NULL)
> > - && use_logical_dp_groups) {
> > + if (parallelization_state != STATE_NULL) {
> > ovs_mutex_destroy(&lflow->odg_lock);
> > }
> > if (lflows) {
> > @@ -13883,24 +13874,15 @@ build_lswitch_and_lrouter_flows(const struct
> hmap *datapaths,
> > int index;
> >
> > lsiv = xcalloc(sizeof(*lsiv), build_lflows_pool->size);
> > - if (use_logical_dp_groups) {
> > - lflow_segs = NULL;
> > - } else {
> > - lflow_segs = xcalloc(sizeof(*lflow_segs),
> build_lflows_pool->size);
> > - }
> > + lflow_segs = NULL;
> >
> > /* Set up "work chunks" for each thread to work on. */
> >
> > for (index = 0; index < build_lflows_pool->size; index++) {
> > - if (use_logical_dp_groups) {
> > - /* if dp_groups are in use we lock a shared lflows hash
> > - * on a per-bucket level instead of merging hash frags */
> > - lsiv[index].lflows = lflows;
> > - } else {
> > - fast_hmap_init(&lflow_segs[index], lflows->mask);
> > - lsiv[index].lflows = &lflow_segs[index];
> > - }
> > -
> > + /* dp_groups are in use so we lock a shared lflows hash
> > + * on a per-bucket level.
> > + */
> > + lsiv[index].lflows = lflows;
> > lsiv[index].datapaths = datapaths;
> > lsiv[index].ports = ports;
> > lsiv[index].port_groups = port_groups;
> > @@ -13919,13 +13901,8 @@ build_lswitch_and_lrouter_flows(const struct
> hmap *datapaths,
> > }
> >
> > /* Run thread pool. */
> > - if (use_logical_dp_groups) {
> > - run_pool_callback(build_lflows_pool, NULL, NULL,
> > - noop_callback);
> > - fix_flow_map_size(lflows, lsiv, build_lflows_pool->size);
> > - } else {
> > - run_pool_hash(build_lflows_pool, lflows, lflow_segs);
> > - }
> > + run_pool_callback(build_lflows_pool, NULL, NULL, noop_callback);
> > + fix_flow_map_size(lflows, lsiv, build_lflows_pool->size);
> >
> > for (index = 0; index < build_lflows_pool->size; index++) {
> > ds_destroy(&lsiv[index].match);
> > @@ -14077,21 +14054,15 @@ void run_update_worker_pool(int n_threads)
> > lflow_hash_lock_destroy();
> > parallelization_state = STATE_NULL;
> > } else if (parallelization_state != STATE_USE_PARALLELIZATION) {
> > - if (use_logical_dp_groups) {
> > - lflow_hash_lock_init();
> > - parallelization_state = STATE_INIT_HASH_SIZES;
> > - } else {
> > - parallelization_state = STATE_USE_PARALLELIZATION;
> > - }
> > + lflow_hash_lock_init();
> > + parallelization_state = STATE_INIT_HASH_SIZES;
> > }
> > }
> > }
> >
> > -static void worker_pool_init_for_ldp(void)
> > +static void init_worker_pool(void)
> > {
> > - /* If parallelization is enabled, make sure locks are initialized
> > - * when ldp are used.
> > - */
> > + /* If parallelization is enabled, make sure locks are initialized. */
> > if (parallelization_state != STATE_NULL) {
> > lflow_hash_lock_init();
> > parallelization_state = STATE_INIT_HASH_SIZES;
> > @@ -15361,13 +15332,6 @@ ovnnb_db_run(struct northd_input *input_data,
> > nbrec_nb_global_set_options(nb, &options);
> > }
> >
> > - bool old_use_ldp = use_logical_dp_groups;
> > - use_logical_dp_groups = smap_get_bool(&nb->options,
> > - "use_logical_dp_groups", true);
> > - if (use_logical_dp_groups && !old_use_ldp) {
> > - worker_pool_init_for_ldp();
> > - }
> > -
> > use_ct_inv_match = smap_get_bool(&nb->options,
> > "use_ct_inv_match", true);
> >
> > @@ -15666,6 +15630,7 @@ void northd_run(struct northd_input *input_data,
> > struct ovsdb_idl_txn *ovnsb_txn)
> > {
> > stopwatch_start(OVNNB_DB_RUN_STOPWATCH_NAME, time_msec());
> > + init_worker_pool();
> > ovnnb_db_run(input_data, data, ovnnb_txn, ovnsb_txn,
> > input_data->sbrec_chassis_by_name,
> > input_data->sbrec_chassis_by_hostname);
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index 618a48b97..dcc75ea73 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -196,21 +196,14 @@
> >
> > <column name="options" key="use_logical_dp_groups">
> > <p>
> > - If set to <code>true</code>, <code>ovn-northd</code> will
> combine
> > - logical flows that differs only by logical datapath into a
> single
> > - logical flow with logical datapath group attached.
> > + Note: This option is deprecated, the only behavior is to always
> > + combine logical flows by datapath groups. Changing the value
> or
> > + removing this option all toghether will have no effect.
> > </p>
> > <p>
> > - While this should significantly reduce number of logical flows
> stored
> > - in Southbound database this could also increase processing
> complexity
> > - on the <code>ovn-controller</code> side, e.g.,
> > - <code>ovn-controller</code> will re-consider logical flow for
> all
> > - logical datapaths in a group. If the option set to
> > - <code>false</code>, there will be separate logical flow per
> logical
> > - datapath and only this flow will be re-considered.
> > - </p>
> > - <p>
> > - The default value is <code>false</code>.
> > + <code>ovn-northd</code> combines logical flows that differs
> > + only by logical datapath into a single logical flow with
> > + logical datapath group attached.
> > </p>
> > </column>
> > <column name="options" key="use_parallel_build">
> > diff --git a/tests/ovn-macros.at b/tests/ovn-macros.at
> > index c6f0f6251..89f432f16 100644
> > --- a/tests/ovn-macros.at
> > +++ b/tests/ovn-macros.at
> > @@ -246,12 +246,6 @@ ovn_start () {
> > --ic-nb-db=unix:"$ovs_base"/ovn-ic-nb/ovn-ic-nb.sock \
> > --ic-sb-db=unix:"$ovs_base"/ovn-ic-sb/ovn-ic-sb.sock
> > fi
> > -
> > - if test X$NORTHD_USE_DP_GROUPS = Xyes; then
> > - ovn-nbctl set NB_Global . options:use_logical_dp_groups=true
> > - else
> > - ovn-nbctl set NB_Global . options:use_logical_dp_groups=false
> > - fi
> > }
> >
> > # Interconnection networks.
> > @@ -749,21 +743,12 @@ m4_define([OVN_POPULATE_ARP],
> [AT_CHECK(ovn_populate_arp__, [0], [ignore])])
> > # datapath groups.
> > m4_define([OVN_FOR_EACH_NORTHD],
> > [m4_foreach([NORTHD_TYPE], [ovn-northd, ovn-northd-ddlog],
> > - [m4_foreach([NORTHD_USE_DP_GROUPS], [yes, no],
> > - [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], [$1
> > -])])])])
> > -
> > -# Some tests aren't prepared for dp groups to be enabled.
> > -m4_define([OVN_FOR_EACH_NORTHD_WITHOUT_DP_GROUPS],
> > - [m4_foreach([NORTHD_TYPE], [ovn-northd, ovn-northd-ddlog],
> > - [m4_foreach([NORTHD_USE_DP_GROUPS], [no],
> > - [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], [$1
> > -])])])])
> > + [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], [$1
> > +])])])
> >
> > # Some tests aren't prepared for ddlog to be enabled.
> > m4_define([OVN_FOR_EACH_NORTHD_WITHOUT_DDLOG],
> > [m4_foreach([NORTHD_TYPE], [ovn-northd],
> > - [m4_foreach([NORTHD_USE_DP_GROUPS], [yes, no],
> > - [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], [$1
> > -])])])])
> > + [m4_foreach([NORTHD_USE_PARALLELIZATION], [yes, no], [$1
> > +])])])
> >
> > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> > index a94a7d441..e6a2e9d83 100644
> > --- a/tests/ovn-northd.at
> > +++ b/tests/ovn-northd.at
> > @@ -755,7 +755,7 @@ wait_row_count Datapath_Binding 2
> > AT_CLEANUP
> > ])
> >
> > -OVN_FOR_EACH_NORTHD_WITHOUT_DP_GROUPS([
> > +OVN_FOR_EACH_NORTHD([
> > AT_SETUP([northbound database reconnection])
> >
> > ovn_start --backup-northd=none
> > @@ -765,7 +765,7 @@ ovn_start --backup-northd=none
> > check_row_count Datapath_Binding 0
> > check ovn-nbctl --wait=sb ls-add sw0
> > check_row_count Datapath_Binding 1
> > -lf=$(count_rows Logical_Flow)
> > +dp1=$(fetch_column Datapath_Binding _uuid external_ids:name=sw0)
> >
> > # Make nbdb ovsdb-server drop connection from ovn-northd.
> > conn=$(as ovn-nb ovs-appctl -t ovsdb-server
> ovsdb-server/list-remotes|grep ^punix)
> > @@ -777,7 +777,6 @@ check as ovn-nb ovs-appctl -t ovsdb-server
> ovsdb-server/add-remote "$conn2"
> > check ovn-nbctl --db="${conn2#p}" ls-add sw1
> > sleep 5
> > check_row_count Datapath_Binding 1
> > -check_row_count Logical_Flow $lf
> >
> > # Now re-enable the nbdb connection and observe ovn-northd catch up.
> > #
> > @@ -786,12 +785,13 @@ check_row_count Logical_Flow $lf
> > # differently on reconnection.
> > check as ovn-nb ovs-appctl -t ovsdb-server ovsdb-server/add-remote
> "$conn"
> > wait_row_count Datapath_Binding 2
> > -wait_row_count Logical_Flow $(expr 2 \* $lf)
> > +dp2=$(fetch_column Datapath_Binding _uuid external_ids:name=sw1)
> > +wait_column "$dp1 $dp2" Logical_DP_Group datapaths
> >
> > AT_CLEANUP
> > ])
> >
> > -OVN_FOR_EACH_NORTHD_WITHOUT_DP_GROUPS([
> > +OVN_FOR_EACH_NORTHD([
> > AT_SETUP([southbound database reconnection])
> >
> > ovn_start --backup-northd=none
> > @@ -801,7 +801,7 @@ ovn_start --backup-northd=none
> > check_row_count Datapath_Binding 0
> > check ovn-nbctl --wait=sb ls-add sw0
> > check_row_count Datapath_Binding 1
> > -lf=$(count_rows Logical_Flow)
> > +dp1=$(fetch_column Datapath_Binding _uuid external_ids:name=sw0)
> >
> > # Make sbdb ovsdb-server drop connection from ovn-northd.
> > conn=$(as ovn-sb ovs-appctl -t ovsdb-server
> ovsdb-server/list-remotes|grep ^punix)
> > @@ -813,7 +813,6 @@ check as ovn-sb ovs-appctl -t ovsdb-server
> ovsdb-server/add-remote "$conn2"
> > check ovn-nbctl ls-add sw1
> > sleep 5
> > OVN_SB_DB=${conn2#p} check_row_count Datapath_Binding 1
> > -OVN_SB_DB=${conn2#p} check_row_count Logical_Flow $lf
> >
> > # Now re-enable the sbdb connection and observe ovn-northd catch up.
> > #
> > @@ -822,7 +821,8 @@ OVN_SB_DB=${conn2#p} check_row_count Logical_Flow $lf
> > # differently on reconnection.
> > check as ovn-sb ovs-appctl -t ovsdb-server ovsdb-server/add-remote
> "$conn"
> > wait_row_count Datapath_Binding 2
> > -wait_row_count Logical_Flow $(expr 2 \* $lf)
> > +dp2=$(fetch_column Datapath_Binding _uuid external_ids:name=sw1)
> > +wait_column "$dp1 $dp2" Logical_DP_Group datapaths
> >
> > AT_CLEANUP
> > ])
> > @@ -2648,82 +2648,6 @@ AT_CHECK([ovn-sbctl lflow-list sw0 | grep
> ls_in_hairpin | sort | sed 's/table=..
> > AT_CLEANUP
> > ])
> >
> > -OVN_FOR_EACH_NORTHD([
> > -AT_SETUP([logical gatapath groups])
> > -AT_KEYWORDS([use_logical_dp_groups])
> > -ovn_start
> > -
> > -dnl Disabling datapath groups.
> > -ovn-nbctl --wait=sb set NB_Global . options:use_logical_dp_groups=false
> > -
> > -ovn-nbctl ls-add sw1
> > -ovn-nbctl ls-add sw2
> > -ovn-nbctl lsp-add sw1 swp1
> > -ovn-nbctl lsp-add sw2 swp2
> > -ovn-nbctl --wait=sb sync
> > -
> > -sw1_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=sw1)
> > -sw2_sb_uuid=$(fetch_column datapath_binding _uuid external_ids:name=sw2)
> > -
> > -dnl Check that we have no datapath groups.
> > -check_row_count Logical_DP_Group 0
> > -
> > -dnl Number of logical flows that depends on logical switch or multicast
> group.
> > -dnl These will not be combined.
> > -n_flows_specific=$(ovn-sbctl -f table find Logical_Flow | grep -cE 'swp')
> > -echo "Number of specific flows: "${n_flows_specific}
> > -
> > -dnl Both logical switches configured identically, so there should be same
> > -dnl number of logical flows per logical switch/logical datapath.
> > -n_flows=$(count_rows Logical_Flow)
> > -echo "Total number of flows with datapath groups disabled: "${n_flows}
> > -n_flows_half=$((${n_flows} / 2))
> > -check_row_count Logical_Flow ${n_flows_half}
> logical_datapath=${sw1_sb_uuid}
> > -check_row_count Logical_Flow ${n_flows_half}
> logical_datapath=${sw2_sb_uuid}
> > -
> > -dnl Enabling datapath groups.
> > -ovn-nbctl --wait=sb set NB_Global . options:use_logical_dp_groups=true
> > -
> > -dnl Check that one datapath group created.
> > -check_row_count Logical_DP_Group 1
> > -dp_group_uuid=$(fetch_column logical_dp_group _uuid)
> > -
> > -dnl Check that datapath group contains both datapaths.
> > -check_column "${sw1_sb_uuid} ${sw2_sb_uuid}" Logical_DP_Group datapaths
> > -
> > -dnl Calculating number of flows that should be combined for a datapath
> group.
> > -n_flows=$(count_rows Logical_Flow)
> > -echo "Total number of flows with datapath groups enabled: "${n_flows}
> > -n_flows_common=$((${n_flows} - ${n_flows_specific}))
> > -
> > -check_row_count Logical_Flow ${n_flows_common}
> logical_dp_group=${dp_group_uuid}
> > -check_row_count Logical_Flow ${n_flows_common} logical_datapath=[[]]
> > -check_row_count Logical_Flow ${n_flows_common} \
> > - logical_dp_group=${dp_group_uuid} logical_datapath=[[]]
> > -
> > -dnl Adding 8 more logical switches and ports.
> > -for i in $(seq 3 10); do
> > - ovn-nbctl ls-add sw${i}
> > - ovn-nbctl lsp-add sw${i} swp${i}
> > -done
> > -ovn-nbctl --wait=sb sync
> > -
> > -dnl Number of logical flows should be increased only due to specific
> flows.
> > -expected_n_flows=$((${n_flows_common} + 5 * ${n_flows_specific}))
> > -echo "Total number of flows with 10 logical switches should be: " \
> > - ${expected_n_flows}
> > -check_row_count Logical_Flow ${expected_n_flows}
> > -
> > -dnl Should be still only one datapath group.
> > -check_row_count Logical_DP_Group 1
> > -dp_group_uuid=$(fetch_column logical_dp_group _uuid)
> > -
> > -dnl Number of common flows should be the same.
> > -check_row_count Logical_Flow ${n_flows_common}
> logical_dp_group=${dp_group_uuid}
> > -
> > -AT_CLEANUP
> > -])
> > -
> > OVN_FOR_EACH_NORTHD([
> > AT_SETUP([Router policies - ECMP reroute])
> > AT_KEYWORDS([router policies ecmp reroute])
> > diff --git a/tests/ovs-macros.at b/tests/ovs-macros.at
> > index 09c05cf16..5d325559c 100644
> > --- a/tests/ovs-macros.at
> > +++ b/tests/ovs-macros.at
> > @@ -10,11 +10,9 @@ dnl set it as a shell variable as well.
> > dnl - Skip the test if it's for ovn-northd-ddlog but it didn't get built.
> > m4_rename([AT_SETUP], [OVS_AT_SETUP])
> > m4_define([AT_SETUP],
> > - [OVS_AT_SETUP($@[]m4_ifdef([NORTHD_TYPE], [ --
> NORTHD_TYPE])[]m4_ifdef([NORTHD_USE_DP_GROUPS], [ --
> dp-groups=NORTHD_USE_DP_GROUPS])[]m4_ifdef([NORTHD_USE_PARALLELIZATION], [
> -- parallelization=NORTHD_USE_PARALLELIZATION]))
> > + [OVS_AT_SETUP($@[]m4_ifdef([NORTHD_TYPE], [ --
> NORTHD_TYPE])[]m4_ifdef([NORTHD_USE_PARALLELIZATION], [ --
> parallelization=NORTHD_USE_PARALLELIZATION]))
> > m4_ifdef([NORTHD_TYPE], [[NORTHD_TYPE]=NORTHD_TYPE
> > ])dnl
> > -m4_ifdef([NORTHD_USE_DP_GROUPS],
> [[NORTHD_USE_DP_GROUPS]=NORTHD_USE_DP_GROUPS
> > -])dnl
> > m4_ifdef([NORTHD_USE_PARALLELIZATION],
> [[NORTHD_USE_PARALLELIZATION]=NORTHD_USE_PARALLELIZATION
> > ])dnl
> > m4_ifdef([NORTHD_DUMMY_NUMA], [[NORTHD_DUMMY_NUMA]=NORTHD_DUMMY_NUMA
> > --
> > 2.31.1
> >
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
