[...]
> > +
> > + uint32_t hash = ovn_logical_flow_hash(
> > + ovn_stage_get_table(S_SWITCH_IN_L2_LKUP),
> > + ovn_stage_get_pipeline_name(S_SWITCH_IN_L2_LKUP), 90,
> > + ds_cstr(match), action);
> > +
> > + for (size_t i = 0; i < lb->n_nb_lr; i++) {
> > + struct ovn_datapath *od = lb->nb_lr[i];
>
> Let's establish here that od is a logical router. This will be the basis for
> my comments for the rest of this review.
right
>
> > + if (!od->is_gw_router && !od->n_l3dgw_ports) {
> > + continue;
> > + }
> > +
> > + for (size_t j = 0; j < od->n_router_ports; j++) {
>
> od->router_ports and od->n_router_ports are only used by logical switches.
> Each logical switch port of type "router" is included in this array. In
> other words, od->router_ports is a list of logical switch ports that are
> connected to logical routers. So I think this means that this for loop is a
> no-op.
>
> I think the appropriate change is to use od->nbr->n_ports here...
>
> > + struct ovn_port *op = ovn_port_get_peer(ports,
> > + od->router_ports[j]);
>
> ... and use od->nbr->ports[j] here.
you are right, I fixed the issue before and for some reason I reintroduced it
:(.
we should use instead:
- od->nbr->n_ports
- struct ovn_port *op = ovn_port_find(ports, od->nbr->ports[j]->name);
unfortunatelly doing so we will reintroduce a hash_string() that grubs the
adavantage we have "caching" hash and lflow_ref pointer :(. Below it is the
perf report I collected during testing development:
10.00% -001 ovn-northd ovn-northd [.] hash_bytes
|
--9.99%--hash_bytes
|
|--6.07%--ovn_logical_flow_hash
| |
| --6.02%--ovn_lflow_add_at
| |
|
|--2.93%--build_lrouter_nat_flows_for_lb
| |
build_lrouter_flows_for_lb
| |
build_lswitch_and_lrouter_flows
| | build_lflows
| | ovnnb_db_run
| | ovn_db_run
| | main
| | 0x7f58f1cc01e2
| | 0x495641000deecb3d
| |
| |--1.55%--build_lb_rules
| |
build_lswitch_flows_for_lb
| |
build_lswitch_and_lrouter_flows
| | build_lflows
| | ovnnb_db_run
| | ovn_db_run
| | main
| | 0x7f58f1cc01e2
| | 0x495641000deecb3d
| |
|
--1.31%--build_lrouter_defrag_flows_for_lb
|
build_lswitch_and_lrouter_flows
| build_lflows
| ovnnb_db_run
| ovn_db_run
| main
| 0x7f58f1cc01e2
| 0x495641000deecb3d
|
--1.84%--ovn_port_find__
|
--1.84%--ovn_port_find
|
--1.44%--build_lflows_for_unreachable_vips
build_lrouter_flows_for_lb
build_lswitch_and_lrouter_flows
build_lflows
ovnnb_db_run
ovn_db_run
main
0x7f58f1cc01e2
0x495641000deecb3d
4.31% -001 ovn-northd ovn-northd [.] hmap_next_with_hash__
|
--4.29%--hmap_next_with_hash__
|
--4.29%--hmap_first_with_hash
|
|--2.68%--ovn_port_find__
| |
| --2.67%--ovn_port_find
| |
|
--2.59%--build_lflows_for_unreachable_vips
|
build_lrouter_flows_for_lb
|
build_lswitch_and_lrouter_flows
| build_lflows
| ovnnb_db_run
| ovn_db_run
| main
|
0x7f58f1cc01e2
|
0x495641000deecb3d
|
|--0.84%--ovn_datapath_find
| |
| --0.84%--ovn_datapath_from_sbrec
| build_lflows
| ovnnb_db_run
| ovn_db_run
| main
| 0x7f58f1cc01e2
| 0x495641000deecb3d
|
--0.77%--ovn_lflow_find
3.79% -001 ovn-northd ovn-northd [.] hmapx_add
|
--3.73%--hmapx_add
|
--1.90%--0
3.01% -001 ovn-northd libc-2.32.so [.] 0x000000000015d42e
|
---0x7f58f1df5432
|
|--1.28%--ovn_port_find
| |
| --1.26%--build_lflows_for_unreachable_vips
| build_lrouter_flows_for_lb
| build_lswitch_and_lrouter_flows
| build_lflows
| ovnnb_db_run
| ovn_db_run
| main
| 0x7f58f1cc01e2
| 0x495641000deecb3d
|
--0.73%--build_lflows_for_unreachable_vips
build_lrouter_flows_for_lb
build_lswitch_and_lrouter_flows
build_lflows
ovnnb_db_run
ovn_db_run
main
0x7f58f1cc01e2
0x495641000deecb3d
Am I missing something?
Anyway I spotted a couple of other routine we can optimize :)
Regards,
Lorenzo
>
> Also, ovn_port_get_peer() only works for logical switch ports (it should
> probably be renamed), so this will return NULL every time if a logical
> router port is passed in.
>
> > + if (!op) {
> > + continue;
> > + }
> > +
> > + if (!od->is_gw_router && !is_l3dgw_port(op)) {
>
> You already checked od->is_gw_router outside this loop, so you can remove
> the redundant check here.
>
> Also, this code is hurting my brain a bit. In the outer loop, "od" is a
> logical router. We then iterate over od's ports and set "op" to the *peer*
> of the current port. So "op" should presumably be a logical switch port. So
> testing is_l3dgw_port(op) should always return false, right?
>
> > + continue;
> > + }
> > +
> > + struct ovn_port *peer = op->peer;
> > + if (!peer || !peer->nbsp || lsp_is_external(peer->nbsp)) {
> > + continue;
> > + }
>
> Now we set peer to op->peer. Since we already know that op is the peer of
> the logical router port, then op->peer should just be the logical router
> port again. Checks for peer->nbsp and lsp_is_external(peer->nbsp) will
> always be false.
>
> > +
> > + if ((ipv4 && lrouter_port_ipv4_reachable(op, ipv4_addr)) ||
> > + (!ipv4 && lrouter_port_ipv6_reachable(op, &lb_vip->vip))) {
> > + continue;
> > + }
> > + ovn_lflow_add_at_with_hash(lflows, peer->od,
> > + S_SWITCH_IN_L2_LKUP, 90,
>
> And then finally here, since peer is a logical router port, adding the lflow
> to S_SWITCH_IN_L2_LKUP table doesn't work.
>
> I think the issue here is that "op" shouldn't be set to the peer of the
> logical router port. Instead, I think it should be set directly to the
> logical router port. If you do this, then the rest of the logic in this loop
> should work correctly.
>
> > + ds_cstr(match), action,
> > + NULL, NULL, &peer->nbsp->header_,
> > + OVS_SOURCE_LOCATOR, hash);
> > + }
> > + }
> > +}
> > +
> > static void
> > build_lrouter_flows_for_lb(struct ovn_northd_lb *lb, struct hmap *lflows,
> > - struct shash *meter_groups,
> > + struct hmap *ports, struct shash *meter_groups,
> > struct ds *match, struct ds *action)
> > {
> > if (!lb->n_nb_lr) {
> > @@ -9453,6 +9516,8 @@ build_lrouter_flows_for_lb(struct ovn_northd_lb *lb,
> > struct hmap *lflows,
> > for (size_t i = 0; i < lb->n_vips; i++) {
> > struct ovn_lb_vip *lb_vip = &lb->vips[i];
> > + build_lflows_for_unreachable_vips(lb, lb_vip, lflows, ports,
> > match);
> > +
> > build_lrouter_nat_flows_for_lb(lb_vip, lb, &lb->vips_nb[i],
> > lflows, match, action,
> > meter_groups);
> > @@ -12778,7 +12843,7 @@ build_lflows_thread(void *arg)
> > build_lrouter_defrag_flows_for_lb(lb, lsi->lflows,
> > &lsi->match);
> > build_lrouter_flows_for_lb(lb, lsi->lflows,
> > - lsi->meter_groups,
> > + lsi->ports,
> > lsi->meter_groups,
> > &lsi->match,
> > &lsi->actions);
> > build_lswitch_flows_for_lb(lb, lsi->lflows,
> > lsi->meter_groups,
> > @@ -12947,8 +13012,9 @@ build_lswitch_and_lrouter_flows(struct hmap
> > *datapaths, struct hmap *ports,
> > &lsi.actions,
> > &lsi.match);
> > build_lrouter_defrag_flows_for_lb(lb, lsi.lflows, &lsi.match);
> > - build_lrouter_flows_for_lb(lb, lsi.lflows, lsi.meter_groups,
> > - &lsi.match, &lsi.actions);
> > + build_lrouter_flows_for_lb(lb, lsi.lflows, lsi.ports,
> > + lsi.meter_groups, &lsi.match,
> > + &lsi.actions);
> > build_lswitch_flows_for_lb(lb, lsi.lflows, lsi.meter_groups,
> > &lsi.match, &lsi.actions);
> > }
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
