On 3/11/25 4:28 PM, Numan Siddique wrote:
> On Tue, Mar 11, 2025 at 11:27 AM Numan Siddique <num...@ovn.org> wrote:
>>
>> On Mon, Mar 10, 2025 at 12:08 PM Dumitru Ceara <dce...@redhat.com> wrote:
>>>
>>> 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.
> 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?
>>
>>
>> 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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
