On Tue, Oct 1, 2024 at 5:17 PM Xavier Simonart <[email protected]> wrote:
> In some cases, when a port was going to be postponed, the same port (lsp) > was claimed four times: > - within runtime_data handling pb changes: > - lsp is claimed as handling pb changes, and added to postponed list. > - it's claimed again as handling postponed list. > - within runtime_data handling postponed list, the same two operations: > - lsp is claimed as handling pb changes, and added to postponed list. > - it's claimed again as handling postponed list. > > With this patch, the lsp1 port would only only be claimed twice. > - within runtime_data handling pb changes: > - lsp is claimed as handling pb changes, and added to postponed list. > - within runtime_data handling postponed list, the same two operations: > - it's claimed again as handling postponed list. > > This patch is only about CPU utilization; it should not result in any > flow changes. > > Signed-off-by: Xavier Simonart <[email protected]> > > --- > v2: rebase on origin/main for scapy fix. > --- > Hi Xavier, I have one question down below. > controller/binding.c | 45 +++++++++++++++++++++---------------- > controller/binding.h | 2 ++ > controller/ovn-controller.c | 37 ++++++++++++++++++++++++++++-- > 3 files changed, 63 insertions(+), 21 deletions(-) > > diff --git a/controller/binding.c b/controller/binding.c > index bfdeb99b9..3b9bc8620 100644 > --- a/controller/binding.c > +++ b/controller/binding.c > @@ -3064,6 +3064,32 @@ handle_updated_port(struct binding_ctx_in *b_ctx_in, > return handled; > } > > +bool > +binding_handle_postponed_ports(struct binding_ctx_in *b_ctx_in, > + struct binding_ctx_out *b_ctx_out) > +{ > + /* Also handle any postponed (throttled) ports. */ > + const char *port_name; > + const struct sbrec_port_binding *pb; > + bool handled = true; > + struct sset postponed_ports = SSET_INITIALIZER(&postponed_ports); > + sset_clone(&postponed_ports, b_ctx_out->postponed_ports); > + SSET_FOR_EACH (port_name, &postponed_ports) { > + pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, > + port_name); > + if (!pb) { > Could you elaborate a bit why is the sset_find_and_delete() call not needed here anymore? > + continue; > + } > + handled = handle_updated_port(b_ctx_in, b_ctx_out, pb); > + if (!handled) { > + break; > + } > + } > + sset_destroy(&postponed_ports); > + cleanup_claimed_port_timestamps(); > + return handled; > +} > + > /* Returns true if the port binding changes resulted in local binding > * updates, false otherwise. > */ > @@ -3210,25 +3236,6 @@ delete_done: > } > } > > - /* Also handle any postponed (throttled) ports. */ > - const char *port_name; > - struct sset postponed_ports = SSET_INITIALIZER(&postponed_ports); > - sset_clone(&postponed_ports, b_ctx_out->postponed_ports); > - SSET_FOR_EACH (port_name, &postponed_ports) { > - pb = lport_lookup_by_name(b_ctx_in->sbrec_port_binding_by_name, > - port_name); > - if (!pb) { > - sset_find_and_delete(b_ctx_out->postponed_ports, port_name); > - continue; > - } > - handled = handle_updated_port(b_ctx_in, b_ctx_out, pb); > - if (!handled) { > - break; > - } > - } > - sset_destroy(&postponed_ports); > - cleanup_claimed_port_timestamps(); > - > if (handled) { > /* There may be new local datapaths added by the above handling, > so go > * through each port_binding of newly added local datapaths to > update > diff --git a/controller/binding.h b/controller/binding.h > index f44f95833..904418590 100644 > --- a/controller/binding.h > +++ b/controller/binding.h > @@ -196,6 +196,8 @@ bool binding_cleanup(struct ovsdb_idl_txn > *ovnsb_idl_txn, > > bool binding_handle_ovs_interface_changes(struct binding_ctx_in *, > struct binding_ctx_out *); > +bool binding_handle_postponed_ports(struct binding_ctx_in *, > + struct binding_ctx_out *); > bool binding_handle_port_binding_changes(struct binding_ctx_in *, > struct binding_ctx_out *); > void binding_tracked_dp_destroy(struct hmap *tracked_datapaths); > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 168167b1a..dad815c29 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -1536,6 +1536,38 @@ runtime_data_ovs_interface_shadow_handler(struct > engine_node *node, void *data) > return true; > } > > +static bool > +runtime_data_postponed_ports_handler(struct engine_node *node, void *data) > +{ > + struct ed_type_runtime_data *rt_data = data; > + struct binding_ctx_in b_ctx_in; > + struct binding_ctx_out b_ctx_out; > + init_binding_ctx(node, rt_data, &b_ctx_in, &b_ctx_out); > + if (!b_ctx_in.chassis_rec) { > + return false; > + } > + > + rt_data->tracked = true; > + b_ctx_out.tracked_dp_bindings = &rt_data->tracked_dp_bindings; > + > + if (!binding_handle_postponed_ports(&b_ctx_in, &b_ctx_out)) { > + return false; > + } > + > + rt_data->local_lports_changed = b_ctx_out.local_lports_changed; > + rt_data->localnet_learn_fdb = b_ctx_out.localnet_learn_fdb; > + rt_data->localnet_learn_fdb_changed = > b_ctx_out.localnet_learn_fdb_changed; > + if (b_ctx_out.related_lports_changed || > + b_ctx_out.non_vif_ports_changed || > + b_ctx_out.local_lports_changed || > + b_ctx_out.localnet_learn_fdb_changed || > + !hmap_is_empty(b_ctx_out.tracked_dp_bindings)) { > + engine_set_node_state(node, EN_UPDATED); > + } > + > + return true; > +} > + > static bool > runtime_data_sb_port_binding_handler(struct engine_node *node, void *data) > { > @@ -5174,9 +5206,10 @@ main(int argc, char *argv[]) > runtime_data_sb_datapath_binding_handler); > engine_add_input(&en_runtime_data, &en_sb_port_binding, > runtime_data_sb_port_binding_handler); > - /* Reuse the same handler for any previously postponed ports. */ > + /* Run postponed_ports_handler after port_binding_handler in case > port get > + * deleted */ > engine_add_input(&en_runtime_data, &en_postponed_ports, > - runtime_data_sb_port_binding_handler); > + runtime_data_postponed_ports_handler); > /* Run sb_ro_handler after port_binding_handler in case port get > deleted */ > engine_add_input(&en_runtime_data, &en_sb_ro, > runtime_data_sb_ro_handler); > > -- > 2.31.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
