On Fri, Jun 13, 2014 at 09:31:47AM +0900, Simon Horman wrote:
> On Thu, Jun 12, 2014 at 01:17:21PM -0700, Ben Pfaff wrote:
> > On Wed, Jun 11, 2014 at 09:28:06AM +0900, Simon Horman wrote:
> > > In some cases an pop MPLS action changes a packet to be a non-mpls packet.
> > > In this case subsequent any L3+ actions require access to portions
> > > of the packet which were not decoded as they were opaque when the
> > > packet was MPLS. Allow such actions to be translated by
> > > first recirculating the packet.
> > >
> > > Signed-off-by: Simon Horman <ho...@verge.net.au>
> >
> > Weaving the was_mpls checks into do_xlate_actions() makes the code
> > harder to read. It also might be harder to maintain as we add new
> > kinds of actions, since it adds something else that one must consider
> > during translation, that is easy to forget about.
> >
> > Can we use a separate function to do these checks? Then it is nicely
> > separated from the main translation code and the compiler will remind
> > us when we add a new action.
> >
> > Here is my idea. Does it work? Will you review it?
>
> Hi Ben,
>
> thanks, this looks very clean to me.
>
> I found one problem in your proposal, which I will describe with the
> following incremental patch. I'm not sure if there is a better way to
> handle goto_table after recirculation.
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 8f59085..76bc186 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3544,7 +3544,12 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t
> ofpacts_len,
> case OFPACT_GOTO_TABLE: {
> struct ofpact_goto_table *ogt = ofpact_get_GOTO_TABLE(a);
>
> - ovs_assert(ctx->table_id < ogt->table_id);
> + /* Allow ctx->table_id == TBL_INTERNAL, which will be greater
> + * than ogt->table_id. This is to allow goto_table actions that
> + * triggered recirculation: ctx->table_id will be TBL_INTERNAL
> + * after recirculation. */
> + ovs_assert(ctx->table_id == TBL_INTERNAL
> + || ctx->table_id < ogt->table_id);
> xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
> ogt->table_id, true, true);
> break;
>
>
> A second problem I found, which is not strictly related to your proposal, is
> that dd51dae29bccca3 ("ofproto: Move logic for collecting read-only rules
> into rule_criteria.") seems to cause some tests to fail with this series
> applied. I assume it relates to tests added by this series.
>
> This occurs both with my original version of this patch and your revised
> version. I will investigate further.
I have proposed a fix for this problem in
"ofproto: Initialise return value of modify_flows__"
>
>
> >
> > Thanks,
> >
> > Ben.
> >
> > --8<--------------------------cut here-------------------------->8--
> >
> > From: Simon Horman <ho...@verge.net.au>
> > Date: Wed, 11 Jun 2014 09:28:06 +0900
> > Subject: [PATCH] ofproto-dpif: MPLS recirculation
> >
> > In some cases an pop MPLS action changes a packet to be a non-mpls packet.
> > In this case subsequent any L3+ actions require access to portions
> > of the packet which were not decoded as they were opaque when the
> > packet was MPLS. Allow such actions to be translated by
> > first recirculating the packet.
> >
> > Signed-off-by: Simon Horman <ho...@verge.net.au>
> > Co-authored-by: Ben Pfaff <b...@nicira.com>
> > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > ---
> > ofproto/ofproto-dpif-xlate.c | 159
> > +++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 158 insertions(+), 1 deletion(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 71eaad1..8f59085 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -61,6 +61,9 @@ VLOG_DEFINE_THIS_MODULE(ofproto_dpif_xlate);
> > #define MAX_INTERNAL_RESUBMITS 1 /* Max resbmits allowed using rules in
> > internal table. */
> >
> > +/* Timeout for internal rules created to handle recirculation */
> > +#define RECIRC_TIMEOUT 60
> > +
> > /* Maximum number of resubmit actions in a flow translation, whether they
> > are
> > * recursive or not. */
> > #define MAX_RESUBMITS (MAX_RESUBMIT_RECURSION * MAX_RESUBMIT_RECURSION)
> > @@ -194,6 +197,12 @@ struct xlate_ctx {
> > struct xlate_recirc recirc; /* Information used for generating
> > * recirculation actions */
> >
> > + /* True if a packet was but is no longer MPLS (due to an MPLS pop
> > action).
> > + * This is a trigger for recirculation in cases where translating an
> > action
> > + * or looking up a flow requires access to the fields of the packet
> > after
> > + * the MPLS label stack that was originally present. */
> > + bool was_mpls;
> > +
> > /* OpenFlow 1.1+ action set.
> > *
> > * 'action_set' accumulates "struct ofpact"s added by
> > OFPACT_WRITE_ACTIONS.
> > @@ -2695,6 +2704,67 @@ execute_controller_action(struct xlate_ctx *ctx, int
> > len,
> > }
> >
> > static void
> > +compose_recirculate_action(struct xlate_ctx *ctx,
> > + const struct ofpact *ofpacts_base,
> > + const struct ofpact *ofpact_current,
> > + size_t ofpacts_base_len)
> > +{
> > + uint32_t id;
> > + int error;
> > + unsigned ofpacts_len;
> > + struct match match;
> > + struct rule *rule;
> > + struct ofpbuf ofpacts;
> > +
> > + ofpacts_len = ofpacts_base_len -
> > + ((uint8_t *)ofpact_current - (uint8_t *)ofpacts_base);
> > +
> > + if (ctx->rule) {
> > + id = rule_dpif_get_recirc_id(ctx->rule);
> > + } else {
> > + /* In the case where ctx has no rule then allocate a recirc id.
> > + * The life-cycle of this recirc id is managed by associating it
> > + * with the internal rule that is created to to handle
> > + * recirculation below.
> > + *
> > + * The known use-case of this is packet_out which
> > + * translates actions without a rule */
> > + id = ofproto_dpif_alloc_recirc_id(ctx->xbridge->ofproto);
> > + }
> > + if (!id) {
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > + VLOG_ERR_RL(&rl, "Failed to allocate recirculation id");
> > + ctx->exit = true;
> > + return;
> > + }
> > +
> > + match_init_catchall(&match);
> > + match_set_recirc_id(&match, id);
> > + ofpbuf_use_const(&ofpacts, ofpact_current, ofpacts_len);
> > + error = ofproto_dpif_add_internal_flow(ctx->xbridge->ofproto, &match,
> > + RECIRC_RULE_PRIORITY,
> > + RECIRC_TIMEOUT, &ofpacts,
> > &rule);
> > + if (error) {
> > + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > + VLOG_ERR_RL(&rl, "Failed to add post recirculation flow %s",
> > + match_to_string(&match, 0));
> > + ctx->exit = true;
> > + return;
> > + }
> > + /* If ctx has no rule then associate the recirc id, which
> > + * was allocated above, with the internal rule. This allows
> > + * the recirc id to be released when the internal rule times out. */
> > + if (!ctx->rule) {
> > + rule_set_recirc_id(rule, id);
> > + }
> > +
> > + ctx->xout->slow |= commit_odp_actions(&ctx->xin->flow, &ctx->base_flow,
> > + &ctx->xout->odp_actions,
> > + &ctx->xout->wc);
> > + nl_msg_put_u32(&ctx->xout->odp_actions, OVS_ACTION_ATTR_RECIRC, id);
> > +}
> > +
> > +static void
> > compose_mpls_push_action(struct xlate_ctx *ctx, struct ofpact_push_mpls
> > *mpls)
> > {
> > struct flow_wildcards *wc = &ctx->xout->wc;
> > @@ -2733,7 +2803,11 @@ compose_mpls_pop_action(struct xlate_ctx *ctx,
> > ovs_be16 eth_type)
> > struct flow *flow = &ctx->xin->flow;
> > int n = flow_count_mpls_labels(flow, wc);
> >
> > - if (!flow_pop_mpls(flow, n, eth_type, wc) && n >=
> > FLOW_MAX_MPLS_LABELS) {
> > + if (flow_pop_mpls(flow, n, eth_type, wc)) {
> > + if (ctx->xbridge->enable_recirc && !eth_type_mpls(eth_type)) {
> > + ctx->was_mpls = true;
> > + }
> > + } else if (n >= FLOW_MAX_MPLS_LABELS) {
> > if (ctx->xin->packet != NULL) {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > VLOG_WARN_RL(&rl, "bridge %s: dropping packet on which an "
> > @@ -3111,6 +3185,83 @@ xlate_action_set(struct xlate_ctx *ctx)
> > ofpbuf_uninit(&action_list);
> > }
> >
> > +static bool
> > +ofpact_needs_recirculation_after_mpls(const struct xlate_ctx *ctx,
> > + const struct ofpact *a)
> > +{
> > + struct flow_wildcards *wc = &ctx->xout->wc;
> > + struct flow *flow = &ctx->xin->flow;
> > +
> > + switch (a->type) {
> > + case OFPACT_OUTPUT:
> > + case OFPACT_GROUP:
> > + case OFPACT_CONTROLLER:
> > + case OFPACT_STRIP_VLAN:
> > + case OFPACT_SET_VLAN_PCP:
> > + case OFPACT_SET_VLAN_VID:
> > + case OFPACT_ENQUEUE:
> > + case OFPACT_PUSH_VLAN:
> > + case OFPACT_SET_ETH_SRC:
> > + case OFPACT_SET_ETH_DST:
> > + case OFPACT_SET_TUNNEL:
> > + case OFPACT_SET_QUEUE:
> > + case OFPACT_POP_QUEUE:
> > + case OFPACT_POP_MPLS:
> > + case OFPACT_DEC_MPLS_TTL:
> > + case OFPACT_SET_MPLS_TTL:
> > + case OFPACT_SET_MPLS_TC:
> > + case OFPACT_SET_MPLS_LABEL:
> > + case OFPACT_NOTE:
> > + case OFPACT_OUTPUT_REG:
> > + case OFPACT_EXIT:
> > + case OFPACT_METER:
> > + case OFPACT_WRITE_METADATA:
> > + case OFPACT_WRITE_ACTIONS:
> > + case OFPACT_CLEAR_ACTIONS:
> > + case OFPACT_SAMPLE:
> > + return false;
> > +
> > + case OFPACT_SET_IPV4_SRC:
> > + case OFPACT_SET_IPV4_DST:
> > + case OFPACT_SET_IP_DSCP:
> > + case OFPACT_SET_IP_ECN:
> > + case OFPACT_SET_IP_TTL:
> > + case OFPACT_SET_L4_SRC_PORT:
> > + case OFPACT_SET_L4_DST_PORT:
> > + case OFPACT_RESUBMIT:
> > + case OFPACT_STACK_PUSH:
> > + case OFPACT_STACK_POP:
> > + case OFPACT_DEC_TTL:
> > + case OFPACT_MULTIPATH:
> > + case OFPACT_BUNDLE:
> > + case OFPACT_LEARN:
> > + case OFPACT_FIN_TIMEOUT:
> > + case OFPACT_GOTO_TABLE:
> > + return true;
> > +
> > + case OFPACT_REG_MOVE:
> > + return (mf_is_l3_or_higher(ofpact_get_REG_MOVE(a)->dst.field) ||
> > + mf_is_l3_or_higher(ofpact_get_REG_MOVE(a)->src.field));
> > +
> > + case OFPACT_REG_LOAD:
> > + return mf_is_l3_or_higher(ofpact_get_REG_LOAD(a)->dst.field);
> > +
> > + case OFPACT_SET_FIELD:
> > + return mf_is_l3_or_higher(ofpact_get_SET_FIELD(a)->field);
> > +
> > + case OFPACT_PUSH_MPLS:
> > + /* Recirculate if it is an IP packet with a zero ttl. This may
> > + * indicate that the packet was previously MPLS and an MPLS pop
> > action
> > + * converted it to IP. In this case recirculating should reveal
> > the IP
> > + * TTL which is used as the basis for a new MPLS LSE. */
> > + return (!flow_count_mpls_labels(flow, wc)
> > + && flow->nw_ttl == 0
> > + && is_ip_any(flow));
> > + }
> > +
> > + OVS_NOT_REACHED();
> > +}
> > +
> > static void
> > do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
> > struct xlate_ctx *ctx)
> > @@ -3131,6 +3282,11 @@ do_xlate_actions(const struct ofpact *ofpacts,
> > size_t ofpacts_len,
> > break;
> > }
> >
> > + if (ctx->was_mpls && ofpact_needs_recirculation_after_mpls(ctx,
> > a)) {
> > + compose_recirculate_action(ctx, ofpacts, a, ofpacts_len);
> > + return;
> > + }
> > +
> > switch (a->type) {
> > case OFPACT_OUTPUT:
> > xlate_output_action(ctx, ofpact_get_OUTPUT(a)->port,
> > @@ -3607,6 +3763,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out
> > *xout)
> > ctx.table_id = 0;
> > ctx.exit = false;
> > ctx.use_recirc = false;
> > + ctx.was_mpls = false;
> >
> > if (!xin->ofpacts && !ctx.rule) {
> > ctx.table_id = rule_dpif_lookup(ctx.xbridge->ofproto, flow,
> > --
> > 1.7.10.4
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
