On Mon, Nov 3, 2014 at 12:31 PM, Ben Pfaff <b...@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 <pshe...@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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
