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

[Jarno Rajahalme]

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>

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?)
 *

> +     *
> +     * The loop fills 'ops' with an array of operations to execute in the
> +     * datapath. */
>     n_ops = 0;
> -    HMAP_FOR_EACH (miss, hmap_node, &fmb->misses) {
> -        execute_flow_miss(miss, ops, &n_ops);
> +    LIST_FOR_EACH (upcall, list_node, upcalls) {
> +        struct flow_miss *miss = upcall->flow_miss;
> +        struct ofpbuf *packet = upcall->dpif_upcall.packet;
> +
> +        if (miss->xout.slow) {
> +            struct rule_dpif *rule;
> +            struct xlate_in xin;
> +
> +            rule_dpif_lookup(miss->ofproto, &miss->flow, NULL, &rule);
> +            xlate_in_init(&xin, miss->ofproto, &miss->flow, rule, 0, packet);
> +            xlate_actions_for_side_effects(&xin);
> +            rule_dpif_unref(rule);
> +        }
> +
> +        if (miss->xout.odp_actions.size) {
> +            struct dpif_op *op;
> +
> +            if (miss->flow.in_port.ofp_port
> +                != vsp_realdev_to_vlandev(miss->ofproto,
> +                                          miss->flow.in_port.ofp_port,
> +                                          miss->flow.vlan_tci)) {
> +                /* This packet was received on a VLAN splinter port.  We
> +                 * added a VLAN to the packet to make the packet resemble
> +                 * the flow, but the actions were composed assuming that
> +                 * the packet contained no VLAN.  So, we must remove the
> +                 * VLAN header from the packet before trying to execute the
> +                 * actions. */
> +                eth_pop_vlan(packet);
> +            }
> +
> +            op = &ops[n_ops++];
> +            op->type = DPIF_OP_EXECUTE;
> +            op->u.execute.key = miss->key;
> +            op->u.execute.key_len = miss->key_len;
> +            op->u.execute.packet = packet;
> +            op->u.execute.actions = miss->xout.odp_actions.data;
> +            op->u.execute.actions_len = miss->xout.odp_actions.size;
> +        }
>     }
> -    ovs_assert(n_ops <= ARRAY_SIZE(ops));