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

Mark Michelson mmichels at redhat.com
Mon Oct 28 20:47:17 UTC 2019


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 <nusiddiq at redhat.com 
> <mailto:nusiddiq at redhat.com>> wrote:
> 
> 
> 
>     On Mon, Oct 28, 2019, 9:29 PM Mark Michelson <mmichels at redhat.com
>     <mailto:mmichels at redhat.com>> wrote:
> 
>         On 10/28/19 11:33 AM, Numan Siddique wrote:
>          > On Sat, Oct 26, 2019 at 2:37 AM Mark Michelson
>         <mmichels at redhat.com <mailto:mmichels at 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 <mmichels at redhat.com
>         <mailto:mmichels at redhat.com>>
> 
> 
> Acked-by: Numan Siddique <numans at ovn.org <mailto:numans at ovn.org>>
> 
>          >> ---
>          >>   controller/lflow.c  |  5 ++--
>          >>   controller/ofctrl.c | 73
>         +++++++++++++++++++++++++++++++++++++++++++++--------
>          >>   controller/ofctrl.h |  6 +++++
>          >>   tests/ovn.at <http://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, &m->match,
>          >> -                            &conj, &lflow->header_.uuid);
>          >> +
>          >> +            ofctrl_add_or_append_flow(flow_table, ptable,
>         lflow->priority, 0,
>          >> +                                      &m->match, &conj,
>         &lflow->header_.uuid);
>          >>               ofpbuf_uninit(&conj);
>          >>           }
>          >>       }
>          >> 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(&f->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(&f->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(&desired_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, compound_stub,
>         sizeof(compound_stub));
>          >> +        ofpbuf_put(&compound, existing->ofpacts,
>         existing->ofpacts_len);
>          >> +        ofpbuf_put(&compound, 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.
> 
> 
>     Oops. Please ignore my comment. I thought existing->ofpacts is of
>     type ofpbuf.
> 
>     Sorry for the noise.
> 
>     Numan
> 
>         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 the
>         existing->ofpacts buffer? I was looking and here are the issues:
> 
>         1) ofpbuf_clone_data() creates a copy of the data passed in
>         rather than
>         using it directly. So it still requires freeing
>         existing->ofpacts and
>         reassigning it.
>         2) ofpbuf_use_stub() would result in overwriting the data passed
>         into it
>         unless we adjust the ofpbuf's size so that it points past the
>         end of the
>         data we passed in. I can't find any example of this being done
>         in OVS.
> 
>         If you have a good idea on how to re-use existing->ofpacts, then
>         I'll
>         happily do it.
> 
>          >
>          > 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 = xmemdup(compound.data,
>         compound.size);
>          >> +        existing->ofpacts_len = compound.size;
>          >> +
>          >> +        ovn_flow_destroy(f);
>          >> +    } else {
>          >> +        hmap_insert(&desired_flows->match_flow_table,
>         &f->match_hmap_node,
>          >> +                    f->match_hmap_node.hash);
>          >> +        hindex_insert(&desired_flows->uuid_flow_table,
>         &f->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 struct uuid *sb_uuid)
>          >> +{
>          >> +    struct ovn_flow *f = xmalloc(sizeof *f);
>          >> +    f->table_id = table_id;
>          >> +    f->priority = priority;
>          >> +    minimatch_init(&f->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(&f->sb_uuid);
>          >> +    f->cookie = cookie;
>          >> +
>          >> +    return f;
>          >> +}
>          >> +
>          >>   /* Returns a hash of the match key in 'f'. */
>          >>   static uint32_t
>          >>   ovn_flow_match_hash(const struct ovn_flow *f)
>          >> diff --git a/controller/ofctrl.h b/controller/ofctrl.h
>          >> index 1e9ac16b9..21d2ce648 100644
>          >> --- a/controller/ofctrl.h
>          >> +++ b/controller/ofctrl.h
>          >> @@ -70,6 +70,12 @@ void ofctrl_add_flow(struct
>         ovn_desired_flow_table *, uint8_t table_id,
>          >>                        const struct match *, const struct
>         ofpbuf *ofpacts,
>          >>                        const struct uuid *);
>          >>
>          >> +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);
>          >> +
>          >>   void ofctrl_remove_flows(struct ovn_desired_flow_table *,
>         const struct uuid *);
>          >>
>          >>   void ovn_desired_flow_table_init(struct
>         ovn_desired_flow_table *);
>          >> diff --git a/tests/ovn.at <http://ovn.at> b/tests/ovn.at
>         <http://ovn.at>
>          >> index 641a646fc..50d8efeec 100644
>          >> --- a/tests/ovn.at <http://ovn.at>
>          >> +++ b/tests/ovn.at <http://ovn.at>
>          >> @@ -12247,7 +12247,7 @@ ovn-nbctl create Address_Set name=set1 \
>          >>   addresses=\"10.0.0.4\",\"10.0.0.5\",\"10.0.0.6\"
>          >>   ovn-nbctl create Address_Set name=set2 \
>          >>   addresses=\"10.0.0.7\",\"10.0.0.8\",\"10.0.0.9\"
>          >> -ovn-nbctl acl-add ls1 to-lport 1002 \
>          >> +ovn-nbctl acl-add ls1 to-lport 1001 \
>          >>   'ip4 && ip4.src == $set1 && ip4.dst == $set1' allow
>          >>   ovn-nbctl acl-add ls1 to-lport 1001 \
>          >>   'ip4 && ip4.src == $set1 && ip4.dst == $set2' drop
>          >> @@ -12296,7 +12296,7 @@ cat 2.expected > expout
>          >>   $PYTHON "$ovs_srcdir/utilities/ovs-pcap.in
>         <http://ovs-pcap.in>" hv1/vif2-tx.pcap > 2.packets
>          >>   AT_CHECK([cat 2.packets], [0], [expout])
>          >>
>          >> -# There should be total of 12 flows present with
>         conjunction action and 2 flows
>          >> +# There should be total of 9 flows present with conjunction
>         action and 2 flows
>          >>   # with conj match. Eg.
>          >>   # table=44, priority=2002,conj_id=2,metadata=0x1
>         actions=resubmit(,45)
>          >>   # table=44, priority=2001,conj_id=3,metadata=0x1 actions=drop
>          >> @@ -12306,14 +12306,11 @@ AT_CHECK([cat 2.packets], [0],
>         [expout])
>          >>   # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.7
>         actions=conjunction(3,2/2)
>          >>   # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.9
>         actions=conjunction(3,2/2)
>          >>   # priority=2001,ip,metadata=0x1,nw_dst=10.0.0.8
>         actions=conjunction(3,2/2)
>          >> -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.6
>         actions=conjunction(2,1/2)
>          >> -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4
>         actions=conjunction(2,1/2)
>          >> -# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5
>         actions=conjunction(2,1/2)
>          >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.6
>         actions=conjunction(3,1/2)
>          >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.4
>         actions=conjunction(3,1/2)
>          >> -# priority=2001,ip,metadata=0x1,nw_src=10.0.0.5
>         actions=conjunction(3,1/2)
>          >> -
>          >> -OVS_WAIT_UNTIL([test 12 = `as hv1 ovs-ofctl dump-flows
>         br-int | \
>          >> +# priority=2002,ip,metadata=0x1,nw_src=10.0.0.6
>         actions=conjunction(2,1/2),conjunction(3,1/2)
>          >> +# priority=2002,ip,metadata=0x1,nw_src=10.0.0.4
>         actions=conjunction(2,1/2),conjunction(3,1/2)
>          >> +# priority=2002,ip,metadata=0x1,nw_src=10.0.0.5
>         actions=conjunction(2,1/2),conjunction(3,1/2)
>          >> +
>          >> +OVS_WAIT_UNTIL([test 9 = `as hv1 ovs-ofctl dump-flows
>         br-int | \
>          >>   grep conjunction | wc -l`])
>          >>   OVS_WAIT_UNTIL([test 2 = `as hv1 ovs-ofctl dump-flows
>         br-int | \
>          >>   grep conj_id | wc -l`])
>          >> --
>          >> 2.14.5
>          >>
>          >> _______________________________________________
>          >> dev mailing list
>          >> dev at openvswitch.org <mailto:dev at openvswitch.org>
>          >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> 



More information about the dev mailing list