On Tue, Jan 30, 2024 at 2:59 PM Mohammad Heib <mh...@redhat.com> wrote:
> ovn-controller immediately removes the vport_bindings requests that were > generated by VIFs after handling them locally, this approach is intended > to avoid binding the vport to one VIF only and allocate the vport > between the different VIFs that exist in the vport:virtual-parents. > > Although the behavior mentioned above is correct, in some cases when the > SB Database is busy the transaction that binds this vport to the desired > VIF/chassis can fail and the controller will not re-try to bind the > vport again because we deleted the bind_vport request in the previous > loop/TXN. > > This patch aims to change the above behavior by storing the bind_vport > requests for a bit longer time and this is done by the following: > 1. add relevancy_time for each new bind_vport request and > mark this request as new. > > 2. loop0: ovn-controller will try to handle this bind_vport request > for the first time as usual (no change). > > 3. loop0: ovn-controller will try to delete the already handled > bind_vport > request as usual but first, it will check if this request is marked > as new and > if the relevancy_time is still valid if so the controller will mark > this > request as an old request and keep it, otherwise remove it. > > 4.loop1: ovn-controller will try to commit the same change again for > the old request, if the previous commit in loop0 succeeded the > change will not have any effect on SB, otherwise we will try to > commit the same vport_bind request again. > > 5. loop1: delete the old bind_vport request. > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1954659 > Signed-off-by: Mohammad Heib <mh...@redhat.com> > --- > Hi Mohammad, overall the change makes sense, I have a couple of comments see down below. controller/pinctrl.c | 58 +++++++++++++++++++++++++++++++++++++++----- > 1 file changed, 52 insertions(+), 6 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index bd3bd3d81..152962448 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -6519,10 +6519,52 @@ struct put_vport_binding { > uint32_t vport_key; > > uint32_t vport_parent_key; > + > + /* This vport record Only relevant if "relevancy_time" > + * is earlier than the current_time, "new_record" is true. > + */ > + long long int relevancy_time; > The intention of the variable should be probably clearer e.g. relevant_until_ms. Also reading through the rest of the code it doesn't seem possible that the binding wouldn't be deleted, hence I think there isn't any need for the relevancy time, it should be enough to have a flag that will be flipped. In any case we will try to commit twice. I would leave out the whole relevancy and keep the flag flipping it on first commit WDYT? > + bool new_record; > }; > > /* Contains "struct put_vport_binding"s. */ > static struct hmap put_vport_bindings; > +/* the relevance time in ms of vport record before deleteing. */ > +#define VPORT_RELEVANCE_TIME 1500 > + > +/* > + * Validate if the vport_binding record that was added > + * by the pinctrl thread is still relevant and needs > + * to be updated in the SBDB or not. > + * > + * vport_binding record is only relevant and needs to be updated in SB if: > + * 1. The put_vport_binding:relevancy_time still valid. > + * 2. The put_vport_binding:new_record is true: > + * The new_record will be set to "true" when this vport record is > created > + * by function "pinctrl_handle_bind_vport". > + * > + * After the first attempt to bind this vport to the chassis and > + * virtual_parent by function "run_put_vport_bindings" we will set > the > + * value of vpb:new_record to "false" and keep it in > "put_vport_bindings" > + * > + * After the second attempt of binding the vpb it will be removed by > + * this function. > + * > + * The above guarantees that we will try to bind the vport twice in > + * a certain amount of time. > + * > +*/ > +static bool > +is_vport_binding_relevant(struct put_vport_binding *vpb) > +{ > + long long int cur_time = time_msec(); > + > + if (vpb->new_record && vpb->relevancy_time > cur_time) { > + vpb->new_record = false; > This is a nasty side effect that I wouldn't expect from starting with is_. > + return true; > + } > + return false; > +} > > static void > init_put_vport_bindings(void) > @@ -6531,18 +6573,21 @@ init_put_vport_bindings(void) > } > > static void > -flush_put_vport_bindings(void) > +flush_put_vport_bindings(bool force_flush) > { > struct put_vport_binding *vport_b; > - HMAP_FOR_EACH_POP (vport_b, hmap_node, &put_vport_bindings) { > - free(vport_b); > + HMAP_FOR_EACH_SAFE (vport_b, hmap_node, &put_vport_bindings) { > + if (!is_vport_binding_relevant(vport_b) || force_flush) { > + hmap_remove(&put_vport_bindings, &vport_b->hmap_node); > + free(vport_b); > + } > } > } > > static void > destroy_put_vport_bindings(void) > { > - flush_put_vport_bindings(); > + flush_put_vport_bindings(true); > hmap_destroy(&put_vport_bindings); > } > > @@ -6620,7 +6665,7 @@ run_put_vport_bindings(struct ovsdb_idl_txn > *ovnsb_idl_txn, > sbrec_port_binding_by_key, chassis, vpb); > } > > - flush_put_vport_bindings(); > + flush_put_vport_bindings(false); > } > > /* Called with in the pinctrl_handler thread context. */ > @@ -6658,7 +6703,8 @@ pinctrl_handle_bind_vport( > vpb->dp_key = dp_key; > vpb->vport_key = vport_key; > vpb->vport_parent_key = vport_parent_key; > - > + vpb->new_record = true; > + vpb->relevancy_time = time_msec() + VPORT_RELEVANCE_TIME; > notify_pinctrl_main(); > } > > -- > 2.34.3 > > _______________________________________________ > dev mailing list > d...@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> amu...@redhat.com <https://red.ht/sig> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
