[ovs-dev] [PATCH 1/4] datapath: Add vlan acceleration field for older kernels.

[Jesse Gross]

On Mon, Feb 7, 2011 at 9:48 AM, Ben Pfaff <blp at nicira.com> wrote:
> On Sat, Feb 05, 2011 at 04:14:46PM -0800, Jesse Gross wrote:
>> Kernels prior to 2.6.27 did not have a vlan_tci field in struct
>> sk_buff for vlan acceleration.  It's very convenient to use this
>> field for manipulating vlan tags, so we would like to use it as
>> the primary mechanism.  To enable this, this commit adds similar
>> infrastructure to the OVS_CB on the kernels that need it and a
>> set of functions to use the correct location.
>>
>> Signed-off-by: Jesse Gross <jesse at nicira.com>
>
> The new functions for pre-2.6.27 are all trivial.  Why aren't they
> inlined into vlan.h?

These functions depend on datapath.h, which in turn depends on vlan.h.
 The dependency of datapath.h on vlan.h is pretty trivial (just
NEED_VLAN_FIELD) and wouldn't be hard to remove.  However, it seemed
somewhat cleaner this way and I'm not too concerned about optimizing
for older kernels.

>
> The two occurrences of #ifdef X/#undef X/#endif can be simplified to
> just #undef X.

Fixed, thanks.

>
> I don't understand the name of vlan_copy_skb_tci().  It doesn't copy
> anything and only one of two of its invocations happen just after a
> (possible) skb copy.  Maybe vlan_init_skb_tci()?

The implementations don't actually copy anything but the logical
action that it is exposing is copying from skb->vlan_tci.  I think
it's more consistent than init because that really isn't what is
happening on newer kernels.

>
> I'd be really tempted to just define one base macro, like this:
>        #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,27)
>        #define SKB_VLAN_TCI(skb) OVS_CB(skb)->vlan_tci
>        #else
>        #define SKB_VLAN_TCI(skb) (skb)->vlan_tci
>        #endif
> or even something like this (although the naming is poor):
>        #if LINUX_VERSION_CODE < KERNEL_VERSION(2,6,27)
>        #define VLAN_STRUCT(skb) OVS_CB(skb)
>        #else
>        #define VLAN_STRUCT(skb) (skb)
>        #endif
> and then get rid of the vlan_set_tci() and vlan_get_tci() functions.  I
> think that it would be slightly more amenable to search-and-replace for
> upstreaming.  But I'll understand if you think that's too clever.

This has the same issue that with recursive dependencies if put in
vlan.h.  My plan for upstreaming is to segregate the compat code as
much as possible and when the time comes I can just delete these files
and the compiler will make it pretty obvious what needs to be fixed
up.

It actually seems a little too convenient to me - the macro makes it
appear that we are accessing a valid member in all cases, though just
in different locations.  In reality though, the compat version needs
to be initialized and copied out, so I like it being a little more
explicit.