[ovs-dev] [PATCH] ovn-controller: Add incremental processing for multicast groups

Flaviof flavio at flaviof.com
Mon Jul 25 01:39:47 UTC 2016


On Sun, Jul 24, 2016 at 8:20 PM, Ryan Moats <rmoats at us.ibm.com> wrote:

> Flaviof <flavio at flaviof.com> wrote on 07/24/2016 08:14:19 PM:
>
> > From: Flaviof <flavio at flaviof.com>
> > To: Ryan Moats/Omaha/IBM at IBMUS
> > Cc: ovs dev <dev at 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 <rmoats at 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 <rmoats at us.ibm.com>
> >
> > short of nitpicks below...
> >
> > Acked-by: Flavio Fernandes <flavio at 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



More information about the dev mailing list