On Tue, Mar 18, 2025 at 4:05 AM Ales Musil <amu...@redhat.com> wrote:
>
>
>
> On Thu, Mar 13, 2025 at 5:28 PM <num...@ovn.org> wrote:
>>
>> From: Numan Siddique <num...@ovn.org>
>>
>> Make use of this function instead of calling lport_lookup_by_name()
>> for the chassis-redirect-port name in various places.
>>
>> Suggested-by: Dumitru Ceara <dce...@redhat.com>
>> Signed-off-by: Numan Siddique <num...@ovn.org>
>> ---
>
>
> HI Numan,
>
> thank you for the patch, I have one comment down below.
>
>> controller/local_data.c | 10 ++++------
>> controller/lport.c | 36 +++++++++++++++++++++++++++++-------
>> controller/lport.h | 9 +++++++++
>> controller/pinctrl.c | 16 ++++++----------
>> controller/route.c | 18 ++++++++++--------
>> 5 files changed, 58 insertions(+), 31 deletions(-)
>>
>> diff --git a/controller/local_data.c b/controller/local_data.c
>> index 4aee39d6b2..d464f69732 100644
>> --- a/controller/local_data.c
>> +++ b/controller/local_data.c
>> @@ -149,18 +149,16 @@ need_add_peer_to_local(
>> }
>>
>> /* If it is a regular patch port, it is fully distributed, add the
>> peer. */
>> - const char *crp = smap_get(&pb->options, "chassis-redirect-port");
>> - if (!crp) {
>> + const char *crp_name = smap_get(&pb->options, "chassis-redirect-port");
>> + if (!crp_name) {
>> return true;
>> }
>>
>> /* A DGP, find its chassis-redirect-port pb. */
>> const struct sbrec_port_binding *pb_crp =
>> - lport_lookup_by_name(sbrec_port_binding_by_name, crp);
>> + lport_get_cr_port(sbrec_port_binding_by_name, pb, crp_name, true);
>> +
>> if (!pb_crp) {
>> - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>> - VLOG_WARN_RL(&rl, "Chassis-redirect-port %s for DGP %s is not
>> found.",
>> - pb->logical_port, crp);
>
>
> I think we should leave the warn here, it's the only place that uses this.
>
>> return false;
>> }
>>
>> diff --git a/controller/lport.c b/controller/lport.c
>> index 178fbb6a95..9b26d2825f 100644
>> --- a/controller/lport.c
>> +++ b/controller/lport.c
>> @@ -63,7 +63,7 @@ lport_lookup_by_key(struct ovsdb_idl_index
>> *sbrec_datapath_binding_by_key,
>> port_key);
>> }
>>
>> -static bool
>> +bool
>> lport_pb_is_chassis_resident(const struct sbrec_chassis *chassis,
>> const struct sset *active_tunnels,
>> const struct sbrec_port_binding *pb)
>> @@ -107,13 +107,10 @@ lport_is_local(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> return true;
>> }
>>
>> - const char *crp = smap_get(&pb->options, "chassis-redirect-port");
>> - if (!crp) {
>> - return false;
>> - }
>> + const struct sbrec_port_binding *cr_pb =
>> + lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL, false);
>>
>> - return lport_is_chassis_resident(sbrec_port_binding_by_name, chassis,
>> - active_tunnels, crp);
>> + return lport_pb_is_chassis_resident(chassis, active_tunnels, cr_pb);
>> }
>>
>> const struct sbrec_port_binding *
>> @@ -136,6 +133,31 @@ lport_get_l3gw_peer(const struct sbrec_port_binding *pb,
>> return get_peer_lport(pb, sbrec_port_binding_by_name);
>> }
>>
>> +const struct sbrec_port_binding *
>> +lport_get_cr_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>> + const struct sbrec_port_binding *pb,
>> + const char *crp_name, bool warn)
>> +{
>> + ovs_assert(pb);
>> +
>> + if (!crp_name) {
>> + crp_name = smap_get(&pb->options, "chassis-redirect-port");
>> + if (!crp_name) {
>> + return NULL;
>> + }
>> + }
>> +
>> + const struct sbrec_port_binding *cr_pb =
>> + lport_lookup_by_name(sbrec_port_binding_by_name, crp_name);
>> + if (warn && !cr_pb) {
>> + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
>> + VLOG_WARN_RL(&rl, "chassis-redirect-port %s for DGP %s is not
>> found.",
>> + crp_name, pb->logical_port);
>> + }
>> +
>> + return cr_pb;
>> +}
>> +
>> enum can_bind
>> lport_can_bind_on_this_chassis(const struct sbrec_chassis *chassis_rec,
>> const struct sbrec_port_binding *pb)
>> diff --git a/controller/lport.h b/controller/lport.h
>> index c410454e4c..387eb21dd0 100644
>> --- a/controller/lport.h
>> +++ b/controller/lport.h
>> @@ -64,6 +64,11 @@ lport_is_chassis_resident(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> const struct sbrec_chassis *chassis,
>> const struct sset *active_tunnels,
>> const char *port_name);
>> +bool
>> +lport_pb_is_chassis_resident(const struct sbrec_chassis *chassis,
>> + const struct sset *active_tunnels,
>> + const struct sbrec_port_binding *pb);
>> +
>> bool lport_is_local(struct ovsdb_idl_index *sbrec_port_binding_by_name,
>> const struct sbrec_chassis *chassis,
>> const struct sset *active_tunnels,
>> @@ -74,6 +79,10 @@ const struct sbrec_port_binding *lport_get_peer(
>> const struct sbrec_port_binding *lport_get_l3gw_peer(
>> const struct sbrec_port_binding *,
>> struct ovsdb_idl_index *sbrec_port_binding_by_name);
>> +const struct sbrec_port_binding *lport_get_cr_port(
>> + struct ovsdb_idl_index *sbrec_port_binding_by_name,
>> + const struct sbrec_port_binding *,
>> + const char *crp_name, bool warn);
>> bool
>> lport_is_activated_by_activation_strategy(const struct sbrec_port_binding
>> *pb,
>> const struct sbrec_chassis
>> *chassis);
>> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
>> index 429eceaee6..c73cb46e00 100644
>> --- a/controller/pinctrl.c
>> +++ b/controller/pinctrl.c
>> @@ -4926,21 +4926,17 @@ run_buffered_binding(const struct
>> sbrec_mac_binding_table *mac_binding_table,
>>
>> mac_binding_add(&recent_mbs, mb_data, smb, 0);
>>
>> - const char *redirect_port =
>> - smap_get(&pb->options, "chassis-redirect-port");
>> - if (!redirect_port) {
>> - continue;
>> - }
>> -
>> - pb = lport_lookup_by_name(sbrec_port_binding_by_name,
>> redirect_port);
>> - if (!pb || pb->datapath->tunnel_key != smb->datapath->tunnel_key ||
>> - strcmp(pb->type, "chassisredirect")) {
>> + const struct sbrec_port_binding *cr_pb =
>> + lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL, false);
>> + if (!cr_pb ||
>> + cr_pb->datapath->tunnel_key != smb->datapath->tunnel_key ||
>> + strcmp(cr_pb->type, "chassisredirect")) {
>> continue;
>> }
>>
>> /* Add the same entry also for chassisredirect port as the buffered
>> * traffic might be buffered on the cr port. */
>> - mb_data.port_key = pb->tunnel_key;
>> + mb_data.port_key = cr_pb->tunnel_key;
>> mac_binding_add(&recent_mbs, mb_data, smb, 0);
>> }
>>
>> diff --git a/controller/route.c b/controller/route.c
>> index 57af1ed91f..7318136ec6 100644
>> --- a/controller/route.c
>> +++ b/controller/route.c
>> @@ -61,18 +61,20 @@ route_exchange_find_port(struct ovsdb_idl_index
>> *sbrec_port_binding_by_name,
>> if (route_exchange_relevant_port(pb)) {
>> return pb;
>> }
>> - const char *crp = smap_get(&pb->options, "chassis-redirect-port");
>> - if (!crp) {
>> +
>> + const struct sbrec_port_binding *cr_pb =
>> + lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL, false);
>> +
>> + if (!cr_pb) {
>> return NULL;
>> }
>> - if (!lport_is_chassis_resident(sbrec_port_binding_by_name, chassis,
>> - active_tunnels, crp)) {
>> +
>> + if (!lport_pb_is_chassis_resident(chassis, active_tunnels, cr_pb)) {
>> return NULL;
>> }
>> - const struct sbrec_port_binding *crpbp = lport_lookup_by_name(
>> - sbrec_port_binding_by_name, crp);
>> - if (route_exchange_relevant_port(crpbp)) {
>> - return crpbp;
>> +
>> + if (route_exchange_relevant_port(cr_pb)) {
>> + return cr_pb;
>> }
>> return NULL;
>> }
>> --
>> 2.48.1
>>
>> _______________________________________________
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>
> With that addressed:
> Acked-by: Ales Musil <amu...@redhat.com>
Thanks Ales for the reviews.
I addressed your comment and applied this patch series to main with
the below changes.
And also backported patch 2 to branch-25.03.
---------------------------------------------------------------------------------------------
diff --git a/controller/local_data.c b/controller/local_data.c
index d464f69732..5383f49cf8 100644
--- a/controller/local_data.c
+++ b/controller/local_data.c
@@ -156,9 +156,12 @@ need_add_peer_to_local(
/* A DGP, find its chassis-redirect-port pb. */
const struct sbrec_port_binding *pb_crp =
- lport_get_cr_port(sbrec_port_binding_by_name, pb, crp_name, true);
+ lport_get_cr_port(sbrec_port_binding_by_name, pb, crp_name);
if (!pb_crp) {
+ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
+ VLOG_WARN_RL(&rl, "chassis-redirect-port %s for DGP %s is not found.",
+ crp_name, pb->logical_port);
return false;
}
diff --git a/controller/lport.c b/controller/lport.c
index 9b26d2825f..d8ba7ca164 100644
--- a/controller/lport.c
+++ b/controller/lport.c
@@ -108,7 +108,7 @@ lport_is_local(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
}
const struct sbrec_port_binding *cr_pb =
- lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL, false);
+ lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL);
return lport_pb_is_chassis_resident(chassis, active_tunnels, cr_pb);
}
@@ -135,8 +135,7 @@ lport_get_l3gw_peer(const struct sbrec_port_binding *pb,
const struct sbrec_port_binding *
lport_get_cr_port(struct ovsdb_idl_index *sbrec_port_binding_by_name,
- const struct sbrec_port_binding *pb,
- const char *crp_name, bool warn)
+ const struct sbrec_port_binding *pb, const char *crp_name)
{
ovs_assert(pb);
@@ -147,15 +146,7 @@ lport_get_cr_port(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
}
}
- const struct sbrec_port_binding *cr_pb =
- lport_lookup_by_name(sbrec_port_binding_by_name, crp_name);
- if (warn && !cr_pb) {
- static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
- VLOG_WARN_RL(&rl, "chassis-redirect-port %s for DGP %s is not found.",
- crp_name, pb->logical_port);
- }
-
- return cr_pb;
+ return lport_lookup_by_name(sbrec_port_binding_by_name, crp_name);
}
enum can_bind
diff --git a/controller/lport.h b/controller/lport.h
index 387eb21dd0..8463c96232 100644
--- a/controller/lport.h
+++ b/controller/lport.h
@@ -64,10 +64,9 @@ lport_is_chassis_resident(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
const struct sset *active_tunnels,
const char *port_name);
-bool
-lport_pb_is_chassis_resident(const struct sbrec_chassis *chassis,
- const struct sset *active_tunnels,
- const struct sbrec_port_binding *pb);
+bool lport_pb_is_chassis_resident(const struct sbrec_chassis *chassis,
+ const struct sset *active_tunnels,
+ const struct sbrec_port_binding *pb);
bool lport_is_local(struct ovsdb_idl_index *sbrec_port_binding_by_name,
const struct sbrec_chassis *chassis,
@@ -81,8 +80,7 @@ const struct sbrec_port_binding *lport_get_l3gw_peer(
struct ovsdb_idl_index *sbrec_port_binding_by_name);
const struct sbrec_port_binding *lport_get_cr_port(
struct ovsdb_idl_index *sbrec_port_binding_by_name,
- const struct sbrec_port_binding *,
- const char *crp_name, bool warn);
+ const struct sbrec_port_binding *, const char *crp_name);
bool
lport_is_activated_by_activation_strategy(const struct sbrec_port_binding *pb,
const struct sbrec_chassis *chassis);
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index c73cb46e00..1481b2547a 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -4927,7 +4927,7 @@ run_buffered_binding(const struct
sbrec_mac_binding_table *mac_binding_table,
mac_binding_add(&recent_mbs, mb_data, smb, 0);
const struct sbrec_port_binding *cr_pb =
- lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL, false);
+ lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL);
if (!cr_pb ||
cr_pb->datapath->tunnel_key != smb->datapath->tunnel_key ||
strcmp(cr_pb->type, "chassisredirect")) {
diff --git a/controller/route.c b/controller/route.c
index 7318136ec6..a5862b9b9c 100644
--- a/controller/route.c
+++ b/controller/route.c
@@ -63,7 +63,7 @@ route_exchange_find_port(struct ovsdb_idl_index
*sbrec_port_binding_by_name,
}
const struct sbrec_port_binding *cr_pb =
- lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL, false);
+ lport_get_cr_port(sbrec_port_binding_by_name, pb, NULL);
if (!cr_pb) {
return NULL;
---------------------------------------------------------------------------------------------
Thanks
Numan
>
> Thanks,
> Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
