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

Mark Michelson mmichels at redhat.com
Fri Sep 4 13:12:10 UTC 2020


On 9/3/20 5:44 PM, Han Zhou wrote:
> 
> 
> On Thu, Sep 3, 2020 at 12:47 PM Mark Michelson <mmichels at redhat.com 
> <mailto: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 <mailto: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?

To be clear, my thought was to have structures like this in ofctrl.c:

struct ovn_flow {
     /* Key. */ 
 
 

     uint8_t table_id; 
 
 

     uint16_t priority; 
 
 

     struct minimatch match; 
 
 

 
 
 

     /* Data. */ 
 
 

     struct uuid sb_uuid; 
 
 

     struct ofpact *ofpacts; 
 
 

     size_t ofpacts_len; 
 
 

     uint64_t cookie;
};

struct ovn_desired_flow {
     struct ovn_flow *flow;
     struct hmap_node match_node;
     struct ovs_list list_node; /* For putting this in lists. Currently 
only used during flood removal */
     struct ovs_list sb_refs;   /* List of sb_ref_flows */
     struct ovn_installed_flow *installed /* Called "peer" in this patch */;
     struct ovs_list installed_ref_list_node;
};

struct ovn_installed_flow {
    struct ovn_flow *flow;
    struct hmap_node match_node;
    struct ovs_list desired;  /* List of coresponding ovn_desired_flows */
    struct ovn_desired_flow *installed_desired; /* Actual installed 
desiredflow (called "peer" in this patch) */
};

struct sb_ref_flow {
     struct ovs_list sb_list; /* List node in ovn_desired_flow.sb_refs. 
*/ 
 

     struct ovs_list flow_list; /* List node in sb_to_flow.ovn_flows. */ 
 
 

     struct ovn_desired_flow *flow; 
 
 

     struct uuid sb_uuid;
};

Since these structures are all local to ofctrl.c, it shouldn't result in 
any new APIs being created. The existing ofctrl_add_flow(), 
ofctrl_add_or_append_flow(), ofctrl_remove_flows(), and 
ofctrl_check_and_add_flow() should all remain the same from an outside 
perspective. Internally, these would now be creating ovn_desired_flows 
instead of simply ovn_flows. The case where you currently call 
ovn_flow_dup() to create an installed flow from a desired flow now also 
need to allocate the ovn_installed_flow structure to house the 
duplicated ovn_flow. Within ofctrl.c, you would need to change some of 
the internal functions to use the proper wrapper type instead of 
ovn_flow. The only fields that are the same between ovn_installed_flow 
and ovn_desired_flow are the inner ovn_flow and the hmap_node.

The advantages to this type of structuring are:
* ovn_flow is reduced down to pure flow information. Information about 
how the flow is used is separated out. An ovn_flow can't directly be in 
a list or hmap. It has to be wrapped as the appropriate type of flow first.
* ovn_installed_flow can't directly be in a list. So you can't 
accidentally add an installed flow into installed->desired by accident.
* The former "peer" fields on each struct are typed to ensure that you 
cannot accidentally peer an installed flow with another installed flow 
or a desired flow with another desired flow.
* The structure fields have names that more accurately reflect what they 
are for. I'm not saying you have to use the names I came up with in this 
email, but you can see how the names have the possibility to better 
describe what the fields are used for.

IMO, using separate types is a win-win since the compiler can catch 
potential problems that might slip past our radar, and the code is 
better self-documenting. Also IMO, I think it's worth doing this now 
rather than putting the current version in and then adding the new types 
later.

> 
>  > >
>  > >       /* 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