[ovs-dev] [PATCH v10 5/5] userspace: add non-tap (l3) support to GRE vports

[Simon Horman]

On Tue, May 31, 2016 at 08:20:24PM -0700, Ben Pfaff wrote:
> On Wed, May 04, 2016 at 04:34:25PM +0900, Simon Horman wrote:
> > Add support for layer 3 GRE vports (non-tap aka non-VTEP).
> > 
> > This makes use of a vport mode configuration for the existing (tap/VTEP)
> > GRE vports.
> > 
> > In order to differentiate packets for two different types of GRE vports a
> > new flow key attribute, OVS_KEY_ATTR_NEXT_BASE_LAYER, is used.  It is
> > intended that this attribute is only used in userspace as there appears to
> > be no need for it to be used in the kernel datapath.
> 
> Should the OVS_KEY_ATTR_NEXT_BASE_LAYER declaration in an #ifndef
> __KERNEL__ block or similar?  At least a comment on the declaration
> would be helpful.

Good point, I think its reasonable to so something like this:

--- a/datapath/linux/compat/include/linux/openvswitch.h
+++ b/datapath/linux/compat/include/linux/openvswitch.h
@@ -358,6 +358,9 @@ enum ovs_key_attr {
 #ifdef __KERNEL__
 	/* Only used within kernel data path. */
 	OVS_KEY_ATTR_TUNNEL_INFO,  /* struct ovs_tunnel_info */
+#else
+	/* Only used within user-space data path. */
+	OVS_KEY_ATTR_NEXT_BASE_LAYER, /* base layer of encapsulated packet */
 #endif
        __OVS_KEY_ATTR_MAX
 };

> > It is envisaged that this attribute may be used for other encapsulation
> > protocols that support both layer3 and layer2 inner-packets.
> > 
> > Signed-off-by: Simon Horman <simon.horman at netronome.com>
> 
> miniflow_extract() has some tabs that should be spaces for indentation.

Thanks, I'll fix that.

> There's a change to tnl_port_show() to "Skip ports with duplicate 'port'
> field".  I don't understand this change.  Can you explain it?  (It's
> O(n**2) in the number of ports, too.)

The problem that this tries to address is that it is now possible
to have an l3 and non-l3 tunnel which share the same port and when
the ports are dumped this shows up as a duplicate.

For example in the tunnel-push-pop.at test updated by this patch the
following duplicate gre port appears without this portion of the change:

# ovs-appctl tnl/ports/show
 Listening ports:
  genev_sys_6081 (6081)
  gre_sys (3)
  gre_sys (3)
  vxlan_sys_4789 (4789)

Perhaps rather than hiding this duplication it would be best just to
provide a bit more information to the users like this.

# ovs-appctl tnl/ports/show
 Listening ports:
  genev_sys_6081 (6081)
  gre_sys (3)
  gre_sys (3) (layer3)
  vxlan_sys_4789 (4789)

Which can be achieved as follows without altering the O() of the code.

diff --git a/lib/tnl-ports.c b/lib/tnl-ports.c
@@ -354,7 +371,8 @@ tnl_port_show(struct unixctl_conn *conn, int argc OVS_UNUSED
,
     }
 
     LIST_FOR_EACH(p, node, &port_list) {
-        ds_put_format(&ds, "%s (%"PRIu32")\n", p->dev_name, p->port);
+        ds_put_format(&ds, "%s (%"PRIu32")%s\n", p->dev_name, p->port,
+                      p->is_layer3 ? " (layer3)" : "");
     }
 
 out:

> In vswitch.xml, there's a couple of typos: s/recieved/received/ and
> s/ethernet/Ethernet/.

Thanks, I'll fix that too.