[ovs-dev] [PATCH v11 2/5] ovn: avoid snat recirc only on gateway routers

Guru Shetty guru at ovn.org
Tue Jan 24 22:08:56 UTC 2017


On 21 January 2017 at 16:52, Mickey Spiegel <mickeys.dev at gmail.com> 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 <mickeys.dev at gmail.com>
>
Acked-by: Gurucharan Shetty <guru at ovn.org>

> ---
>  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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list