On Thu, Mar 13, 2025 at 5:28 PM <[email protected]> wrote:
> From: Numan Siddique <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Numan Siddique <[email protected]>
> ---
>
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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
With that addressed:
Acked-by: Ales Musil <[email protected]>
Thanks,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
