subject:"\[ovs\-dev\] \[PATCH 2\/3\] Combine conjunctions with identical matches into one flow."

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

2019-10-28 Thread Mark Michelson

I pushed the first two patches of the series to master. I'll resubmit 
patch three as a separate submission.


On 10/28/19 1:45 PM, Numan Siddique wrote:



On Mon, Oct 28, 2019, 9:54 PM Numan Siddique > wrote:




On Mon, Oct 28, 2019, 9:29 PM Mark Michelson mailto:mmich...@redhat.com>> wrote:

On 10/28/19 11:33 AM, Numan Siddique wrote:
 > On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson
mailto:mmich...@redhat.com>> wrote:
 >>
 >> As stated in previous commits, conjunctive matches have an
issue where
 >> it is possible to install multiple flows that have identical
matches.
 >> This results in ambiguity, and can lead to features (such as
ACLs) not
 >> functioning properly.
 >>
 >> This change fixes the problem by combining conjunctions with
identical
 >> matches into a single flow. As an example, in the past we
may have had
 >> something like:
 >>
 >> nw_dst=10.0.0.1 actions=conjunction(2,1/2)
 >> nw_dst=10.0.0.1 actions=conjunction(3,1/2)
 >>
 >> This commit changes this into
 >>
 >> nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2)
 >>
 >> This way, there is only a single flow with the proscribed
match, and
 >> there is no ambiguity.
 >>
 >> Signed-off-by: Mark Michelson mailto:mmich...@redhat.com>>


Acked-by: Numan Siddique mailto:num...@ovn.org>>

 >> ---
 >>   controller/lflow.c  |  5 ++--
 >>   controller/ofctrl.c | 73
