[ovs-dev] [PATCH ovn 6/9] ofctrl.c: Maintain references between installed flows and desired flows.

Han Zhou hzhou at ovn.org
Thu Sep 3 21:44:50 UTC 2020


On Thu, Sep 3, 2020 at 12:47 PM Mark Michelson <mmichels at redhat.com> wrote:
>
> On 8/21/20 3:16 PM, Han Zhou wrote:
> > Currently there is no link maintained between installed flows and
desired
> > flows. This patch maintains the mapping between them, which will be
useful
> > for a future patch that incrementally processing the flow installation
without
> > having to do the full comparison between them.
> >
> > Signed-off-by: Han Zhou <hzhou at ovn.org>
> > ---
> >   controller/ofctrl.c | 93
++++++++++++++++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 85 insertions(+), 8 deletions(-)
> >
> > diff --git a/controller/ofctrl.c b/controller/ofctrl.c
> > index c500f52..3db1fa0 100644
> > --- a/controller/ofctrl.c
> > +++ b/controller/ofctrl.c
> > @@ -55,11 +55,30 @@ VLOG_DEFINE_THIS_MODULE(ofctrl);
> >   struct ovn_flow {
> >       struct hmap_node match_hmap_node; /* For match based hashing. */
> >       struct ovs_list list_node; /* For handling lists of flows. */
> > -    struct ovs_list references; /* A list of struct sb_flow_ref nodes,
which
> > -                                   references this flow. (There are
cases
> > -                                   that multiple SB entities share the
same
> > -                                   desired OpenFlow flow, e.g. when
> > -                                   conjunction is used.) */
> > +
> > +    /* For a flow in desired table, this field maintains a list of
struct
> > +     * sb_flow_ref nodes, which references this flow. (There are cases
that
> > +     * multiple SB entities share the same desired OpenFlow flow, e.g.
when
> > +     * conjunction is used.)
> > +     *
> > +     * For a flow in installed table, this field maintains a list of
desired
> > +     * ovn_flow nodes (linked by ovn_flow.installed_ref_list_node),
which
> > +     * reference this installed flow.  (There are cases that multiple
desired
> > +     * flows reference the same installed flow, e.g. when there are
> > +     * conflict/duplicated ACLs that generates same match conditions).
*/
> > +    struct ovs_list references;
> > +
> > +    /* For a flow in desired table, this field represents the
corresponding
> > +     * flow in installed table.
> > +     *
> > +     * For a flow in installed table, this field represents the
corresponding
> > +     * flow in desired table. It must be one of the flows in
references list.
> > +     * If there are more than one flows in references list, this is
the one
> > +     * that is actually installed. */
> > +    struct ovn_flow *peer;
> > +
> > +    /* For desired flows only: node in references list. */
> > +    struct ovs_list installed_ref_list_node;
>
> With all these qualifying comments, it seems like desired flows and
> installed flows should be treated as different types. Perhaps create
> wrapper types ovn_installed_flow and ovn_desired_flow. This way, you can
> let the type system work with you better and not have to have different
> behaviors for the same structure fields depending on which hmap the
> ovn_flow is in.
>
I agree with you it helps the type system with two separate structures - it
would be done a lot easier with OO programming languages.
However, most things are still common between these two types and the
different parts are still manageable, hopefully with the help of the
comments. So I am not sure if it worth the separation, because it would end
up with more APIs somehow redundant just for handling different input
parameter types. I can try to refactor it and see how it works. Probably it
is better to be done separately as a follow up patch. What do you think?

> >
> >       /* Key. */
> >       uint8_t table_id;
> > @@ -661,6 +680,45 @@ ofctrl_recv(const struct ofp_header *oh, enum
ofptype type)
> >       }
> >   }
> >
> > +static void
> > +link_installed_to_desired(struct ovn_flow *i, struct ovn_flow *d)
> > +{
> > +    if (i->peer == d) {
> > +        return;
> > +    }
> > +
> > +    if (ovs_list_is_empty(&i->references)) {
> > +        ovs_assert(!i->peer);
> > +        i->peer = d;
> > +    }
> > +    ovs_list_insert(&i->references, &d->installed_ref_list_node);
> > +    d->peer = i;
> > +}
> > +
> > +static void
> > +unlink_installed_to_desired(struct ovn_flow *i, struct ovn_flow *d)
> > +{
> > +    ovs_assert(i && i->peer && !ovs_list_is_empty(&i->references));
> > +    ovs_assert(d && d->peer == i);
> > +    ovs_list_remove(&d->installed_ref_list_node);
> > +    d->peer = NULL;
> > +    if (i->peer == d) {
> > +        i->peer = ovs_list_is_empty(&i->references) ? NULL :
> > +            CONTAINER_OF(ovs_list_front(&i->references),
> > +                         struct ovn_flow,
> > +                         installed_ref_list_node);
> > +    }
> > +}
> > +
> > +static void
> > +unlink_all_refs_for_installed_flow(struct ovn_flow *i)
> > +{
> > +    struct ovn_flow *d, *next;
> > +    LIST_FOR_EACH_SAFE (d, next, installed_ref_list_node,
&i->references) {
> > +        unlink_installed_to_desired(i, d);
> > +    }
> > +}
> > +
> >   static struct sb_to_flow *
> >   sb_to_flow_find(struct hmap *uuid_flow_table, const struct uuid
*sb_uuid)
> >   {
> > @@ -812,6 +870,9 @@ remove_flows_from_sb_to_flow(struct
ovn_desired_flow_table *flow_table,
> >               }
> >               hmap_remove(&flow_table->match_flow_table,
> >                           &f->match_hmap_node);
> > +            if (f->peer) {
> > +                unlink_installed_to_desired(f->peer, f);
> > +            }
> >               ovn_flow_destroy(f);
> >           }
> >       }
> > @@ -887,6 +948,9 @@ flood_remove_flows_for_sb_uuid(struct
ovn_desired_flow_table *flow_table,
> >                * be empty in most cases. */
> >               hmap_remove(&flow_table->match_flow_table,
> >                           &f->match_hmap_node);
> > +            if (f->peer) {
> > +                unlink_installed_to_desired(f->peer, f);
> > +            }
> >               ovn_flow_destroy(f);
> >           } else {
> >               ovs_list_insert(&to_be_removed, &f->list_node);
> > @@ -921,6 +985,9 @@ flood_remove_flows_for_sb_uuid(struct
ovn_desired_flow_table *flow_table,
> >           ovs_list_remove(&f->list_node);
> >           hmap_remove(&flow_table->match_flow_table,
> >                       &f->match_hmap_node);
> > +        if (f->peer) {
> > +            unlink_installed_to_desired(f->peer, f);
> > +        }
> >           ovn_flow_destroy(f);
> >       }
> >
> > @@ -952,6 +1019,8 @@ ovn_flow_alloc(uint8_t table_id, uint16_t
priority, uint64_t cookie,
> >       struct ovn_flow *f = xmalloc(sizeof *f);
> >       ovs_list_init(&f->references);
> >       ovs_list_init(&f->list_node);
> > +    ovs_list_init(&f->installed_ref_list_node);
> > +    f->peer = NULL;
> >       f->table_id = table_id;
> >       f->priority = priority;
> >       minimatch_init(&f->match, match);
> > @@ -977,6 +1046,9 @@ ovn_flow_dup(struct ovn_flow *src)
> >   {
> >       struct ovn_flow *dst = xmalloc(sizeof *dst);
> >       ovs_list_init(&dst->references);
> > +    ovs_list_init(&dst->list_node);
> > +    ovs_list_init(&dst->installed_ref_list_node);
> > +    dst->peer = NULL;
> >       dst->table_id = src->table_id;
> >       dst->priority = src->priority;
> >       minimatch_clone(&dst->match, &src->match);
> > @@ -1050,6 +1122,7 @@ ovn_flow_destroy(struct ovn_flow *f)
> >   {
> >       if (f) {
> >           ovs_assert(ovs_list_is_empty(&f->references));
> > +        ovs_assert(!f->peer);
> >           minimatch_destroy(&f->match);
> >           free(f->ofpacts);
> >           free(f);
> > @@ -1088,6 +1161,7 @@ ovn_installed_flow_table_clear(void)
> >       struct ovn_flow *f, *next;
> >       HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, &installed_flows) {
> >           hmap_remove(&installed_flows, &f->match_hmap_node);
> > +        unlink_all_refs_for_installed_flow(f);
> >           ovn_flow_destroy(f);
> >       }
> >   }
> > @@ -1413,6 +1487,7 @@ ofctrl_put(struct ovn_desired_flow_table
*flow_table,
> >        * actions, update them. */
> >       struct ovn_flow *i, *next;
> >       HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) {
> > +        unlink_all_refs_for_installed_flow(i);
> >           struct ovn_flow *d =
ovn_flow_lookup(&flow_table->match_flow_table,
> >                                                i, NULL);
> >           if (!d) {
> > @@ -1458,6 +1533,7 @@ ofctrl_put(struct ovn_desired_flow_table
*flow_table,
> >                   i->ofpacts = xmemdup(d->ofpacts, d->ofpacts_len);
> >                   i->ofpacts_len = d->ofpacts_len;
> >               }
> > +            link_installed_to_desired(i, d);
> >
> >           }
> >       }
> > @@ -1482,10 +1558,11 @@ ofctrl_put(struct ovn_desired_flow_table
*flow_table,
> >               ovn_flow_log(d, "adding installed");
> >
> >               /* Copy 'd' from 'flow_table' to installed_flows. */
> > -            struct ovn_flow *new_node = ovn_flow_dup(d);
> > -            hmap_insert(&installed_flows, &new_node->match_hmap_node,
> > -                        new_node->match_hmap_node.hash);
> > +            i= ovn_flow_dup(d);
> > +            hmap_insert(&installed_flows, &i->match_hmap_node,
> > +                        i->match_hmap_node.hash);
> >           }
> > +        link_installed_to_desired(i, d);
> >       }
> >
> >       /* Iterate through the installed groups from previous runs. If
they
> >
>


More information about the dev mailing list