[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