[ovs-dev] [PATCH] [RFC] Alternate approach to MPLS.

Simon Horman horms at verge.net.au
Tue Dec 17 00:22:29 UTC 2013


On Mon, Dec 16, 2013 at 08:56:30AM -0800, Ben Pfaff wrote:
> 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.

Yes, I think so too. Somehow I missed the two rounds of commit when
I looked over your code.



More information about the dev mailing list