On Thu, Jan 16, 2014 at 04:30:24PM -0800, Ben Pfaff wrote:
> On Wed, Jan 15, 2014 at 04:13:24PM +0900, Simon Horman wrote:
> > This is an proposed enhancement to
> > "Implement OpenFlow support for MPLS, for up to 3 labels."
> >
> > Signed-off-by: Simon Horman <ho...@verge.net.au>
> > ---
> > ofproto/ofproto-dpif-xlate.c | 26 +++++++++++++++-----------
> > 1 file changed, 15 insertions(+), 11 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index f14f308..57e8978 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -2155,18 +2155,22 @@ compose_mpls_pop_action(struct xlate_ctx *ctx,
> > ovs_be16 eth_type)
> > struct flow_wildcards *wc = &ctx->xout->wc;
> > struct flow *flow = &ctx->xin->flow;
> >
> > - if (!flow_pop_mpls(flow, eth_type, wc) &&
> > - flow_count_mpls_labels(flow, wc) >= ARRAY_SIZE(flow->mpls_lse)) {
> > - 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 "
> > - "MPLS pop action can't be performed as it has "
> > - "have more MPLS LSEs than the %"PRIuSIZE" "
> > - "that can be handled.",
> > - ctx->xbridge->name, ARRAY_SIZE(flow->mpls_lse));
> > + if (!flow_pop_mpls(flow, eth_type, wc)) {
> > + int n = flow_count_mpls_labels(flow, wc);
> > + size_t max_stack = MIN(ARRAY_SIZE(flow->mpls_lse),
> > + ctx->xbridge->max_mpls_depth);
> > + if (n >= max_stack) {
> > + 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 "
> > + "MPLS pop action can't be performed as it has
> > "
> > + "have more MPLS LSEs than the %"PRIuSIZE" "
> > + "that can be handled.", ctx->xbridge->name,
> > + max_stack);
> > + }
> > + ctx->exit = true;
> > + ofpbuf_clear(&ctx->xout->odp_actions);
>
> I think that this can be simplified to just
> if (!flow_pop_mpls(flow, eth_type, wc)
> && flow_count_mpls_labels(flow, wc) >= ARRAY_SIZE(flow->mpls_lse)) {
> 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 "
> "MPLS pop action can't be performed as it has "
> "more MPLS LSEs than the %"PRIuSIZE" "
> "that can be handled.", ctx->xbridge->name,
> max_stack);
> }
> ctx->exit = true;
> ofpbuf_clear(&ctx->xout->odp_actions);
> }
> because flow_pop_mpls() is not going to complain based on
> max_mpls_depth but only on ARRAY_SIZE(flow->mpls_lse), and I think
> that's OK because we should flag anything with more MPLS labels than
> max_mpls_depth but fewer than ARRAY_SIZE(flow->mpls_lse) as
> ODP_FIT_TOO_LITTLE so that we get to look at every packet. (Actually,
> we don't seem to do that, so I'll propose a patch myself.)
Thanks, that sounds fine to me.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
