On Fri, Sep 4, 2020 at 11:50 AM Han Zhou <[email protected]> wrote: > > > > 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.
Please take a look at v2 here: https://patchwork.ozlabs.org/project/ovn/list/?series=199854 It took more time than I expected but it does look cleaner. > > > > > > > > > > > > > > /* 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