+
 >>   controller/ofctrl.h |  6 +
 >>   tests/ovn.at         | 17 +
 >>   4 files changed, 79 insertions(+), 22 deletions(-)
 >>
 >> diff --git a/controller/lflow.c b/controller/lflow.c
 >> index e3ed20cd4..34b7c36a6 100644
 >> --- a/controller/lflow.c
 >> +++ b/controller/lflow.c
 >> @@ -736,8 +736,9 @@ consider_logical_flow(
 >>                   dst->clause = src->clause;
 >>                   dst->n_clauses = src->n_clauses;
 >>               }
 >> -            ofctrl_add_flow(flow_table, ptable,
lflow->priority, 0, >match,
 >> -                            , >header_.uuid);
 >> +
 >> +            ofctrl_add_or_append_flow(flow_table, ptable,
lflow->priority, 0,
 >> +                                      >match, ,
>header_.uuid);
 >>               ofpbuf_uninit();
 >>           }
 >>       }
 >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
 >> index 3131baff0..afb0036f1 100644
 >> --- a/controller/ofctrl.c
 >> +++ b/controller/ofctrl.c
 >> @@ -69,6 +69,11 @@ struct ovn_flow {
 >>       uint64_t cookie;
 >>   };
 >>
 >> +static struct ovn_flow *ovn_flow_alloc(uint8_t table_id,
uint16_t priority,
 >> +                                       uint64_t cookie,
 >> +                                       const struct match
*match,
 >> +                                       const struct ofpbuf
*actions,
 >> +                                       const struct uuid
*sb_uuid);
 >>   static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
 >>   static struct ovn_flow *ovn_flow_lookup(struct hmap
*flow_table,
 >>                                           const struct
ovn_flow *target,
 >> @@ -657,16 +662,8 @@ ofctrl_check_and_add_flow(struct
ovn_desired_flow_table *flow_table,
 >>                             const struct uuid *sb_uuid,
 >>                             bool log_duplicate_flow)
 >>   {
 >> -    struct ovn_flow *f = xmalloc(sizeof *f);
 >> -    f->table_id = table_id;
 >> -    f->priority = priority;
 >> -    minimatch_init(>match, match);
 >> -    f->ofpacts = xmemdup(actions->data, actions->size);
 >> -    f->ofpacts_len = actions->size;
 >> -    f->sb_uuid = *sb_uuid;
 >> -    f->match_hmap_node.hash = ovn_flow_match_hash(f);
 >> -    f->uuid_hindex_node.hash = uuid_hash(>sb_uuid);
 >> -    f->cookie = cookie;
 >> +    struct ovn_flow *f = ovn_flow_alloc(table_id, priority,
cookie, match,
 >> +                                        actions, sb_uuid);
 >>
 >>       ovn_flow_log(f, "ofctrl_add_flow");
 >>
 >> @@ -721,9 +718,65 @@ ofctrl_add_flow(struct
ovn_desired_flow_table *desired_flows,
 >>       ofctrl_check_and_add_flow(desired_flows, table_id,
priority, cookie,
 >>                                 match,

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

2019-10-28 Thread Numan Siddique

On Mon, Oct 28, 2019, 9:54 PM Numan Siddique  wrote:

>
>
> On Mon, Oct 28, 2019, 9:29 PM Mark Michelson  wrote:
>
>> On 10/28/19 11:33 AM, Numan Siddique wrote:
>> > On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson 
>> wrote:
>> >>
>> >> As stated in previous commits, conjunctive matches have an issue where
>> >> it is possible to install multiple flows that have identical matches.
>> >> This results in ambiguity, and can lead to features (such as ACLs) not
>> >> functioning properly.
>> >>
>> >> This change fixes the problem by combining conjunctions with identical
>> >> matches into a single flow. As an example, in the past we may have had
>> >> something like:
>> >>
>> >> nw_dst=10.0.0.1 actions=conjunction(2,1/2)
>> >> nw_dst=10.0.0.1 actions=conjunction(3,1/2)
>> >>
>> >> This commit changes this into
>> >>
>> >> nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2)
>> >>
>> >> This way, there is only a single flow with the proscribed match, and
>> >> there is no ambiguity.
>> >>
>> >> Signed-off-by: Mark Michelson 
>>
>
Acked-by: Numan Siddique 

>> ---
>> >>   controller/lflow.c  |  5 ++--
>> >>   controller/ofctrl.c | 73
>> +
>> >>   controller/ofctrl.h |  6 +
>> >>   tests/ovn.at| 17 +
>> >>   4 files changed, 79 insertions(+), 22 deletions(-)
>> >>
>> >> diff --git a/controller/lflow.c b/controller/lflow.c
>> >> index e3ed20cd4..34b7c36a6 100644
>> >> --- a/controller/lflow.c
>> >> +++ b/controller/lflow.c
>> >> @@ -736,8 +736,9 @@ consider_logical_flow(
>> >>   dst->clause = src->clause;
>> >>   dst->n_clauses = src->n_clauses;
>> >>   }
>> >> -ofctrl_add_flow(flow_table, ptable, lflow->priority, 0,
>> >match,
>> >> -, >header_.uuid);
>> >> +
>> >> +ofctrl_add_or_append_flow(flow_table, ptable,
>> lflow->priority, 0,
>> >> +  >match, ,
>> >header_.uuid);
>> >>   ofpbuf_uninit();
>> >>   }
>> >>   }
>> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
>> >> index 3131baff0..afb0036f1 100644
>> >> --- a/controller/ofctrl.c
>> >> +++ b/controller/ofctrl.c
>> >> @@ -69,6 +69,11 @@ struct ovn_flow {
>> >>   uint64_t cookie;
>> >>   };
>> >>
>> >> +static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t
>> priority,
>> >> +   uint64_t cookie,
>> >> +   const struct match *match,
>> >> +   const struct ofpbuf *actions,
>> >> +   const struct uuid *sb_uuid);
>> >>   static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
>> >>   static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
>> >>   const struct ovn_flow
>> *target,
>> >> @@ -657,16 +662,8 @@ ofctrl_check_and_add_flow(struct
>> ovn_desired_flow_table *flow_table,
>> >> const struct uuid *sb_uuid,
>> >> bool log_duplicate_flow)
>> >>   {
>> >> -struct ovn_flow *f = xmalloc(sizeof *f);
>> >> -f->table_id = table_id;
>> >> -f->priority = priority;
>> >> -minimatch_init(>match, match);
>> >> -f->ofpacts = xmemdup(actions->data, actions->size);
>> >> -f->ofpacts_len = actions->size;
>> >> -f->sb_uuid = *sb_uuid;
>> >> -f->match_hmap_node.hash = ovn_flow_match_hash(f);
>> >> -f->uuid_hindex_node.hash = uuid_hash(>sb_uuid);
>> >> -f->cookie = cookie;
>> >> +struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie,
>> match,
>> >> +actions, sb_uuid);
>> >>
>> >>   ovn_flow_log(f, "ofctrl_add_flow");
>> >>
>> >> @@ -721,9 +718,65 @@ ofctrl_add_flow(struct ovn_desired_flow_table
>> *desired_flows,
>> >>   ofctrl_check_and_add_flow(desired_flows, table_id, priority,
>> cookie,
>> >> match, actions, sb_uuid, true);
>> >>   }
>> >> +
>> >> +void
>> >> +ofctrl_add_or_append_flow(struct ovn_desired_flow_table
>> *desired_flows,
>> >> +  uint8_t table_id, uint16_t priority,
>> uint64_t cookie,
>> >> +  const struct match *match,
>> >> +  const struct ofpbuf *actions,
>> >> +  const struct uuid *sb_uuid)
>> >> +{
>> >> +struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie,
>> match,
>> >> +actions, sb_uuid);
>> >> +
>> >> +ovn_flow_log(f, "ofctrl_add_or_append_flow");
>> >> +
>> >> +struct ovn_flow *existing;
>> >> +existing = ovn_flow_lookup(_flows->match_flow_table, f,
>> false);
>> >> +if (existing) {
>> >> +/* There's already a flow with this particular match. Append
>> the
>> >> + * action to that flow rather than adding a new flow
>> >> +

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

2019-10-28 Thread Numan Siddique

On Mon, Oct 28, 2019, 9:29 PM Mark Michelson  wrote:

> On 10/28/19 11:33 AM, Numan Siddique wrote:
> > On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson 
> wrote:
> >>
> >> As stated in previous commits, conjunctive matches have an issue where
> >> it is possible to install multiple flows that have identical matches.
> >> This results in ambiguity, and can lead to features (such as ACLs) not
> >> functioning properly.
> >>
> >> This change fixes the problem by combining conjunctions with identical
> >> matches into a single flow. As an example, in the past we may have had
> >> something like:
> >>
> >> nw_dst=10.0.0.1 actions=conjunction(2,1/2)
> >> nw_dst=10.0.0.1 actions=conjunction(3,1/2)
> >>
> >> This commit changes this into
> >>
> >> nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2)
> >>
> >> This way, there is only a single flow with the proscribed match, and
> >> there is no ambiguity.
> >>
> >> Signed-off-by: Mark Michelson 
> >> ---
> >>   controller/lflow.c  |  5 ++--
> >>   controller/ofctrl.c | 73
> +
> >>   controller/ofctrl.h |  6 +
> >>   tests/ovn.at| 17 +
> >>   4 files changed, 79 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/controller/lflow.c b/controller/lflow.c
> >> index e3ed20cd4..34b7c36a6 100644
> >> --- a/controller/lflow.c
> >> +++ b/controller/lflow.c
> >> @@ -736,8 +736,9 @@ consider_logical_flow(
> >>   dst->clause = src->clause;
> >>   dst->n_clauses = src->n_clauses;
> >>   }
> >> -ofctrl_add_flow(flow_table, ptable, lflow->priority, 0,
> >match,
> >> -, >header_.uuid);
> >> +
> >> +ofctrl_add_or_append_flow(flow_table, ptable,
> lflow->priority, 0,
> >> +  >match, ,
> >header_.uuid);
> >>   ofpbuf_uninit();
> >>   }
> >>   }
> >> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> >> index 3131baff0..afb0036f1 100644
> >> --- a/controller/ofctrl.c
> >> +++ b/controller/ofctrl.c
> >> @@ -69,6 +69,11 @@ struct ovn_flow {
> >>   uint64_t cookie;
> >>   };
> >>
> >> +static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t
> priority,
> >> +   uint64_t cookie,
> >> +   const struct match *match,
> >> +   const struct ofpbuf *actions,
> >> +   const struct uuid *sb_uuid);
> >>   static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
> >>   static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
> >>   const struct ovn_flow *target,
> >> @@ -657,16 +662,8 @@ ofctrl_check_and_add_flow(struct
> ovn_desired_flow_table *flow_table,
> >> const struct uuid *sb_uuid,
> >> bool log_duplicate_flow)
> >>   {
> >> -struct ovn_flow *f = xmalloc(sizeof *f);
> >> -f->table_id = table_id;
> >> -f->priority = priority;
> >> -minimatch_init(>match, match);
> >> -f->ofpacts = xmemdup(actions->data, actions->size);
> >> -f->ofpacts_len = actions->size;
> >> -f->sb_uuid = *sb_uuid;
> >> -f->match_hmap_node.hash = ovn_flow_match_hash(f);
> >> -f->uuid_hindex_node.hash = uuid_hash(>sb_uuid);
> >> -f->cookie = cookie;
> >> +struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie,
> match,
> >> +actions, sb_uuid);
> >>
> >>   ovn_flow_log(f, "ofctrl_add_flow");
> >>
> >> @@ -721,9 +718,65 @@ ofctrl_add_flow(struct ovn_desired_flow_table
> *desired_flows,
> >>   ofctrl_check_and_add_flow(desired_flows, table_id, priority,
> cookie,
> >> match, actions, sb_uuid, true);
> >>   }
> >> +
> >> +void
> >> +ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
> >> +  uint8_t table_id, uint16_t priority,
> uint64_t cookie,
> >> +  const struct match *match,
> >> +  const struct ofpbuf *actions,
> >> +  const struct uuid *sb_uuid)
> >> +{
> >> +struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie,
> match,
> >> +actions, sb_uuid);
> >> +
> >> +ovn_flow_log(f, "ofctrl_add_or_append_flow");
> >> +
> >> +struct ovn_flow *existing;
> >> +existing = ovn_flow_lookup(_flows->match_flow_table, f,
> false);
> >> +if (existing) {
> >> +/* There's already a flow with this particular match. Append
> the
> >> + * action to that flow rather than adding a new flow
> >> + */
> >> +uint64_t compound_stub[64 / 8];
> >> +struct ofpbuf compound;
> >> +ofpbuf_use_stub(, compound_stub,
> sizeof(compound_stub));
> >> +ofpbuf_put(, existing->ofpacts,
>

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

2019-10-28 Thread Mark Michelson


On 10/28/19 11:33 AM, Numan Siddique wrote:

On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson  wrote:


As stated in previous commits, conjunctive matches have an issue where
it is possible to install multiple flows that have identical matches.
This results in ambiguity, and can lead to features (such as ACLs) not
functioning properly.

This change fixes the problem by combining conjunctions with identical
matches into a single flow. As an example, in the past we may have had
something like:

nw_dst=10.0.0.1 actions=conjunction(2,1/2)
nw_dst=10.0.0.1 actions=conjunction(3,1/2)

This commit changes this into

nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2)

This way, there is only a single flow with the proscribed match, and
there is no ambiguity.

Signed-off-by: Mark Michelson 
---
  controller/lflow.c  |  5 ++--
  controller/ofctrl.c | 73 +
  controller/ofctrl.h |  6 +
  tests/ovn.at| 17 +
  4 files changed, 79 insertions(+), 22 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index e3ed20cd4..34b7c36a6 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -736,8 +736,9 @@ consider_logical_flow(
  dst->clause = src->clause;
  dst->n_clauses = src->n_clauses;
  }
-ofctrl_add_flow(flow_table, ptable, lflow->priority, 0, >match,
-, >header_.uuid);
+
+ofctrl_add_or_append_flow(flow_table, ptable, lflow->priority, 0,
+  >match, , >header_.uuid);
  ofpbuf_uninit();
  }
  }
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 3131baff0..afb0036f1 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -69,6 +69,11 @@ struct ovn_flow {
  uint64_t cookie;
  };

