Hi Ales, Thanks for your review and comments. See below
Thanks Xavier On Mon, Nov 24, 2025 at 8:53 AM Ales Musil <[email protected]> wrote: > > > On Thu, Nov 20, 2025 at 9:04 AM Xavier Simonart via dev < > [email protected]> wrote: > >> seq_read/seq_wait should be used before processing any changes. >> Before this patch, if pinctrl thread tried to notify main thread >> while ovn-controller already run pinctrl_run but not yet pinctrl_wait, >> the wake up was lost. >> >> Signed-off-by: Xavier Simonart <[email protected]> >> --- >> > > Hi Xavier, > > thank you for the patch. I have one question/comment down below. > > >> controller/pinctrl.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/controller/pinctrl.c b/controller/pinctrl.c >> index 84a1375b2..7fc111b24 100644 >> --- a/controller/pinctrl.c >> +++ b/controller/pinctrl.c >> @@ -4073,6 +4073,13 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn, >> int64_t cur_cfg) >> { >> ovs_mutex_lock(&pinctrl_mutex); >> + >> + /* Do not wake up if no txn available. We will wake up on sb >> update.*/ >> + if (ovnsb_idl_txn) { >> + int64_t new_seq = seq_read(pinctrl_main_seq); >> > > If I understand that correctly we only need to read here before > processing any updates, the wait can still be in the pinctrl_wait. > It would make more sense logically if we would do the seq_wait() > in pinctrl_wait() WDYT? > I think that both are functionally correct, but I agree that having all 'waits' in poll_wait makes more sense, so I'll send v2. > > Also it seems that statctrl has the same problem. > Will fix in v2. > And for whatever > reason we are using int64_t, but seq_read is returning uint64_t. > So we should probably fix that too while at it. > > + seq_wait(pinctrl_main_seq, new_seq); >> + } >> + >> > Will do in v2. > run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key, >> sbrec_port_binding_by_key, >> sbrec_mac_binding_by_lport_ip); >> @@ -4593,8 +4600,6 @@ pinctrl_wait(struct ovsdb_idl_txn *ovnsb_idl_txn) >> wait_put_mac_bindings(ovnsb_idl_txn); >> wait_controller_event(ovnsb_idl_txn); >> wait_put_vport_bindings(ovnsb_idl_txn); >> - int64_t new_seq = seq_read(pinctrl_main_seq); >> - seq_wait(pinctrl_main_seq, new_seq); >> wait_put_fdbs(ovnsb_idl_txn); >> wait_activated_ports(); >> ovs_mutex_unlock(&pinctrl_mutex); >> -- >> 2.47.1 >> >> _______________________________________________ >> dev mailing list >> [email protected] >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> >> > Regards, > Ales > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
