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 <jrajaha...@nicira.com>
On Sep 17, 2013, at 4:08 PM, Ben Pfaff <b...@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));
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