+static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t priority,
+   uint64_t cookie,
+   const struct match *match,
+   const struct ofpbuf *actions,
+   const struct uuid *sb_uuid);
  static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
  static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
  const struct ovn_flow *target,
@@ -657,16 +662,8 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table 
*flow_table,
const struct uuid *sb_uuid,
bool log_duplicate_flow)
  {
-struct ovn_flow *f = xmalloc(sizeof *f);
-f->table_id = table_id;
-f->priority = priority;
-minimatch_init(>match, match);
-f->ofpacts = xmemdup(actions->data, actions->size);
-f->ofpacts_len = actions->size;
-f->sb_uuid = *sb_uuid;
-f->match_hmap_node.hash = ovn_flow_match_hash(f);
-f->uuid_hindex_node.hash = uuid_hash(>sb_uuid);
-f->cookie = cookie;
+struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
+actions, sb_uuid);

  ovn_flow_log(f, "ofctrl_add_flow");

@@ -721,9 +718,65 @@ ofctrl_add_flow(struct ovn_desired_flow_table 
*desired_flows,
  ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie,
match, actions, sb_uuid, true);
  }
+
+void
+ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
+  uint8_t table_id, uint16_t priority, uint64_t cookie,
+  const struct match *match,
+  const struct ofpbuf *actions,
+  const struct uuid *sb_uuid)
+{
+struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
+actions, sb_uuid);
+
+ovn_flow_log(f, "ofctrl_add_or_append_flow");
+
+struct ovn_flow *existing;
+existing = ovn_flow_lookup(_flows->match_flow_table, f, false);
+if (existing) {
+/* There's already a flow with this particular match. Append the
+ * action to that flow rather than adding a new flow
+ */
+uint64_t compound_stub[64 / 8];
+struct ofpbuf compound;
+ofpbuf_use_stub(, compound_stub, sizeof(compound_stub));
+ofpbuf_put(, existing->ofpacts, existing->ofpacts_len);
+ofpbuf_put(, f->ofpacts, f->ofpacts_len);


Instead of making use of a stub and copying the existing and new
actions, can't we just
copy the new actions to "existing->ofpacts" using ofpbuf_put() ?


You can't use ofpbuf_put() directly since existing->ofpacts is of type 
ofpact, not ofpbuf. And I don't think you can cast existing->ofpacts to 
an ofpbuf since existing->ofpacts was created from the 'data' member of 
an ofpbuf; we don't have the metadata, such as the method by which the 
data was allocated.


So maybe it's possible to create a new ofpbuf but have it use

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

2019-10-28 Thread Numan Siddique

On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson  wrote:
>
> As stated in previous commits, conjunctive matches have an issue where
> it is possible to install multiple flows that have identical matches.
> This results in ambiguity, and can lead to features (such as ACLs) not
> functioning properly.
>
> This change fixes the problem by combining conjunctions with identical
> matches into a single flow. As an example, in the past we may have had
> something like:
>
> nw_dst=10.0.0.1 actions=conjunction(2,1/2)
> nw_dst=10.0.0.1 actions=conjunction(3,1/2)
>
> This commit changes this into
>
> nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2)
>
> This way, there is only a single flow with the proscribed match, and
> there is no ambiguity.
>
> Signed-off-by: Mark Michelson 
> ---
>  controller/lflow.c  |  5 ++--
>  controller/ofctrl.c | 73 
> +
>  controller/ofctrl.h |  6 +
>  tests/ovn.at| 17 +
>  4 files changed, 79 insertions(+), 22 deletions(-)
>
> diff --git a/controller/lflow.c b/controller/lflow.c
> index e3ed20cd4..34b7c36a6 100644
> --- a/controller/lflow.c
> +++ b/controller/lflow.c
> @@ -736,8 +736,9 @@ consider_logical_flow(
>  dst->clause = src->clause;
>  dst->n_clauses = src->n_clauses;
>  }
> -ofctrl_add_flow(flow_table, ptable, lflow->priority, 0, 
> >match,
> -, >header_.uuid);
> +
> +ofctrl_add_or_append_flow(flow_table, ptable, lflow->priority, 0,
> +  >match, , 
> >header_.uuid);
>  ofpbuf_uninit();
>  }
>  }
> diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> index 3131baff0..afb0036f1 100644
> --- a/controller/ofctrl.c
> +++ b/controller/ofctrl.c
> @@ -69,6 +69,11 @@ struct ovn_flow {
>  uint64_t cookie;
>  };
>
> +static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t priority,
> +   uint64_t cookie,
> +   const struct match *match,
> +   const struct ofpbuf *actions,
> +   const struct uuid *sb_uuid);
>  static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
>  static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
>  const struct ovn_flow *target,
> @@ -657,16 +662,8 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table 
> *flow_table,
>const struct uuid *sb_uuid,
>bool log_duplicate_flow)
>  {
> -struct ovn_flow *f = xmalloc(sizeof *f);
> -f->table_id = table_id;
> -f->priority = priority;
> -minimatch_init(>match, match);
> -f->ofpacts = xmemdup(actions->data, actions->size);
> -f->ofpacts_len = actions->size;
> -f->sb_uuid = *sb_uuid;
> -f->match_hmap_node.hash = ovn_flow_match_hash(f);
> -f->uuid_hindex_node.hash = uuid_hash(>sb_uuid);
> -f->cookie = cookie;
> +struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
> +actions, sb_uuid);
>
>  ovn_flow_log(f, "ofctrl_add_flow");
>
> @@ -721,9 +718,65 @@ ofctrl_add_flow(struct ovn_desired_flow_table 
> *desired_flows,
>  ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie,
>match, actions, sb_uuid, true);
>  }
> +
> +void
> +ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
> +  uint8_t table_id, uint16_t priority, uint64_t 
> cookie,
> +  const struct match *match,
> +  const struct ofpbuf *actions,
> +  const struct uuid *sb_uuid)
> +{
> +struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
> +actions, sb_uuid);
> +
> +ovn_flow_log(f, "ofctrl_add_or_append_flow");
> +
> +struct ovn_flow *existing;
> +existing = ovn_flow_lookup(_flows->match_flow_table, f, false);
> +if (existing) {
> +/* There's already a flow with this particular match. Append the
> + * action to that flow rather than adding a new flow
> + */
> +uint64_t compound_stub[64 / 8];
> +struct ofpbuf compound;
> +ofpbuf_use_stub(, compound_stub, sizeof(compound_stub));
> +ofpbuf_put(, existing->ofpacts, existing->ofpacts_len);
> +ofpbuf_put(, f->ofpacts, f->ofpacts_len);

Instead of making use of a stub and copying the existing and new
actions, can't we just
copy the new actions to "existing->ofpacts" using ofpbuf_put() ?

ofpbuf_put() will take care of reallocating the memory if required.

Other than that, this and the 1st patch of this series, looks good to me.

Thanks
Numan

> +
> +free(existing->ofpacts);
> +existing->ofpacts =

[ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

2019-10-25 Thread Mark Michelson

As stated in previous commits, conjunctive matches have an issue where
it is possible to install multiple flows that have identical matches.
This results in ambiguity, and can lead to features (such as ACLs) not
functioning properly.

This change fixes the problem by combining conjunctions with identical
matches into a single flow. As an example, in the past we may have had
something like:

nw_dst=10.0.0.1 actions=conjunction(2,1/2)
nw_dst=10.0.0.1 actions=conjunction(3,1/2)

This commit changes this into

nw_dst=10.0.0.1 actions=conjunction(2,1/2),conjunction(3,1/2)

This way, there is only a single flow with the proscribed match, and
there is no ambiguity.

Signed-off-by: Mark Michelson 
---
 controller/lflow.c  |  5 ++--
 controller/ofctrl.c | 73 +
 controller/ofctrl.h |  6 +
 tests/ovn.at| 17 +
 4 files changed, 79 insertions(+), 22 deletions(-)

diff --git a/controller/lflow.c b/controller/lflow.c
index e3ed20cd4..34b7c36a6 100644
--- a/controller/lflow.c
+++ b/controller/lflow.c
@@ -736,8 +736,9 @@ consider_logical_flow(
 dst->clause = src->clause;
 dst->n_clauses = src->n_clauses;
 }
-ofctrl_add_flow(flow_table, ptable, lflow->priority, 0, >match,
-, >header_.uuid);
+
+ofctrl_add_or_append_flow(flow_table, ptable, lflow->priority, 0,
+  >match, , >header_.uuid);
 ofpbuf_uninit();
 }
 }
