Re: [ovs-dev] [RFC PATCH 1/1]: Enhance conjunctive match support in OVN

2018-02-09 Thread Numan Siddique 
On Feb 9, 2018 11:19 PM, "Ben Pfaff"  wrote:

Oh, and Numan, would you mind including these results or a least a link
to them in the archives in the next version of the patch?  It's OK if
they aren't completely up-to-date, the idea is to make it clear from
someone reading the commit log later that there was a big benefit.


Yeah. Sure. I will do that.

Thanks Mark for testing it out and sharing the results.

Thanks
Numan


On Fri, Feb 09, 2018 at 09:41:57AM -0800, Ben Pfaff wrote:
> Those are fantastic results, thanks for passing them along!
>
> On Thu, Feb 08, 2018 at 05:11:26PM -0600, Mark Michelson wrote:
> > Hi Numan,
> >
> > I did a test with this where I created two address sets with ~1000
addresses
> > in them. Then I created a series of ACLs that used these two address
sets in
> > ACLs like so:
> >
> > ovn-nbctl acl-add ls0 to-lport 1000 "outport == \"lsp${COUNT}\" &&
ip4.src
> > == \$set1 && ip4.dst == \$set2" allow
> >
> > The ${COUNT} variable would increase with each loop iteration.
> >
> > Without your patch, after adding 3 ACLs, it took multiple minutes to
add the
> > next ACL and ground my system to a halt.
> >
> > With your patch, I was able to add hundreds of ACLs. While I did see
> > processing slow down, it was several orders of magnitude better than
without
> > your patch.
> >
> > It makes sense. Without your patch, each ACL results in ~1 million flows
> > being generated. With your patch, each ACL results in ~2000 flows being
> > generated.
> >
> > Definitely an improvement! If you make a v2, I'll test it as well.
> >
> > Tested-by: Mark Michelson 
> >
> >
> > On 02/02/2018 09:36 AM, nusid...@redhat.com wrote:
> > >From: Numan Siddique 
> > >
> > >Presently, if a logical flow has possible conjunctive matches, OVN
expression
> > >parser has support for that. But certain fields like ip4.src, ip4.dst
are not
> > >considered as dimensions in the conjunctive matches.
> > >
> > >In order to support all the possible fields as dimensions, this patch
has added
> > >a new expression type 'EXPR_T_CONJ'. After a expression is simplified
by
> > >expr_simplify(), before calling expr_normalize(), a new function
> > >expr_eval_conj() is called, which evaluates for possible dimensions for
> > >conjunctive matches.
> > >
> > >For example if the match expression is
> > >"ip4 && ip4.src == {10.0.0.4, 10.0.0.5, 10.0.0.6} &&
> > >  ip4.dst == {20.0.0.4, 20.0.0.5, 20.0.0.6}"
> > >
> > >expr_simplify() would have generated the expression as -
> > >
> > >AND(CMP(IP4),
> > > OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> > > CMP(ip4.src == 10.0.0.6)),
> > > OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.src == 20.0.0.5),
> > > CMP(ip4.src == 20.0.0.6))).
> > >
> > >expr_eval_conj() would return a new expression something like
> > >
> > >CONJ(AND(CMP(IP4),
> > >  OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> > >  CMP(ip4.src == 10.0.0.6))),
> > >  AND(CMP(IP4),
> > >  OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.dst == 20.0.0.5),
> > >  CMP(ip4.dst == 20.0.0.6
> > >
> > >expr_normalize() would normalize each individual 'AND' clause in the
CONJ and
> > >expr_to_matches() would add the necessary conjunctive matches.
> > >
> > >TODO: If the proposed approach is reasonable, then test cases and
necessary
> > >code comments needs to be added.
> > >
> > >Signed-off-by: Numan Siddique 
> > >---
> > >  include/ovn/expr.h |   7 ++-
> > >  ovn/controller/lflow.c |   2 +
> > >  ovn/lib/expr.c | 166 ++
+++
> > >  tests/test-ovn.c   |   2 +-
> > >  4 files changed, 175 insertions(+), 2 deletions(-)
> > >
> > >diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> > >index 711713e08..4259e5938 100644
> > >--- a/include/ovn/expr.h
> > >+++ b/include/ovn/expr.h
> > >@@ -295,6 +295,7 @@ enum expr_type {
> > >  EXPR_T_CONDITION,   /* Conditional to be evaluated in the
> > >   * controller during expr_simplify(),
> > >   * prior to constructing OpenFlow
matches. */
> > >+EXPR_T_CONJ,
> > >  };
> > >  /* Expression condition type. */
> > >@@ -366,12 +367,16 @@ struct expr {
> > >  /* XXX Should arguments for conditions be generic? */
> > >  char *string;
> > >  } cond;
> > >+
> > >+/* EXPR_T_CONJ. */
> > >+struct ovs_list conj;
> > >  };
> > >  };
> > >  struct expr *expr_create_boolean(bool b);
> > >  struct expr *expr_create_andor(enum expr_type);
> > >  struct expr *expr_combine(enum expr_type, struct expr *a, struct
expr *b);
> > >+struct expr *expr_create_conj(enum expr_type);
> > >  static inline struct expr *
> > >  expr_from_node(const struct ovs_list *node)
> > >@@ -500,5 +505,5 @@ void expr_addr_sets_add(struct shash *addr_sets,
const char *name,
> > >

Re: [ovs-dev] [RFC PATCH 1/1]: Enhance conjunctive match support in OVN

2018-02-09 Thread Ben Pfaff 
Those are fantastic results, thanks for passing them along!

On Thu, Feb 08, 2018 at 05:11:26PM -0600, Mark Michelson wrote:
> Hi Numan,
> 
> I did a test with this where I created two address sets with ~1000 addresses
> in them. Then I created a series of ACLs that used these two address sets in
> ACLs like so:
> 
> ovn-nbctl acl-add ls0 to-lport 1000 "outport == \"lsp${COUNT}\" && ip4.src
> == \$set1 && ip4.dst == \$set2" allow
> 
> The ${COUNT} variable would increase with each loop iteration.
> 
> Without your patch, after adding 3 ACLs, it took multiple minutes to add the
> next ACL and ground my system to a halt.
> 
> With your patch, I was able to add hundreds of ACLs. While I did see
> processing slow down, it was several orders of magnitude better than without
> your patch.
> 
> It makes sense. Without your patch, each ACL results in ~1 million flows
> being generated. With your patch, each ACL results in ~2000 flows being
> generated.
> 
> Definitely an improvement! If you make a v2, I'll test it as well.
> 
> Tested-by: Mark Michelson 
> 
> 
> On 02/02/2018 09:36 AM, nusid...@redhat.com wrote:
> >From: Numan Siddique 
> >
> >Presently, if a logical flow has possible conjunctive matches, OVN expression
> >parser has support for that. But certain fields like ip4.src, ip4.dst are not
> >considered as dimensions in the conjunctive matches.
> >
> >In order to support all the possible fields as dimensions, this patch has 
> >added
> >a new expression type 'EXPR_T_CONJ'. After a expression is simplified by
> >expr_simplify(), before calling expr_normalize(), a new function
> >expr_eval_conj() is called, which evaluates for possible dimensions for
> >conjunctive matches.
> >
> >For example if the match expression is
> >"ip4 && ip4.src == {10.0.0.4, 10.0.0.5, 10.0.0.6} &&
> >  ip4.dst == {20.0.0.4, 20.0.0.5, 20.0.0.6}"
> >
> >expr_simplify() would have generated the expression as -
> >
> >AND(CMP(IP4),
> > OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> > CMP(ip4.src == 10.0.0.6)),
> > OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.src == 20.0.0.5),
> > CMP(ip4.src == 20.0.0.6))).
> >
> >expr_eval_conj() would return a new expression something like
> >
> >CONJ(AND(CMP(IP4),
> >  OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> >  CMP(ip4.src == 10.0.0.6))),
> >  AND(CMP(IP4),
> >  OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.dst == 20.0.0.5),
> >  CMP(ip4.dst == 20.0.0.6
> >
> >expr_normalize() would normalize each individual 'AND' clause in the CONJ and
> >expr_to_matches() would add the necessary conjunctive matches.
> >
> >TODO: If the proposed approach is reasonable, then test cases and necessary
> >code comments needs to be added.
> >
> >Signed-off-by: Numan Siddique 
> >---
> >  include/ovn/expr.h |   7 ++-
> >  ovn/controller/lflow.c |   2 +
> >  ovn/lib/expr.c | 166 
> > +
> >  tests/test-ovn.c   |   2 +-
> >  4 files changed, 175 insertions(+), 2 deletions(-)
> >
> >diff --git a/include/ovn/expr.h b/include/ovn/expr.h
> >index 711713e08..4259e5938 100644
> >--- a/include/ovn/expr.h
> >+++ b/include/ovn/expr.h
> >@@ -295,6 +295,7 @@ enum expr_type {
> >  EXPR_T_CONDITION,   /* Conditional to be evaluated in the
> >   * controller during expr_simplify(),
> >   * prior to constructing OpenFlow matches. 
> > */
> >+EXPR_T_CONJ,
> >  };
> >  /* Expression condition type. */
> >@@ -366,12 +367,16 @@ struct expr {
> >  /* XXX Should arguments for conditions be generic? */
> >  char *string;
> >  } cond;
> >+
> >+/* EXPR_T_CONJ. */
> >+struct ovs_list conj;
> >  };
> >  };
> >  struct expr *expr_create_boolean(bool b);
> >  struct expr *expr_create_andor(enum expr_type);
> >  struct expr *expr_combine(enum expr_type, struct expr *a, struct expr *b);
> >+struct expr *expr_create_conj(enum expr_type);
> >  static inline struct expr *
> >  expr_from_node(const struct ovs_list *node)
> >@@ -500,5 +505,5 @@ void expr_addr_sets_add(struct shash *addr_sets, const 
> >char *name,
> >  const char * const *values, size_t n_values);
> >  void expr_addr_sets_remove(struct shash *addr_sets, const char *name);
> >  void expr_addr_sets_destroy(struct shash *addr_sets);
> >-
> >+struct expr *expr_eval_conj(struct expr *expr);
> >  #endif /* ovn/expr.h */
> >diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
> >index 1e79a5355..13413c77d 100644
> >--- a/ovn/controller/lflow.c
> >+++ b/ovn/controller/lflow.c
> >@@ -289,6 +289,7 @@ consider_logical_flow(struct controller_ctx *ctx,
> >  expr = expr_combine(EXPR_T_AND, expr, prereqs);
> >  prereqs = NULL;
> >  }
> >+
> >  expr = expr_annotate(expr, , );
> >  }
> >

