On Wed, Jan 09, 2013 at 03:43:45PM -0800, Ethan Jackson wrote:
> From: Jesse Gross <je...@nicira.com>
>
> 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 simplify the kernel and take advantage of userspace's
> information. Once tunnel packets are redirected into this code,
> the redundant pieces can be removed from other places.
>
> Signed-off-by: Jesse Gross <je...@nicira.com>
> Signed-off-by: Ethan Jackson <et...@nicira.com>
tunnel.c includes daemon.h, dirs.h, and unixctl.h, but it doesn't seem
to need any of them.
tunnel.h should not #include <config.h>. I would personally also drop
the "flow.h" and "netdev.h" #includes in favor of just writing
"#include <stdint.h>" and "struct flow; struct netdev_stats;" but
that's more a matter of taste.
I'm not sure of the benefit of:
+BUILD_ASSERT_DECL(sizeof(struct tnl_match) == 24);
because all the users zero the whole structure and the structure
doesn't seem to be used in protocol situations, etc.
There's an extra blank line at the end of the "if" block in tnl_run().
The is_ip() function is the same as the is_ip_any() function in
meta-flow.c. Add a common helper in packets.h?
The treatment of failed calls to tnl_port_receive() deserves some
attention. In a later patch in the series, a NULL return value from
tnl_port_receive() causes the upcall packet to be discarded without
setting up a flow, even one that drops the packet. Previously, that
behavior was possible only for "can't happen" situations like a port
that shouldn't exist. But tnl_port_receive() can return NULL in a
couple of situations caused by contents of packets, like contradictory
ECN bits and packets to tunnels not set up. If I'm reading the code
right, that would make it a lot easier for invalid network flows to
waste a lot of CPU time in userspace.
Both callers of tnl_port_fmt() would find it more convenient if it
returned a malloc()'d string instead of taking a struct ds.
In tnl_port_send(), the true clause of the 'if' statement here
essentially does "a = a;":
> + if (cfg->out_key_flow) {
> + flow->tunnel.tun_id = flow->tunnel.tun_id;
> + } else {
> + flow->tunnel.tun_id = cfg->out_key;
> + }
I think that tnl_hash() could be a little faster if we wrote it as:
BUILD_ASSERT_DECL(sizeof *match % sizeof(uint32_t) == 0);
return hash_words(match, sizeof *match / sizeof(uint32_t), 0);
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
