Re: [ovs-dev] [PATCH RFC] ovn-controller: Optimize processing for non-local datapath without patch ports.

[Han Zhou]

On Wed, Apr 13, 2016 at 10:35 AM, Guru Shetty  wrote:

>
>
> On 28 March 2016 at 00:10, Han Zhou  wrote:
>
>> For non-local datapaths, if there are no patch ports attached, it
>> means the lflows and port bindings would never be needed on the
>> Chassis. Skipping the processing for such lflows and port bindings
>> can save significant amount of CPU, at the same time largely reduce
>> the number of rules in local openflow tables.
>>
>
> If I understand this correctly, is a logical switch is used to connect
> multiple routers (without any local ports), this patch will prevent the
> traffic from going through. Am I right?
>
>
> If a lswitch is connected to logical router(s), there will be patch
port(s) on this lswitch, and this patch won't change the behavior in that
case.

-- 
Best regards,
Han
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH RFC] ovn-controller: Optimize processing for non-local datapath without patch ports.

[Guru Shetty]

On 28 March 2016 at 00:10, Han Zhou  wrote:

> For non-local datapaths, if there are no patch ports attached, it
> means the lflows and port bindings would never be needed on the
> Chassis. Skipping the processing for such lflows and port bindings
> can save significant amount of CPU, at the same time largely reduce
> the number of rules in local openflow tables.
>

If I understand this correctly, is a logical switch is used to connect
multiple routers (without any local ports), this patch will prevent the
traffic from going through. Am I right?


