> On 11/7/24 16:06, Lorenzo Bianconi wrote:
> > On Nov 07, Dumitru Ceara wrote:
> >> On 11/7/24 15:17, Lorenzo Bianconi wrote:
> >>>> 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.
> >>>>>
> >>>
> >>> [...]
> >>>>> +const struct sbrec_ecmp_nexthop *
> >>>>> +ecmp_nexthop_lookup(struct ovsdb_idl_index *sbrec_ecmp_by_nexthop,
> >>>>> + const char *nexthop)
> >>>>
> >>>> static?
> >>>
> >>> ack, I will fix it
> >>>
> >>>>
> >>>>> +{
> >>>>> + 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.
> >>>
> >>> ack, I will fix it.
> >>>
> >>>>
> >>>>> 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?
> >>>
> >>> I guess we have some races/issue when a given mac entry is removed from
> >>> the
> >>> mac_binding table. What do you think? If you think it is not a problem, I
> >>> can
> >>> remove it.
> >>>
> >>
> >> Do you have an example of such a race? I'm not sure I understand the
> >> problem.
> >
> > Maybe I missing something, but what does it happen if the given mac address
> > is resolved by ovn-controller running mac_binding_add_to_sb() before the
> > related SBREC_ECMP_NEXTHOP entry is created in build_ecmp_nexthop_table()?
> >
>
> Good point, ovn-controller should do the lookup then when processing the
> SB.ECMP_nexthop addition/updae to the SB. ovn-controller already
> maintains the mac_binding by port and IP already.
ack, I will fix it.
Regards,
Lorenzo
>
> > Regards,
> > Lorenzo
> >
> >>
> >> Thanks,
> >> Dumitru
> >>
> >>>>
> >>>>> +
> >>>>> + 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.
> >>>
> >>> ack, I will fix it.
> >>>
> >>> Regards,
> >>> Lorenzo
> >>>
> >>>>
> >>>>> const struct sbrec_ecmp_nexthop_table *);
> >>>>>
> >>>>> struct lflow_table;
> >>>>
> >>>> Regards,
> >>>> Dumitru
> >>>>
> >>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
