[ovs-dev] [PATCH v1 4/4] openvswitch: Userspace tunneling.

[Ben Pfaff]

On Fri, Oct 24, 2014 at 09:43:39AM -0700, Ben Pfaff wrote:
> On Thu, Oct 16, 2014 at 11:38:20AM -0700, Pravin B Shelar wrote:
> > Following patch adds support for userspace tunneling. Tunneling
> > needs three more component first is routing table which is configured by
> > caching kernel routes and second is ARP cache which build automatically
> > by snooping arp. And third is tunnel protocol table which list all
> > listening protocols which is populated by vswitchd as tunnel ports
> > are added.
> > 
> > Tunneling works as follows:
> > On packet receive vswitchd check if this packet is targeted to tunnel
> > port. If it is then vswitchd inserts tunnel pop action which pops
> > header and sends packet to tunnel port.
> > On packet xmit rather than generating Set tunnel action it generate
> > tunnel push action which has tunnel header data. datapath can use
> > tunnel-push action data to generate header for each packet and
> > forward this packet to output port. Since tunnel-push action
> > contains most of packet header it need to lookup routing table and
> > arp table to build this action.
> > 
> > Signed-off-by: Pravin B Shelar <pshelar at nicira.com>
>
> I need to restart reading the code at netdev-vport.c.

Sorry about the long delay, here's some more comments, although I
still have not yet finished.  Thanks again for building all this
infrastructure, it should be useful.

tnl_udp_port_min and tnl_udp_port_max are only used in netdev-vport.c
so they should be static.  (They could be statically initialized too.)

I spent a minute or two puzzling over push_ip_header().  Here's an
improved comment:

/* Pushes the 'size' bytes of 'header' into the headroom of 'packet',
 * reallocating the packet if necessary.  'header' should contain an Ethernet
 * header, followed by an IPv4 header (without options), and an L4 header.
 *
 * This function sets the IP header's ip_tot_len field (which should be zeroed
 * as part of 'header') and puts its value into '*ip_tot_size' as well.  Also
 * updates IP header checksum.
 *
 * Return pointer to the L4 header added to 'packet'. */

ip_hdr() and gre_hdr() might be avoided if the ofpbuf's header support
were used.

ip_hdr() and gre_hdr() each don't need their outermost cast.

What's the 'md' in the name of ip_extract_md()?  I guess not
"metadata", since packet headers aren't metadata.

In parse_gre_header(), I would be inclined to use ovs_16aligned_be32
instead of ovs_be32 for 'options', to avoid problems in corner cases
on RISC architectures.

I think that the checksum code in parse_gre_header() assumes that the
packet has no IP options and no VLAN header.  I guess it would be
better to count bytes starting after the GRE header rather than the
subtraction technique used there.