Hi Lorenzo,
On 9/19/24 19:08, Lorenzo Bianconi wrote:
> Set the mac address column in the ECMP_Nexthop table updating
> sb MAC_Binding table. This is a preliminary patch to introduce the
> capability to flush stale ECMP CT entries.
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
> ---
> controller/ovn-controller.c | 4 +++
> controller/pinctrl.c | 50 +++++++++++++++++++++++++++++--------
> controller/pinctrl.h | 1 +
> northd/en-northd.c | 4 +++
> northd/inc-proc-northd.c | 6 +++++
> northd/northd.c | 17 +++++++++++++
> northd/northd.h | 1 +
> 7 files changed, 72 insertions(+), 11 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index e72d28731..4dc9bc8cb 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -4939,6 +4939,9 @@ main(int argc, char *argv[])
> = ovsdb_idl_index_create2(ovnsb_idl_loop.idl,
> &sbrec_fdb_col_mac,
> &sbrec_fdb_col_dp_key);
> + struct ovsdb_idl_index *sbrec_ecmp_by_nexthop
> + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl,
> + &sbrec_ecmp_nexthop_col_nexthop);
> struct ovsdb_idl_index *sbrec_mac_binding_by_datapath
> = mac_binding_by_datapath_index_create(ovnsb_idl_loop.idl);
> struct ovsdb_idl_index *sbrec_static_mac_binding_by_datapath
> @@ -5645,6 +5648,7 @@ main(int argc, char *argv[])
> sbrec_igmp_group,
> sbrec_ip_multicast,
> sbrec_fdb_by_dp_key_mac,
> + sbrec_ecmp_by_nexthop,
> sbrec_dns_table_get(ovnsb_idl_loop.idl),
> sbrec_controller_event_table_get(
> ovnsb_idl_loop.idl),
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index daf0bfa41..092613c4a 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -206,11 +206,15 @@ static void pinctrl_handle_put_mac_binding(const struct
> flow *md,
> OVS_REQUIRES(pinctrl_mutex);
> static void init_put_mac_bindings(void);
> static void destroy_put_mac_bindings(void);
> +const struct sbrec_ecmp_nexthop *ecmp_nexthop_lookup(
> + struct ovsdb_idl_index *sbrec_ecmp_by_nexthop,
> + const char *nexthop);
> static void run_put_mac_bindings(
> struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> struct ovsdb_idl_index *sbrec_port_binding_by_key,
> - struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
> + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> + struct ovsdb_idl_index *sbrec_ecmp_by_nexthop)
> OVS_REQUIRES(pinctrl_mutex);
> static void wait_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn);
> static void send_mac_binding_buffered_pkts(struct rconn *swconn)
> @@ -4160,6 +4164,7 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_igmp_groups,
> struct ovsdb_idl_index *sbrec_ip_multicast_opts,
> struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac,
> + struct ovsdb_idl_index *sbrec_ecmp_by_nexthop,
> const struct sbrec_dns_table *dns_table,
> const struct sbrec_controller_event_table *ce_table,
> const struct sbrec_service_monitor_table *svc_mon_table,
> @@ -4177,7 +4182,8 @@ pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> ovs_mutex_lock(&pinctrl_mutex);
> run_put_mac_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> sbrec_port_binding_by_key,
> - sbrec_mac_binding_by_lport_ip);
> + sbrec_mac_binding_by_lport_ip,
> + sbrec_ecmp_by_nexthop);
> run_put_vport_bindings(ovnsb_idl_txn, sbrec_datapath_binding_by_key,
> sbrec_port_binding_by_key, chassis);
> send_garp_rarp_prepare(ovnsb_idl_txn, sbrec_port_binding_by_datapath,
> @@ -4831,13 +4837,9 @@ mac_binding_add_to_sb(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> const char *logical_port,
> const struct sbrec_datapath_binding *dp,
> - struct eth_addr ea, const char *ip,
> + char *mac_string, const char *ip,
> bool update_only)
> {
> - /* Convert ethernet argument to string form for database. */
> - char mac_string[ETH_ADDR_STRLEN + 1];
> - snprintf(mac_string, sizeof mac_string, ETH_ADDR_FMT, ETH_ADDR_ARGS(ea));
> -
> const struct sbrec_mac_binding *b =
> mac_binding_lookup(sbrec_mac_binding_by_lport_ip,
> logical_port, ip);
> @@ -4896,18 +4898,37 @@ send_garp_locally(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ds ip_s = DS_EMPTY_INITIALIZER;
>
> ip_format_masked(ip, OVS_BE32_MAX, &ip_s);
> + /* Convert ethernet argument to string form for database. */
> + char mac_string[ETH_ADDR_STRLEN + 1];
> + snprintf(mac_string, sizeof mac_string, ETH_ADDR_FMT,
> + ETH_ADDR_ARGS(ea));
> mac_binding_add_to_sb(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
> remote->logical_port, remote->datapath,
> - ea, ds_cstr(&ip_s), update_only);
> + mac_string, ds_cstr(&ip_s), update_only);
> ds_destroy(&ip_s);
> }
> }
>
> +const struct sbrec_ecmp_nexthop *
> +ecmp_nexthop_lookup(struct ovsdb_idl_index *sbrec_ecmp_by_nexthop,
> + const char *nexthop)
static?
> +{
> + struct sbrec_ecmp_nexthop *ecmp_nh =
> + sbrec_ecmp_nexthop_index_init_row(sbrec_ecmp_by_nexthop);
> + sbrec_ecmp_nexthop_set_nexthop(ecmp_nh, nexthop);
> + const struct sbrec_ecmp_nexthop *retval =
> + sbrec_ecmp_nexthop_index_find(sbrec_ecmp_by_nexthop, ecmp_nh);
> + sbrec_ecmp_nexthop_index_destroy_row(ecmp_nh);
> +
> + return retval;
> +}
> +
> static void
> run_put_mac_binding(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> struct ovsdb_idl_index *sbrec_port_binding_by_key,
> struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> + struct ovsdb_idl_index *sbrec_ecmp_by_nexthop,
> const struct mac_binding *mb)
> {
> /* Convert logical datapath and logical port key into lport. */
> @@ -4930,8 +4951,13 @@ run_put_mac_binding(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> struct ds ip_s = DS_EMPTY_INITIALIZER;
> ipv6_format_mapped(&mb->data.ip, &ip_s);
> mac_binding_add_to_sb(ovnsb_idl_txn, sbrec_mac_binding_by_lport_ip,
> - pb->logical_port, pb->datapath, mb->data.mac,
> + pb->logical_port, pb->datapath, mac_string,
> ds_cstr(&ip_s), false);
> + const struct sbrec_ecmp_nexthop *ecmp_nh =
> + ecmp_nexthop_lookup(sbrec_ecmp_by_nexthop, ds_cstr(&ip_s));
> + if (ecmp_nh) {
> + sbrec_ecmp_nexthop_set_mac(ecmp_nh, mac_string);
> + }
Instead of doing the ecmp nexthop mac update here I'd do it inside
mac_binding_add_to_sb(). It would also cover the case when the next-hop
is an OVN logical router port. And it would avoid duplicating the code
that builds the 'mac_string' string.
> ds_destroy(&ip_s);
> }
>
> @@ -4941,7 +4967,8 @@ static void
> run_put_mac_bindings(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_datapath_binding_by_key,
> struct ovsdb_idl_index *sbrec_port_binding_by_key,
> - struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip)
> + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> + struct ovsdb_idl_index *sbrec_ecmp_by_nexthop)
> OVS_REQUIRES(pinctrl_mutex)
> {
> if (!ovnsb_idl_txn) {
> @@ -4956,7 +4983,8 @@ run_put_mac_bindings(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
> run_put_mac_binding(ovnsb_idl_txn,
> sbrec_datapath_binding_by_key,
> sbrec_port_binding_by_key,
> - sbrec_mac_binding_by_lport_ip, mb);
> + sbrec_mac_binding_by_lport_ip,
> + sbrec_ecmp_by_nexthop, mb);
> mac_binding_remove(&put_mac_bindings, mb);
> }
> }
> diff --git a/controller/pinctrl.h b/controller/pinctrl.h
> index 5c8ea7aea..f1968f31c 100644
> --- a/controller/pinctrl.h
> +++ b/controller/pinctrl.h
> @@ -50,6 +50,7 @@ void pinctrl_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> struct ovsdb_idl_index *sbrec_igmp_groups,
> struct ovsdb_idl_index *sbrec_ip_multicast_opts,
> struct ovsdb_idl_index *sbrec_fdb_by_dp_key_mac,
> + struct ovsdb_idl_index *sbrec_ecmp_by_nexthop,
> const struct sbrec_dns_table *,
> const struct sbrec_controller_event_table *,
> const struct sbrec_service_monitor_table *,
> diff --git a/northd/en-northd.c b/northd/en-northd.c
> index b8227617b..b3e612958 100644
> --- a/northd/en-northd.c
> +++ b/northd/en-northd.c
> @@ -412,9 +412,13 @@ en_ecmp_nexthop_run(struct engine_node *node, void *data
> OVS_UNUSED)
> engine_get_input_data("static_routes", node);
> const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table =
> EN_OVSDB_GET(engine_get_input("SB_ecmp_nexthop", node));
> + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip =
> + engine_ovsdb_node_get_index(engine_get_input("SB_mac_binding", node),
> + "sbrec_mac_binding_by_lport_ip");
>
> build_ecmp_nexthop_table(eng_ctx->ovnsb_idl_txn,
> &static_routes_data->parsed_routes,
> + sbrec_mac_binding_by_lport_ip,
> sbrec_ecmp_nexthop_table);
> engine_set_node_state(node, EN_UPDATED);
> }
> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c
> index cdc080ef0..d29bdca3e 100644
> --- a/northd/inc-proc-northd.c
> +++ b/northd/inc-proc-northd.c
> @@ -266,6 +266,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_add_input(&en_bfd_sync, &en_route_policies, NULL);
> engine_add_input(&en_bfd_sync, &en_northd,
> bfd_sync_northd_change_handler);
>
> + engine_add_input(&en_ecmp_nexthop, &en_sb_mac_binding, NULL);
> engine_add_input(&en_ecmp_nexthop, &en_sb_ecmp_nexthop, NULL);
> engine_add_input(&en_ecmp_nexthop, &en_static_routes, NULL);
>
> @@ -369,6 +370,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> = static_mac_binding_index_create(sb->idl);
> struct ovsdb_idl_index *sbrec_mac_binding_by_datapath
> = mac_binding_by_datapath_index_create(sb->idl);
> + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip
> + = mac_binding_by_lport_ip_index_create(sb->idl);
> struct ovsdb_idl_index *fdb_by_dp_key =
> ovsdb_idl_index_create1(sb->idl, &sbrec_fdb_col_dp_key);
>
> @@ -395,6 +398,9 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb,
> engine_ovsdb_node_add_index(&en_sb_mac_binding,
> "sbrec_mac_binding_by_datapath",
> sbrec_mac_binding_by_datapath);
> + engine_ovsdb_node_add_index(&en_sb_mac_binding,
> + "sbrec_mac_binding_by_lport_ip",
> + sbrec_mac_binding_by_lport_ip);
> engine_ovsdb_node_add_index(&en_sb_fdb,
> "fdb_by_dp_key",
> fdb_by_dp_key);
> diff --git a/northd/northd.c b/northd/northd.c
> index 3e3bb84ef..4e99e2687 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -10669,6 +10669,7 @@ void
> build_ecmp_nexthop_table(
> struct ovsdb_idl_txn *ovnsb_txn,
> struct hmap *routes,
> + struct ovsdb_idl_index *sbrec_mac_binding_by_lport_ip,
> const struct sbrec_ecmp_nexthop_table *sbrec_ecmp_nexthop_table)
> {
> if (!ovnsb_txn) {
> @@ -10699,6 +10700,22 @@ build_ecmp_nexthop_table(
> sb_ecmp_nexthop = sbrec_ecmp_nexthop_insert(ovnsb_txn);
> sbrec_ecmp_nexthop_set_nexthop(sb_ecmp_nexthop, r->nexthop);
> sbrec_ecmp_nexthop_set_port(sb_ecmp_nexthop, pr->out_port->key);
> +
> + struct sbrec_mac_binding *mb_index_row =
> + sbrec_mac_binding_index_init_row(
> + sbrec_mac_binding_by_lport_ip);
> + sbrec_mac_binding_index_set_ip(mb_index_row, r->nexthop);
> + sbrec_mac_binding_index_set_logical_port(mb_index_row,
> + pr->out_port->key);
> +
> + const struct sbrec_mac_binding *mb =
> + sbrec_mac_binding_index_find(sbrec_mac_binding_by_lport_ip,
> + mb_index_row);
> + if (mb) {
> + sbrec_ecmp_nexthop_set_mac(sb_ecmp_nexthop, mb->mac);
> + }
Maintaining this index and looking up is costly (O(log n)) for each mac
binding insertion and for each static route. If we update the mac
address in ovn-controller like I suggested inside
mac_binding_add_to_sb() can't we avoid doing it in ovn-northd?
> +
> + sbrec_mac_binding_index_destroy_row(mb_index_row);
> }
> sset_add(&nb_nexthops_sset, r->nexthop);
> }
> diff --git a/northd/northd.h b/northd/northd.h
> index c4c2a5d7e..83239197f 100644
> --- a/northd/northd.h
> +++ b/northd/northd.h
> @@ -747,6 +747,7 @@ void bfd_sync_init(struct bfd_sync_data *);
> void bfd_sync_destroy(struct bfd_sync_data *);
>
> void build_ecmp_nexthop_table(struct ovsdb_idl_txn *, struct hmap *,
> + struct ovsdb_idl_index *,
Please add a name, the type is not explicit enough.
> const struct sbrec_ecmp_nexthop_table *);
>
> struct lflow_table;
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
