All your comments sound reasonable, thanks. Ethan
On Wed, May 9, 2012 at 12:24 PM, Ben Pfaff <[email protected]> wrote: > 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
