On Fri, Sep 4, 2020 at 6:12 AM Mark Michelson <[email protected]> wrote:
>
> On 9/3/20 5:44 PM, Han Zhou wrote:
> >
> >
> > On Thu, Sep 3, 2020 at 12:47 PM Mark Michelson <[email protected]
> > <mailto:[email protected]>> 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 <[email protected] <mailto:[email protected]>>
> > > > ---
> > > > 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.
>
My bad. For "APIs" I meant internal (static) interfaces such as
alloc/lookup/destroy/dup. It is not a big deal though.
> 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.
>
I agree. I will do it now and send a v2.
> >
> > > >
> > > > /* 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
> > > >
> > >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