Re: [ovs-dev] [RFC PATCH 1/1]: Enhance conjunctive match support in OVN

2018-02-08 Thread Mark Michelson 

Hi Numan,

I did a test with this where I created two address sets with ~1000 
addresses in them. Then I created a series of ACLs that used these two 
address sets in ACLs like so:


ovn-nbctl acl-add ls0 to-lport 1000 "outport == \"lsp${COUNT}\" && 
ip4.src == \$set1 && ip4.dst == \$set2" allow


The ${COUNT} variable would increase with each loop iteration.

Without your patch, after adding 3 ACLs, it took multiple minutes to add 
the next ACL and ground my system to a halt.


With your patch, I was able to add hundreds of ACLs. While I did see 
processing slow down, it was several orders of magnitude better than 
without your patch.


It makes sense. Without your patch, each ACL results in ~1 million flows 
being generated. With your patch, each ACL results in ~2000 flows being 
generated.


Definitely an improvement! If you make a v2, I'll test it as well.

Tested-by: Mark Michelson 


On 02/02/2018 09:36 AM, nusid...@redhat.com wrote:

From: Numan Siddique 

Presently, if a logical flow has possible conjunctive matches, OVN expression
parser has support for that. But certain fields like ip4.src, ip4.dst are not
considered as dimensions in the conjunctive matches.

