Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.
Ben Pfaff <b...@ovn.org> wrote on 08/12/2016 04:48:26 PM: > From: Ben Pfaff <b...@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 08/12/2016 04:48 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created > for now deleted SB database rows. > > On Fri, Aug 12, 2016 at 02:47:09PM -0700, Ben Pfaff wrote: > > On Thu, Jul 28, 2016 at 05:12:04PM -0500, Ryan Moats wrote: > > > > > > Ben Pfaff <b...@ovn.org> wrote on 07/28/2016 04:23:57 PM: > > > > > > > From: Ben Pfaff <b...@ovn.org> > > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > > Cc: dev@openvswitch.org > > > > Date: 07/28/2016 04:24 PM > > > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created > > > > for now deleted SB database rows. > > > > > > > > On Thu, Jul 28, 2016 at 07:54:30PM +, Ryan Moats wrote: > > > > > Ensure that rows created for deleted port binding and > > > > > multicast group rows are cleared when doing full processing. > > > > > > > > > > Signed-off-by: Ryan Moats <rmo...@us.ibm.com> > > > > > > > > I'm choosing to overlook storing UUIDs as strings in a set of strings. > > > > > > > > How about this simplification? > > > > > > > > --8<--cut here--> 8-- > > > > > > > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > > > > index 6bc0095..a4a8fcf 100644 > > > > --- a/ovn/controller/physical.c > > > > +++ b/ovn/controller/physical.c > > > > @@ -598,25 +598,22 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, > > > > sset_destroy(_chassis); > > > > } > > > > > > > > +/* Deletes the flows whose UUIDs are in 'old' but not 'new', and > > > > then replaces > > > > + * 'old' by 'new'. */ > > > > static void > > > > rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new) > > > > { > > > > -const char *uuid_s, *next_uuid; > > > > -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, old) { > > > > +const char *uuid_s; > > > > +SSET_FOR_EACH (uuid_s, old) { > > > > if (!sset_find(new, uuid_s)) { > > > > struct uuid uuid; > > > > if (uuid_from_string(, uuid_s)) { > > > > ofctrl_remove_flows(); > > > > } > > > > -sset_find_and_delete(old, uuid_s); > > > > } > > > > } > > > > -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, new) { > > > > -if (!sset_find(old, uuid_s)) { > > > > -sset_add(old, uuid_s); > > > > -} > > > > -sset_find_and_delete(new, uuid_s); > > > > -} > > > > +sset_swap(old, new); > > > > +sset_clear(new); > > > > } > > > > > > > > void > > > > > > > > > > Works for me... Ryan > > > > I noticed a memory leak in this patch, so I folded in the following and > > applied this to master. > > Oh, whoops, there was a newer version. I've un-applied this and I'll > look at the newer one. > Yeah, I decided to take you at your word in re using ssets for string representation of UUIDs and replaced that hack with hmaps of the actual uuids... ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.
On Fri, Aug 12, 2016 at 02:47:09PM -0700, Ben Pfaff wrote: > On Thu, Jul 28, 2016 at 05:12:04PM -0500, Ryan Moats wrote: > > > > Ben Pfaff <b...@ovn.org> wrote on 07/28/2016 04:23:57 PM: > > > > > From: Ben Pfaff <b...@ovn.org> > > > To: Ryan Moats/Omaha/IBM@IBMUS > > > Cc: dev@openvswitch.org > > > Date: 07/28/2016 04:24 PM > > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created > > > for now deleted SB database rows. > > > > > > On Thu, Jul 28, 2016 at 07:54:30PM +, Ryan Moats wrote: > > > > Ensure that rows created for deleted port binding and > > > > multicast group rows are cleared when doing full processing. > > > > > > > > Signed-off-by: Ryan Moats <rmo...@us.ibm.com> > > > > > > I'm choosing to overlook storing UUIDs as strings in a set of strings. > > > > > > How about this simplification? > > > > > > --8<--cut here-->8-- > > > > > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > > > index 6bc0095..a4a8fcf 100644 > > > --- a/ovn/controller/physical.c > > > +++ b/ovn/controller/physical.c > > > @@ -598,25 +598,22 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, > > > sset_destroy(_chassis); > > > } > > > > > > +/* Deletes the flows whose UUIDs are in 'old' but not 'new', and > > > then replaces > > > + * 'old' by 'new'. */ > > > static void > > > rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new) > > > { > > > -const char *uuid_s, *next_uuid; > > > -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, old) { > > > +const char *uuid_s; > > > +SSET_FOR_EACH (uuid_s, old) { > > > if (!sset_find(new, uuid_s)) { > > > struct uuid uuid; > > > if (uuid_from_string(, uuid_s)) { > > > ofctrl_remove_flows(); > > > } > > > -sset_find_and_delete(old, uuid_s); > > > } > > > } > > > -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, new) { > > > -if (!sset_find(old, uuid_s)) { > > > -sset_add(old, uuid_s); > > > -} > > > -sset_find_and_delete(new, uuid_s); > > > -} > > > +sset_swap(old, new); > > > +sset_clear(new); > > > } > > > > > > void > > > > > > > Works for me... Ryan > > I noticed a memory leak in this patch, so I folded in the following and > applied this to master. Oh, whoops, there was a newer version. I've un-applied this and I'll look at the newer one. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.
On Thu, Jul 28, 2016 at 05:12:04PM -0500, Ryan Moats wrote: > > Ben Pfaff <b...@ovn.org> wrote on 07/28/2016 04:23:57 PM: > > > From: Ben Pfaff <b...@ovn.org> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: dev@openvswitch.org > > Date: 07/28/2016 04:24 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created > > for now deleted SB database rows. > > > > On Thu, Jul 28, 2016 at 07:54:30PM +, Ryan Moats wrote: > > > Ensure that rows created for deleted port binding and > > > multicast group rows are cleared when doing full processing. > > > > > > Signed-off-by: Ryan Moats <rmo...@us.ibm.com> > > > > I'm choosing to overlook storing UUIDs as strings in a set of strings. > > > > How about this simplification? > > > > --8<--cut here-->8-- > > > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > > index 6bc0095..a4a8fcf 100644 > > --- a/ovn/controller/physical.c > > +++ b/ovn/controller/physical.c > > @@ -598,25 +598,22 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, > > sset_destroy(_chassis); > > } > > > > +/* Deletes the flows whose UUIDs are in 'old' but not 'new', and > > then replaces > > + * 'old' by 'new'. */ > > static void > > rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new) > > { > > -const char *uuid_s, *next_uuid; > > -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, old) { > > +const char *uuid_s; > > +SSET_FOR_EACH (uuid_s, old) { > > if (!sset_find(new, uuid_s)) { > > struct uuid uuid; > > if (uuid_from_string(, uuid_s)) { > > ofctrl_remove_flows(); > > } > > -sset_find_and_delete(old, uuid_s); > > } > > } > > -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, new) { > > -if (!sset_find(old, uuid_s)) { > > -sset_add(old, uuid_s); > > -} > > -sset_find_and_delete(new, uuid_s); > > -} > > +sset_swap(old, new); > > +sset_clear(new); > > } > > > > void > > > > Works for me... Ryan I noticed a memory leak in this patch, so I folded in the following and applied this to master. --8<--cut here-->8-- diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index b63aca2..c60611d 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -658,6 +658,14 @@ rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new) sset_clear(new); } +static void +add_uuid_to_sset(struct sset *set, const struct uuid *uuid) +{ +char uuid_str[UUID_LEN + 1]; +snprintf(uuid_str, sizeof uuid_str, UUID_FMT, UUID_ARGS(uuid)); +sset_add(set, uuid_str); +} + void physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, const struct ovsrec_bridge *br_int, const char *this_chassis_id, @@ -822,8 +830,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, ofctrl_remove_flows(>header_.uuid); consider_port_binding(mff_ovn_geneve, ct_zones, local_datapaths, patched_datapaths, binding, ); -sset_add(_port_binding_uuids, - xasprintf(UUID_FMT, UUID_ARGS(>header_.uuid))); +add_uuid_to_sset(_port_binding_uuids, >header_.uuid); } rationalize_ssets_and_delete_flows(_binding_uuids, _port_binding_uuids); @@ -856,8 +863,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, ofctrl_remove_flows(>header_.uuid); consider_mc_group(mff_ovn_geneve, ct_zones, local_datapaths, mc, , _ofpacts); -sset_add(_multicast_group_uuids, - xasprintf(UUID_FMT, UUID_ARGS(>header_.uuid))); +add_uuid_to_sset(_multicast_group_uuids, >header_.uuid); } rationalize_ssets_and_delete_flows(_group_uuids, _multicast_group_uuids); ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.
Ben Pfaff <b...@ovn.org> wrote on 07/28/2016 04:23:57 PM: > From: Ben Pfaff <b...@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 07/28/2016 04:24 PM > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created > for now deleted SB database rows. > > On Thu, Jul 28, 2016 at 07:54:30PM +, Ryan Moats wrote: > > Ensure that rows created for deleted port binding and > > multicast group rows are cleared when doing full processing. > > > > Signed-off-by: Ryan Moats <rmo...@us.ibm.com> > > I'm choosing to overlook storing UUIDs as strings in a set of strings. > > How about this simplification? > > --8<--cut here-->8-- > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > index 6bc0095..a4a8fcf 100644 > --- a/ovn/controller/physical.c > +++ b/ovn/controller/physical.c > @@ -598,25 +598,22 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, > sset_destroy(_chassis); > } > > +/* Deletes the flows whose UUIDs are in 'old' but not 'new', and > then replaces > + * 'old' by 'new'. */ > static void > rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new) > { > -const char *uuid_s, *next_uuid; > -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, old) { > +const char *uuid_s; > +SSET_FOR_EACH (uuid_s, old) { > if (!sset_find(new, uuid_s)) { > struct uuid uuid; > if (uuid_from_string(, uuid_s)) { > ofctrl_remove_flows(); > } > -sset_find_and_delete(old, uuid_s); > } > } > -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, new) { > -if (!sset_find(old, uuid_s)) { > -sset_add(old, uuid_s); > -} > -sset_find_and_delete(new, uuid_s); > -} > +sset_swap(old, new); > +sset_clear(new); > } > > void > Works for me... Ryan ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.
On Thu, Jul 28, 2016 at 07:54:30PM +, Ryan Moats wrote: > Ensure that rows created for deleted port binding and > multicast group rows are cleared when doing full processing. > > Signed-off-by: Ryan MoatsI'm choosing to overlook storing UUIDs as strings in a set of strings. How about this simplification? --8<--cut here-->8-- diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index 6bc0095..a4a8fcf 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -598,25 +598,22 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, sset_destroy(_chassis); } +/* Deletes the flows whose UUIDs are in 'old' but not 'new', and then replaces + * 'old' by 'new'. */ static void rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new) { -const char *uuid_s, *next_uuid; -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, old) { +const char *uuid_s; +SSET_FOR_EACH (uuid_s, old) { if (!sset_find(new, uuid_s)) { struct uuid uuid; if (uuid_from_string(, uuid_s)) { ofctrl_remove_flows(); } -sset_find_and_delete(old, uuid_s); } } -SSET_FOR_EACH_SAFE (uuid_s, next_uuid, new) { -if (!sset_find(old, uuid_s)) { -sset_add(old, uuid_s); -} -sset_find_and_delete(new, uuid_s); -} +sset_swap(old, new); +sset_clear(new); } void ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.
Ensure that rows created for deleted port binding and multicast group rows are cleared when doing full processing. Signed-off-by: Ryan Moats--- ovn/controller/physical.c | 39 +++ 1 file changed, 39 insertions(+) diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c index 2e9fb73..9e9343d 100644 --- a/ovn/controller/physical.c +++ b/ovn/controller/physical.c @@ -58,6 +58,10 @@ static struct simap localvif_to_ofport = SIMAP_INITIALIZER(_to_ofport); static struct hmap tunnels = HMAP_INITIALIZER(); +static struct sset port_binding_uuids = SSET_INITIALIZER(_binding_uuids); +static struct sset multicast_group_uuids = +SSET_INITIALIZER(_group_uuids); + /* UUID to identify OF flows not associated with ovsdb rows. */ static struct uuid *hc_uuid = NULL; static bool full_binding_processing = false; @@ -594,6 +598,27 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve, sset_destroy(_chassis); } +static void +rationalize_ssets_and_delete_flows(struct sset *old, struct sset *new) +{ +const char *uuid_s, *next_uuid; +SSET_FOR_EACH_SAFE (uuid_s, next_uuid, old) { +if (!sset_find(new, uuid_s)) { +struct uuid uuid; +if (uuid_from_string(, uuid_s)) { +ofctrl_remove_flows(); +} +sset_find_and_delete(old, uuid_s); +} +} +SSET_FOR_EACH_SAFE (uuid_s, next_uuid, new) { +if (!sset_find(old, uuid_s)) { +sset_add(old, uuid_s); +} +sset_find_and_delete(new, uuid_s); +} +} + void physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, const struct ovsrec_bridge *br_int, const char *this_chassis_id, @@ -750,6 +775,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, * 64 for logical-to-physical translation. */ const struct sbrec_port_binding *binding; if (full_binding_processing) { +struct sset new_port_binding_uuids = +SSET_INITIALIZER(_port_binding_uuids); SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) { /* Because it is possible in the above code to enter this * for loop without having cleared the flow table first, we @@ -757,7 +784,12 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, ofctrl_remove_flows(>header_.uuid); consider_port_binding(mff_ovn_geneve, ct_zones, local_datapaths, patched_datapaths, binding, ); +sset_add(_port_binding_uuids, + xasprintf(UUID_FMT, UUID_ARGS(>header_.uuid))); } +rationalize_ssets_and_delete_flows(_binding_uuids, + _port_binding_uuids); +sset_destroy(_port_binding_uuids); full_binding_processing = false; } else { SBREC_PORT_BINDING_FOR_EACH_TRACKED (binding, ctx->ovnsb_idl) { @@ -777,6 +809,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, const struct sbrec_multicast_group *mc; struct ofpbuf remote_ofpacts; ofpbuf_init(_ofpacts, 0); +struct sset new_multicast_group_uuids = +SSET_INITIALIZER(_multicast_group_uuids); SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) { /* As multicast groups are always reprocessed each time, * the first step is to clean the old flows for the group @@ -784,7 +818,12 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve, ofctrl_remove_flows(>header_.uuid); consider_mc_group(mff_ovn_geneve, ct_zones, local_datapaths, mc, , _ofpacts); +sset_add(_multicast_group_uuids, + xasprintf(UUID_FMT, UUID_ARGS(>header_.uuid))); } +rationalize_ssets_and_delete_flows(_group_uuids, + _multicast_group_uuids); +sset_destroy(_multicast_group_uuids); ofpbuf_uninit(_ofpacts); -- 1.9.1 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev