Hi Ilya, Thank you for your quick review On Thu, Jul 25, 2024 at 12:00 AM Ilya Maximets <i.maxim...@ovn.org> wrote:
> On 7/24/24 11:56, Mohammad Heib wrote: > > Northd will not update/add flows for a virtual port if their > > parent ports were created/recreated after creating this virtual port. > > > > This change will fix the above issue by triggering an update for a > > virtual port if their parent port has been created. > > > > Rreported-at: https://issues.redhat.com/browse/FDP-710 > > Signed-off-by: Mohammad Heib <mh...@redhat.com> > > --- > > northd/northd.c | 19 ++++++++++- > > tests/ovn.at | 84 +++++++++++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 102 insertions(+), 1 deletion(-) > > > > Hi, Mohammad. Thanks for the patch! > See some comments inline. > > > diff --git a/northd/northd.c b/northd/northd.c > > index 5b50ea191..39c19b07f 100644 > > --- a/northd/northd.c > > +++ b/northd/northd.c > > @@ -4524,7 +4524,7 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > bool ls_had_only_router_ports = (od->n_router_ports > > && (od->n_router_ports == hmap_count(&od->ports))); > > > > - struct ovn_port *op; > > + struct ovn_port *op, *op_v; > > HMAP_FOR_EACH (op, dp_node, &od->ports) { > > op->visited = false; > > } > > @@ -4547,6 +4547,23 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > goto fail; > > } > > add_op_to_northd_tracked_ports(&trk_lsps->created, op); > > + > > + /* > > + * Update old virtual ports that had this VIF as parent port > > + * this code handles cases where the virtual port was > created > > + * before the parent port or when the parent port was > recreated. > > + */ > > + if (!strcmp(new_nbsp->type, "")) { > > + for (size_t i = 0; i < changed_ls->n_ports; i++) { > > This seems inefficient and may potentially tank performance at scale. > There should be a way to find all these ports without introducing > quadratic complexity. > yes, I agree that code is inefficient. > > On option might be to iterate once, if trk_lsps->created is not empty > after the loop, find all the parent ports and check if they are in > trk_lsps->created. > if you please can take a look at v2 <https://patchwork.ozlabs.org/project/ovn/patch/20240725072049.3884358-1-mh...@redhat.com/>, i adopted your idea with some changes. First, i will save all existing virtual ports during the first loop, these ports are already exist so no need to check if they are in both updated and created lists. afterward, at the end of the function, i will go through all the found virtual ports and for each vport i will check if one of the newly created port is a parent, and if so i will add it to the update list. worst case scenario complexity will be: Number of existing virtual ports * the number of newly created ports. which i think is the best i can do :(. > > Not sure if that's the most efficient way, but seems better than > quadratic. What do you think? > > > + op_v = ovn_port_find_in_datapath(od, > changed_ls->ports[i]); > > + if (op_v && !strcmp(op_v->nbsp->type, "virtual") && > > + strstr(smap_get_def(&op_v->nbsp->options, > > + "virtual-parents", ""), new_nbsp->name)) > { > > + > add_op_to_northd_tracked_ports(&trk_lsps->updated, > > + op_v); > > Is there a chance that these ports end up in both updated and created? > Can this cause issues later? > > Best regards, Ilya Maximets. > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
