On 2/28/25 10:40 PM, num...@ovn.org wrote:
> From: Numan Siddique <num...@ovn.org>
>
> 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 <num...@ovn.org>
> ---
Hi Numan,
Sorry for the delay in reviewing this series. 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.
> }
>
> - 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
