Re: [ovs-dev] [PATCH ovn 1/2] northd: refactor unreachable IPs lb flows
> Hi Lorenzo,
>
> I have some minor findings below.
Hi Mark,
thx for the fast review :)
>
[...]
>
> s/,od/, od/
ack, I will fix it
>
> > + io_port, ctrl_meter, stage_hint, where,
> > hash);
> > }
> > /* Adds a row with the specified contents to the Logical_Flow table. */
> > @@ -6870,16 +6880,11 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> > *op,
> > /* Check if the ovn port has a network configured on which we
> > could
> >* expect ARP requests for the LB VIP.
> >*/
> > -if (ip_parse(ip_addr, _addr)) {
> > -if (lrouter_port_ipv4_reachable(op, ipv4_addr)) {
> > -build_lswitch_rport_arp_req_flow_for_reachable_ip(
> > -ip_addr, AF_INET, sw_op, sw_od, 80, lflows,
> > -stage_hint);
> > -} else {
> > -build_lswitch_rport_arp_req_flow_for_unreachable_ip(
> > -ip_addr, AF_INET, sw_od, 90, lflows,
> > -stage_hint);
> > -}
> > +if (ip_parse(ip_addr, _addr) &&
> > +lrouter_port_ipv4_reachable(op, ipv4_addr)) {
> > +build_lswitch_rport_arp_req_flow_for_reachable_ip(
> > +ip_addr, AF_INET, sw_op, sw_od, 80, lflows,
> > +stage_hint);
> > }
> > }
> > SSET_FOR_EACH (ip_addr, >od->lb_ips_v6) {
> > @@ -6888,16 +6893,11 @@ build_lswitch_rport_arp_req_flows(struct ovn_port
> > *op,
> > /* Check if the ovn port has a network configured on which we
> > could
> >* expect NS requests for the LB VIP.
> >*/
> > -if (ipv6_parse(ip_addr, _addr)) {
> > -if (lrouter_port_ipv6_reachable(op, _addr)) {
> > -build_lswitch_rport_arp_req_flow_for_reachable_ip(
> > -ip_addr, AF_INET6, sw_op, sw_od, 80, lflows,
> > -stage_hint);
> > -} else {
> > -build_lswitch_rport_arp_req_flow_for_unreachable_ip(
> > -ip_addr, AF_INET6, sw_od, 90, lflows,
> > -stage_hint);
> > -}
> > +if (ipv6_parse(ip_addr, _addr) &&
> > +lrouter_port_ipv6_reachable(op, _addr)) {
> > +build_lswitch_rport_arp_req_flow_for_reachable_ip(
> > +ip_addr, AF_INET6, sw_op, sw_od, 80, lflows,
> > +stage_hint);
> > }
> > }
> > @@ -9422,9 +9422,70 @@ build_lrouter_defrag_flows_for_lb(struct
> > ovn_northd_lb *lb,
> > ds_destroy(_actions);
> > }
> > +static void
> > +build_lrouter_flows_for_unreachable_ips(struct ovn_northd_lb *lb,
>
> The name of this function is misleading. "build_lrouter_*" typically means
> that we are adding flows to a logical router. In this case we are adding
> flows to logical switches that are connected to logical routers.
ack, I will fix it
>
> > +struct ovn_lb_vip *lb_vip,
> > +struct hmap *lflows,
> > +struct hmap *ports,
> > +struct ds *match,
> > +struct ds *action)
> > +{
> > +bool ipv4 = IN6_IS_ADDR_V4MAPPED(_vip->vip);
> > +ovs_be32 ipv4_addr;
> > +
> > +ds_clear(match);
> > +if (ipv4) {
> > +if (!ip_parse(lb_vip->vip_str, _addr)) {
> > +return;
> > +}
> > +ds_put_format(match, "%s && arp.op == 1 && arp.tpa == %s",
> > + FLAGBIT_NOT_VXLAN, lb_vip->vip_str);
> > +} else {
> > +ds_put_format(match, "%s && nd_ns && nd.target == %s",
> > + FLAGBIT_NOT_VXLAN, lb_vip->vip_str);
> > +}
> > +
> > +ds_clear(action);
> > +ds_put_cstr(action, "outport = \""MC_FLOOD"\"; output;");
>
> Since the action is always the same, you could just use a `static const char
> *` for the action instead of `struct ds`.
ack, I will fix it
>
> > +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), ds_cstr(action));
> > +
> > +for (size_t i = 0; i < lb->n_nb_lr; i++) {
>
> The body of this for loop appears to be over-indented by 4 spaces.
ack, I will fix it
>
> > +struct ovn_datapath *od = lb->nb_lr[i];
> > +for (size_t j = 0; j < od->n_router_ports; j++) {
> > +struct ovn_port *op = ovn_port_get_peer(ports,
> > +
> > od->router_ports[j]);
> > +if (!op) {
> > +continue;
> > +}
> > +
> > +struct ovn_port *peer = op->peer;
> > +if (!peer || !peer->nbsp || lsp_is_external(peer->nbsp)) {
> > +
Re: [ovs-dev] [PATCH ovn 1/2] northd: refactor unreachable IPs lb flows
Hi Lorenzo,
I have some minor findings below.
On 8/24/21 2:02 PM, Lorenzo Bianconi wrote:
Refactor unreachable IPs for vip load-balancers inverting the logic used
during the lb flow creation in order to visit lb first and then related
datapath/ovn_ports. This is a preliminary patch to avoid recomputing
flow hash and lflow lookup when not necessary.
Signed-off-by: Lorenzo Bianconi
---
northd/ovn-northd.c | 137
1 file changed, 101 insertions(+), 36 deletions(-)
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3d8e21a4f..a8e69b3cb 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4391,6 +4391,26 @@ do_ovn_lflow_add(struct hmap *lflow_map, struct
ovn_datapath *od,
}
/* Adds a row with the specified contents to the Logical_Flow table. */
+static void
+ovn_lflow_add_at_with_hash(struct hmap *lflow_map, struct ovn_datapath *od,
+ enum ovn_stage stage, uint16_t priority,
+ const char *match, const char *actions,
+ const char *io_port, const char *ctrl_meter,
+ const struct ovsdb_idl_row *stage_hint,
+ const char *where, uint32_t hash)
+{
+ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od));
+if (use_logical_dp_groups && use_parallel_build) {
+lock_hash_row(_locks, hash);
+do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,
+ actions, io_port, stage_hint, where, ctrl_meter);
+unlock_hash_row(_locks, hash);
+} else {
+do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,
+ actions, io_port, stage_hint, where, ctrl_meter);
+}
+}
+
static void
ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
enum ovn_stage stage, uint16_t priority,
@@ -4398,24 +4418,14 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct
ovn_datapath *od,
const char *ctrl_meter,
const struct ovsdb_idl_row *stage_hint, const char *where)
{
-ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od));
-
uint32_t hash;
hash = ovn_logical_flow_hash(ovn_stage_get_table(stage),
ovn_stage_get_pipeline_name(stage),
priority, match,
actions);
-
-if (use_logical_dp_groups && use_parallel_build) {
-lock_hash_row(_locks, hash);
-do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,
- actions, io_port, stage_hint, where, ctrl_meter);
-unlock_hash_row(_locks, hash);
-} else {
-do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,
- actions, io_port, stage_hint, where, ctrl_meter);
-}
+ovn_lflow_add_at_with_hash(lflow_map,od, stage, priority, match, actions,
s/,od/, od/
+ io_port, ctrl_meter, stage_hint, where, hash);
}
/* Adds a row with the specified contents to the Logical_Flow table. */
@@ -6870,16 +6880,11 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
/* Check if the ovn port has a network configured on which we could
* expect ARP requests for the LB VIP.
*/
-if (ip_parse(ip_addr, _addr)) {
-if (lrouter_port_ipv4_reachable(op, ipv4_addr)) {
-build_lswitch_rport_arp_req_flow_for_reachable_ip(
-ip_addr, AF_INET, sw_op, sw_od, 80, lflows,
-stage_hint);
-} else {
-build_lswitch_rport_arp_req_flow_for_unreachable_ip(
-ip_addr, AF_INET, sw_od, 90, lflows,
-stage_hint);
-}
+if (ip_parse(ip_addr, _addr) &&
+lrouter_port_ipv4_reachable(op, ipv4_addr)) {
+build_lswitch_rport_arp_req_flow_for_reachable_ip(
+ip_addr, AF_INET, sw_op, sw_od, 80, lflows,
+stage_hint);
}
}
SSET_FOR_EACH (ip_addr, >od->lb_ips_v6) {
@@ -6888,16 +6893,11 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
/* Check if the ovn port has a network configured on which we could
* expect NS requests for the LB VIP.
*/
-if (ipv6_parse(ip_addr, _addr)) {
-if (lrouter_port_ipv6_reachable(op, _addr)) {
-build_lswitch_rport_arp_req_flow_for_reachable_ip(
-ip_addr, AF_INET6, sw_op, sw_od, 80, lflows,
-stage_hint);
-} else {
-build_lswitch_rport_arp_req_flow_for_unreachable_ip(
-ip_addr, AF_INET6, sw_od, 90, lflows,
-stage_hint);
-}
+if (ipv6_parse(ip_addr, _addr) &&
+
[ovs-dev] [PATCH ovn 1/2] northd: refactor unreachable IPs lb flows
Refactor unreachable IPs for vip load-balancers inverting the logic used
during the lb flow creation in order to visit lb first and then related
datapath/ovn_ports. This is a preliminary patch to avoid recomputing
flow hash and lflow lookup when not necessary.
Signed-off-by: Lorenzo Bianconi
---
northd/ovn-northd.c | 137
1 file changed, 101 insertions(+), 36 deletions(-)
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index 3d8e21a4f..a8e69b3cb 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -4391,6 +4391,26 @@ do_ovn_lflow_add(struct hmap *lflow_map, struct
ovn_datapath *od,
}
/* Adds a row with the specified contents to the Logical_Flow table. */
+static void
+ovn_lflow_add_at_with_hash(struct hmap *lflow_map, struct ovn_datapath *od,
+ enum ovn_stage stage, uint16_t priority,
+ const char *match, const char *actions,
+ const char *io_port, const char *ctrl_meter,
+ const struct ovsdb_idl_row *stage_hint,
+ const char *where, uint32_t hash)
+{
+ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od));
+if (use_logical_dp_groups && use_parallel_build) {
+lock_hash_row(_locks, hash);
+do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,
+ actions, io_port, stage_hint, where, ctrl_meter);
+unlock_hash_row(_locks, hash);
+} else {
+do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,
+ actions, io_port, stage_hint, where, ctrl_meter);
+}
+}
+
static void
ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
enum ovn_stage stage, uint16_t priority,
@@ -4398,24 +4418,14 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct
ovn_datapath *od,
const char *ctrl_meter,
const struct ovsdb_idl_row *stage_hint, const char *where)
{
-ovs_assert(ovn_stage_to_datapath_type(stage) == ovn_datapath_get_type(od));
-
uint32_t hash;
hash = ovn_logical_flow_hash(ovn_stage_get_table(stage),
ovn_stage_get_pipeline_name(stage),
priority, match,
actions);
-
-if (use_logical_dp_groups && use_parallel_build) {
-lock_hash_row(_locks, hash);
-do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,
- actions, io_port, stage_hint, where, ctrl_meter);
-unlock_hash_row(_locks, hash);
-} else {
-do_ovn_lflow_add(lflow_map, od, hash, stage, priority, match,
- actions, io_port, stage_hint, where, ctrl_meter);
-}
+ovn_lflow_add_at_with_hash(lflow_map,od, stage, priority, match, actions,
+ io_port, ctrl_meter, stage_hint, where, hash);
}
/* Adds a row with the specified contents to the Logical_Flow table. */
@@ -6870,16 +6880,11 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
/* Check if the ovn port has a network configured on which we could
* expect ARP requests for the LB VIP.
*/
-if (ip_parse(ip_addr, _addr)) {
-if (lrouter_port_ipv4_reachable(op, ipv4_addr)) {
-build_lswitch_rport_arp_req_flow_for_reachable_ip(
-ip_addr, AF_INET, sw_op, sw_od, 80, lflows,
-stage_hint);
-} else {
-build_lswitch_rport_arp_req_flow_for_unreachable_ip(
-ip_addr, AF_INET, sw_od, 90, lflows,
-stage_hint);
-}
+if (ip_parse(ip_addr, _addr) &&
+lrouter_port_ipv4_reachable(op, ipv4_addr)) {
+build_lswitch_rport_arp_req_flow_for_reachable_ip(
+ip_addr, AF_INET, sw_op, sw_od, 80, lflows,
+stage_hint);
}
}
SSET_FOR_EACH (ip_addr, >od->lb_ips_v6) {
@@ -6888,16 +6893,11 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op,
/* Check if the ovn port has a network configured on which we could
* expect NS requests for the LB VIP.
*/
-if (ipv6_parse(ip_addr, _addr)) {
-if (lrouter_port_ipv6_reachable(op, _addr)) {
-build_lswitch_rport_arp_req_flow_for_reachable_ip(
-ip_addr, AF_INET6, sw_op, sw_od, 80, lflows,
-stage_hint);
-} else {
-build_lswitch_rport_arp_req_flow_for_unreachable_ip(
-ip_addr, AF_INET6, sw_od, 90, lflows,
-stage_hint);
-}
+if (ipv6_parse(ip_addr, _addr) &&
+lrouter_port_ipv6_reachable(op, _addr)) {
+build_lswitch_rport_arp_req_flow_for_reachable_ip(
+ip_addr, AF_INET6, sw_op, sw_od, 80, lflows,
+
