On Mon, Feb 07, 2011 at 11:01:35AM -0800, Jesse Gross wrote: > 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.
OK, fair enough. _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev_openvswitch.org
