subject:"\[ovs\-dev\] \[PATCH\] ovn\: Discard flows for non\-local ports."

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

2017-09-20 Thread Miguel Angel Ajo Pelayo

Wow, amazing, that makes sense.

I also ran manual tests locally and everything seemed fine.

On Tue, Sep 19, 2017 at 6:22 PM, Russell Bryant  wrote:

> On Tue, Sep 19, 2017 at 9:38 AM, Russell Bryant  wrote:
> > On Mon, Sep 18, 2017 at 7:31 PM, Han Zhou  wrote:
> >> Thanks Russell for the quick work!
> >>
> >> On Mon, Sep 18, 2017 at 8:24 AM, Russell Bryant 
> wrote:
> >>
> >>> @@ -301,6 +305,22 @@ consider_logical_flow(struct controller_ctx *ctx,
> >>>  if (m->match.wc.masks.conj_id) {
> >>>  m->match.flow.conj_id += *conj_id_ofs;
> >>>  }
> >>> +if (is_switch(ldp)) {
> >>> +unsigned int reg_index
> >>> += (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) -
> >>> MFF_REG0;
> >>> +int64_t port_id = m->match.flow.regs[reg_index];
> >>> +if (port_id) {
> >>> +int64_t dp_id = lflow->logical_datapath->tunnel_key;
> >>> +char buf[16];
> >>> +snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id,
> >>> port_id);
> >>> +if (!sset_contains(local_lport_ids, buf)) {
> >>> +//VLOG_INFO("Matching on port id %"PRId64" dp
> >>> %"PRId64", is NOT local", port_id, dp_id);
> >>> +continue;
> >>> +} else {
> >>> +//VLOG_INFO("Matching on port id %"PRId64" dp
> >>> %"PRId64", is local", port_id, dp_id);
> >>> +}
> >>> +}
> >>> +}
> >>>  if (!m->n) {
> >>>  ofctrl_add_flow(flow_table, ptable, lflow->priority,
> >>>  lflow->header_.uuid.parts[0], >match,
> >>> );
> >>
> >> I remember the expr_parse_string() is one of the biggest cost in
> >> ovn-controller, so I wonder would it be better to move the check for
> >> local_lport_ids before the parse happens, i.e. check against logical
> flows
> >> instead of ovs flows?
> >
> > Yes, that may be better.  This was just a quick fix for the memory
> > consumption issue.  There is also a small improvement to CPU usage in
> > my basic testing.  I created 1000 ports with ACLs that used a 1000
> > member address set.  Only one port was local.  It finished about 10%
> > faster with the patch.
> >
> > Moving this to before the logical flow to OpenFlow translation would
> > obviously be better, but it's not quite straight forward.  It's easy
> > if you make assumptions about what the expression looks like, but
> > doing it in a general way seems just as complicated as the existing
> > expression parser.
> >
> >> Acked-by: Han Zhou 
> >
> > Thanks!
> >
> > I think I'll do a bit more testing before applying this.  I'm also not
> > sure how far this should be backported.  The memory usage is bad
> > enough with the default OpenStack security groups on large networks
> > that I tend to think this should go back to 2.8 and 2.7 as well.
>
> I ran this patch through OpenStack CI and it works there too.  I
> applied this to master, branch-2.8, and branch-2.7.
>
> I think the next piece that makes sense is looking at improving use of
> conjunctive flows.
>
> --
> Russell Bryant
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

2017-09-19 Thread Russell Bryant

On Tue, Sep 19, 2017 at 9:38 AM, Russell Bryant  wrote:
> On Mon, Sep 18, 2017 at 7:31 PM, Han Zhou  wrote:
>> Thanks Russell for the quick work!
>>
>> On Mon, Sep 18, 2017 at 8:24 AM, Russell Bryant  wrote:
>>
>>> @@ -301,6 +305,22 @@ consider_logical_flow(struct controller_ctx *ctx,
>>>  if (m->match.wc.masks.conj_id) {
>>>  m->match.flow.conj_id += *conj_id_ofs;
>>>  }
>>> +if (is_switch(ldp)) {
>>> +unsigned int reg_index
>>> += (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) -
>>> MFF_REG0;
>>> +int64_t port_id = m->match.flow.regs[reg_index];
>>> +if (port_id) {
>>> +int64_t dp_id = lflow->logical_datapath->tunnel_key;
>>> +char buf[16];
>>> +snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id,
>>> port_id);
>>> +if (!sset_contains(local_lport_ids, buf)) {
>>> +//VLOG_INFO("Matching on port id %"PRId64" dp
>>> %"PRId64", is NOT local", port_id, dp_id);
>>> +continue;
>>> +} else {
>>> +//VLOG_INFO("Matching on port id %"PRId64" dp
>>> %"PRId64", is local", port_id, dp_id);
>>> +}
>>> +}
>>> +}
>>>  if (!m->n) {
>>>  ofctrl_add_flow(flow_table, ptable, lflow->priority,
>>>  lflow->header_.uuid.parts[0], >match,
>>> );
>>
>> I remember the expr_parse_string() is one of the biggest cost in
>> ovn-controller, so I wonder would it be better to move the check for
>> local_lport_ids before the parse happens, i.e. check against logical flows
>> instead of ovs flows?
>
> Yes, that may be better.  This was just a quick fix for the memory
> consumption issue.  There is also a small improvement to CPU usage in
> my basic testing.  I created 1000 ports with ACLs that used a 1000
> member address set.  Only one port was local.  It finished about 10%
> faster with the patch.
>
> Moving this to before the logical flow to OpenFlow translation would
> obviously be better, but it's not quite straight forward.  It's easy
> if you make assumptions about what the expression looks like, but
> doing it in a general way seems just as complicated as the existing
> expression parser.
>
>> Acked-by: Han Zhou 
>
> Thanks!
>
> I think I'll do a bit more testing before applying this.  I'm also not
> sure how far this should be backported.  The memory usage is bad
> enough with the default OpenStack security groups on large networks
> that I tend to think this should go back to 2.8 and 2.7 as well.

I ran this patch through OpenStack CI and it works there too.  I
applied this to master, branch-2.8, and branch-2.7.

I think the next piece that makes sense is looking at improving use of
conjunctive flows.

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

2017-09-19 Thread Russell Bryant

On Mon, Sep 18, 2017 at 7:31 PM, Han Zhou  wrote:
> Thanks Russell for the quick work!
>
> On Mon, Sep 18, 2017 at 8:24 AM, Russell Bryant  wrote:
>
>> @@ -301,6 +305,22 @@ consider_logical_flow(struct controller_ctx *ctx,
>>  if (m->match.wc.masks.conj_id) {
>>  m->match.flow.conj_id += *conj_id_ofs;
>>  }
>> +if (is_switch(ldp)) {
>> +unsigned int reg_index
>> += (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) -
>> MFF_REG0;
>> +int64_t port_id = m->match.flow.regs[reg_index];
>> +if (port_id) {
>> +int64_t dp_id = lflow->logical_datapath->tunnel_key;
>> +char buf[16];
>> +snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id,
>> port_id);
>> +if (!sset_contains(local_lport_ids, buf)) {
>> +//VLOG_INFO("Matching on port id %"PRId64" dp
>> %"PRId64", is NOT local", port_id, dp_id);
>> +continue;
>> +} else {
>> +//VLOG_INFO("Matching on port id %"PRId64" dp
>> %"PRId64", is local", port_id, dp_id);
>> +}
>> +}
>> +}
>>  if (!m->n) {
>>  ofctrl_add_flow(flow_table, ptable, lflow->priority,
>>  lflow->header_.uuid.parts[0], >match,
>> );
>
> I remember the expr_parse_string() is one of the biggest cost in
> ovn-controller, so I wonder would it be better to move the check for
> local_lport_ids before the parse happens, i.e. check against logical flows
> instead of ovs flows?

Yes, that may be better.  This was just a quick fix for the memory
consumption issue.  There is also a small improvement to CPU usage in
my basic testing.  I created 1000 ports with ACLs that used a 1000
member address set.  Only one port was local.  It finished about 10%
faster with the patch.

Moving this to before the logical flow to OpenFlow translation would
obviously be better, but it's not quite straight forward.  It's easy
if you make assumptions about what the expression looks like, but
doing it in a general way seems just as complicated as the existing
expression parser.

> Acked-by: Han Zhou 

Thanks!

I think I'll do a bit more testing before applying this.  I'm also not
sure how far this should be backported.  The memory usage is bad
enough with the default OpenStack security groups on large networks
that I tend to think this should go back to 2.8 and 2.7 as well.

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

2017-09-18 Thread Han Zhou

Thanks Russell for the quick work!

On Mon, Sep 18, 2017 at 8:24 AM, Russell Bryant  wrote:

> @@ -301,6 +305,22 @@ consider_logical_flow(struct controller_ctx *ctx,
>  if (m->match.wc.masks.conj_id) {
>  m->match.flow.conj_id += *conj_id_ofs;
>  }
> +if (is_switch(ldp)) {
> +unsigned int reg_index
> += (ingress ? MFF_LOG_INPORT : MFF_LOG_OUTPORT) -
MFF_REG0;
> +int64_t port_id = m->match.flow.regs[reg_index];
> +if (port_id) {
> +int64_t dp_id = lflow->logical_datapath->tunnel_key;
> +char buf[16];
> +snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64, dp_id,
port_id);
> +if (!sset_contains(local_lport_ids, buf)) {
> +//VLOG_INFO("Matching on port id %"PRId64" dp
%"PRId64", is NOT local", port_id, dp_id);
> +continue;
> +} else {
> +//VLOG_INFO("Matching on port id %"PRId64" dp
%"PRId64", is local", port_id, dp_id);
> +}
> +}
> +}
>  if (!m->n) {
>  ofctrl_add_flow(flow_table, ptable, lflow->priority,
>  lflow->header_.uuid.parts[0], >match,
);

I remember the expr_parse_string() is one of the biggest cost in
ovn-controller, so I wonder would it be better to move the check for
local_lport_ids before the parse happens, i.e. check against logical flows
instead of ovs flows?

Acked-by: Han Zhou 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

2017-09-18 Thread Russell Bryant

On Mon, Sep 18, 2017 at 11:24 AM, Russell Bryant  wrote:
> Discard some OpenFlow flows that will never match.  This includes
> flows that match on a non-local inport in the ingress pipeline or a
> non-local outport in the egress pipeline of a logical switch.
>
> This is most useful for networks with a large number of ports or ACLs
> that use large address sets.
>
> Signed-off-by: Russell Bryant 
> Tested-by: Miguel Angel Ajo Pelayo 
> ---
>  ovn/controller/binding.c| 29 +
>  ovn/controller/binding.h|  2 +-
>  ovn/controller/lflow.c  | 33 +++--
>  ovn/controller/lflow.h  |  3 ++-
>  ovn/controller/ovn-controller.c |  9 +++--
>  5 files changed, 62 insertions(+), 14 deletions(-)
>
> diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
> index ca1d43395..3532c6014 100644
> --- a/ovn/controller/binding.c
> +++ b/ovn/controller/binding.c
> @@ -371,6 +371,17 @@ setup_qos(const char *egress_iface, struct hmap 
> *queue_map)
>  }
>
>  static void
> +update_local_lport_ids(struct sset *local_lport_ids,
> +   const struct sbrec_port_binding *binding_rec)
> +{
> +char buf[16];
> +snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
> + binding_rec->datapath->tunnel_key,
> + binding_rec->tunnel_key);
> +sset_add(local_lport_ids, buf);
> +}
> +
> +static void
>  consider_local_datapath(struct controller_ctx *ctx,
>  const struct chassis_index *chassis_index,
>  struct sset *active_tunnels,
> @@ -379,7 +390,8 @@ consider_local_datapath(struct controller_ctx *ctx,
>  struct hmap *qos_map,
>  struct hmap *local_datapaths,
>  struct shash *lport_to_iface,
> -struct sset *local_lports)
> +struct sset *local_lports,
> +struct sset *local_lport_ids)
>  {
>  const struct ovsrec_interface *iface_rec
>  = shash_find_data(lport_to_iface, binding_rec->logical_port);
> @@ -399,7 +411,7 @@ consider_local_datapath(struct controller_ctx *ctx,
>  get_qos_params(binding_rec, qos_map);
>  }
>  /* This port is in our chassis unless it is a localport. */
> -   if (strcmp(binding_rec->type, "localport")) {
> +if (strcmp(binding_rec->type, "localport")) {
>  our_chassis = true;
>  }
>  } else if (!strcmp(binding_rec->type, "l2gateway")) {
> @@ -439,6 +451,14 @@ consider_local_datapath(struct controller_ctx *ctx,
>  our_chassis = false;
>  }
>
> +if (our_chassis
> +|| !strcmp(binding_rec->type, "patch")
> +|| !strcmp(binding_rec->type, "localport")
> +|| !strcmp(binding_rec->type, "vtep")
> +|| !strcmp(binding_rec->type, "localnet")) {
> +update_local_lport_ids(local_lport_ids, binding_rec);
> +}
> +
>  if (ctx->ovnsb_idl_txn) {
>  const char *vif_chassis = smap_get(_rec->options,
> "requested-chassis");
> @@ -508,7 +528,8 @@ binding_run(struct controller_ctx *ctx, const struct 
> ovsrec_bridge *br_int,
>  const struct sbrec_chassis *chassis_rec,
>  const struct chassis_index *chassis_index,
>  struct sset *active_tunnels,
> -struct hmap *local_datapaths, struct sset *local_lports)
> +struct hmap *local_datapaths, struct sset *local_lports,
> +struct sset *local_lport_ids)
>  {
>  if (!chassis_rec) {
>  return;
> @@ -533,7 +554,7 @@ binding_run(struct controller_ctx *ctx, const struct 
> ovsrec_bridge *br_int,
>  active_tunnels, chassis_rec, binding_rec,
>  sset_is_empty(_ifaces) ? NULL :
>  _map, local_datapaths, _to_iface,
> -local_lports);
> +local_lports, local_lport_ids);
>
>  }
>
> diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h
> index c78f8d932..89fc2ec8f 100644
> --- a/ovn/controller/binding.h
> +++ b/ovn/controller/binding.h
> @@ -32,7 +32,7 @@ void binding_run(struct controller_ctx *, const struct 
> ovsrec_bridge *br_int,
>   const struct sbrec_chassis *,
>   const struct chassis_index *,
>   struct sset *active_tunnels, struct hmap *local_datapaths,
> - struct sset *all_lports);
> + struct sset *local_lports, struct sset *local_lport_ids);
>  bool binding_cleanup(struct controller_ctx *, const struct sbrec_chassis *);
>
>  #endif /* ovn/binding.h */
> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> index 6d9f02cb2..c1f5de2ab 100644
> --- a/ovn/controller/lflow.c
> +++

[ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

2017-09-18 Thread Russell Bryant

Discard some OpenFlow flows that will never match.  This includes
flows that match on a non-local inport in the ingress pipeline or a
non-local outport in the egress pipeline of a logical switch.

This is most useful for networks with a large number of ports or ACLs
that use large address sets.

Signed-off-by: Russell Bryant 
Tested-by: Miguel Angel Ajo Pelayo 
---
 ovn/controller/binding.c| 29 +
 ovn/controller/binding.h|  2 +-
 ovn/controller/lflow.c  | 33 +++--
 ovn/controller/lflow.h  |  3 ++-
 ovn/controller/ovn-controller.c |  9 +++--
 5 files changed, 62 insertions(+), 14 deletions(-)

diff --git a/ovn/controller/binding.c b/ovn/controller/binding.c
index ca1d43395..3532c6014 100644
--- a/ovn/controller/binding.c
+++ b/ovn/controller/binding.c
@@ -371,6 +371,17 @@ setup_qos(const char *egress_iface, struct hmap *queue_map)
 }
 
 static void
+update_local_lport_ids(struct sset *local_lport_ids,
+   const struct sbrec_port_binding *binding_rec)
+{
+char buf[16];
+snprintf(buf, sizeof(buf), "%"PRId64"_%"PRId64,
+ binding_rec->datapath->tunnel_key,
+ binding_rec->tunnel_key);
+sset_add(local_lport_ids, buf);
+}
+
+static void
 consider_local_datapath(struct controller_ctx *ctx,
 const struct chassis_index *chassis_index,
 struct sset *active_tunnels,
@@ -379,7 +390,8 @@ consider_local_datapath(struct controller_ctx *ctx,
 struct hmap *qos_map,
 struct hmap *local_datapaths,
 struct shash *lport_to_iface,
-struct sset *local_lports)
+struct sset *local_lports,
+struct sset *local_lport_ids)
 {
 const struct ovsrec_interface *iface_rec
 = shash_find_data(lport_to_iface, binding_rec->logical_port);
@@ -399,7 +411,7 @@ consider_local_datapath(struct controller_ctx *ctx,
 get_qos_params(binding_rec, qos_map);
 }
 /* This port is in our chassis unless it is a localport. */
-   if (strcmp(binding_rec->type, "localport")) {
+if (strcmp(binding_rec->type, "localport")) {
 our_chassis = true;
 }
 } else if (!strcmp(binding_rec->type, "l2gateway")) {
@@ -439,6 +451,14 @@ consider_local_datapath(struct controller_ctx *ctx,
 our_chassis = false;
 }
 
+if (our_chassis
+|| !strcmp(binding_rec->type, "patch")
+|| !strcmp(binding_rec->type, "localport")
+|| !strcmp(binding_rec->type, "vtep")
+|| !strcmp(binding_rec->type, "localnet")) {
+update_local_lport_ids(local_lport_ids, binding_rec);
+}
+
 if (ctx->ovnsb_idl_txn) {
 const char *vif_chassis = smap_get(_rec->options,
"requested-chassis");
@@ -508,7 +528,8 @@ binding_run(struct controller_ctx *ctx, const struct 
ovsrec_bridge *br_int,
 const struct sbrec_chassis *chassis_rec,
 const struct chassis_index *chassis_index,
 struct sset *active_tunnels,
-struct hmap *local_datapaths, struct sset *local_lports)
+struct hmap *local_datapaths, struct sset *local_lports,
+struct sset *local_lport_ids)
 {
 if (!chassis_rec) {
 return;
@@ -533,7 +554,7 @@ binding_run(struct controller_ctx *ctx, const struct 
ovsrec_bridge *br_int,
 active_tunnels, chassis_rec, binding_rec,
 sset_is_empty(_ifaces) ? NULL :
 _map, local_datapaths, _to_iface,
-local_lports);
+local_lports, local_lport_ids);
 
 }
 
diff --git a/ovn/controller/binding.h b/ovn/controller/binding.h
index c78f8d932..89fc2ec8f 100644
--- a/ovn/controller/binding.h
+++ b/ovn/controller/binding.h
@@ -32,7 +32,7 @@ void binding_run(struct controller_ctx *, const struct 
ovsrec_bridge *br_int,
  const struct sbrec_chassis *,
  const struct chassis_index *,
  struct sset *active_tunnels, struct hmap *local_datapaths,
- struct sset *all_lports);
+ struct sset *local_lports, struct sset *local_lport_ids);
 bool binding_cleanup(struct controller_ctx *, const struct sbrec_chassis *);
 
 #endif /* ovn/binding.h */
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 6d9f02cb2..c1f5de2ab 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -68,7 +68,8 @@ static void consider_logical_flow(struct controller_ctx *ctx,
   uint32_t *conj_id_ofs,
   const struct shash *addr_sets,
   struct hmap *flow_table,
-

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

Re: [ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

[ovs-dev] [PATCH] ovn: Discard flows for non-local ports.

6 matches

Site Navigation

Mail list logo

Footer information