In order to support all the possible fields as dimensions, this patch has added
a new expression type 'EXPR_T_CONJ'. After a expression is simplified by
expr_simplify(), before calling expr_normalize(), a new function
expr_eval_conj() is called, which evaluates for possible dimensions for
conjunctive matches.

For example if the match expression is
"ip4 && ip4.src == {10.0.0.4, 10.0.0.5, 10.0.0.6} &&
  ip4.dst == {20.0.0.4, 20.0.0.5, 20.0.0.6}"

expr_simplify() would have generated the expression as -

AND(CMP(IP4),
 OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
 CMP(ip4.src == 10.0.0.6)),
 OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.src == 20.0.0.5),
 CMP(ip4.src == 20.0.0.6))).

expr_eval_conj() would return a new expression something like

CONJ(AND(CMP(IP4),
  OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
  CMP(ip4.src == 10.0.0.6))),
  AND(CMP(IP4),
  OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.dst == 20.0.0.5),
  CMP(ip4.dst == 20.0.0.6

expr_normalize() would normalize each individual 'AND' clause in the CONJ and
expr_to_matches() would add the necessary conjunctive matches.

TODO: If the proposed approach is reasonable, then test cases and necessary
code comments needs to be added.

Signed-off-by: Numan Siddique 
---
  include/ovn/expr.h |   7 ++-
  ovn/controller/lflow.c |   2 +
  ovn/lib/expr.c | 166 +
  tests/test-ovn.c   |   2 +-
  4 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index 711713e08..4259e5938 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -295,6 +295,7 @@ enum expr_type {
  EXPR_T_CONDITION,   /* Conditional to be evaluated in the
   * controller during expr_simplify(),
   * prior to constructing OpenFlow matches. */
+EXPR_T_CONJ,
  };
  
  /* Expression condition type. */

@@ -366,12 +367,16 @@ struct expr {
  /* XXX Should arguments for conditions be generic? */
  char *string;
  } cond;
+
+/* EXPR_T_CONJ. */
+struct ovs_list conj;
  };
  };
  
  struct expr *expr_create_boolean(bool b);

  struct expr *expr_create_andor(enum expr_type);
  struct expr *expr_combine(enum expr_type, struct expr *a, struct expr *b);
