On Mon, Dec 16, 2013 at 02:55:39PM +0900, Simon Horman wrote:
> On Fri, Dec 13, 2013 at 12:28:21AM -0800, Ben Pfaff wrote:
> > I've been a little frustrated with the current approach to MPLS, because it
> > seems quite difficult to understand. One particularly difficult bit for
> > me is the variables used during translation, e.g. mpls_depth_delta and
> > pre_push_mpls_lse. And what we end up with is support for a single MPLS
> > label, which I don't think is going to make any real-world users happy.
> >
> > I think that we can do something easier to understand and more powerful
> > by just keeping track of all the labels in struct flow. This patch is a
> > first stab at that. It's incomplete--in particular I have not implemented
> > commit_mpls_action() because it seems slightly tricky and it's very late
> > at night here--and obviously untested, but I'd appreciate feedback on the
> > approach.
>
> Its quite a bit of code to digest but in general my feeling
> at this point is that this approach could be made to work.
I think so, yes.
> There are a number of corner cases that my patchset (the previous approach)
> tries to address. And I believe they contributed to most of the more
> difficult to understand code.
I'd really like to hear about the corner cases you have in mind, because
I'd like to commit the solution that most cleanly avoids or solves the
real problems.
> I have tried mentally running through a number of them to see how they
> might work out with this new approach. One that seems slightly tricky
> is the following in the case that MPLS LSEs are being pushed before VLAN tags.
>
> Assuming no VLAN is present in the original packet:
>
> push_vlan,push_mpls,push_vlan
>
> Or, alternatively, if a VLAN is present in the original packet:
>
> set_vlan_vid,push_mpls,push_vlan
>
> I believe that with this new approach the first VLAN action would be
> silently dropped. I believe that this case could be handled using
> recirculation and in the mean time detected and rejected. But I wanted to
> bring it to your attention to see how you feel about it.
I think that both of these cases can be gracefully handled with the
approach that I propose. Take a look at the compose_mpls_push_action()
that I propose:
static void
compose_mpls_push_action(struct xlate_ctx *ctx, struct ofpact_push_mpls *mpls)
{
struct flow_wildcards *wc = &ctx->xout->wc;
struct flow *flow = &ctx->xin->flow;
ovs_be16 vlan_tci = flow->vlan_tci;
int n;
ovs_assert(eth_type_mpls(mpls->ethertype));
n = flow_count_mpls_labels(flow);
if (!n) {
if (mpls->position == OFPACT_MPLS_BEFORE_VLAN) {
vlan_tci = 0;
} else {
flow->vlan_tci = 0;
}
ctx->xout->slow |= commit_odp_actions(flow, &ctx->base_flow,
&ctx->xout->odp_actions,
&ctx->xout->wc);
} else if (n >= ARRAY_SIZE(flow->mpls_lse)) {
return;
}
flow_push_mpls(flow, mpls->ethertype, wc);
flow->vlan_tci = vlan_tci;
}
For OFPACT_MPLS_BEFORE_VLAN, the idea is that, before we modify the flow
for the first MPLS label, we commit all the existing actions. Afterward
we discard the VLAN from the flow, pretending it isn't there at all. I
think that this effectively solves that corner case.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
