[ovs-dev] [PATCH v2 2/2] netdev-linux: Read packet auxdata to obtain vlan_tid

[Simon Horman]

On Thu, Dec 12, 2013 at 06:12:32PM -0800, Ben Pfaff wrote:
> On Fri, Dec 13, 2013 at 09:44:06AM +0900, Simon Horman wrote:
> > On Thu, Dec 12, 2013 at 09:56:18AM -0800, Ben Pfaff wrote:
> > > On Thu, Dec 12, 2013 at 05:38:59PM +0900, Simon Horman wrote:
> > > > If VLAN acceleration is used when the kernel receives a packet
> > > > then the outer-most VLAN tag will not be present in the packet
> > > > when it is received by netdev-linux. Rather, it will be present
> > > > in auxdata.
> > > > 
> > > > This patch uses recvmsg() instead of recv() to read auxdata for
> > > > each packet and if the vlan_tid is set then it is added to the packet.
> > >
> > > In netdev_linux_rx_recv_sock(), I think we might want to
> > > ofpbuf_reserve() VLAN_HEADER_LEN bytes at the beginning, to ensure that
> > > there is headroom to insert a VLAN header.  On the other hand, that
> > > would mean that we lose four bytes of tailroom that are important if the
> > > VLAN header is actually embedded in the packet.  I think that means that
> > > we should advise callers to supply 4 bytes of space beyond what they
> > > think they need.
> > 
> > I believe that sufficient headroom for one VLAN tag is already reserved
> > in the caller, dpif_netdev_run():
> > 
> > 	ofpbuf_reserve(&packet, DP_NETDEV_HEADROOM)
> > 
> > Is that for some other purpose?
> 
> As-is, it's a layering violation to rely on that being there.  If you
> want netdev_rx_recv() to rely on the caller reserving headroom for a
> VLAN headroom, then we need to document that.

Understood. I'll implement your original suggestion.

> > > I don't think the cast here, or in netdev_linux_rx_recv(), is necessary:
> > 
> > I believe the cast happens implicitly so I wanted to make
> > it more obvious what was going on. But I also believe that the
> > code will both compile and run fine without the cast. I will remove it
> > if you like.
> 
> I prefer to remove it.

Will do.