[ovs-dev] [RFC PATCH 4/5] tunnel: Userspace implementation of tunnel manipulation.

[Jesse Gross]

On Fri, Sep 28, 2012 at 2:18 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Fri, Sep 28, 2012 at 11:12:24AM -0700, Jesse Gross wrote:
>> The kernel tunneling code currently needs to handle a large number
>> of operations when tunnel packets are encapsulated and decapsulated.
>> Some examples of this are: finding the correct tunnel port on receive,
>> TTL and ToS inheritance, ECN handling, etc.  All of these can be done
>> on a per-flow basis in userspace now that we have both the inner and
>> outer header information, which allows us to both simpify the kernel
>> and take advantage of userspace's information.  This ports the logic
>> from the kernel to userspace and also pulls in the tunnel-specific
>> configuration handling from netdev-vport.c.  Once tunnel packets are
>> redirected into this code, the redundant pieces can be removed from
>> other places.
>>
>> Signed-off-by: Jesse Gross <jesse at nicira.com>
>
> I guess that DF inheritance is not an important feature?  (Should we
> accept the option and just warn about it, for better compatibility?)

By default we always set the DF bit to on and the times that I've
heard of people wanting to change that is when they know things are
broken and want to completely turn it off, so I doubt that inheritance
is really useful.

I wish that we had exposed this as one of our fragment states in the
flow setup (as in "none; none, don't fragment; first; later) but we
don't currently have that information in userspace.  We could add it
to the interface but it doesn't seem worth it for something that will
probably never get used.

We do accept and ignore unknown flags with a warning.  It will be a
generic message in this case but I think that's probably fine here.

> In tunnel.c, I would move #include "tunnel.h" just after #include
> <config.h>, to ensure that tunnel.h is self-contained.

OK.

> In tunnel.c, we usually put a line break just before the function's
> name in a function definition.

OK, I wasn't sure whether we were coming or going to that style.

> It looks like tnl_port_add__() succeeds if the tunnel type is invalid,
> but I don't know why.  Oh, maybe I see--this layer is meant to be a
> sort of shim that leaves non-tunnels alone?  Some kind of high-level
> comment (maybe in tunnel.h) would be helpful.

Yes, currently it allows non-tunnels to be passed in to the various
public functions.  I'll leave it to Justin as to whether that's the
best behavior once we have better integration.  For now I've added a
comment to document the current behavior.

> In tnl_hash(), since you made sure that struct tnl_match is a multiple
> of 4 bytes, you could use hash_words() instead of hash_bytes() for a
> slight speedup.

Good idea.

Thanks for the reviews.  I'm going to send out the updated series, not
because I think that it has changed enough to require more review but
to make sure that anyone trying to build on top of it has an updated
version.