+struct expr *expr_create_conj(enum expr_type);
  
  static inline struct expr *

  expr_from_node(const struct ovs_list *node)
@@ -500,5 +505,5 @@ void expr_addr_sets_add(struct shash *addr_sets, const char 
*name,
  const char * const *values, size_t n_values);
  void expr_addr_sets_remove(struct shash *addr_sets, const char *name);
  void expr_addr_sets_destroy(struct shash *addr_sets);
-
+struct expr *expr_eval_conj(struct expr *expr);
  #endif /* ovn/expr.h */
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 1e79a5355..13413c77d 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -289,6 +289,7 @@ consider_logical_flow(struct controller_ctx *ctx,
  expr = expr_combine(EXPR_T_AND, expr, prereqs);
  prereqs = NULL;
  }
+
  expr = expr_annotate(expr, , );
  }
  if (error) {
@@ -304,6 +305,7 @@ consider_logical_flow(struct controller_ctx *ctx,
  struct condition_aux cond_aux = { ctx->ovnsb_idl, chassis, active_tunnels,
chassis_index};
  expr = expr_simplify(expr, is_chassis_resident_cb, _aux);
+expr = expr_eval_conj(expr);
  expr = expr_normalize(expr);
  uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, ,

Re: [ovs-dev] [RFC PATCH 1/1]: Enhance conjunctive match support in OVN

2018-02-06 Thread Numan Siddique 
On Feb 7, 2018 1:06 AM, "Ben Pfaff"  wrote:

On Wed, Feb 07, 2018 at 12:12:38AM +0530, Numan Siddique wrote:
> On Tue, Feb 6, 2018 at 11:31 PM, Ben Pfaff  wrote:
>
> > On Fri, Feb 02, 2018 at 09:06:39PM +0530, nusid...@redhat.com wrote:
> > > From: Numan Siddique 
> > >
> > > Presently, if a logical flow has possible conjunctive matches, OVN
> > expression
> > > parser has support for that. But certain fields like ip4.src, ip4.dst
> > are not
> > > considered as dimensions in the conjunctive matches.
> > >
> > > In order to support all the possible fields as dimensions, this patch
> > has added
> > > a new expression type 'EXPR_T_CONJ'. After a expression is simplified
by
> > > expr_simplify(), before calling expr_normalize(), a new function
> > > expr_eval_conj() is called, which evaluates for possible dimensions
for
> > > conjunctive matches.
> > >
> > > For example if the match expression is
> > > "ip4 && ip4.src == {10.0.0.4, 10.0.0.5, 10.0.0.6} &&
> > >  ip4.dst == {20.0.0.4, 20.0.0.5, 20.0.0.6}"
> > >
> > > expr_simplify() would have generated the expression as -
> > >
> > > AND(CMP(IP4),
> > > OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> > > CMP(ip4.src == 10.0.0.6)),
> > > OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.src == 20.0.0.5),
> > > CMP(ip4.src == 20.0.0.6))).
> > >
> > > expr_eval_conj() would return a new expression something like
> > >
> > > CONJ(AND(CMP(IP4),
> > >  OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> > >  CMP(ip4.src == 10.0.0.6))),
> > >  AND(CMP(IP4),
> > >  OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.dst == 20.0.0.5),
> > >  CMP(ip4.dst == 20.0.0.6
> > >
> > > expr_normalize() would normalize each individual 'AND' clause in the
> > CONJ and
> > > expr_to_matches() would add the necessary conjunctive matches.
> > >
> > > TODO: If the proposed approach is reasonable, then test cases and
> > necessary
> > > code comments needs to be added.
> >
> > I think I like this approach, but I also think that it's worthwhile
> > trying to figure out whether it's possible to do it without adding the
> > extra EXPR_T_CONJ type and the extra processing step.  I started playing
> > with that idea yesterday.  I think it's possible, but I didn't actually
> > finish implementing it.
> >
> >
> That's great. Looking forward for those patches whenever they are ready
:).

