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

[Pravin Shelar]

On Mon, Nov 3, 2014 at 12:31 PM, Ben Pfaff <blp at nicira.com> wrote:
> 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.)
>
ok.

> 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'. */
>
ok.

> ip_hdr() and gre_hdr() might be avoided if the ofpbuf's header support
> were used.
>
ofpbuf header might not be set in all cases.

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

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

Packet tunnel info is staored in metadata, that why I called md, I
will change name to ip_extract_tnl_md()

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

> 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.
>
ok, I am going to push first three patches from the series and will
post updated forth patch.