diff --git a/controller/ofctrl.c b/controller/ofctrl.c
index 3131baff0..afb0036f1 100644
--- a/controller/ofctrl.c
+++ b/controller/ofctrl.c
@@ -69,6 +69,11 @@ struct ovn_flow {
 uint64_t cookie;
 };
 
+static struct ovn_flow *ovn_flow_alloc(uint8_t table_id, uint16_t priority,
+   uint64_t cookie,
+   const struct match *match,
+   const struct ofpbuf *actions,
+   const struct uuid *sb_uuid);
 static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
 static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
 const struct ovn_flow *target,
@@ -657,16 +662,8 @@ ofctrl_check_and_add_flow(struct ovn_desired_flow_table 
*flow_table,
   const struct uuid *sb_uuid,
   bool log_duplicate_flow)
 {
-struct ovn_flow *f = xmalloc(sizeof *f);
-f->table_id = table_id;
-f->priority = priority;
-minimatch_init(>match, match);
-f->ofpacts = xmemdup(actions->data, actions->size);
-f->ofpacts_len = actions->size;
-f->sb_uuid = *sb_uuid;
-f->match_hmap_node.hash = ovn_flow_match_hash(f);
-f->uuid_hindex_node.hash = uuid_hash(>sb_uuid);
-f->cookie = cookie;
+struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
+actions, sb_uuid);
 
 ovn_flow_log(f, "ofctrl_add_flow");
 
