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

Jesse Gross jesse at nicira.com
Mon Feb 7 20:26:04 UTC 2011

On Mon, Feb 7, 2011 at 10:46 AM, Ben Pfaff <blp at nicira.com> wrote:
> 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.

I'm not too worried about missed places as there are only a few
locations where vlan tags matter: ingress, egress, and tag
manipulations.  So certainly go ahead if you want but I'm not sure
that it's strictly necessary.

> 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?

I agree that it is strange.  Grepping the kernel source shows only one
usage of VLAN_ETH_ALEN, in __vlan_put_tag(), which is obviously the
analog of this function.  Even other places in if_vlan.h don't use it,
so I dropped it here as well.

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

Thanks, I dropped VLAN_HLEN here.  I'll remove it from make_writable()
in a future patch, though I want to do a more complete overhaul of it,
so maybe I'll just wait until then.

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

The conditions are actually slightly different - I tried commenting it
but it's very subtle.

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

Fixed thanks.

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

Ouch, nice catch.  I'll fix this in a follow up patch.

> 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?

It's probably a good idea.  I've done that now.

> Also, the third copy, in
> datapath/vport-internal_dev.c, does not do the vlan_set_tci(skb, 0)
> call.  Is this a mistake?

Yes, we should zero it out.  I was thinking that it wasn't necessary
because it not used on these kernels but that's not true: between
2.6.27 and 2.6.37 other people could see it (though they will probably
will ignore it.

More information about the dev mailing list