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 <[email protected]>
The new functions for pre-2.6.27 are all trivial. Why aren't they
inlined into vlan.h?
The two occurrences of #ifdef X/#undef X/#endif can be simplified to
just #undef X.
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()?
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.
Acked-by: Ben Pfaff <[email protected]>
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
