| Message ID | 20210827191742.2307529-3-i.maximets@ovn.org |
|---|---|
| State | Accepted |
| Headers | show |
| Series | northd: Rework and optimize reconciliation of datapath groups. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
On 8/27/21 9:17 PM, Ilya Maximets wrote: > The '/* Push changes to the Logical_Flow table to database. */' loop > reads all the datapaths from the datapath group and checks them even > if it's not required. > > To check that flow has at least one valid datapath, we need to find > one valid datapath. To find Sb logical flow we need a datapath type > and for this we only need one datapath again. Technically, the only > place where we need to check all datapaths is when we're checking > certain Sb datapath group for the first time. In all other cases > we can validate/discard the flow by other characteristics that we > already know. > > The change saves a fair amount of time in this loop. Up to a few > seconds in case of a big database. > > More comments added to make the code easier to read. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- [...] > @@ -13307,24 +13299,46 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, > * updates. */ > bool update_dp_group = false; > > - if (n_datapaths != hmapx_count(&lflow->od_group)) { > - /* Groups are different. */ > + if ((!lflow->dpg && dp_group) || (lflow->dpg && !dp_group)) { > + /* Need to add or delete datapath group. */ > update_dp_group = true; > - } else if (lflow->dpg && lflow->dpg->dp_group) { > + } else if (!lflow->dpg && !dp_group) { > + /* No datapath group and not needed. */ > + } else if (lflow->dpg->dp_group) { > /* We know the datapath group in Sb that should be used. */ > if (lflow->dpg->dp_group != dp_group) { > /* Flow has different datapath group in the database. */ > update_dp_group = true; > } > /* Datapath group is already up to date. */ > - } else if (n_datapaths) { > + } else { > + /* There is a datapath group and we need to perfrom Typo: perfrom Except for this, I think the rest is fine. You might want to rebase though as I had to manually apply the second patch in this series on top of the latest code. Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks!
On Tue, Aug 31, 2021 at 9:41 AM Dumitru Ceara <dceara@redhat.com> wrote: > > On 8/27/21 9:17 PM, Ilya Maximets wrote: > > The '/* Push changes to the Logical_Flow table to database. */' loop > > reads all the datapaths from the datapath group and checks them even > > if it's not required. > > > > To check that flow has at least one valid datapath, we need to find > > one valid datapath. To find Sb logical flow we need a datapath type > > and for this we only need one datapath again. Technically, the only > > place where we need to check all datapaths is when we're checking > > certain Sb datapath group for the first time. In all other cases > > we can validate/discard the flow by other characteristics that we > > already know. > > > > The change saves a fair amount of time in this loop. Up to a few > > seconds in case of a big database. > > > > More comments added to make the code easier to read. > > > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > > --- > > [...] > > > @@ -13307,24 +13299,46 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, > > * updates. */ > > bool update_dp_group = false; > > > > - if (n_datapaths != hmapx_count(&lflow->od_group)) { > > - /* Groups are different. */ > > + if ((!lflow->dpg && dp_group) || (lflow->dpg && !dp_group)) { > > + /* Need to add or delete datapath group. */ > > update_dp_group = true; > > - } else if (lflow->dpg && lflow->dpg->dp_group) { > > + } else if (!lflow->dpg && !dp_group) { > > + /* No datapath group and not needed. */ > > + } else if (lflow->dpg->dp_group) { > > /* We know the datapath group in Sb that should be used. */ > > if (lflow->dpg->dp_group != dp_group) { > > /* Flow has different datapath group in the database. */ > > update_dp_group = true; > > } > > /* Datapath group is already up to date. */ > > - } else if (n_datapaths) { > > + } else { > > + /* There is a datapath group and we need to perfrom > > Typo: perfrom > > Except for this, I think the rest is fine. You might want to rebase > though as I had to manually apply the second patch in this series on top > of the latest code. > > Acked-by: Dumitru Ceara <dceara@redhat.com> Thanks Ilya and Dumitru. I corrected the typo and applied both the patches to the main branch. Numan > > Thanks! > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index ae5874518..b7388d317 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -13231,21 +13231,10 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, const struct sbrec_logical_flow *sbflow, *next_sbflow; SBREC_LOGICAL_FLOW_FOR_EACH_SAFE (sbflow, next_sbflow, ctx->ovnsb_idl) { struct sbrec_logical_dp_group *dp_group = sbflow->logical_dp_group; - struct ovn_datapath **od, *logical_datapath_od = NULL; - int n_datapaths = 0; + struct ovn_datapath *logical_datapath_od = NULL; size_t i; - od = xmalloc((dp_group ? dp_group->n_datapaths + 1 : 1) * sizeof *od); - /* Check all logical datapaths from the group. */ - for (i = 0; dp_group && i < dp_group->n_datapaths; i++) { - od[n_datapaths] = ovn_datapath_from_sbrec(datapaths, - dp_group->datapaths[i]); - if (!od[n_datapaths] || ovn_datapath_is_stale(od[n_datapaths])) { - continue; - } - n_datapaths++; - } - + /* Find one valid datapath to get the datapath type. */ struct sbrec_datapath_binding *dp = sbflow->logical_datapath; if (dp) { logical_datapath_od = ovn_datapath_from_sbrec(datapaths, dp); @@ -13254,26 +13243,29 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, logical_datapath_od = NULL; } } + for (i = 0; dp_group && i < dp_group->n_datapaths; i++) { + logical_datapath_od = ovn_datapath_from_sbrec( + datapaths, dp_group->datapaths[i]); + if (logical_datapath_od + && !ovn_datapath_is_stale(logical_datapath_od)) { + break; + } + logical_datapath_od = NULL; + } - if (!n_datapaths && !logical_datapath_od) { + if (!logical_datapath_od) { /* This lflow has no valid logical datapaths. */ sbrec_logical_flow_delete(sbflow); - free(od); continue; } enum ovn_pipeline pipeline = !strcmp(sbflow->pipeline, "ingress") ? P_IN : P_OUT; - enum ovn_datapath_type dp_type; - if (n_datapaths) { - dp_type = od[0]->nbs ? DP_SWITCH : DP_ROUTER; - } else { - dp_type = logical_datapath_od->nbs ? DP_SWITCH : DP_ROUTER; - } lflow = ovn_lflow_find( - &lflows, logical_datapath_od, - ovn_stage_build(dp_type, pipeline, sbflow->table_id), + &lflows, dp_group ? NULL : logical_datapath_od, + ovn_stage_build(ovn_datapath_get_type(logical_datapath_od), + pipeline, sbflow->table_id), sbflow->priority, sbflow->match, sbflow->actions, sbflow->controller_meter, sbflow->hash); if (lflow) { @@ -13307,24 +13299,46 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, * updates. */ bool update_dp_group = false; - if (n_datapaths != hmapx_count(&lflow->od_group)) { - /* Groups are different. */ + if ((!lflow->dpg && dp_group) || (lflow->dpg && !dp_group)) { + /* Need to add or delete datapath group. */ update_dp_group = true; - } else if (lflow->dpg && lflow->dpg->dp_group) { + } else if (!lflow->dpg && !dp_group) { + /* No datapath group and not needed. */ + } else if (lflow->dpg->dp_group) { /* We know the datapath group in Sb that should be used. */ if (lflow->dpg->dp_group != dp_group) { /* Flow has different datapath group in the database. */ update_dp_group = true; } /* Datapath group is already up to date. */ - } else if (n_datapaths) { + } else { + /* There is a datapath group and we need to perfrom + * a full comparison. */ + struct ovn_datapath **od; + int n_datapaths = 0; + + od = xmalloc(dp_group->n_datapaths * sizeof *od); + /* Check all logical datapaths from the group. */ + for (i = 0; i < dp_group->n_datapaths; i++) { + od[n_datapaths] = ovn_datapath_from_sbrec( + datapaths, dp_group->datapaths[i]); + if (!od[n_datapaths] + || ovn_datapath_is_stale(od[n_datapaths])) { + continue; + } + n_datapaths++; + } + + if (n_datapaths != hmapx_count(&lflow->od_group)) { + update_dp_group = true; + } /* Have to compare datapath groups in full. */ - for (i = 0; i < n_datapaths; i++) { + for (i = 0; !update_dp_group && i < n_datapaths; i++) { if (od[i] && !hmapx_contains(&lflow->od_group, od[i])) { update_dp_group = true; - break; } } + free(od); } if (update_dp_group) { @@ -13341,7 +13355,6 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths, } else { sbrec_logical_flow_delete(sbflow); } - free(od); } struct ovn_lflow *next_lflow;
The '/* Push changes to the Logical_Flow table to database. */' loop reads all the datapaths from the datapath group and checks them even if it's not required. To check that flow has at least one valid datapath, we need to find one valid datapath. To find Sb logical flow we need a datapath type and for this we only need one datapath again. Technically, the only place where we need to check all datapaths is when we're checking certain Sb datapath group for the first time. In all other cases we can validate/discard the flow by other characteristics that we already know. The change saves a fair amount of time in this loop. Up to a few seconds in case of a big database. More comments added to make the code easier to read. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- northd/ovn-northd.c | 73 ++++++++++++++++++++++++++------------------- 1 file changed, 43 insertions(+), 30 deletions(-)