Re: [Linux-ATM-General] [RFC][PATCH 0/2] atm: br2682: Improve encapsulation checks and headroom allocation

[Pascal H. <pa...@pl...>]

chas williams - CONTRACTOR a écrit :
> On Fri, 12 Aug 2011 22:01:10 +0200
> Pascal Hambourg <pa...@pl...> wrote:
> 
>> I updated and resent the patch 1/2 (v2) accordingly :
[...]
> this looks fine at a glance.  i do wonder if:
> 
> *((u16 *) (skb->data + 8)) == 0
> 
> works on all platforms.  cpu's other than x86 sometimes have issues
> with arbitrary alignments of data.   it should probalby be a memcmp()
> but that should probably be fixed in a seperate patch.

I copied this line from the VC-MUX part, even though I didn't like it.
But from some readings I understood that skb data are supposed to be
aligned on multiple of at least 2, and no one complained so far, so I
thought it should be fine. Also I felt that memcmp() may not be the most
efficient way to check that two consecutive bytes are 0. However I'll be
glad to replace the line with something like

	memcmp(skb->data + 8, pad, BR2684_PAD_LEN) == 0
or
	skb->data[8] == 0 && skb->data[9] == 0
or
	(skb->data[8] | skb->data[9]) == 0

Which one is fastest ? I'm afraid this is platform-dependent.
Or is there a built-in function in the kernel which can do this
efficiently ?

>> The optimizations you suggested are way beyond my skill and knowledge of the
>> kernel code. My goal here was just to provide fixes with minimal changes which
>> could also be easily backported to the -stable and -longterm trees.
> 
> my only suggestion is that besides getting minheadroom correct, is
> that you should set hard_headerlen correctly as well when the interface
> is setup.  in br2684_setup_routed() you should also do something
> like netdev->hard_header_len = 2;

I haven't read about net_device parameters yet and don't know how
hard_header_len is used, so I'm not comfortable with messing with it.
How is it used ?

> sometimes this should be 0 but you dont know the encapsulation until
> the vcc is registered so at this point you are best just picking the
> worst case.
> 
> also, for the bridged case we know that the hard_header_len is going to 
> atmost ETH_HLEN + 10 since we will also always prepend something.
> 
> this should keep the need to skb_realloc_headroom() to a minimum.

Can't hard_header_len be changed after the interface is created ?