On Thu, Sep 26, 2013 at 12:24:56PM -0700, Ben Pfaff wrote: > On Thu, Sep 26, 2013 at 05:34:49PM +0900, Simon Horman wrote: > > 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 <ho...@verge.net.au> > > > > > > > > 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. > > I'm happy enough with that. > > Do you have a v3? I don't see one yet.
I will send it shortly. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