Oh, I didn't want to take over the series from you, I'm suggesting a
possible direction.  Is that a direction you could try to go?


Ok. Got it. I will try to explore in that direction.

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

Re: [ovs-dev] [RFC PATCH 1/1]: Enhance conjunctive match support in OVN

2018-02-06 Thread Ben Pfaff 
On Wed, Feb 07, 2018 at 12:12:38AM +0530, Numan Siddique wrote:
> On Tue, Feb 6, 2018 at 11:31 PM, Ben Pfaff  wrote:
> 
> > On Fri, Feb 02, 2018 at 09:06:39PM +0530, nusid...@redhat.com wrote:
> > > From: Numan Siddique 
> > >
> > > Presently, if a logical flow has possible conjunctive matches, OVN
> > expression
> > > parser has support for that. But certain fields like ip4.src, ip4.dst
> > are not
> > > considered as dimensions in the conjunctive matches.
> > >
> > > In order to support all the possible fields as dimensions, this patch
> > has added
> > > a new expression type 'EXPR_T_CONJ'. After a expression is simplified by
> > > expr_simplify(), before calling expr_normalize(), a new function
> > > expr_eval_conj() is called, which evaluates for possible dimensions for
> > > conjunctive matches.
> > >
> > > For example if the match expression is
> > > "ip4 && ip4.src == {10.0.0.4, 10.0.0.5, 10.0.0.6} &&
> > >  ip4.dst == {20.0.0.4, 20.0.0.5, 20.0.0.6}"
> > >
> > > expr_simplify() would have generated the expression as -
> > >
> > > AND(CMP(IP4),
> > > OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> > > CMP(ip4.src == 10.0.0.6)),
> > > OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.src == 20.0.0.5),
> > > CMP(ip4.src == 20.0.0.6))).
> > >
> > > expr_eval_conj() would return a new expression something like
> > >
> > > CONJ(AND(CMP(IP4),
> > >  OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> > >  CMP(ip4.src == 10.0.0.6))),
> > >  AND(CMP(IP4),
> > >  OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.dst == 20.0.0.5),
> > >  CMP(ip4.dst == 20.0.0.6
> > >
> > > expr_normalize() would normalize each individual 'AND' clause in the
> > CONJ and
> > > expr_to_matches() would add the necessary conjunctive matches.
> > >
> > > TODO: If the proposed approach is reasonable, then test cases and
> > necessary
> > > code comments needs to be added.
> >
> > I think I like this approach, but I also think that it's worthwhile
> > trying to figure out whether it's possible to do it without adding the
> > extra EXPR_T_CONJ type and the extra processing step.  I started playing
> > with that idea yesterday.  I think it's possible, but I didn't actually
> > finish implementing it.
> >
> >
> That's great. Looking forward for those patches whenever they are ready :).

Oh, I didn't want to take over the series from you, I'm suggesting a
possible direction.  Is that a direction you could try to go?
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [RFC PATCH 1/1]: Enhance conjunctive match support in OVN

2018-02-06 Thread Numan Siddique 
On Tue, Feb 6, 2018 at 11:31 PM, Ben Pfaff  wrote:

> On Fri, Feb 02, 2018 at 09:06:39PM +0530, nusid...@redhat.com wrote:
> > From: Numan Siddique 
> >
> > Presently, if a logical flow has possible conjunctive matches, OVN
> expression
> > parser has support for that. But certain fields like ip4.src, ip4.dst
> are not
> > considered as dimensions in the conjunctive matches.
> >
> > In order to support all the possible fields as dimensions, this patch
> has added
> > a new expression type 'EXPR_T_CONJ'. After a expression is simplified by
> > expr_simplify(), before calling expr_normalize(), a new function
> > expr_eval_conj() is called, which evaluates for possible dimensions for
> > conjunctive matches.
> >
> > For example if the match expression is
> > "ip4 && ip4.src == {10.0.0.4, 10.0.0.5, 10.0.0.6} &&
> >  ip4.dst == {20.0.0.4, 20.0.0.5, 20.0.0.6}"
> >
> > expr_simplify() would have generated the expression as -
> >
> > AND(CMP(IP4),
> > OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> > CMP(ip4.src == 10.0.0.6)),
> > OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.src == 20.0.0.5),
> > CMP(ip4.src == 20.0.0.6))).
> >
> > expr_eval_conj() would return a new expression something like
> >
> > CONJ(AND(CMP(IP4),
> >  OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
> >  CMP(ip4.src == 10.0.0.6))),
> >  AND(CMP(IP4),
> >  OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.dst == 20.0.0.5),
> >  CMP(ip4.dst == 20.0.0.6
> >
> > expr_normalize() would normalize each individual 'AND' clause in the
> CONJ and
> > expr_to_matches() would add the necessary conjunctive matches.
> >
> > TODO: If the proposed approach is reasonable, then test cases and
> necessary
> > code comments needs to be added.
>
> I think I like this approach, but I also think that it's worthwhile
> trying to figure out whether it's possible to do it without adding the
> extra EXPR_T_CONJ type and the extra processing step.  I started playing
> with that idea yesterday.  I think it's possible, but I didn't actually
> finish implementing it.
>
>
That's great. Looking forward for those patches whenever they are ready :).




> I did come up with a few independent patches to improve the expr code,
> though.  Would you take a look?  They are at:
> https://patchwork.ozlabs.org/project/openvswitch/list/?
> series=27221


Thanks Ben. I will try out these patches tomorrow.

Thanks
Numan



>
> This patch causes one warning from Clang (and I think it's right that
> this code is fishy):
>
> ../ovn/lib/expr.c:1470:16: error: implicit conversion from enumeration
> type 'enum expr_type' to different enumeration type 'enum expr_level'
> [-Werror,-Wenum-conversion]
>
> Thanks,
>
> Ben.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [RFC PATCH 1/1]: Enhance conjunctive match support in OVN

2018-02-02 Thread nusiddiq 
From: Numan Siddique 

Presently, if a logical flow has possible conjunctive matches, OVN expression
parser has support for that. But certain fields like ip4.src, ip4.dst are not
considered as dimensions in the conjunctive matches.

In order to support all the possible fields as dimensions, this patch has added
a new expression type 'EXPR_T_CONJ'. After a expression is simplified by
expr_simplify(), before calling expr_normalize(), a new function
expr_eval_conj() is called, which evaluates for possible dimensions for
conjunctive matches.

For example if the match expression is
"ip4 && ip4.src == {10.0.0.4, 10.0.0.5, 10.0.0.6} &&
 ip4.dst == {20.0.0.4, 20.0.0.5, 20.0.0.6}"

expr_simplify() would have generated the expression as -

AND(CMP(IP4),
OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
CMP(ip4.src == 10.0.0.6)),
OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.src == 20.0.0.5),
CMP(ip4.src == 20.0.0.6))).

