diff mbox

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

Message ID 20160601050455.GA19976@vergenet.net
State Not Applicable
Headers show

Commit Message

Simon Horman June 1, 2016, 5:04 a.m. UTC 
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:


> > 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@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.

Comments

Ben Pfaff June 8, 2016, 8:10 p.m. UTC | #1 
On Wed, Jun 01, 2016 at 02:04:57PM +0900, Simon Horman wrote:
> On Tue, May 31, 2016 at 08:20:24PM -0700, Ben Pfaff wrote:
> > 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:

OK, so I understand the solution now, but I don't yet understand the
problem: why are these represented as two separate tunnels, and why do
those separate tunnels have the same port number?

Thanks,

Ben.
Simon Horman June 23, 2016, 2:08 a.m. UTC | #2 
On Wed, Jun 08, 2016 at 01:10:54PM -0700, Ben Pfaff wrote:
> On Wed, Jun 01, 2016 at 02:04:57PM +0900, Simon Horman wrote:
> > On Tue, May 31, 2016 at 08:20:24PM -0700, Ben Pfaff wrote:
> > > 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:
> 
> OK, so I understand the solution now, but I don't yet understand the
> problem: why are these represented as two separate tunnels, and why do
> those separate tunnels have the same port number?

Up until now the usage of tunnel vports has been such that
each datapath vport corresponds to exactly one tunnel vport.

The way the l3 tunnel support is structured in this patchset
this is no longer the case and a single datapath vport may
be used for both an l3 and non-l3 tunnel.

Or more concretely, GRE carrying TEB packets and GRE carrying non-TEB packets
use the same kernel netdev and thus kernel vport but different tunnel
vports in ovs-vswitchd.
diff mbox

Patch

--- 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
 };