On Tue, Mar 11, 2025 at 12:54 PM Dumitru Ceara <[email protected]> wrote:
>
> On 3/11/25 4:28 PM, Numan Siddique wrote:
> > On Tue, Mar 11, 2025 at 11:27 AM Numan Siddique <[email protected]> wrote:
> >>
> >> On Mon, Mar 10, 2025 at 12:08 PM Dumitru Ceara <[email protected]> wrote:
> >>>
> >>> On 2/28/25 10:40 PM, [email protected] wrote:
> >>>> From: Numan Siddique <[email protected]>
> >>>>
> >>>> A sset is maintained in the 'struct local_datapath' of the
> >>>> logical ports claimed for it. A later patch will make use
> >>>> of this to check if a local_datapath is still relevant to
> >>>> the chassis or not.
> >>>>
> >>>> Signed-off-by: Numan Siddique <[email protected]>
> >>>> ---
> >>>
> >>> Hi Numan,
> >>>
> >>> Sorry for the delay in reviewing this series.
> > No worries. Thanks for the review comments.
> >
> > This patch looks OK, just
> >>> one question below.
> >>>
> >>>> controller/binding.c | 84 ++++++++++++++++++++++++++---------------
> >>>> controller/local_data.c | 14 ++++---
> >>>> controller/local_data.h | 4 +-
> >>>> 3 files changed, 64 insertions(+), 38 deletions(-)
> >>>>
> >>>> diff --git a/controller/binding.c b/controller/binding.c
> >>>> index cbd7749494..c76a0c06c5 100644
> >>>> --- a/controller/binding.c
> >>>> +++ b/controller/binding.c
> >>>> @@ -1351,6 +1351,7 @@ is_postponed_port(const char *port_name)
> >>>> */
> >>>> static bool
> >>>> claim_lport(const struct sbrec_port_binding *pb,
> >>>> + struct local_datapath *ld,
> >>>> const struct sbrec_port_binding *parent_pb,
> >>>> const struct sbrec_chassis *chassis_rec,
> >>>> const struct ovsrec_interface *iface_rec,
> >>>> @@ -1362,6 +1363,10 @@ claim_lport(const struct sbrec_port_binding *pb,
> >>>> {
> >>>> bool update_tracked = false;
> >>>>
> >>>> + if (ld) {
> >>>> + sset_add(&ld->claimed_lports, pb->logical_port);
> >>>> + }
> >>>> +
> >>>> if (can_bind == CAN_BIND_AS_MAIN) {
> >>>> if (pb->chassis != chassis_rec) {
> >>>> long long int now = time_msec();
> >>>> @@ -1489,6 +1494,7 @@ release_lport_additional_chassis(const struct
> >>>> sbrec_port_binding *pb,
> >>>>
> >>>> static bool
> >>>> release_lport(const struct sbrec_port_binding *pb,
> >>>> + struct local_datapath *ld,
> >>>> const struct sbrec_chassis *chassis_rec, bool sb_readonly,
> >>>> struct hmap *tracked_datapaths, struct if_status_mgr
> >>>> *if_mgr)
> >>>> {
> >>>> @@ -1505,6 +1511,10 @@ release_lport(const struct sbrec_port_binding *pb,
> >>>> }
> >>>> update_lport_tracking(pb, tracked_datapaths, false);
> >>>> if_status_mgr_release_iface(if_mgr, pb->logical_port);
> >>>> +
> >>>> + if (ld) {
> >>>> + sset_find_and_delete(&ld->claimed_lports, pb->logical_port);
> >>>> + }
> >>>> return true;
> >>>> }
> >>>>
> >>>> @@ -1548,7 +1558,11 @@ release_binding_lport(const struct sbrec_chassis
> >>>> *chassis_rec,
> >>>> remove_related_lport(b_lport->pb, b_ctx_out);
> >>>> }
> >>>> if (is_binding_lport_this_chassis(b_lport, chassis_rec)) {
> >>>> - if (!release_lport(b_lport->pb, chassis_rec, sb_readonly,
> >>>> + struct local_datapath *ld =
> >>>> + get_local_datapath(b_ctx_out->local_datapaths,
> >>>> + b_lport->pb->datapath->tunnel_key);
> >>>> +
> >>>> + if (!release_lport(b_lport->pb, ld, chassis_rec, sb_readonly,
> >>>> b_ctx_out->tracked_dp_bindings,
> >>>> b_ctx_out->if_mgr)) {
> >>>> return false;
> >>>> @@ -1574,7 +1588,15 @@ consider_vif_lport_(const struct
> >>>> sbrec_port_binding *pb,
> >>>> const struct sbrec_port_binding *parent_pb =
> >>>> binding_lport_get_parent_pb(b_lport);
> >>>>
> >>>> - if (!claim_lport(pb, parent_pb, b_ctx_in->chassis_rec,
> >>>> + struct local_datapath *ld =
> >>>> +
> >>>> add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> >>>> +
> >>>> b_ctx_in->sbrec_port_binding_by_datapath,
> >>>> + b_ctx_in->sbrec_port_binding_by_name,
> >>>> + pb->datapath, b_ctx_in->chassis_rec,
> >>>> + b_ctx_out->local_datapaths,
> >>>> + b_ctx_out->tracked_dp_bindings);
> >>>> +
> >>>> + if (!claim_lport(pb, ld, parent_pb, b_ctx_in->chassis_rec,
> >>>> b_lport->lbinding->iface,
> >>>> !b_ctx_in->ovnsb_idl_txn,
> >>>> !parent_pb, b_ctx_out->tracked_dp_bindings,
> >>>> @@ -1583,12 +1605,6 @@ consider_vif_lport_(const struct
> >>>> sbrec_port_binding *pb,
> >>>> return false;
> >>>
> >>> If we failed to claim the port, do we need to remove the local datapath
> >>> here? Before your patch we were only adding to local datapaths if we
> >>> successfully claimed the port.
> >>
> >> I don't think it is necessary for the following reasons:
> >>
> >> 1. claim_lport() returns FALSE only if ovnsb_idl_txn is NULL.
> >> 2. binding_run() (a full recompute) will not be called if ovnsb_idl_txn
> >> is NULL
> >> 3. If ovnsb_idl_txn is NULL during I-P handling,
> >> consider_vif_lport_() will return FALSE
> >> and the handler will return FALSE and we will fallback to
> >> recompute next time when ovnsb_idl_txn is NULL.
> > when ovnsb_idl_txn is *NOT* NULL.
> >
> >
>
> It still feels a bit weird that we rely on knowledge about the internal
> implementation of claim_lport(), to be honest. Should we change
> claim_lport() so that it doesn't return bool anymore and just not call
> it when sb_readonly == false?
Since claim_lport() is a static function within binding.c, I think
it's ok to know its internals.
But I think to be on safer side, I'll change the code back to the
original one i..e add the datapath of the
claimed lport to the local datapaths only after claim_lport() return true
and then do - "sset_add(&ld->claimed_lports, pb->logical_port);"
Thanks
Numan
>
> >>
> >>
> >> I can add some comments about this in the next version. wdyt ?
>
> If we decide to keep claim_lport() as is, adding comments would help.
>
> >>
> >> Thanks
> >> Numan
> >>
>
> Thanks,
> Dumitru
>
> >>>
> >>>> }
> >>>>
> >>>> - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> >>>> - b_ctx_in->sbrec_port_binding_by_datapath,
> >>>> - b_ctx_in->sbrec_port_binding_by_name,
> >>>> - pb->datapath, b_ctx_in->chassis_rec,
> >>>> - b_ctx_out->local_datapaths,
> >>>> - b_ctx_out->tracked_dp_bindings);
> >>>> update_related_lport(pb, b_ctx_out);
> >>>> update_local_lports(pb->logical_port, b_ctx_out);
> >>>> if (binding_lport_update_port_sec(b_lport, pb) &&
> >>>> @@ -1625,7 +1641,10 @@ consider_vif_lport_(const struct
> >>>> sbrec_port_binding *pb,
> >>>>
> >>>> if (!lbinding_set || !can_bind) {
> >>>> remove_related_lport(pb, b_ctx_out);
> >>>> - return release_lport(pb, b_ctx_in->chassis_rec,
> >>>> + struct local_datapath *ld =
> >>>> + get_local_datapath(b_ctx_out->local_datapaths,
> >>>> + pb->datapath->tunnel_key);
> >>>> + return release_lport(pb, ld, b_ctx_in->chassis_rec,
> >>>> !b_ctx_in->ovnsb_idl_txn,
> >>>> b_ctx_out->tracked_dp_bindings,
> >>>> b_ctx_out->if_mgr);
> >>>> @@ -1761,7 +1780,10 @@ consider_container_lport(const struct
> >>>> sbrec_port_binding *pb,
> >>>> * if it was bound earlier. */
> >>>> if (is_binding_lport_this_chassis(container_b_lport,
> >>>> b_ctx_in->chassis_rec)) {
> >>>> - return release_lport(pb, b_ctx_in->chassis_rec,
> >>>> + struct local_datapath *ld =
> >>>> + get_local_datapath(b_ctx_out->local_datapaths,
> >>>> + pb->datapath->tunnel_key);
> >>>> + return release_lport(pb, ld, b_ctx_in->chassis_rec,
> >>>> !b_ctx_in->ovnsb_idl_txn,
> >>>> b_ctx_out->tracked_dp_bindings,
> >>>> b_ctx_out->if_mgr);
> >>>> @@ -1900,12 +1922,12 @@ consider_nonvif_lport_(const struct
> >>>> sbrec_port_binding *pb,
> >>>> if (our_chassis) {
> >>>> update_local_lports(pb->logical_port, b_ctx_out);
> >>>> if (!ld) {
> >>>> - add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> >>>> - b_ctx_in->sbrec_port_binding_by_datapath,
> >>>> - b_ctx_in->sbrec_port_binding_by_name,
> >>>> - pb->datapath, b_ctx_in->chassis_rec,
> >>>> - b_ctx_out->local_datapaths,
> >>>> - b_ctx_out->tracked_dp_bindings);
> >>>> + ld =
> >>>> add_local_datapath(b_ctx_in->sbrec_datapath_binding_by_key,
> >>>> +
> >>>> b_ctx_in->sbrec_port_binding_by_datapath,
> >>>> +
> >>>> b_ctx_in->sbrec_port_binding_by_name,
> >>>> + pb->datapath, b_ctx_in->chassis_rec,
> >>>> + b_ctx_out->local_datapaths,
> >>>> + b_ctx_out->tracked_dp_bindings);
> >>>> } else {
> >>>> /* Add the peer datapath to the local datapaths if it's
> >>>> * not present yet.
> >>>> @@ -1926,7 +1948,7 @@ consider_nonvif_lport_(const struct
> >>>> sbrec_port_binding *pb,
> >>>> update_related_lport(pb, b_ctx_out);
> >>>> enum can_bind can_bind = lport_can_bind_on_this_chassis(
> >>>> b_ctx_in->chassis_rec, pb);
> >>>> - return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL,
> >>>> + return claim_lport(pb, ld, NULL, b_ctx_in->chassis_rec, NULL,
> >>>> !b_ctx_in->ovnsb_idl_txn, false,
> >>>> b_ctx_out->tracked_dp_bindings,
> >>>> b_ctx_out->if_mgr,
> >>>> @@ -1940,10 +1962,10 @@ consider_nonvif_lport_(const struct
> >>>> sbrec_port_binding *pb,
> >>>> || is_additional_chassis(pb, b_ctx_in->chassis_rec)
> >>>> || if_status_is_port_claimed(b_ctx_out->if_mgr,
> >>>> pb->logical_port)) {
> >>>> - if (!release_lport(pb, b_ctx_in->chassis_rec,
> >>>> - !b_ctx_in->ovnsb_idl_txn,
> >>>> - b_ctx_out->tracked_dp_bindings,
> >>>> - b_ctx_out->if_mgr)) {
> >>>> + if (!release_lport(pb, ld, b_ctx_in->chassis_rec,
> >>>> + !b_ctx_in->ovnsb_idl_txn,
> >>>> + b_ctx_out->tracked_dp_bindings,
> >>>> + b_ctx_out->if_mgr)) {
> >>>> return false;
> >>>> }
> >>>>
> >>>> @@ -2839,9 +2861,9 @@ handle_deleted_vif_lport(const struct
> >>>> sbrec_port_binding *pb,
> >>>> struct binding_ctx_in *b_ctx_in,
> >>>> struct binding_ctx_out *b_ctx_out)
> >>>> {
> >>>> + struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
> >>>> struct local_binding *lbinding = NULL;
> >>>>
> >>>> - struct shash *binding_lports = &b_ctx_out->lbinding_data->lports;
> >>>> struct binding_lport *b_lport = binding_lport_find(binding_lports,
> >>>> pb->logical_port);
> >>>> if (b_lport) {
> >>>> lbinding = b_lport->lbinding;
> >>>> @@ -2966,13 +2988,13 @@ consider_patch_port_for_local_datapaths(const
> >>>> struct sbrec_port_binding *pb,
> >>>> if (peer_ld && need_add_peer_to_local(
> >>>> b_ctx_in->sbrec_port_binding_by_name, peer,
> >>>> b_ctx_in->chassis_rec)) {
> >>>> - add_local_datapath(
> >>>> - b_ctx_in->sbrec_datapath_binding_by_key,
> >>>> - b_ctx_in->sbrec_port_binding_by_datapath,
> >>>> - b_ctx_in->sbrec_port_binding_by_name,
> >>>> - pb->datapath, b_ctx_in->chassis_rec,
> >>>> - b_ctx_out->local_datapaths,
> >>>> - b_ctx_out->tracked_dp_bindings);
> >>>> + ld = add_local_datapath(
> >>>> + b_ctx_in->sbrec_datapath_binding_by_key,
> >>>> + b_ctx_in->sbrec_port_binding_by_datapath,
> >>>> + b_ctx_in->sbrec_port_binding_by_name,
> >>>> + pb->datapath, b_ctx_in->chassis_rec,
> >>>> + b_ctx_out->local_datapaths,
> >>>> + b_ctx_out->tracked_dp_bindings);
> >>>> }
> >>>> } else {
> >>>> /* Add the peer datapath to the local datapaths if it's
> >>>> @@ -2993,7 +3015,7 @@ consider_patch_port_for_local_datapaths(const
> >>>> struct sbrec_port_binding *pb,
> >>>>
> >>>> /* If this chassis is requested - try to claim. */
> >>>> if (pb->requested_chassis == b_ctx_in->chassis_rec) {
> >>>> - return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL,
> >>>> + return claim_lport(pb, ld, NULL, b_ctx_in->chassis_rec, NULL,
> >>>> !b_ctx_in->ovnsb_idl_txn, false,
> >>>> b_ctx_out->tracked_dp_bindings,
> >>>> b_ctx_out->if_mgr,
> >>>> @@ -3007,7 +3029,7 @@ consider_patch_port_for_local_datapaths(const
> >>>> struct sbrec_port_binding *pb,
> >>>> || if_status_is_port_claimed(b_ctx_out->if_mgr,
> >>>> pb->logical_port)) {
> >>>>
> >>>> remove_local_lports(pb->logical_port, b_ctx_out);
> >>>> - if (!release_lport(pb, b_ctx_in->chassis_rec,
> >>>> + if (!release_lport(pb, ld, b_ctx_in->chassis_rec,
> >>>> !b_ctx_in->ovnsb_idl_txn,
> >>>> b_ctx_out->tracked_dp_bindings,
> >>>> b_ctx_out->if_mgr)) {
> >>>> diff --git a/controller/local_data.c b/controller/local_data.c
> >>>> index 4aee39d6b2..24e871f639 100644
> >>>> --- a/controller/local_data.c
> >>>> +++ b/controller/local_data.c
> >>>> @@ -88,6 +88,7 @@ local_datapath_alloc(const struct
> >>>> sbrec_datapath_binding *dp)
> >>>> ld->is_transit_switch = datapath_is_transit_switch(dp);
> >>>> shash_init(&ld->external_ports);
> >>>> shash_init(&ld->multichassis_ports);
> >>>> + sset_init(&ld->claimed_lports);
> >>>> /* memory accounting - common part. */
> >>>> local_datapath_usage += sizeof *ld;
> >>>>
> >>>> @@ -127,6 +128,7 @@ local_datapath_destroy(struct local_datapath *ld)
> >>>> free(ld->peer_ports);
> >>>> shash_destroy(&ld->external_ports);
> >>>> shash_destroy(&ld->multichassis_ports);
> >>>> + sset_destroy(&ld->claimed_lports);
> >>>> free(ld);
> >>>> }
> >>>>
> >>>> @@ -180,7 +182,7 @@ need_add_peer_to_local(
> >>>> return false;
> >>>> }
> >>>>
> >>>> -void
> >>>> +struct local_datapath *
> >>>> add_local_datapath(struct ovsdb_idl_index
> >>>> *sbrec_datapath_binding_by_key,
> >>>> struct ovsdb_idl_index
> >>>> *sbrec_port_binding_by_datapath,
> >>>> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >>>> @@ -189,11 +191,11 @@ add_local_datapath(struct ovsdb_idl_index
> >>>> *sbrec_datapath_binding_by_key,
> >>>> struct hmap *local_datapaths,
> >>>> struct hmap *tracked_datapaths)
> >>>> {
> >>>> - add_local_datapath__(sbrec_datapath_binding_by_key,
> >>>> - sbrec_port_binding_by_datapath,
> >>>> - sbrec_port_binding_by_name, 0,
> >>>> - dp, chassis, local_datapaths,
> >>>> - tracked_datapaths);
> >>>> + return add_local_datapath__(sbrec_datapath_binding_by_key,
> >>>> + sbrec_port_binding_by_datapath,
> >>>> + sbrec_port_binding_by_name, 0,
> >>>> + dp, chassis, local_datapaths,
> >>>> + tracked_datapaths);
> >>>> }
> >>>>
> >>>> void
> >>>> diff --git a/controller/local_data.h b/controller/local_data.h
> >>>> index 1d60dada86..d2eb33b1eb 100644
> >>>> --- a/controller/local_data.h
> >>>> +++ b/controller/local_data.h
> >>>> @@ -63,6 +63,8 @@ struct local_datapath {
> >>>>
> >>>> struct shash external_ports;
> >>>> struct shash multichassis_ports;
> >>>> +
> >>>> + struct sset claimed_lports;
> >>>> };
> >>>>
> >>>> struct local_datapath *local_datapath_alloc(
> >>>> @@ -78,7 +80,7 @@ need_add_peer_to_local(
> >>>> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >>>> const struct sbrec_port_binding *,
> >>>> const struct sbrec_chassis *);
> >>>> -void add_local_datapath(
> >>>> +struct local_datapath * add_local_datapath(
> >>>> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> >>>> struct ovsdb_idl_index *sbrec_port_binding_by_datapath,
> >>>> struct ovsdb_idl_index *sbrec_port_binding_by_name,
> >>>
> >>> Regards,
> >>> Dumitru
> >>>
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
