| Message ID | 20220412152624.3541-1-dceara@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] northd: Avoid looking up port peers when not needed. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
Hi Dumitru, This change makes it so that ovn_port_get_peer() is only relevant before op->peer is set; otherwise, you just use op->peer directly. This means that there is only one place in the code now that calls ovn_port_get_peer(). I have a few ideas here. In order of preference: 1) Instead of removing all the calls to ovn_port_get_peer(), modify ovn_port_get_peer() to attempt to return op->peer if it is non-NULL. Otherwise, it can fall back to looking up by router-port. This way developers always call ovn_port_get_peer() no matter the circumstances, lessening the cognitive load. 2) Get rid of ovn_port_get_peer() and put its logic in-line in join_logical_ports(). This gets rid of a single-use function and keeps other developers from calling it unnecessarily. 3) Leave your code as-is but add a comment to ovn_port_get_peer() to explain that it is unnecessary the vast majority of the time. What do you think? Mark On 4/12/22 11:26, Dumitru Ceara wrote: > There's no need to call ovn_port_get_peer() to find the peer port of a > logical switch port that's connected to a logical router. We already > store those in op->peer. > > Also, factor out addition of router ports to a logical switch's > ovn_datapath and use x2nrealloc(). > > Signed-off-by: Dumitru Ceara <dceara@redhat.com> > --- > Spotted during code inspection. I don't really think it will improve > performance but it can't hurt and it should make the code a bit more > clear. > --- > northd/northd.c | 42 +++++++++++++++++++++++------------------- > 1 file changed, 23 insertions(+), 19 deletions(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 01ae7d7fd7..6308259159 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -601,6 +601,7 @@ struct ovn_datapath { > /* Logical switch data. */ > struct ovn_port **router_ports; > size_t n_router_ports; > + size_t n_allocated_router_ports; > > struct hmap port_tnlids; > uint32_t port_key_hint; > @@ -1081,6 +1082,17 @@ ovn_datapath_from_sbrec(const struct hmap *datapaths, > return NULL; > } > > +static void > +ovn_datapath_add_router_port(struct ovn_datapath *od, struct ovn_port *op) > +{ > + if (od->n_router_ports == od->n_allocated_router_ports) { > + od->router_ports = x2nrealloc(od->router_ports, > + &od->n_allocated_router_ports, > + sizeof *od->router_ports); > + } > + od->router_ports[od->n_router_ports++] = op; > +} > + > static bool > lrouter_is_enabled(const struct nbrec_logical_router *lrouter) > { > @@ -2657,12 +2669,9 @@ join_logical_ports(struct northd_input *input_data, > continue; > } > > + ovn_datapath_add_router_port(op->od, op); > peer->peer = op; > op->peer = peer; > - op->od->router_ports = xrealloc( > - op->od->router_ports, > - sizeof *op->od->router_ports * (op->od->n_router_ports + 1)); > - op->od->router_ports[op->od->n_router_ports++] = op; > > /* Fill op->lsp_addrs for op->nbsp->addresses[] with > * contents "router", which was skipped in the loop above. */ > @@ -11142,8 +11151,8 @@ build_ip_routing_pre_flows_for_lrouter(struct ovn_datapath *od, > * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 for the selected ECMP member. > */ > static void > -build_ip_routing_flows_for_lrouter_port( > - struct ovn_port *op, const struct hmap *ports, struct hmap *lflows) > +build_ip_routing_flows_for_lrouter_port(struct ovn_port *op, > + struct hmap *lflows) > { > if (op->nbrp) { > > @@ -11161,14 +11170,13 @@ build_ip_routing_flows_for_lrouter_port( > &op->nbrp->header_, false, ROUTE_PRIO_OFFSET_CONNECTED); > } > } else if (lsp_is_router(op->nbsp)) { > - struct ovn_port *peer = ovn_port_get_peer(ports, op); > + struct ovn_port *peer = op->peer; > if (!peer || !peer->nbrp || !peer->lrp_networks.n_ipv4_addrs) { > return; > } > > for (int i = 0; i < op->od->n_router_ports; i++) { > - struct ovn_port *router_port = ovn_port_get_peer( > - ports, op->od->router_ports[i]); > + struct ovn_port *router_port = op->od->router_ports[i]->peer; > if (!router_port || !router_port->nbrp || router_port == peer) { > continue; > } > @@ -11547,8 +11555,7 @@ build_arp_resolve_flows_for_lrouter_port( > /* Get the Logical_Router_Port that the > * Logical_Switch_Port is connected to, as > * 'peer'. */ > - struct ovn_port *peer = ovn_port_get_peer( > - ports, op->od->router_ports[k]); > + struct ovn_port *peer = op->od->router_ports[k]->peer; > if (!peer || !peer->nbrp) { > continue; > } > @@ -11578,8 +11585,7 @@ build_arp_resolve_flows_for_lrouter_port( > /* Get the Logical_Router_Port that the > * Logical_Switch_Port is connected to, as > * 'peer'. */ > - struct ovn_port *peer = ovn_port_get_peer( > - ports, op->od->router_ports[k]); > + struct ovn_port *peer = op->od->router_ports[k]->peer; > if (!peer || !peer->nbrp) { > continue; > } > @@ -11639,8 +11645,7 @@ build_arp_resolve_flows_for_lrouter_port( > !op->sb->chassis) { > /* The virtual port is not claimed yet. */ > for (size_t i = 0; i < op->od->n_router_ports; i++) { > - struct ovn_port *peer = ovn_port_get_peer( > - ports, op->od->router_ports[i]); > + struct ovn_port *peer = op->od->router_ports[i]->peer; > if (!peer || !peer->nbrp) { > continue; > } > @@ -11675,8 +11680,7 @@ build_arp_resolve_flows_for_lrouter_port( > /* Get the Logical_Router_Port that the > * Logical_Switch_Port is connected to, as > * 'peer'. */ > - struct ovn_port *peer = > - ovn_port_get_peer(ports, vp->od->router_ports[j]); > + struct ovn_port *peer = vp->od->router_ports[j]->peer; > if (!peer || !peer->nbrp) { > continue; > } > @@ -11713,7 +11717,7 @@ build_arp_resolve_flows_for_lrouter_port( > * we need to add logical flows such that it can resolve > * ARP entries for all the other router ports connected to > * the switch in question. */ > - struct ovn_port *peer = ovn_port_get_peer(ports, op); > + struct ovn_port *peer = op->peer; > if (!peer || !peer->nbrp) { > return; > } > @@ -13626,7 +13630,7 @@ build_lswitch_and_lrouter_iterate_by_op(struct ovn_port *op, > &lsi->actions); > build_neigh_learning_flows_for_lrouter_port(op, lsi->lflows, &lsi->match, > &lsi->actions); > - build_ip_routing_flows_for_lrouter_port(op, lsi->ports, lsi->lflows); > + build_ip_routing_flows_for_lrouter_port(op, lsi->lflows); > build_ND_RA_flows_for_lrouter_port(op, lsi->lflows, &lsi->match, > &lsi->actions, lsi->meter_groups); > build_arp_resolve_flows_for_lrouter_port(op, lsi->lflows, lsi->ports, >
On 4/18/22 19:32, Mark Michelson wrote: > Hi Dumitru, > Hi Mark, Thanks for reviewing this patch! > This change makes it so that ovn_port_get_peer() is only relevant before > op->peer is set; otherwise, you just use op->peer directly. This means > that there is only one place in the code now that calls > ovn_port_get_peer(). I have a few ideas here. In order of preference: > > 1) Instead of removing all the calls to ovn_port_get_peer(), modify > ovn_port_get_peer() to attempt to return op->peer if it is non-NULL. > Otherwise, it can fall back to looking up by router-port. This way > developers always call ovn_port_get_peer() no matter the circumstances, > lessening the cognitive load. I will do this. > > 2) Get rid of ovn_port_get_peer() and put its logic in-line in > join_logical_ports(). This gets rid of a single-use function and keeps > other developers from calling it unnecessarily. > I don't really like this option because join_logical_ports() is really long and complex already. > 3) Leave your code as-is but add a comment to ovn_port_get_peer() to > explain that it is unnecessary the vast majority of the time. Normally, I'd prefer this solution, but that's because the code populating ovn_port and ovn_datapath structures should be, in my opinion, separated modules, distinct from the code that uses ovn_port and ovn_datapath. That's not currently the case though. > > What do you think? > I'll post v2, implementing option "1)". > Mark > Thanks, Dumitru > On 4/12/22 11:26, Dumitru Ceara wrote: >> There's no need to call ovn_port_get_peer() to find the peer port of a >> logical switch port that's connected to a logical router. We already >> store those in op->peer. >> >> Also, factor out addition of router ports to a logical switch's >> ovn_datapath and use x2nrealloc(). >> >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> >> --- >> Spotted during code inspection. I don't really think it will improve >> performance but it can't hurt and it should make the code a bit more >> clear. >> --- >> northd/northd.c | 42 +++++++++++++++++++++++------------------- >> 1 file changed, 23 insertions(+), 19 deletions(-) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index 01ae7d7fd7..6308259159 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -601,6 +601,7 @@ struct ovn_datapath { >> /* Logical switch data. */ >> struct ovn_port **router_ports; >> size_t n_router_ports; >> + size_t n_allocated_router_ports; >> struct hmap port_tnlids; >> uint32_t port_key_hint; >> @@ -1081,6 +1082,17 @@ ovn_datapath_from_sbrec(const struct hmap >> *datapaths, >> return NULL; >> } >> +static void >> +ovn_datapath_add_router_port(struct ovn_datapath *od, struct ovn_port >> *op) >> +{ >> + if (od->n_router_ports == od->n_allocated_router_ports) { >> + od->router_ports = x2nrealloc(od->router_ports, >> + &od->n_allocated_router_ports, >> + sizeof *od->router_ports); >> + } >> + od->router_ports[od->n_router_ports++] = op; >> +} >> + >> static bool >> lrouter_is_enabled(const struct nbrec_logical_router *lrouter) >> { >> @@ -2657,12 +2669,9 @@ join_logical_ports(struct northd_input >> *input_data, >> continue; >> } >> + ovn_datapath_add_router_port(op->od, op); >> peer->peer = op; >> op->peer = peer; >> - op->od->router_ports = xrealloc( >> - op->od->router_ports, >> - sizeof *op->od->router_ports * >> (op->od->n_router_ports + 1)); >> - op->od->router_ports[op->od->n_router_ports++] = op; >> /* Fill op->lsp_addrs for op->nbsp->addresses[] with >> * contents "router", which was skipped in the loop >> above. */ >> @@ -11142,8 +11151,8 @@ build_ip_routing_pre_flows_for_lrouter(struct >> ovn_datapath *od, >> * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 for the selected ECMP member. >> */ >> static void >> -build_ip_routing_flows_for_lrouter_port( >> - struct ovn_port *op, const struct hmap *ports, struct hmap >> *lflows) >> +build_ip_routing_flows_for_lrouter_port(struct ovn_port *op, >> + struct hmap *lflows) >> { >> if (op->nbrp) { >> @@ -11161,14 +11170,13 @@ build_ip_routing_flows_for_lrouter_port( >> &op->nbrp->header_, false, >> ROUTE_PRIO_OFFSET_CONNECTED); >> } >> } else if (lsp_is_router(op->nbsp)) { >> - struct ovn_port *peer = ovn_port_get_peer(ports, op); >> + struct ovn_port *peer = op->peer; >> if (!peer || !peer->nbrp || !peer->lrp_networks.n_ipv4_addrs) { >> return; >> } >> for (int i = 0; i < op->od->n_router_ports; i++) { >> - struct ovn_port *router_port = ovn_port_get_peer( >> - ports, op->od->router_ports[i]); >> + struct ovn_port *router_port = >> op->od->router_ports[i]->peer; >> if (!router_port || !router_port->nbrp || router_port == >> peer) { >> continue; >> } >> @@ -11547,8 +11555,7 @@ build_arp_resolve_flows_for_lrouter_port( >> /* Get the Logical_Router_Port that the >> * Logical_Switch_Port is connected to, as >> * 'peer'. */ >> - struct ovn_port *peer = ovn_port_get_peer( >> - ports, op->od->router_ports[k]); >> + struct ovn_port *peer = >> op->od->router_ports[k]->peer; >> if (!peer || !peer->nbrp) { >> continue; >> } >> @@ -11578,8 +11585,7 @@ build_arp_resolve_flows_for_lrouter_port( >> /* Get the Logical_Router_Port that the >> * Logical_Switch_Port is connected to, as >> * 'peer'. */ >> - struct ovn_port *peer = ovn_port_get_peer( >> - ports, op->od->router_ports[k]); >> + struct ovn_port *peer = >> op->od->router_ports[k]->peer; >> if (!peer || !peer->nbrp) { >> continue; >> } >> @@ -11639,8 +11645,7 @@ build_arp_resolve_flows_for_lrouter_port( >> !op->sb->chassis) { >> /* The virtual port is not claimed yet. */ >> for (size_t i = 0; i < op->od->n_router_ports; i++) { >> - struct ovn_port *peer = ovn_port_get_peer( >> - ports, op->od->router_ports[i]); >> + struct ovn_port *peer = op->od->router_ports[i]->peer; >> if (!peer || !peer->nbrp) { >> continue; >> } >> @@ -11675,8 +11680,7 @@ build_arp_resolve_flows_for_lrouter_port( >> /* Get the Logical_Router_Port that the >> * Logical_Switch_Port is connected to, as >> * 'peer'. */ >> - struct ovn_port *peer = >> - ovn_port_get_peer(ports, >> vp->od->router_ports[j]); >> + struct ovn_port *peer = >> vp->od->router_ports[j]->peer; >> if (!peer || !peer->nbrp) { >> continue; >> } >> @@ -11713,7 +11717,7 @@ build_arp_resolve_flows_for_lrouter_port( >> * we need to add logical flows such that it can resolve >> * ARP entries for all the other router ports connected to >> * the switch in question. */ >> - struct ovn_port *peer = ovn_port_get_peer(ports, op); >> + struct ovn_port *peer = op->peer; >> if (!peer || !peer->nbrp) { >> return; >> } >> @@ -13626,7 +13630,7 @@ build_lswitch_and_lrouter_iterate_by_op(struct >> ovn_port *op, >> &lsi->actions); >> build_neigh_learning_flows_for_lrouter_port(op, lsi->lflows, >> &lsi->match, >> &lsi->actions); >> - build_ip_routing_flows_for_lrouter_port(op, lsi->ports, >> lsi->lflows); >> + build_ip_routing_flows_for_lrouter_port(op, lsi->lflows); >> build_ND_RA_flows_for_lrouter_port(op, lsi->lflows, &lsi->match, >> &lsi->actions, >> lsi->meter_groups); >> build_arp_resolve_flows_for_lrouter_port(op, lsi->lflows, >> lsi->ports, >> >
diff --git a/northd/northd.c b/northd/northd.c index 01ae7d7fd7..6308259159 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -601,6 +601,7 @@ struct ovn_datapath { /* Logical switch data. */ struct ovn_port **router_ports; size_t n_router_ports; + size_t n_allocated_router_ports; struct hmap port_tnlids; uint32_t port_key_hint; @@ -1081,6 +1082,17 @@ ovn_datapath_from_sbrec(const struct hmap *datapaths, return NULL; } +static void +ovn_datapath_add_router_port(struct ovn_datapath *od, struct ovn_port *op) +{ + if (od->n_router_ports == od->n_allocated_router_ports) { + od->router_ports = x2nrealloc(od->router_ports, + &od->n_allocated_router_ports, + sizeof *od->router_ports); + } + od->router_ports[od->n_router_ports++] = op; +} + static bool lrouter_is_enabled(const struct nbrec_logical_router *lrouter) { @@ -2657,12 +2669,9 @@ join_logical_ports(struct northd_input *input_data, continue; } + ovn_datapath_add_router_port(op->od, op); peer->peer = op; op->peer = peer; - op->od->router_ports = xrealloc( - op->od->router_ports, - sizeof *op->od->router_ports * (op->od->n_router_ports + 1)); - op->od->router_ports[op->od->n_router_ports++] = op; /* Fill op->lsp_addrs for op->nbsp->addresses[] with * contents "router", which was skipped in the loop above. */ @@ -11142,8 +11151,8 @@ build_ip_routing_pre_flows_for_lrouter(struct ovn_datapath *od, * REG_NEXT_HOP_IPV4/REG_NEXT_HOP_IPV6 for the selected ECMP member. */ static void -build_ip_routing_flows_for_lrouter_port( - struct ovn_port *op, const struct hmap *ports, struct hmap *lflows) +build_ip_routing_flows_for_lrouter_port(struct ovn_port *op, + struct hmap *lflows) { if (op->nbrp) { @@ -11161,14 +11170,13 @@ build_ip_routing_flows_for_lrouter_port( &op->nbrp->header_, false, ROUTE_PRIO_OFFSET_CONNECTED); } } else if (lsp_is_router(op->nbsp)) { - struct ovn_port *peer = ovn_port_get_peer(ports, op); + struct ovn_port *peer = op->peer; if (!peer || !peer->nbrp || !peer->lrp_networks.n_ipv4_addrs) { return; } for (int i = 0; i < op->od->n_router_ports; i++) { - struct ovn_port *router_port = ovn_port_get_peer( - ports, op->od->router_ports[i]); + struct ovn_port *router_port = op->od->router_ports[i]->peer; if (!router_port || !router_port->nbrp || router_port == peer) { continue; } @@ -11547,8 +11555,7 @@ build_arp_resolve_flows_for_lrouter_port( /* Get the Logical_Router_Port that the * Logical_Switch_Port is connected to, as * 'peer'. */ - struct ovn_port *peer = ovn_port_get_peer( - ports, op->od->router_ports[k]); + struct ovn_port *peer = op->od->router_ports[k]->peer; if (!peer || !peer->nbrp) { continue; } @@ -11578,8 +11585,7 @@ build_arp_resolve_flows_for_lrouter_port( /* Get the Logical_Router_Port that the * Logical_Switch_Port is connected to, as * 'peer'. */ - struct ovn_port *peer = ovn_port_get_peer( - ports, op->od->router_ports[k]); + struct ovn_port *peer = op->od->router_ports[k]->peer; if (!peer || !peer->nbrp) { continue; } @@ -11639,8 +11645,7 @@ build_arp_resolve_flows_for_lrouter_port( !op->sb->chassis) { /* The virtual port is not claimed yet. */ for (size_t i = 0; i < op->od->n_router_ports; i++) { - struct ovn_port *peer = ovn_port_get_peer( - ports, op->od->router_ports[i]); + struct ovn_port *peer = op->od->router_ports[i]->peer; if (!peer || !peer->nbrp) { continue; } @@ -11675,8 +11680,7 @@ build_arp_resolve_flows_for_lrouter_port( /* Get the Logical_Router_Port that the * Logical_Switch_Port is connected to, as * 'peer'. */ - struct ovn_port *peer = - ovn_port_get_peer(ports, vp->od->router_ports[j]); + struct ovn_port *peer = vp->od->router_ports[j]->peer; if (!peer || !peer->nbrp) { continue; } @@ -11713,7 +11717,7 @@ build_arp_resolve_flows_for_lrouter_port( * we need to add logical flows such that it can resolve * ARP entries for all the other router ports connected to * the switch in question. */ - struct ovn_port *peer = ovn_port_get_peer(ports, op); + struct ovn_port *peer = op->peer; if (!peer || !peer->nbrp) { return; } @@ -13626,7 +13630,7 @@ build_lswitch_and_lrouter_iterate_by_op(struct ovn_port *op, &lsi->actions); build_neigh_learning_flows_for_lrouter_port(op, lsi->lflows, &lsi->match, &lsi->actions); - build_ip_routing_flows_for_lrouter_port(op, lsi->ports, lsi->lflows); + build_ip_routing_flows_for_lrouter_port(op, lsi->lflows); build_ND_RA_flows_for_lrouter_port(op, lsi->lflows, &lsi->match, &lsi->actions, lsi->meter_groups); build_arp_resolve_flows_for_lrouter_port(op, lsi->lflows, lsi->ports,
There's no need to call ovn_port_get_peer() to find the peer port of a logical switch port that's connected to a logical router. We already store those in op->peer. Also, factor out addition of router ports to a logical switch's ovn_datapath and use x2nrealloc(). Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- Spotted during code inspection. I don't really think it will improve performance but it can't hurt and it should make the code a bit more clear. --- northd/northd.c | 42 +++++++++++++++++++++++------------------- 1 file changed, 23 insertions(+), 19 deletions(-)