[ovs-dev] [PATCH 2/2] ofproto: Make NXAST_RESUBMIT take header modifications into account.

Justin Pettit jpettit at nicira.com
Tue Apr 13 07:59:24 UTC 2010


This looks fine to me.  We will need to be careful that any new vendor actions that modify the flow also do this.  For example, Jesse will be adding a "set GRE key" vendor action very soon.  For now, I think it would be good to at least document this in the OFPAT_VENDOR case statement, and probably xlate_nicira_action() for good measure.  It just seems like the type of thing that could be easily overlooked.

--Justin


On Apr 9, 2010, at 3:48 PM, Ben Pfaff wrote:

> Until now, the NXAST_RESUBMIT action has always looked up the original
> flow except for the updated in_port.  This commit changes the semantics to
> instead look up the flow as modified by any preceding actions that affect
> it, e.g. if OFPAT_SET_VLAN_VID precedes NXAST_RESUBMIT, then NXAST_RESUBMIT
> now looks up the flow with the modified VLAN, not the original (as well as
> the modified in_port).
> 
> Also, document how NXAST_RESUBMIT is supposed to work.
> 
> Suggested-by: Paul Ingram <paul at nicira.com>
> ---
> include/openflow/nicira-ext.h |   23 ++++++++++++++++++++++-
> ofproto/ofproto.c             |   20 +++++++++++++-------
> 2 files changed, 35 insertions(+), 8 deletions(-)
> 
> diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
> index 7232d57..535cfc3 100644
> --- a/include/openflow/nicira-ext.h
> +++ b/include/openflow/nicira-ext.h
> @@ -57,7 +57,28 @@ OFP_ASSERT(sizeof(struct nicira_header) == 16);
> 
> enum nx_action_subtype {
>     NXAST_SNAT__OBSOLETE,           /* No longer used. */
> -    NXAST_RESUBMIT                  /* Throw against flow table again. */
> +
> +    /* Searches the flow table again, using a flow that is slightly modified
> +     * from the original lookup:
> +     *
> +     *    - The flow's in_port is changed to that specified in the 'in_port'
> +     *      member of struct nx_action_resubmit.
> +     *
> +     *    - If NXAST_RESUBMIT is preceded by actions that affect the flow
> +     *      (e.g. OFPAT_SET_VLAN_VID), then the flow is updated with the new
> +     *      values.
> +     *
> +     * If the modified flow matches in the flow table, then the corresponding
> +     * actions are executed, except that NXAST_RESUBMIT actions found in the
> +     * secondary set of actions are ignored.  Afterward, actions following
> +     * NXAST_RESUBMIT in the original set of actions, if any, are executed; any
> +     * changes made to the packet (e.g. changes to VLAN) by secondary actions
> +     * persist when those actions are executed, although the original in_port
> +     * is restored.
> +     *
> +     * NXAST_RESUBMIT may be used any number of times within a set of actions.
> +     */
> +    NXAST_RESUBMIT
> };
> 
> /* Action structure for NXAST_RESUBMIT. */
> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> index 9ddf6b1..4c62d3f 100644
> --- a/ofproto/ofproto.c
> +++ b/ofproto/ofproto.c
> @@ -2183,53 +2183,59 @@ do_xlate_actions(const union ofp_action *in, size_t n_in,
> 
>         case OFPAT_SET_VLAN_VID:
>             oa = odp_actions_add(ctx->out, ODPAT_SET_VLAN_VID);
> -            oa->vlan_vid.vlan_vid = ia->vlan_vid.vlan_vid;
> +            ctx->flow.dl_vlan = oa->vlan_vid.vlan_vid = ia->vlan_vid.vlan_vid;
>             break;
> 
>         case OFPAT_SET_VLAN_PCP:
>             oa = odp_actions_add(ctx->out, ODPAT_SET_VLAN_PCP);
> -            oa->vlan_pcp.vlan_pcp = ia->vlan_pcp.vlan_pcp;
> +            ctx->flow.dl_vlan_pcp = oa->vlan_pcp.vlan_pcp = ia->vlan_pcp.vlan_pcp;
>             break;
> 
>         case OFPAT_STRIP_VLAN:
>             odp_actions_add(ctx->out, ODPAT_STRIP_VLAN);
> +            ctx->flow.dl_vlan = OFP_VLAN_NONE;
> +            ctx->flow.dl_vlan_pcp = 0;
>             break;
> 
>         case OFPAT_SET_DL_SRC:
>             oa = odp_actions_add(ctx->out, ODPAT_SET_DL_SRC);
>             memcpy(oa->dl_addr.dl_addr,
>                    ((struct ofp_action_dl_addr *) ia)->dl_addr, ETH_ADDR_LEN);
> +            memcpy(ctx->flow.dl_src,
> +                   ((struct ofp_action_dl_addr *) ia)->dl_addr, ETH_ADDR_LEN);
>             break;
> 
>         case OFPAT_SET_DL_DST:
>             oa = odp_actions_add(ctx->out, ODPAT_SET_DL_DST);
>             memcpy(oa->dl_addr.dl_addr,
>                    ((struct ofp_action_dl_addr *) ia)->dl_addr, ETH_ADDR_LEN);
> +            memcpy(ctx->flow.dl_dst,
> +                   ((struct ofp_action_dl_addr *) ia)->dl_addr, ETH_ADDR_LEN);
>             break;
> 
>         case OFPAT_SET_NW_SRC:
>             oa = odp_actions_add(ctx->out, ODPAT_SET_NW_SRC);
> -            oa->nw_addr.nw_addr = ia->nw_addr.nw_addr;
> +            ctx->flow.nw_src = oa->nw_addr.nw_addr = ia->nw_addr.nw_addr;
>             break;
> 
>         case OFPAT_SET_NW_DST:
>             oa = odp_actions_add(ctx->out, ODPAT_SET_NW_DST);
> -            oa->nw_addr.nw_addr = ia->nw_addr.nw_addr;
> +            ctx->flow.nw_dst = oa->nw_addr.nw_addr = ia->nw_addr.nw_addr;
>             break;
> 
>         case OFPAT_SET_NW_TOS:
>             oa = odp_actions_add(ctx->out, ODPAT_SET_NW_TOS);
> -            oa->nw_tos.nw_tos = ia->nw_tos.nw_tos;
> +            ctx->flow.nw_tos = oa->nw_tos.nw_tos = ia->nw_tos.nw_tos;
>             break;
> 
>         case OFPAT_SET_TP_SRC:
>             oa = odp_actions_add(ctx->out, ODPAT_SET_TP_SRC);
> -            oa->tp_port.tp_port = ia->tp_port.tp_port;
> +            ctx->flow.tp_src = oa->tp_port.tp_port = ia->tp_port.tp_port;
>             break;
> 
>         case OFPAT_SET_TP_DST:
>             oa = odp_actions_add(ctx->out, ODPAT_SET_TP_DST);
> -            oa->tp_port.tp_port = ia->tp_port.tp_port;
> +            ctx->flow.tp_dst = oa->tp_port.tp_port = ia->tp_port.tp_port;
>             break;
> 
>         case OFPAT_VENDOR:
> -- 
> 1.6.6.1
> 
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev_openvswitch.org





More information about the dev mailing list