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

Jarno Rajahalme jrajahalme at nicira.com
Fri Mar 6 23:10:19 UTC 2015


> On Mar 4, 2015, at 4:42 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> 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.

Done.

> 
> 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.)

OK, just returning NULL. I checked all users and they all bail out on error return.

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

This is not only correct but GREAT explanation of the process. Thanks!

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

I changed this paragraph to read:

    * The simplest part of the work to be done is to commit existing changes to
    * the packet, which produces datapath actions corresponding to the changes,
    * and after this, add an OVS_ACTION_ATTR_RECIRC datapath action.

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

Good point. The location in the code where I added the insertion of the unroll actions is where the corresponding values in struct flow and the translation context are restored, as it is more obviously correct to do that. However, as the struct flow and translation context values are restored, we could postpone the insertion of the unroll actions, based on those restored values, later in the process, where we have more information regarding if they are needed or not.

However, your proposed change below will insert multiple UNROLL_METADATA actions, one for each resubmit level. If any of the actions stored for a previous level changed the metadata values, the additional UNROLL_METADATA would undo those changes.

In summary, the UNROLL_METADATA should be inserted once for each bridge, initially to be executed as the first action post-recirculation, and after each return from a patch port output, as we do not want the metadata values from one bridge to leak into another. But I now see that I missed the restoration of the bridge itself after crossing back from a patch port. For example, if output to a patch port (connecting br0 to br1) recirculates, we can have these post-recirculation actions:

UNROLL_METADATA(on br1), <remaining actions on br1, e.g. OUTPUT>, UNROLL_METADATA(on br0), <remaining actions on br0, e.g., OUTPUT>

From this it should be clear that the ctx->xbridge needs to be restored by the second UNROLL_METADATA. It is implicitly restored in the beginning (before the 1st UNROLL_METADATA), as part of the recirc_id lookup process. To make this work I need to move the ofproto pointer from the recirc ID context to the UNROLL_METADATA action.

UNROLL_METADATA can be skipped if there are no remaining actions for the whole bridge. This could be done by noting that UNROLL_METADATA is needed, and then inserting it before the first action that may depend on the metadata is copied over.

Similarly, at most one UNROLL_XLATE is needed for each resubmit recursion level. It can be skipped if there are no remaining actions on that level or if none of those action can trigger packet in messages.

  Jarno

> 
>        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