On 21 January 2017 at 16:52, Mickey Spiegel <[email protected]> wrote:
> Currently, for performance reasons on gateway routers, ct_snat > that does not specify an IP address does not immediately trigger > recirculation. On gateway routers, ct_snat that does not specify > an IP address happens in the UNSNAT pipeline stage, which is > followed by the DNAT pipeline stage that triggers recirculation > for all packets. This DNAT pipeline stage recirculation takes > care of the recirculation needs of UNSNAT as well as other cases > such as UNDNAT. > > On distributed routers, UNDNAT is handled in the egress pipeline > stage, separately from DNAT in the ingress pipeline stages. The > DNAT pipeline stage only triggers recirculation for some packets. > Due to this difference in design, UNSNAT needs to trigger its own > recirculation. > > This patch restricts the logic that avoids recirculation for > ct_snat, so that it only applies to datapaths representing > gateway routers. > > Signed-off-by: Mickey Spiegel <[email protected]> > Acked-by: Gurucharan Shetty <[email protected]> > --- > include/ovn/actions.h | 3 +++ > ovn/controller/lflow.c | 10 ++++++++++ > ovn/lib/actions.c | 15 +++++++++------ > ovn/ovn-sb.xml | 23 +++++++++++++++++++---- > tests/ovn.at | 2 +- > 5 files changed, 42 insertions(+), 11 deletions(-) > > diff --git a/include/ovn/actions.h b/include/ovn/actions.h > index 1d7bd69..d2510fd 100644 > --- a/include/ovn/actions.h > +++ b/include/ovn/actions.h > @@ -445,6 +445,9 @@ struct ovnact_encode_params { > /* 'true' if the flow is for a switch. */ > bool is_switch; > > + /* 'true' if the flow is for a gateway router. */ > + bool is_gateway_router; > + > /* A map from a port name to its connection tracking zone. */ > const struct simap *ct_zones; > > diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c > index 2d9213b..fa00db2 100644 > --- a/ovn/controller/lflow.c > +++ b/ovn/controller/lflow.c > @@ -107,6 +107,15 @@ is_switch(const struct sbrec_datapath_binding *ldp) > > } > > +static bool > +is_gateway_router(const struct sbrec_datapath_binding *ldp, > + const struct hmap *local_datapaths) > +{ > + struct local_datapath *ld = > + get_local_datapath(local_datapaths, ldp->tunnel_key); > + return ld ? ld->has_local_l3gateway : false; > +} > + > /* Adds the logical flows from the Logical_Flow table to flow tables. */ > static void > add_logical_flows(struct controller_ctx *ctx, const struct lport_index > *lports, > @@ -221,6 +230,7 @@ consider_logical_flow(const struct lport_index *lports, > .lookup_port = lookup_port_cb, > .aux = &aux, > .is_switch = is_switch(ldp), > + .is_gateway_router = is_gateway_router(ldp, local_datapaths), > .ct_zones = ct_zones, > .group_table = group_table, > > diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c > index 90a2add..fff838b 100644 > --- a/ovn/lib/actions.c > +++ b/ovn/lib/actions.c > @@ -829,12 +829,15 @@ encode_ct_nat(const struct ovnact_ct_nat *cn, > ct = ofpacts->header; > if (cn->ip) { > ct->flags |= NX_CT_F_COMMIT; > - } else if (snat) { > - /* XXX: For performance reasons, we try to prevent additional > - * recirculations. So far, ct_snat which is used in a gateway > router > - * does not need a recirculation. ct_snat(IP) does need a > - * recirculation. Should we consider a method to let the actions > - * specify whether an action needs recirculation if there more use > + } else if (snat && ep->is_gateway_router) { > + /* For performance reasons, we try to prevent additional > + * recirculations. ct_snat which is used in a gateway router > + * does not need a recirculation. ct_snat(IP) does need a > + * recirculation. ct_snat in a distributed router needs > + * recirculation regardless of whether an IP address is > + * specified. > + * XXX Should we consider a method to let the actions specify > + * whether an action needs recirculation if there are more use > * cases?. */ > ct->recirc_table = NX_CT_RECIRC_NONE; > } > diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml > index f806af7..b33afd3 100644 > --- a/ovn/ovn-sb.xml > +++ b/ovn/ovn-sb.xml > @@ -1128,11 +1128,26 @@ > <dd> > <p> > <code>ct_snat</code> sends the packet through the SNAT zone to > - unSNAT any packet that was SNATed in the opposite direction. > If > - the packet needs to be sent to the next tables, then it > should be > - followed by a <code>next;</code> action. The next tables > will not > - see the changes in the packet caused by the connection > tracker. > + unSNAT any packet that was SNATed in the opposite direction. > The > + behavior on gateway routers differs from the behavior on a > + distributed router: > </p> > + <ul> > + <li> > + On a gateway router, if the packet needs to be sent to the > next > + tables, then it should be followed by a <code>next;</code> > + action. The next tables will not see the changes in the > packet > + caused by the connection tracker. > + </li> > + <li> > + On a distributed router, if the connection tracker finds a > + connection that was SNATed in the opposite direction, then > the > + destination IP address of the packet is UNSNATed. The > packet is > + automatically sent to the next tables as if followed by the > + <code>next;</code> action. The next tables will see the > changes > + in the packet caused by the connection tracker. > + </li> > + </ul> > <p> > <code>ct_snat(<var>IP</var>)</code> sends the packet through > the > SNAT zone to change the source IP address of the packet to > diff --git a/tests/ovn.at b/tests/ovn.at > index 126574c..6eed020 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -872,7 +872,7 @@ ct_dnat(); > > # ct_snat > ct_snat; > - encodes as ct(zone=NXM_NX_REG12[0..15],nat) > + encodes as ct(table=27,zone=NXM_NX_REG12[0..15],nat) > has prereqs ip > ct_snat(192.168.1.2); > encodes as ct(commit,table=27,zone=NXM_NX_REG12[0..15],nat(src=192. > 168.1.2)) > -- > 1.9.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
