On Thu, Sep 26, 2013 at 09:42:20AM +0900, Simon Horman wrote: > On Wed, Sep 25, 2013 at 09:09:27AM -0700, Ben Pfaff wrote: > > On Wed, Sep 25, 2013 at 01:31:07PM +0900, Simon Horman wrote: > > > Rather than tracking the MPLS depth as a field in the > > > flow, which is an entirely poor place for it, just track > > > the delta to the MPLS depth during translation. > > > > > > This logic was developed while implementing recirculation > > > and intended to be used to detect when recirculation should > > > occur. This variant of the patch uses the logic to determine > > > if processing of actions should stop due to an MPLS > > > action which cannot be translated (without recirculation). > > > > > > A side-effect of this patch is that it resolves a bug > > > whereby ovs-vswitchd will abort due to to an assertion > > > on eth_type_mpls(ctx->xin->flow.dl_type) in compose_mpls_pop_action(() > > > if the actions of a flow include pop_mpls twice without > > > a push_mpls in between. > > > > > > Signed-off-by: Simon Horman <[email protected]> > > > > The code seems fine. > > > > This commit appears to introduce one subtle semantic change. > > Previously, if a packet had an MPLS ethertype followed by too few bytes > > to constitute a full MPLS label, then the flow indicated that with > > mpls_depth=0, mpls_lse=0. Now, there is no way to distinguish this case > > from a single label with mpls_lse=0. I don't know whether this is > > important. > > I need to think about this a little but my initial reaction > is that it is not important as I believe mpls_depth was only used > when composing actions. And I'm not sure that the subtle change that > you point out changes the way that occurs.
I have looked into this more closely and found three areas where the behaviour changes: 1. In dpctl_normalize_actions() the mpls label will now be printed whereas previously it was omitted. This seems to be a purely cosmetic difference. 2. compose_mpls_pop_action() will allow the LSE to be popped whereas previously it was not. I believe this new behaviour is reasonable. In the case of the user-space datapath my understanding is that packets are padded out to 60 (or is it 64?) bytes, so there should be no way to run off the end of the skb. In the case of the kernel datapath some testing shows that it is possible to run off the end of the skb. I propose adding a check to the kernel datapath version of pop_mpls() so that it skips popping the LSE if the skb is too short. To clarify: the kernel datapath code is in a patchset that hasn't been merged yet. 3. compose_mpls_push_action() will push an LSE at the end of the stack (of one LSE) rather than pushing the LSE as the first LSE of a new stack. This will lead the TTL new LSE to be based on that of the top-most LSE (0) rather than the inner packet (e.g. IP). This new behaviour seems reasonable to me. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
