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.
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
> >
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
