Ben Pfaff <b...@ovn.org> wrote on 08/31/2016 04:26:37 PM:
> From: Ben Pfaff <b...@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: ovs dev <dev@openvswitch.org>, Jesse Gross <je...@kernel.org>
> Date: 08/31/2016 04:26 PM
> Subject: Re: [ovs-dev] [PATCH] ovn-controller: Convert encaps module
> back to full processing
>
> On Wed, Aug 31, 2016 at 12:59:05PM -0500, Ryan Moats wrote:
> > Ben Pfaff <b...@ovn.org> wrote on 08/31/2016 12:46:17 PM:
> >
> > > From: Ben Pfaff <b...@ovn.org>
> > > To: Jesse Gross <je...@kernel.org>
> > > Cc: Ryan Moats/Omaha/IBM@IBMUS, ovs dev <dev@openvswitch.org>
> > > Date: 08/31/2016 12:46 PM
> > > Subject: Re: [ovs-dev] [PATCH] ovn-controller: Convert encaps module
> > > back to full processing
> > >
> > > On Tue, Aug 30, 2016 at 09:04:10AM -0700, Jesse Gross wrote:
> > > > On Sun, Aug 28, 2016 at 3:51 PM, Ryan Moats <rmo...@us.ibm.com>
wrote:
> > > > > diff --git a/ovn/controller/encaps.c b/ovn/controller/encaps.c
> > > > > index d99ba05..844447f 100644
> > > > > --- a/ovn/controller/encaps.c
> > > > > +++ b/ovn/controller/encaps.c
> > > > > + SBREC_CHASSIS_FOR_EACH (chassis_rec, ctx->ovnsb_idl) {
> > > > > + for (int i = 0; i < chassis_rec->n_encaps; i++) {
> > > > > + encap_rec = chassis_rec->encaps[i];
> > > > > +
> > > > > + struct encap_hash_node *encap_hash_node;
> > > > > + encap_hash_node =
> > > lookup_encap_uuid(&encap_rec->header_.uuid);
> > > > > + if (encap_hash_node) {
> > > > > + /* A change might have invalidated our mapping.
> > > Process the
> > > > > + * new version and then iterate over everything
> > > to see if it
> > > > > + * is OK. */
> > > > > + delete_encap_uuid(encap_hash_node);
> > > > > + poll_immediate_wake();
> > > > > }
> > > >
> > > > Doesn't this result in essentially infinite wakeups? It used to be
> > > > that this loop would run only when a chassis was
add/removed/changed
> > > > and so the if (encap_hash_node) condition would only trigger when
an
> > > > existing chassis is modified. However, after this change every
wakeup
> > > > will loop through all chassises and any existing ones will trigger
> > > > another wakeup and loop, etc.
> > > >
> > > > In general, I don't think it makes sense to remove incremental
> > > > processing without removing persistent state. The result ends up
being
> > > > not very logical and actually more complicated.
> > >
> > > I want to second Jesse's feedback here. I think that this should
really
> > > be stateless, not half-incremental.
> >
> > Yes that poll_immediate_wake does result in infinite wakeups and yes
> > it has been removed from v2 of the patch set.
> >
> > I explained the situation in [1] and that "cry for help" still holds...
> >
> > [1] http://openvswitch.org/pipermail/dev/2016-August/078692.html
>
> I sent my suggestion as a 2-patch series:
> https://patchwork.ozlabs.org/patch/664685/
> https://patchwork.ozlabs.org/patch/664686/
>
I will give them a review first chance I get...
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
