On Tue, May 08, 2012 at 04:57:31PM -0700, Ethan Jackson wrote:
> Any particular reason we need to use a bitmask to store the slow path
> reason instead of a simple enum? It seems to me that every slow path
> reason is mutually exclusive. Would doing this simplify the code? I
> suppose the one exception I see is that perhaps something could be
> both lacp and match, for example. But in this case, just reporting it
> as lacp seems fine to me. Of course, if the code is simpler as it is
> here, the current approach seems fine as well.
Most of the reasons are mostly mutually exclusive. I've updated
comments to try to clear this up:
/* Reasons why a subfacet might not be fast-pathable. */
enum slow_path_reason {
/* These reasons are mutually exclusive. */
SLOW_CFM = 1 << 0, /* CFM packets need per-packet processing. */
SLOW_LACP = 1 << 1, /* LACP packets need per-packet processing. */
SLOW_STP = 1 << 2, /* STP packets need per-packet processing. */
SLOW_IN_BAND = 1 << 3, /* In-band control needs every packet. */
/* Mutually exclusive with SLOW_CFM, SLOW_LACP, SLOW_STP.
* Could possibly appear with SLOW_IN_BAND. */
SLOW_CONTROLLER = 1 << 4, /* Packets must go to OpenFlow controller. */
/* This can appear on its own, or, theoretically at least, along with any
* other combination of reasons. */
SLOW_MATCH = 1 << 5, /* Datapath can't match specifically enough. */
};
The main reason that I used a bitmap is that most of the code only
wants to know "does this need to go in the slowpath?" and for that
it's more convenient to have a single piece of data that one can test
with a single condition, e.g. "if (subfacet->slow)", rather than
needing two conditions, e.g. "if (subfacet->special ||
subfacet->key_fitness == ODP_FIT_TOO_LITTLE)". I tried something like
the latter out first, and even with a helper function I didn't like it
as much.
> > + for (bit = 1; ; bit <<= 1) {
>
> Here I think we want "for (bit = 1; bit; bit <<= 1) {"
Oops. I guess this needs a test. Coming up.
> > + /* The actions for slow-path flows may legitimately vary from
> > one
> > + * packet to the next. We're done. */
>
> Is this true? Seems to me that it's either something like a
> lacp/stp/cfm packet in which case the actions are simply "userspace",
> or its a slow_match in which case the actions are dictated by the
> openflow table and very on a per flow not per-packet basis. I may be
> missing something though.
VLAN splinters are the case I had in mind.
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
