On Thu, Jun 3, 2021 at 5:29 AM <num...@ovn.org> wrote:
>
> From: Numan Siddique <num...@ovn.org>
>
> After the commit [1], we do a full recompute of ct zone I-P engine for
> any datapath binding change. This results in physical_flow() getting
> called. In a highly scaled environment this can result in costly CPU
> cycles. This patch addressed this by handling the datapath binding
> changes incrementally in the ct_zone engine node. ct zone recomputation
> is required only when a datapath binding is deleted or a logical router
> datapath binding changes the snat-ct-zone option value. This patch
> handles this.
>
> [1] - f9cab11d5fab("Allow explicit setting of the SNAT zone on a gateway
router.")
>
> Signed-off-by: Numan Siddique <num...@ovn.org>
> ---
> controller/ovn-controller.c | 55 ++++++++++++++++++++++++++++++++++++-
> controller/physical.c | 5 ++++
> tests/ovn-performance.at | 26 ++++++++++++++++++
> 3 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index b49786441..5b55a45b7 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -1734,6 +1734,58 @@ en_ct_zones_run(struct engine_node *node, void
*data)
> engine_set_node_state(node, EN_UPDATED);
> }
>
> +/* Handles datapath binding changes for the ct_zones engine.
> + * Returns false if the datapath is deleted or if the requested snat
> + * ct zone doesn't match with the ct_zones data. */
> +static bool
> +ct_zones_datapath_binding_handler(struct engine_node *node, void *data)
> +{
> + struct ed_type_ct_zones *ct_zones_data = data;
> + const struct sbrec_datapath_binding *dp;
> + struct ed_type_runtime_data *rt_data =
> + engine_get_input_data("runtime_data", node);
> + struct sbrec_datapath_binding_table *dp_table =
> + (struct sbrec_datapath_binding_table *)EN_OVSDB_GET(
> + engine_get_input("SB_datapath_binding", node));
> +
> + SBREC_DATAPATH_BINDING_TABLE_FOR_EACH_TRACKED (dp, dp_table) {
> + if (!get_local_datapath(&rt_data->local_datapaths,
> + dp->tunnel_key)) {
> + continue;
> + }
> +
> + if (sbrec_datapath_binding_is_deleted(dp)) {
> + /* Fall back to full recompute of ct_zones engine. */
> + return false;
> + }
What if a datapath_binding is created? Should we return false as well?
> +
> + int req_snat_zone = datapath_snat_ct_zone(dp);
> + if (req_snat_zone == -1) {
> + /* datapath snat ct zone is not set. This condition will
also hit
> + * when CMS clears the snat-ct-zone for the logical router.
> + * In this case there is no harm in using the previosly
specified
> + * snat ct zone for this datapath. Also it is hard to know
> + * if this option was cleared or if this option is never
set. */
> + continue;
> + }
> +
> + /* Check if the requested snat zone has changed for the datapath
> + * or not. If so, then fall back to full recompute of
> + * ct_zone engine. */
> + char *snat_dp_zone_key = alloc_nat_zone_key(&dp->header_.uuid,
"snat");
> + struct simap_node *simap_node =
simap_find(&ct_zones_data->current,
> + snat_dp_zone_key);
> + free(snat_dp_zone_key);
> + if (!simap_node || simap_node->data != req_snat_zone) {
> + /* There is no entry yet or the requested snat zone has
changed.
> + * Trigger full recompute of ct_zones engine. */
> + return false;
> + }
> + }
> +
> + return true;
> +}
> +
> /* The data in the ct_zones node is always valid (i.e., no stale
pointers). */
> static bool
> en_ct_zones_is_valid(struct engine_node *node OVS_UNUSED)
> @@ -2758,7 +2810,8 @@ main(int argc, char *argv[])
>
> engine_add_input(&en_ct_zones, &en_ovs_open_vswitch, NULL);
> engine_add_input(&en_ct_zones, &en_ovs_bridge, NULL);
> - engine_add_input(&en_ct_zones, &en_sb_datapath_binding, NULL);
> + engine_add_input(&en_ct_zones, &en_sb_datapath_binding,
> + ct_zones_datapath_binding_handler);
> engine_add_input(&en_ct_zones, &en_runtime_data, NULL);
>
> engine_add_input(&en_runtime_data, &en_ofctrl_is_connected, NULL);
> diff --git a/controller/physical.c b/controller/physical.c
> index 04259d44a..e70efc71d 100644
> --- a/controller/physical.c
> +++ b/controller/physical.c
> @@ -15,6 +15,7 @@
>
> #include <config.h>
> #include "binding.h"
> +#include "coverage.h"
> #include "byte-order.h"
> #include "encaps.h"
> #include "flow.h"
> @@ -48,6 +49,8 @@
>
> VLOG_DEFINE_THIS_MODULE(physical);
>
> +COVERAGE_DEFINE(physical_run);
> +
> /* Datapath zone IDs for connection tracking and NAT */
> struct zone_ids {
> int ct; /* MFF_LOG_CT_ZONE. */
> @@ -1528,6 +1531,8 @@ void
> physical_run(struct physical_ctx *p_ctx,
> struct ovn_desired_flow_table *flow_table)
> {
> + COVERAGE_INC(physical_run);
> +
> if (!hc_uuid) {
> hc_uuid = xmalloc(sizeof(struct uuid));
> uuid_generate(hc_uuid);
> diff --git a/tests/ovn-performance.at b/tests/ovn-performance.at
> index e510c6cef..61356e16d 100644
> --- a/tests/ovn-performance.at
> +++ b/tests/ovn-performance.at
> @@ -462,6 +462,32 @@ OVN_CONTROLLER_EXPECT_HIT_COND(
> [ovn-nbctl --wait=hv lrp-set-gateway-chassis lr1-public hv3 30 &&
ovn-nbctl --wait=hv sync]
> )
>
> +# Test physical_run for logical router and other datapath binding
changes.
> +OVN_CONTROLLER_EXPECT_HIT_COND(
> + [hv1 hv2 hv3 hv4 hv5], [physical_run], [>0 >0 >0 =0 =0],
> + [ovn-nbctl --wait=hv set logical_router lr1 options:snat-ct-zone=10
&& ovn-nbctl --wait=hv sync]
> +)
> +
> +OVN_CONTROLLER_EXPECT_HIT_COND(
> + [hv1 hv2 hv3 hv4 hv5], [physical_run], [>0 >0 >0 =0 =0],
> + [ovn-nbctl --wait=hv set logical_router lr1 options:snat-ct-zone=11
&& ovn-nbctl --wait=hv sync]
> +)
> +
> +OVN_CONTROLLER_EXPECT_NO_HIT(
> + [hv1 hv2 hv3 hv4 hv5], [physical_run],
> + [ovn-nbctl --wait=hv remove logical_router lr1 options snat-ct-zone
&& ovn-nbctl --wait=hv sync]
> +)
> +
> +OVN_CONTROLLER_EXPECT_NO_HIT(
> + [hv1 hv2 hv3 hv4 hv5], [physical_run],
> + [ovn-sbctl set datapath_binding lr1 external_ids:foo=bar &&
ovn-nbctl --wait=hv sync]
> +)
> +
> +OVN_CONTROLLER_EXPECT_NO_HIT(
> + [hv1 hv2 hv3 hv4 hv5], [physical_run],
> + [ovn-sbctl set datapath_binding ls1 external_ids:foo=bar &&
ovn-nbctl --wait=hv sync]
> +)
> +
> # After this, BFD should be enabled from hv1 and hv2 to hv3.
> # So there should be lflow_run hits in hv1, hv2, hv3 and hv4
> OVN_CONTROLLER_EXPECT_HIT_COND(
> --
> 2.31.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
