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

Numan Siddique nusiddiq at redhat.com
Mon Oct 28 16:24:50 UTC 2019


On Mon, Oct 28, 2019, 9:29 PM Mark Michelson <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>
> 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>
> >> ---
> >>   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,
> &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 b/tests/ovn.at
> >> index 641a646fc..50d8efeec 100644
> >> --- a/tests/ovn.at
> >> +++ b/tests/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" 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
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>


More information about the dev mailing list