[ovs-dev] [PATCH 01/16] User-Space MPLS actions and matches

[Simon Horman]

On Tue, Jan 22, 2013 at 10:47:17AM -0800, Ben Pfaff wrote:
> On Fri, Jan 18, 2013 at 09:27:55AM +0900, Simon Horman wrote:
> > This patch implements use-space datapath and non-datapath code
> > to match and use the datapath API set out in Leo Alterman's patch
> > "user-space datapath: Add basic MPLS support to kernel".
> 
> I'm resuming my review starting from packets.c.
> 
> eth_mpls_depth() seems poorly documented to me.  The comment makes an
> incorrect claim about packet->l2, which the function does not use, and
> does not mention packet->l2_5, which the function does use.

Thanks.

I will unify the comment for eth_mpls_depth() with that of eth_push_vlan().
My proposed text is:

/* Return depth of mpls stack.
 *
 * 'packet->l2_5' should initially point to 'packet''s outer-most MPLS header
 * or may be NULL if there are no MPLS headers. */

> In get_ethtype_ptr(), the expression in the "return" looks wrong:
>     if (eh->eth_type == htons(ETH_TYPE_VLAN)) {
>         return (ovs_be16 *)((char *)(packet->l2_5 ? packet->l2_5 : packet->l3)) - 2;
> because casts have higher precedence than subtraction, so I believe this
> will subtract 2*sizeof(uint16) == 4 bytes from the beginning of the L2.5
> or L3 header, which is wrong.  Do any tests cover this?

Thanks, nice catch.

It turns out that you are correct. And that I overlooked exercising this.

I have corrected the problem by moving the final ')' to after the 2 as
follows:

         return (ovs_be16 *)((char *)(packet->l2_5 ? packet->l2_5 : packet->l3) - 2);

And I have verified both that the existing code is broken and the new code
is not by adding a test to ofproto-dpif.at which uses a push_mpls() action
on a packet with a VLAN tag. It is in keeping with other mpls tests that
this series adds to that file.

> I believe that the correct name for an MPLS header is a "label", not a
> "tag".  It would be nice to get this right, although certainly it's not
> a huge deal.

Thanks. Referring to wikipedia[1] it seems that an MPLS label is one
component of a MPLS label stack entry (LSE) which is in turn a component of
the MPLS label stack.

I'll do a search and replace pass over the entire patch set to clean things up.

[1] http://en.wikipedia.org/wiki/Multiprotocol_Label_Switching#MPLS_operation

> set_mpls_lse_values() has ;; at the end of one line.

Fixed.

> push_mpls_lse() isn't documented as putting the new MPLS label before or
> after the existing labels, but it appears to me that it puts the new
> label after the existing labels.  If I'm right about that, it's
> surprising; one would ordinarily expect a new header to be an outermost
> one, not an innermost one.

I believe that it pushes the new MPLS stack entry onto the MPLS stack
before any existing entries.

The memmove() moves everything before l2_5, that is, everything before the
current MPLS label stack. And then (memcpy) puts the new MPLS label stack entry
immediately after the data moved by memove(), still before the existing
MPLS label stack entries.

I have updated the comment above push_mpls_lse() to state its intentions
more clearly:

/* Push an MPLS stack entry onto the MPLS stack before any existing entries
 * and adjust packet->l2 and packet->l2.5 accordingly. */

I also note that push_mpls_lse() makes the following assumptions.

* sizeof *mh == MPLS_HLEN
  This is true.
* packet->l2 == packet->data
  Is this always true? I am unsure.
* packet->l2_5 is set to a value after packet->l2.
  This is not true in at least the following case:
  The packet arrives via flow_compose() with an MPLS ethernet type,
  in which case packet->l2_5 is NULL.

I plan to resolve the problem raised in the last point as follows.
As far as I can see push_mpls() is the only caller of push_mpls_lse().

diff --git a/lib/packets.c b/lib/packets.c
index 190803e..5d9a44e 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -360,6 +361,9 @@ push_mpls(struct ofpbuf *packet, ovs_be16 ethtype, ovs_be32 lse)
     if (!eth_type_mpls(eth_type)) {
         /* Set ethtype and mpls label stack entry. */
         set_ethertype(packet, ethtype);
+    }
+    /* l2_5 may not already be set if this is the first mpls_push. */
+    if (!packet->l2_5) {
         packet->l2_5 = packet->l3;
     }
 

I have added a test to tests/ofproto-dpif.at to exercise both the order
of MPLS label stack entry insertion and the l2_5 NULL problem described
immediately above. It was in the course of adding the test that I found
that problem. The test performs an mpls_push action on an MPLS frame.

I have also added a test to tests/ofproto-dpif.at to exercise this and the
NULL l2_5 problem described below.