expr_eval_conj() would return a new expression something like

CONJ(AND(CMP(IP4),
 OR((CMP(ip4.src == 10.0.0.4), CMP(ip4.src == 10.0.0.5),
 CMP(ip4.src == 10.0.0.6))),
 AND(CMP(IP4),
 OR((CMP(ip4.dst == 20.0.0.4), CMP(ip4.dst == 20.0.0.5),
 CMP(ip4.dst == 20.0.0.6

expr_normalize() would normalize each individual 'AND' clause in the CONJ and
expr_to_matches() would add the necessary conjunctive matches.

TODO: If the proposed approach is reasonable, then test cases and necessary
code comments needs to be added.

Signed-off-by: Numan Siddique 
---
 include/ovn/expr.h |   7 ++-
 ovn/controller/lflow.c |   2 +
 ovn/lib/expr.c | 166 +
 tests/test-ovn.c   |   2 +-
 4 files changed, 175 insertions(+), 2 deletions(-)

diff --git a/include/ovn/expr.h b/include/ovn/expr.h
index 711713e08..4259e5938 100644
--- a/include/ovn/expr.h
+++ b/include/ovn/expr.h
@@ -295,6 +295,7 @@ enum expr_type {
 EXPR_T_CONDITION,   /* Conditional to be evaluated in the
  * controller during expr_simplify(),
  * prior to constructing OpenFlow matches. */
+EXPR_T_CONJ,
 };
 
 /* Expression condition type. */
@@ -366,12 +367,16 @@ struct expr {
 /* XXX Should arguments for conditions be generic? */
 char *string;
 } cond;
+
+/* EXPR_T_CONJ. */
+struct ovs_list conj;
 };
 };
 
 struct expr *expr_create_boolean(bool b);
 struct expr *expr_create_andor(enum expr_type);
 struct expr *expr_combine(enum expr_type, struct expr *a, struct expr *b);
+struct expr *expr_create_conj(enum expr_type);
 
 static inline struct expr *
 expr_from_node(const struct ovs_list *node)
@@ -500,5 +505,5 @@ void expr_addr_sets_add(struct shash *addr_sets, const char 
*name,
 const char * const *values, size_t n_values);
 void expr_addr_sets_remove(struct shash *addr_sets, const char *name);
 void expr_addr_sets_destroy(struct shash *addr_sets);
-
+struct expr *expr_eval_conj(struct expr *expr);
 #endif /* ovn/expr.h */
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 1e79a5355..13413c77d 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -289,6 +289,7 @@ consider_logical_flow(struct controller_ctx *ctx,
 expr = expr_combine(EXPR_T_AND, expr, prereqs);
 prereqs = NULL;
 }
+
 expr = expr_annotate(expr, , );
 }
 if (error) {
@@ -304,6 +305,7 @@ consider_logical_flow(struct controller_ctx *ctx,
 struct condition_aux cond_aux = { ctx->ovnsb_idl, chassis, active_tunnels,
   chassis_index};
 expr = expr_simplify(expr, is_chassis_resident_cb, _aux);
+expr = expr_eval_conj(expr);
 expr = expr_normalize(expr);
 uint32_t n_conjs = expr_to_matches(expr, lookup_port_cb, ,
);
diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c
index 79ff45762..3d4c68dd8 100644
--- a/ovn/lib/expr.c
+++ b/ovn/lib/expr.c
@@ -152,6 +152,14 @@ expr_create_andor(enum expr_type type)
 return e;
 }
 
+struct expr *
+expr_create_conj(enum expr_type type)
+{
+struct expr *e = xmalloc(sizeof *e);
+e->type = type;
+ovs_list_init(>conj);
+return e;
+}
 /* Returns a logical AND or OR expression (according to 'type', which must be
  * EXPR_T_AND or EXPR_T_OR) whose sub-expressions are 'a' and 'b', with some
  * flexibility:
@@ -238,6 +246,7 @@ expr_not(struct expr *expr)
 expr->cond.not = !expr->cond.not;
 break;
 
+case EXPR_T_CONJ:
 default:
 OVS_NOT_REACHED();
 }
@@ -298,6 +307,7 @@ expr_fix(struct expr *expr)
 case EXPR_T_CONDITION:
 return expr;
 
+case EXPR_T_CONJ:
 default:
 OVS_NOT_REACHED();
 }
@@ -442,6 +452,9 @@ expr_format(const struct expr *e, struct ds *s)
 case EXPR_T_CONDITION: