On 1/20/25 5:15 PM, Felix Huettner wrote:
> On Tue, Jan 14, 2025 at 01:37:08PM +0100, Dumitru Ceara wrote:
>> Hi Felix,
>>
>> Thanks for the patch!
>
> Hi Dumitru,
>
> thanks for the review.
> All smaller stuff will be part of v3, for the rest, see below.
>
> I will not finish reworking this patchset today, but i plan to send out
> v3 sometime tomorrow.
>
I see you already posted v3, sorry, I didn't get a chance to reply here
until now.
[...]
>>> +
>>> +
>>> +static void
>>> +publish_host_routes(struct ovsdb_idl_txn *ovnsb_txn,
>>> + struct hmap *route_map,
>>> + const struct lr_stateful_table *lr_stateful_table,
>>> + const struct parsed_route *route,
>>> + struct advertised_route_sync_tracked_data *trk_data)
>>
>> This function is not that obvious on a first read. A comment might be
>> nice to have.
>
> I hope the new comment helps on that.
>>
>>> +{
>>> + struct ovn_port *port;
>>> + struct ovn_datapath *lsp_od = route->out_port->peer->od;
>>
>> Newline for readability.
>>
>>> + uuidset_insert(&trk_data->nb_ls, &lsp_od->nbs->header_.uuid);
>>> + HMAP_FOR_EACH (port, dp_node, &lsp_od->ports) {
>>> + if (port->peer) {
>>> + /* This is a LSP connected to an LRP */
>>> + struct lport_addresses *addresses = &port->peer->lrp_networks;
>>> + publish_lport_addresses(ovnsb_txn, route_map, route->od->sb,
>>> + route->out_port,
>>> + addresses, port->peer);
>>> +
>>> + const struct lr_stateful_record *lr_stateful_rec;
>>> + lr_stateful_rec = lr_stateful_table_find_by_index(
>>> + lr_stateful_table, port->peer->od->index);
>>> + uuidset_insert(&trk_data->nb_lr_stateful,
>>> + &lr_stateful_rec->nbr_uuid);
>>> + struct ovn_port_routable_addresses addrs = get_op_addresses(
>>> + port->peer, lr_stateful_rec, false);
>>
>> THis is quite heavy: we'll do it for each and every route, for each and
>> every port on each switch connected to the route's output router port.
>> Can we precompute "ovn_port_routable_addresses" for all router ports
>> instead?
>
> I think it looks worse than it is. This option needs to be set on
> individual LRPs so we probably only have it enabled on a few selected
> LRPs instead of a large portion of them.
>
> This is also only called for connected routes. So if generally a given
> LRP has just a single address assigned to it, we will traverse this only
> once.
>
> However since the result of this function just depends on the
> route->out_port i have added a filter in advertised_route_table_sync to
> only call it at most once per LRP. This should solve the concern.
>
Ok, I'll check this out again in v3.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
