Re: [ovs-dev] [PATCH] ovn-controller: Remove flows created for now deleted SB database rows.

2016-08-12 Thread Ryan Moats 
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.

2016-08-12 Thread Ben Pfaff 
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.

2016-08-12 Thread Ben Pfaff 
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.

2016-07-28 Thread Ryan Moats 

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.

2016-07-28 Thread Ben Pfaff 
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 

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
___
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.

2016-07-28 Thread Ryan Moats 
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