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

Ben Pfaff blp at nicira.com
Mon Feb 7 19:08:58 UTC 2011


On Mon, Feb 07, 2011 at 11:01:35AM -0800, Jesse Gross wrote:
> 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.

OK, fair enough.




More information about the dev mailing list