[ovs-dev] [PATCH 7/7] ofproto-dpif: Restore metadata and registers on recirculation.

Ben Pfaff blp at nicira.com
Thu Mar 5 00:42:39 UTC 2015


Thanks for the extra info.

I have some more comments.

I think that the addition of 'rule_cookie' could easily be broken out
into a separate patch just before the main one.  If you agree, would
you mind doing that?  It took me a minute to verify that it was
essentially independent.

xlate_lookup_ofproto_() handles its two errors cases fairly
differently.  In the one it just returns NULL and doesn't bother with
the output arguments, in the other it fills out all the output
arguments.  This bothers me a bit; I'd feel better if they had the
same behavior.  (I lean toward just returning NULL without doing
anything else, since the callers don't look at them in that case and
because it would slightly shorten the code.)

After some more reading, I came up with the following comment.  Is it
correct?

    /* These are used for non-bond recirculation.  The recirculation IDs are
     * stored in xout and must be associated with a datapath flow (ukey),
     * otherwise they will be freed when the xout is uninitialized.
     *
     *
     * Steps in Recirculation Translation
     * ==================================
     *
     * At some point during translation, the code recognizes the need for
     * recirculation.  For example, recirculation is necessary when, after
     * popping the last MPLS label, an action or a match tries to examine or
     * modify a field that has been newly revealed following the MPLS label.
     *
     * The simplest part of the work to be done is to commit existing changes
     * to the packet and add an OVS_ACTION_ATTR_RECIRC action to the datapath
     * actions.
     *
     * The main problem here is preserving state.  When the datapath executes
     * OVS_ACTION_ATTR_RECIRC, it will upcall to userspace to get a translation
     * for the post-recirculation actions.  At this point userspace has to
     * resume the translation where it left off, which means that it has to
     * execute the following:
     *
     *     - The action that prompted recirculation, and any actions following
     *       it within the same flow.
     *
     *     - If the action that prompted recirculation was invoked within a
     *       NXAST_RESUBMIT, then any actions following the resubmit.  These
     *       "resubmit"s can be nested, so this has to go all the way up the
     *       control stack.
     *
     *     - The OpenFlow 1.1+ action set.
     *
     * State that actions and flow table lookups can depend on, such as the
     * following, must also be preserved:
     *
     *     - Metadata fields (input port, registers, OF1.1+ metadata, ...).
     *
     *     - The table ID and cookie of the flow being translated at each level
     *       of the control stack (since OFPAT_CONTROLLER actions send these to
     *       the controller).
     *
     * Translation allows for this state preservation through these members and
     * 'action_set'.  When a need for recirculation is identified, the
     * translation process:
     *
     * 1. Sets 'recirc_action_offset' to the current size of 'action_set'.  The
     *    action set is part of what needs to be preserved, so this allows the
     *    action set and the additional state to share the 'action_set' buffer.
     *    Later steps can tell that setup for recirculation is in progress from
     *    the nonnegative value of 'recirc_action_offset'.
     *
     * 2. Sets 'exit' to true to tell later steps that we're exiting from the
     *    translation process.
     *
     * 3. Adds an OFPACT_UNROLL_METADATA action to 'action_set'.  This action
     *    holds the current values of metadata fields so that they can be
     *    restored during a post-recirculation upcall translation.
     *
     * 4. Adds an OFPACT_UNROLL_XLATE action to 'action_set'.  This action
     *    holds the current table ID and cookie so that they can be restored
     *    during a post-recirculation upcall translation.
     *
     * 5. Adds the action that prompted recirculation and any actions following
     *    it within the same flow to 'action_set', so that they can be executed
     *    during a post-recirculation upcall translation.
     *
     * 6. Returns.
     *
     * 7. The action that prompted recirculation might be nested in a stack of
     *    nested "resubmit"s that have actions remaining.  Each of these
     *    notices that we're exiting (from 'exit') and that recirculation setup
     *    is in progress (from 'recirc_action_offset') and responds by adding
     *    more OFPACT_UNROLL_METADATA and OFPACT_UNROLL_XLATE actions to
     *    'action_set', as necessary, and any actions that were yet
     *    unprocessed.
     *
     * The caller stores all the state produced by this process associated with
     * the recirculation ID.  For post-recirculation upcall translation, the
     * caller passes it back in for the new translation to execute.  The
     * process yielded a set of ofpacts that can be translated directly, so it
     * is not much of a special case at that point.
     */

But I still don't yet understand the exact way that
compose_output_action__() and xlate_table_action() deal with
recirculation.  These functions add unroll actions to the action_set.
OK, but what's that good for?  It's not going to get used for
anything, is it?  I guess it's implicitly useful for
do_xlate_actions() if it adds remaining actions in the next loop
iteration, but I don't see why it's better to add the actions before
we return rather than right when we need them, more like this in
do_xlate_actions() and drop the processing elsewhere (and maybe we'd
need something like this to add unroll actions like this at the very
end of processing, too, for the benefit of the action set):

        if (!ctx->exit && ofpact_needs_recirculation_after_mpls(a, ctx)) {
            ctx->recirc_action_offset = ofpbuf_size(&ctx->action_set);
            ctx->exit = true;
        }

        if (ctx->exit) {
            /* Check if need to store the remaining actions for later
             * execution. */
            if (ctx->recirc_action_offset >= 0) {
                struct ofpact_unroll_xlate *unroll;

                ofpact_unroll_metadata_from_flow(
                    ofpact_put_UNROLL_METADATA(&ctx->action_set), flow);

                ctx->last_unroll_offset = ofpbuf_size(&ctx->action_set);
                unroll = ofpact_put_UNROLL_XLATE(&ctx->action_set);
                unroll->rule_table_id = ctx->table_id;
                unroll->rule_cookie = ctx->rule_cookie;

                ofpbuf_put(&ctx->action_set, a,
                           OFPACT_ALIGN(ofpacts_len -
                                        ((uint8_t *)a - (uint8_t *)ofpacts)));
            }
            break;
        }

If this actually works then it seems more efficient since I'd guess
that most "resubmit"s don't have any actions following them and when
that's the case this should avoid adding any unroll actions for each
level.



More information about the dev mailing list