[ovs-dev] [PATCH 2/2] ofproto-dpif: Add coverage counters for facet revalidation.

Ben Pfaff blp at nicira.com
Thu Jun 21 15:27:11 UTC 2012


On Wed, Jun 20, 2012 at 08:44:01PM -0700, Justin Pettit wrote:
> 
> On Jun 14, 2012, at 3:59 PM, Ben Pfaff wrote:
> 
> > forward_bpdu_changed(struct ofproto *ofproto_)
> > {
> >     struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
> > -    /* Revalidate cached flows whenever forward_bpdu option changes. */
> > -    ofproto->need_revalidate = true;
> > +
> > +    ofproto->need_revalidate = REV_RECONFIGURE;
> > }
> 
> If you want to make it similar to the other functions in that part of
> the file, you could leave out the blank line.

Done.

> Setting the "need_revalidate" to false now looks a little odd.  It
> might be nice to either define an enum for that, or at least document
> the behavior in the enum definition.

I added a paragraph to the enum comment:

 * A value of 0 means that there is no need to revalidate.

Is that good?

> I didn't check this, but is it possible that one thing that causes a
> revalidate would always trigger another?  For example, if a
> REV_PORT_TOGGLED always ended up triggering a REV_RECONFIGURE down the
> line, then you might never see the REV_PORT_TOGGLED, since it's just
> an enum.  As I said, I don't know that this could actually occur.

Hmm.  I don't *think* this would occur.  REV_RECONFIGURE generally means
that the database changed.  I don't think that REV_PORT_TOGGLED would
cause that.  I don't see other obvious dependencies, either.

I did think about using a bit-mask instead of discrete enum values.  The
problem with that is that you'd lose the ability to clearly interpret
the coverage counters: currently each rev_* count means that the flow
table was revalidated once for one particular reason, but if multiple
rev_* counters can be incremented on each revalidation then you'd lose
that information.  You could have a separate counter for total flow
revalidations, I guess.

Thanks for the reviews.



More information about the dev mailing list