On Sun, Jul 24, 2016 at 8:20 PM, Ryan Moats <rmo...@us.ibm.com> wrote:
> Flaviof <fla...@flaviof.com> wrote on 07/24/2016 08:14:19 PM: > > > From: Flaviof <fla...@flaviof.com> > > To: Ryan Moats/Omaha/IBM@IBMUS > > Cc: ovs dev <dev@openvswitch.org> > > Date: 07/24/2016 08:14 PM > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Add incremental > > processing for multicast groups > > > > > On Sun, Jul 24, 2016 at 2:33 PM, Ryan Moats <rmo...@us.ibm.com> wrote: > > The various fixes in handling physical and binding changes > > have addressed the problems that the original attempt to > > incrementally process the multicast group table ran into. > > So, re-add incremental processing of the multicast group > > table. > > > > Notes: > > - This patch renders the short term changes in [1] taken > > for multicast group processing unnecessary (and so it > > conflicts with [1]). > > - Running through the ovn unit tests 50 times with -j4 > > didn't show increased test case failures over the > > baseline code prior to adding this patch. > > > > [1] http://patchwork.ozlabs.org/patch/652117/ > > > > Signed-off-by: Ryan Moats <rmo...@us.ibm.com> > > > > short of nitpicks below... > > > > Acked-by: Flavio Fernandes <fla...@flaviof.com> > > > > > > --- > > ovn/controller/physical.c | 26 +++++++++++++++++++++++--- > > 1 file changed, 23 insertions(+), 3 deletions(-) > > > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > > index a104e33..b10baed 100644 > > --- a/ovn/controller/physical.c > > +++ b/ovn/controller/physical.c > > @@ -61,11 +61,13 @@ static struct hmap tunnels = > HMAP_INITIALIZER(&tunnels); > > /* UUID to identify OF flows not associated with ovsdb rows. */ > > static struct uuid *hc_uuid = NULL; > > static bool full_binding_processing = false; > > +static bool full_mcgroup_processing = false; > > > > void > > physical_reset_processing(void) > > { > > full_binding_processing = true; > > + full_mcgroup_processing = true; > > } > > > > /* Maps from a chassis to the OpenFlow port number of the tunnel that > can be > > @@ -675,6 +677,7 @@ physical_run(struct controller_ctx *ctx, enum > > mf_field_id mff_ovn_geneve, > > tun->ofport = u16_to_ofp(ofport); > > tun->type = tunnel_type; > > > > nitpick... > > > > full_binding_processing = true; > > + full_mcgroup_processing = true; > > > > how about replacing the new line and the line above with: > > > > physical_reset_processing(); > > physical_reset_processing is there for other modules to call... > > I'm pretty "meh" on trading two lines of code for a potential > stack push and pop (depending on what the optimizer decides)... > > Ack. Like I said, it is a 'nitpick'. :) Just not sure if a new variable is going to be needed later on; in which case, all you would need would be to add it to the function. -- flaviof _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev