[ovs-dev] [PATCH 2/4] datapath: Use vlan acceleration for vlan operations.

Ben Pfaff blp at nicira.com
Mon Feb 7 18:46:56 UTC 2011

On Sat, Feb 05, 2011 at 04:14:47PM -0800, Jesse Gross wrote:
> Using the kernel vlan acceleration has a number of benefits:
> it enables hardware tagging, allows usage of TSO and checksum
> offloading, and is generally easier to manipulate.  This switches
> the vlan actions to use skb->vlan_tci field for any necessary
> changes.  In places that do not support vlan acceleration in a way
> that we can use (in particular kernels before 2.6.37) we perform
> any necessary conversions, such as tagging and GSO before the
> packet leaves Open vSwitch.
> Signed-off-by: Jesse Gross <jesse at nicira.com>

I examined the changes in this patch pretty carefully, but I didn't do a
big-picture pass over the code to look for places that might have been
missed.  Let me know if you'd like me to do that too.

In strip_vlan(), the use of VLAN_ETH_ALEN is totally weird (although you
didn't introduce it).  Why would an Ethernet address in a VLAN header be
different in length from an Ethernet address anywhere else?

In modify_vlan_tci() we still pass VLAN_HLEN as the headroom parameter
to make_writable(), but I think it could be 0 now since this function is
now just overwriting the VLAN TCI and never adds a new header.  And if
we make that change then make_writable() is always called with a 0
parameter, so further simplification may then be possible.

In check_mtu(), if you calculated vlan_header at the top of the function
then you could just write:
        packet_length = skb->len - ETH_HLEN - vlan_header;
and avoid testing the same condition again inside the pmtud if block.

In handle_offloads() the new comment and the code disagree about minimum
kernel version.  I think that the code is correct and so the comment
should say "On kernels 2.6.37 and later" instead.

While chasing down references, I spotted a potentially serious bug in
need_linearize(): inside the loop, the index should be [i], not [0].

I see three instances of code similar to 
> +	if (vlan_tx_tag_present(skb)) {
> +		skb = __vlan_put_tag(skb, vlan_tx_tag_get(skb));
> +		if (unlikely(!skb)) {
> +			err = -ENOMEM;
> +			goto error;
>  		}
> +		vlan_set_tci(skb, 0);
> +	}
> +#endif
and two more that are almost like it in vport-netdev.c.  Should we
invent a helper function?  Also, the third copy, in
datapath/vport-internal_dev.c, does not do the vlan_set_tci(skb, 0)
call.  Is this a mistake?

Acked-by: Ben Pfaff <blp at nicira.com>

More information about the dev mailing list