On Mon, Feb 7, 2011 at 9:48 AM, Ben Pfaff <[email protected]> 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 <[email protected]> > > 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. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
