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

[Jarno Rajahalme]

Small typo on the first line of the new comment.

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

On Sep 19, 2013, at 10:59 AM, Ben Pfaff <blp at nicira.com> wrote:

> 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. */