[ovs-dev] [reordering v2 1/2] ofproto-dpif-upcall: Forward packets in order of arrival.

[Ben Pfaff]

On Tue, Sep 17, 2013 at 05:24:02PM -0700, Jarno Rajahalme wrote:
> I like this! Much clearer, especially when comparing to the upcall
> processing in the past monolithic ofproto-dpif.
> 
> One small comment about a comment below,
> 
>   Jarno
> 
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

Thanks for the review, see below also.

> On Sep 17, 2013, at 4:08 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> > 
> > -    /* Process each element in the to-do list, constructing the set of
> > -     * operations to batch. */
> > +    /* Now handle each packet individually:
> > +     *
> > +     *   - In the common case, the packet is a member of a flow that doesn't
> > +     *     need per-packet translation.  We already did the translation in the
> > +     *     previous loop, so reuse it.
> > +     *
> > +     *   - Otherwise, we need to do another translation just for this
> > +     *     packet.
> 
> I would find it a bit clearer, if the above comment was replaced with something like this:
> 
> /* Now handle each packet in the order they were received:
>  *
>  *  - In the common case each packet of a miss can share the same actions
>  *
>  *  - Slow-pathed packets, however, need to be translated individually. (it would be nice to know why?)
>  *

How about this:

    /* Now handle the packets individuallys in order of arrival.  In the common
     * case each packet of a miss can share the same actions, but slow-pathed
     * packets need to be translated individually:
     *
     *   - For SLOW_CFM, SLOW_LACP, SLOW_STP, and SLOW_BFD, translation is what
     *     processes received packets for these protocols.
     *
     *   - For SLOW_CONTROLLER, translation sends the packet to the OpenFlow
     *     controller.
     *
     * The loop fills 'ops' with an array of operations to execute in the
     * datapath. */