[ovs-dev] MPLS and VLAN QinQ patch

[Ravi.Kerur at telekom.com]

Thanks Ben. I have addressed all your previous comments with one exception of adding ofpbuf_* common function. I haven't worked on the patch since mid April, will work on it after I receive additional comments on the latest patch. 

Please note that latest patch has gone through additional testing

vlan + mpls (ICMP)
vlan-qinq + mpls (ICMP)

(both kernel and user-space)

vlan + mpls (TCP offload)
mpls (tcp offload)
vlan-qinq (tcp offload)

TCP offload is not perfect yet but works fine so far. I am looking into performance numbers which doesn't look quite right.

Thanks,
Ravi

-----Original Message-----
From: Ben Pfaff [mailto:blp at nicira.com] 
Sent: Tuesday, May 22, 2012 10:49 AM
To: Kerur, Ravi
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] MPLS and VLAN QinQ patch

On Tue, May 22, 2012 at 07:36:32PM +0200, Ravi.Kerur at telekom.com wrote:
> Attached MPLS and VLAN QinQ patch after rebasing to following commit
> 
> commit 046f1f89e6d7716581de207dd0c54421926bc25b
> Author: Ethan Jackson <ethan at nicira.com<mailto:ethan at nicira.com>>
> Date:   Mon May 21 13:20:18 2012 -0700
> 
> Patch(s) have undergone additional integration testing and incorporates initial code review comments from Ben.

Hi Ravi.  Here's some feedback that I had started writing up on a
previous version of the patch that you sent me in private email some
time ago.  Will you please apply any parts of it that still apply to
the current version of the MPLS patch, then post a revised version?

Thanks,

Ben.

It applies, compiles, and passes "sparse" cleanly.  Great!

I don't see any handling for decrementing a TTL of 0.  That's supposed
to be sent to the controller with OFPR_INVALID_TTL.

include/linux/openvswitch.h
---------------------------

I think these should say be16 and be32 since their operands are
in network byte order:
+	OVS_ACTION_ATTR_PUSH_MPLS,    /* u16, ethertype */
+	OVS_ACTION_ATTR_POP_MPLS,     /* u16, ethertype */
+	OVS_ACTION_ATTR_SET_MPLS_LSE, /* u32, mpls label stack entry */


include/openflow/nicira-ext.h
-----------------------------

You can drop this line, it doesn't fit in in the context:
+                                /* MPLS related definitions*/

lib/dpif-netdev.c
-----------------

I don't think there's a benefit to the temporary vars here:
+        case OVS_ACTION_ATTR_PUSH_MPLS:
+             push_ethertype = nl_attr_get_be16(a);
+             push_mpls(packet, push_ethertype);
+             break;
+
+        case OVS_ACTION_ATTR_POP_MPLS:
+             pop_ethertype = nl_attr_get_be16(a);
+             pop_mpls(packet, pop_ethertype);
+             break;
+
+        case OVS_ACTION_ATTR_SET_MPLS_LSE:
+             mpls_lse = nl_attr_get_be32(a);
+             set_mpls_lse(packet, mpls_lse);
+             break;

lib/flow.c
----------

In flow_extract(), I don't think the cast is necessary:
+        packet->l2_5 = (uint32_t *)b.data;

lib/odp-util.c 
--------------

I think that these should be ovs_be<N> types:
+    case OVS_ACTION_ATTR_PUSH_MPLS: return sizeof(uint16_t);
+    case OVS_ACTION_ATTR_POP_MPLS:  return sizeof(uint16_t);
+    case OVS_ACTION_ATTR_SET_MPLS_LSE: return sizeof(uint32_t);

format_odp_action() and parse_odp_action() use different syntax for
push_mpls and pop_mpls (one includes "tpid=", the other doesn't).
Please make them consistent.

(Is 'tpid' correct terminology for MPLS?  I thought it was
VLAN-specific.)

In parse_odp_action(), the numbers passed to strncmp() and returned
differ, but they should be the same.

Code like this appears in a few places.  Should we create a helper
function?
+                            htonl((mpls_label << MPLS_LABEL_SHIFT) |
+                                  (mpls_tc << MPLS_TC_SHIFT)       |
+                                  (mpls_ttl << MPLS_TTL_SHIFT)     |
+                                  (mpls_stack << MPLS_STACK_SHIFT)));
The set_mpls_lse_values() function could also be implemented in terms
of such a helper.

I'd like to see commit_mpls_action() split into a function to pop an
MPLS header and a function to push an MPLS header.  The current
organization is confusing and I'm not convinced that it's 100%
correct.

lib/packets.c
-------------

In set_mpls_lse_ttl(), please drop the unneeded parentheses here:
+    *tag |= (ttl & htonl(MPLS_TTL_MASK));
also in the other similar functions.

I think the logic in dec_mpls_ttl() only works because the TTL is the
low 8 bits of mpls_lse.  Conceptually there's a missing shift.

copy_mpls_ttl_in() and copy_mpls_ttl_out() don't appear to check that
packet data is actually present.

Also in those functions, missing {} here:
+    if (mh == NULL || nh == NULL)
+        return;

The "return;" at the end of push_mpls_lse() may be omitted.

push_mpls() and push_mpls_lse() look like they don't check that IPv4
packet data is actually present.

I think it's time to add ofpbuf_*() functions to insert and remove
data in the middle of a packet.  It's hard to see whether open-coded
moves (there are a few in this file in addition to the ones you added
for MPLS) are correct, so it'd be nice to get it right once and reuse
it.

ofproto/ofproto-dpif.c
----------------------

facet_account() has some stray VLOG_DBG calls.