[ovs-dev] [PATCH 01/19] tunneling: Remove call to eth_type_trans() on receive.

Ben Pfaff blp at nicira.com
Thu Dec 9 19:28:23 UTC 2010


On Thu, Dec 09, 2010 at 11:26:12AM -0800, Jesse Gross wrote:
> On Thu, Dec 9, 2010 at 11:25 AM, Jesse Gross <jesse at nicira.com> wrote:
> > On Thu, Dec 9, 2010 at 9:32 AM, Ben Pfaff <blp at nicira.com> wrote:
> >> On Wed, Dec 08, 2010 at 10:13:59PM -0800, Jesse Gross wrote:
> >>> On receive we call eth_type_trans() to set skb->protocol.  However,
> >>> that function also sets skb->pkt_type, which requires several
> >>> comparisons to MAC addresses.  Nothing in OVS cares about pkt_type,
> >>> so this is wasteful.  If we actually do egress to the IP stack
> >>> through an internal device then we'll call eth_type_trans() to get
> >>> everything correctly setup.  It's possible for device drivers to
> >>> see an incorrect pkt_type or not correctly parse legacy IPX (which
> >>> eth_type_trans() also handles) but it's highly unlikely that they
> >>> will care.
> >>
> >> The old version of this function did a skb_reset_mac_header(),
> >> indirectly in eth_type_trans().  The new version doesn't.  That implies
> >> that the MAC header is already correct, but if that is so then
> >>        struct ethhdr *eh = (struct ethhdr *)skb->data;
> >> could be written as
> >>        struct ethhdr *eh = eth_hdr(skb);
> >> So I don't know what to think here, since it wasn't obvious to me from
> >> the callers that the MAC header had been set properly.
> >>
> >> Otherwise look fine to me, thanks.
> >>
> >
> > The MAC header pointer isn't correct here (it points at the outer
> > header, which is not very useful).  The inner Ethernet header starts
> > at skb->data, which is why that line is written the way it is.
> >
> > Nothing uses the outer Ethernet header but we do use the outer IP
> > header for ECN decapsulation, which is why I didn't reset them here
> > (we were resetting the network header before, which is a bug fixed in
> > the next commit).  Everything gets correctly set in flow_extract() for
> > the packet as processed by OVS.
> 
> I also added this comment to the top of tnl_rcv:
> 
> 	/* Packets received by this function are in the following state:
> 	 * - skb->data points to the inner Ethernet header.
> 	 * - The inner Ethernet header is in the linear data area.
> 	 * - skb->csum does not include the inner Ethernet header.
> 	 * - The layer pointers point at the outer headers.
> 	 */

Thanks, that clears everything up.




More information about the dev mailing list