| Message ID | 20241001151719.1627801-2-xsimonar@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Postpone ports performance fixes. | 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 | fail | github build: failed |
On Tue, Oct 1, 2024 at 5:17 PM Xavier Simonart <xsimonar@redhat.com> 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 <xsimonar@redhat.com> > > --- > 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 > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales
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) { + 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);
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 <xsimonar@redhat.com> --- v2: rebase on origin/main for scapy fix. --- controller/binding.c | 45 +++++++++++++++++++++---------------- controller/binding.h | 2 ++ controller/ovn-controller.c | 37 ++++++++++++++++++++++++++++-- 3 files changed, 63 insertions(+), 21 deletions(-)