>
> Signed-off-by: Han Zhou 
> ---
>  ovn/controller/lflow.c  | 38
> +++---
>  ovn/controller/lflow.h  |  3 ++-
>  ovn/controller/ovn-controller.c | 16 +---
>  ovn/controller/ovn-controller.h |  6 ++
>  ovn/controller/patch.c  | 22 +++---
>  ovn/controller/patch.h  |  2 +-
>  ovn/controller/physical.c   | 34 +-
>  ovn/controller/physical.h   |  2 +-
>  8 files changed, 102 insertions(+), 21 deletions(-)
>
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 0614a54..be10d18 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -198,6 +198,7 @@ static void
>  add_logical_flows(struct controller_ctx *ctx, const struct lport_index
> *lports,
>const struct mcgroup_index *mcgroups,
>const struct hmap *local_datapaths,
> +  const struct hmap *patched_datapaths,
>const struct simap *ct_zones, struct hmap *flow_table)
>  {
>  uint32_t conj_id_ofs = 1;
> @@ -211,17 +212,18 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
>  if (!ldp) {
>  continue;
>  }
> -if (!ingress && is_switch(ldp)) {
> +if (is_switch(ldp)) {
>  /* For a logical switch datapath, local_datapaths tells us if
> there
> - * are any local ports for this datapath.  If not, processing
> - * logical flows for the egress pipeline of this datapath is
> - * unnecessary.
> + * are any local ports for this datapath.  If not, we can skip
> + * processing logical flows if the flow belongs to egress
> pipeline
> + * or if that logical switch datapath is not patched to any
> logical
> + * router.
>   *
> - * We still need the ingress pipeline because even if there
> are no
> - * local ports, we still may need to execute the ingress
> pipeline
> - * after a packet leaves a logical router.  Further
> optimization
> - * is possible, but not based on what we know with
> local_datapaths
> - * right now.
> + * Otherwise, we still need the ingress pipeline because even
> if
> + * there are no local ports, we still may need to execute the
> ingress
> + * pipeline after a packet leaves a logical router.  Further
> + * optimization is possible, but not based on what we know
> with
> + * local_datapaths right now.
>   *
>   * A better approach would be a kind of "flood fill"
> algorithm:
>   *
> @@ -231,12 +233,25 @@ add_logical_flows(struct controller_ctx *ctx, const
> struct lport_index *lports,
>   *   2. For each patch port P in a logical datapath in S, add
> the
>   *  logical datapath of the remote end of P to S.  Iterate
>   *  until S reaches a fixed point.
> + *
> + * This can be implemented in northd, which can generate the
> sets and
> + * save it on each port-binding record in SB, and
> ovn-controller can
> + * use the information directly. However, there can be update
> storms
> + * when a pair of patch ports are added/removed to
> connect/disconnect
> + * large lrouters and lswitches. This need to be studied
> further.
>   */
>
>  struct hmap_node *ld;
>  ld = hmap_first_with_hash(local_datapaths, ldp->tunnel_key);
>  if (!ld) {
> -continue;
> +if (!ingress) {
> +continue;
> +}
> +struct hmap_node *pd;
> +pd = hmap_first_with_hash(patched_datapaths,
> ldp->tunnel_key);
> +if (!pd) {
> +continue;
> +}
>  }
>  }
>
> @@ -416,10 +431,11 @@ void
>  lflow_run(struct controller_ctx *ctx, const struct lport_index *lports,
>const struct mcgroup_index *mcgroups,
>const struct hmap *local_datapaths,
> +  const struct hmap *patched_datapaths,
>const struct simap *ct_zones, struct hmap *flow_table)
>  {
>  

Re: [ovs-dev] [PATCH RFC] ovn-controller: Optimize processing for non-local datapath without patch ports.

[Han Zhou]

On Tue, Mar 29, 2016 at 6:57 AM, Ryan Moats  wrote:
>
> Acked-by: Ryan Moats 
>

Ryan, thanks for the ack.

Scale testing shows very good result:

Test precondition:
2k hypervisors, 20k lports, 200 lswitches (each with a localnet port).

Test case:
step1: add 50 hypervisors (simulated on 1 BM with 40 cores), and wait for
flow updates complete on all new hypervisors.
step2: create a lswitch and a localnet port, create and bind 100 lports
evenly on these hypervisors. Repeat this 5 times.

Before the change:
Step1 took around 20 minutes.
Step2 took 936 seconds.

After the change:
Step1 took less than 1 minute: 20 times faster.
Step2 took 464 seconds: 2x faster.

I will do some more regression test then submit a formal patch.

--
Best regards,
Han
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH RFC] ovn-controller: Optimize processing for non-local datapath without patch ports.

[Ryan Moats]

Acked-by: Ryan Moats <rmo...@us.ibm.com>

"dev" <dev-boun...@openvswitch.org> wrote on 03/28/2016 02:10:35 AM:

> From: Han Zhou <zhou...@gmail.com>
> To: dev@openvswitch.org
> Date: 03/28/2016 02:11 AM
> Subject: [ovs-dev] [PATCH RFC] ovn-controller: Optimize processing
> for non-local datapath without patch ports.
> Sent by: "dev" <dev-boun...@openvswitch.org>
>
> For non-local datapaths, if there are no patch ports attached, it
> means the lflows and port bindings would never be needed on the
> Chassis. Skipping the processing for such lflows and port bindings
> can save significant amount of CPU, at the same time largely reduce
> the number of rules in local openflow tables.
>
> Signed-off-by: Han Zhou <zhou...@gmail.com>
> ---
>  ovn/controller/lflow.c  | 38 ++
+---
>  ovn/controller/lflow.h  |  3 ++-
>  ovn/controller/ovn-controller.c | 16 +---
>  ovn/controller/ovn-controller.h |  6 ++
>  ovn/controller/patch.c  | 22 +++---
>  ovn/controller/patch.h  |  2 +-
>  ovn/controller/physical.c   | 34 +-
>  ovn/controller/physical.h   |  2 +-
>  8 files changed, 102 insertions(+), 21 deletions(-)
>
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 0614a54..be10d18 100644
> --- a/ovn/controller/lflow.c
> +++ b/ovn/controller/lflow.c
> @@ -198,6 +198,7 @@ static void
>  add_logical_flows(struct controller_ctx *ctx, const struct
> lport_index *lports,
>const struct mcgroup_index *mcgroups,
>const struct hmap *local_datapaths,
> +  const struct hmap *patched_datapaths,
>const struct simap *ct_zones, struct hmap *flow_table)
>  {
>  uint32_t conj_id_ofs = 1;
> @@ -211,17 +212,18 @@ add_logical_flows(struct controller_ctx *ctx,
> const struct lport_index *lports,
>  if (!ldp) {
>  continue;
>  }
> -if (!ingress && is_switch(ldp)) {
> +if (is_switch(ldp)) {
>  /* For a logical switch datapath, local_datapaths tells
> us if there
> - * are any local ports for this datapath.  If not,
processing
> - * logical flows for the egress pipeline of this datapath is
> - * unnecessary.
> + * are any local ports for this datapath.  If not, we can
skip
> + * processing logical flows if the flow belongs to
> egress pipeline
> + * or if that logical switch datapath is not patched to
> any logical
> + * router.
>   *
> - * We still need the ingress pipeline because even if
> there are no
> - * local ports, we still may need to execute the ingress
pipeline
> - * after a packet leaves a logical router.  Further
optimization
> - * is possible, but not based on what we know with
> local_datapaths
> - * right now.
> + * Otherwise, we still need the ingress pipeline because
even if
> + * there are no local ports, we still may need to
> execute the ingress
> + * pipeline after a packet leaves a logical router.  Further
> + * optimization is possible, but not based on what we know
with
> + * local_datapaths right now.
>   *
>   * A better approach would be a kind of "flood fill"
algorithm:
>   *
> @@ -231,12 +233,25 @@ add_logical_flows(struct controller_ctx *ctx,
> const struct lport_index *lports,
>   *   2. For each patch port P in a logical datapath in S,
add the
>   *  logical datapath of the remote end of P to S.
Iterate
>   *  until S reaches a fixed point.
> + *
> + * This can be implemented in northd, which can
> generate the sets and
> + * save it on each port-binding record in SB, and ovn-
> controller can
> + * use the information directly. However, there can be
> update storms
> + * when a pair of patch ports are added/removed to
> connect/disconnect
> + * large lrouters and lswitches. This need to be studied
further.
>   */
>
>  struct hmap_node *ld;
>  ld = hmap_first_with_hash(local_datapaths, ldp->tunnel_key);
>  if (!ld) {
> -continue;
> +if (!ingress) {
> +continue;
> +}
> +struct hmap_node *pd;
> +pd = hmap_first_with_hash(patched_datapaths,
> ldp->tunnel_k

[ovs-dev] [PATCH RFC] ovn-controller: Optimize processing for non-local datapath without patch ports.

[Han Zhou]

For non-local datapaths, if there are no patch ports attached, it
means the lflows and port bindings would never be needed on the
Chassis. Skipping the processing for such lflows and port bindings
can save significant amount of CPU, at the same time largely reduce
the number of rules in local openflow tables.

Signed-off-by: Han Zhou 
---
 ovn/controller/lflow.c  | 38 +++---
 ovn/controller/lflow.h  |  3 ++-
 ovn/controller/ovn-controller.c | 16 +---
 ovn/controller/ovn-controller.h |  6 ++
 ovn/controller/patch.c  | 22 +++---
 ovn/controller/patch.h  |  2 +-
 ovn/controller/physical.c   | 34 +-
 ovn/controller/physical.h   |  2 +-
 8 files changed, 102 insertions(+), 21 deletions(-)

diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 0614a54..be10d18 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -198,6 +198,7 @@ static void
 add_logical_flows(struct controller_ctx *ctx, const struct lport_index *lports,
   const struct mcgroup_index *mcgroups,
   const struct hmap *local_datapaths,
+  const struct hmap *patched_datapaths,
   const struct simap *ct_zones, struct hmap *flow_table)
 {
 uint32_t conj_id_ofs = 1;
@@ -211,17 +212,18 @@ add_logical_flows(struct controller_ctx *ctx, const 
struct lport_index *lports,
 if (!ldp) {
 continue;
 }
-if (!ingress && is_switch(ldp)) {
+if (is_switch(ldp)) {
 /* For a logical switch datapath, local_datapaths tells us if there
- * are any local ports for this datapath.  If not, processing
- * logical flows for the egress pipeline of this datapath is
- * unnecessary.
+ * are any local ports for this datapath.  If not, we can skip
+ * processing logical flows if the flow belongs to egress pipeline
+ * or if that logical switch datapath is not patched to any logical
+ * router.
  *
- * We still need the ingress pipeline because even if there are no
- * local ports, we still may need to execute the ingress pipeline
- * after a packet leaves a logical router.  Further optimization
- * is possible, but not based on what we know with local_datapaths
- * right now.
+ * Otherwise, we still need the ingress pipeline because even if
+ * there are no local ports, we still may need to execute the 
ingress
+ * pipeline after a packet leaves a logical router.  Further
+ * optimization is possible, but not based on what we know with
+ * local_datapaths right now.
  *
  * A better approach would be a kind of "flood fill" algorithm:
  *
@@ -231,12 +233,25 @@ add_logical_flows(struct controller_ctx *ctx, const 
struct lport_index *lports,
  *   2. For each patch port P in a logical datapath in S, add the
  *  logical datapath of the remote end of P to S.  Iterate
  *  until S reaches a fixed point.
+ *
+ * This can be implemented in northd, which can generate the sets 
and
+ * save it on each port-binding record in SB, and ovn-controller 
can
+ * use the information directly. However, there can be update 
storms
+ * when a pair of patch ports are added/removed to 
connect/disconnect
+ * large lrouters and lswitches. This need to be studied further.
  */
 
 struct hmap_node *ld;
 ld = hmap_first_with_hash(local_datapaths, ldp->tunnel_key);
 if (!ld) {
-continue;
+if (!ingress) {
+continue;
+}
+struct hmap_node *pd;
+pd = hmap_first_with_hash(patched_datapaths, ldp->tunnel_key);
+if (!pd) {
+continue;
+}
 }
 }
 
@@ -416,10 +431,11 @@ void
 lflow_run(struct controller_ctx *ctx, const struct lport_index *lports,
   const struct mcgroup_index *mcgroups,
   const struct hmap *local_datapaths,
+  const struct hmap *patched_datapaths,
   const struct simap *ct_zones, struct hmap *flow_table)
 {
 add_logical_flows(ctx, lports, mcgroups, local_datapaths,
-  ct_zones, flow_table);
+  patched_datapaths, ct_zones, flow_table);
 add_neighbor_flows(ctx, lports, flow_table);
 }
 
diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
index ff823d4..a3fc50c 100644
--- a/ovn/controller/lflow.h
+++ b/ovn/controller/lflow.h
@@ -61,7 +61,8 @@ struct uuid;
 void lflow_init(void);
 void lflow_run(struct controller_ctx *, const struct