@@ -721,9 +718,65 @@ ofctrl_add_flow(struct ovn_desired_flow_table 
*desired_flows,
 ofctrl_check_and_add_flow(desired_flows, table_id, priority, cookie,
   match, actions, sb_uuid, true);
 }
+
+void
+ofctrl_add_or_append_flow(struct ovn_desired_flow_table *desired_flows,
+  uint8_t table_id, uint16_t priority, uint64_t cookie,
+  const struct match *match,
+  const struct ofpbuf *actions,
+  const struct uuid *sb_uuid)
+{
+struct ovn_flow *f = ovn_flow_alloc(table_id, priority, cookie, match,
+actions, sb_uuid);
+
+ovn_flow_log(f, "ofctrl_add_or_append_flow");
+
+struct ovn_flow *existing;
+existing = ovn_flow_lookup(_flows->match_flow_table, f, false);
+if (existing) {
+/* There's already a flow with this particular match. Append the
+ * action to that flow rather than adding a new flow
+ */
+uint64_t compound_stub[64 / 8];
+struct ofpbuf compound;
+ofpbuf_use_stub(, compound_stub, sizeof(compound_stub));
+ofpbuf_put(, existing->ofpacts, existing->ofpacts_len);
+ofpbuf_put(, f->ofpacts, f->ofpacts_len);
+
+free(existing->ofpacts);
+existing->ofpacts = xmemdup(compound.data, compound.size);
+existing->ofpacts_len = compound.size;
+
+ovn_flow_destroy(f);
+} else {
+hmap_insert(_flows->match_flow_table, >match_hmap_node,
+f->match_hmap_node.hash);
+hindex_insert(_flows->uuid_flow_table, >uuid_hindex_node,
+  f->uuid_hindex_node.hash);
+}
+}
 
 /* ovn_flow. */
 
+static struct ovn_flow *
+ovn_flow_alloc(uint8_t table_id, uint16_t priority, uint64_t cookie,
+   const struct match *match, const struct ofpbuf *actions,
+   const

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

Re: [ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

[ovs-dev] [PATCH 2/3] Combine conjunctions with identical matches into one flow.

6 matches

Site Navigation

Mail list logo

Footer information