[ovs-dev] [slow path 10/11] ofproto-dpif: Introduce "slow path" datapath flows.

[Ethan Jackson]

All your comments sound reasonable, thanks.

Ethan

On Wed, May 9, 2012 at 12:24 PM, Ben Pfaff <blp at nicira.com> 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.