Re: [B.A.T.M.A.N.] [PATCHv3 3/3] batman-adv: Fragment and send skbs larger than mtu

[Antonio Quartulli <ordex@autistici.org>]

[-- Attachment #1: Type: text/plain, Size: 2856 bytes --]

Hi Martin,

On Thu, May 23, 2013 at 07:38:33AM +0200, Martin Hundebøll wrote:
> Hi Antonio,
> 
> On 2013-05-21 21:19, Antonio Quartulli wrote:
> > On Tue, May 21, 2013 at 12:48:19PM +0200, Martin Hundebøll wrote:
> >> +static struct sk_buff *batadv_frag_create(struct sk_buff *skb,
> >> +					  struct batadv_frag_packet *frag_head,
> >> +					  unsigned int mtu)
> >> +{
> >> +	struct sk_buff *skb_fragment;
> >> +	unsigned header_size = sizeof(*frag_head);
> >> +	unsigned fragment_size = mtu - header_size;
> >> +
> >> +	skb_fragment = dev_alloc_skb(mtu + ETH_HLEN);
> >> +	if (!skb_fragment)
> >> +		goto err;
> >> +
> >> +	/* Eat the last mtu-bytes of the skb */
> >> +	skb_reserve(skb_fragment, header_size + ETH_HLEN);
> >> +	skb_split(skb, skb_fragment, skb->len - fragment_size);
> >> +
> >> +	/* Add the header */
> >> +	skb_push(skb_fragment, header_size);
> >> +	memcpy(skb_fragment->data, frag_head, header_size);
> >
> > here we are copying the data right after the Fragment header. However I am not
> > sure we are accessing aligned memory because:
> >
> > ETH_HLEN + header_size = 14 + 20 = 34
> >
> > To speed up the copy, wouldn't it be better to allocate ETH_HLEN +
> > header_size + IP_ALIGN bytes like we do for other packets? (you can use
> > netdev_alloc_skb_ip_align() like we do somewhere else).
> >
> > In this way the memcpy will access a 4bytes aligned memory (if I am not wrong).
> >
> > Can somebody else comment on this?
> 
> I did a quick test by adding some printk-debugging after skb_split() and 
> skb_push():
> 	printk("data: %lu\n", (unsigned long)skb->data % 4);
> 
> and it seems like the skb->data pointer is aligned to boundaries of 4 in 
> both with and without the use of netdev_alloc_skb_ip_align().
> 

Because in x86 NET_IP_ALIGN is 0 since unaligned memory is correctly handled in
hardware using DMA
(http://lingrok.org/xref/linux-net-next/arch/x86/include/asm/processor.h#40).
I don't  know if my explanation is correct, but refer to the link for more
details :D

> This was done on x86_64 virtual machines. Is there another way this 
> should be tested or maybe on some different architecture?
> 

I guess MIPS would be a good choice (like most of the routers out there).
Please, put the printk before and after the push(). Before the push it will give
us an idea of how the header is going to be aligned (which is the most important
case since memcpy does not care about unaligned memory - it accesses memory byte
by byte). You can try to do this tests on the x86 too.

> The next revision is pretty much ready, but I will keep it back a bit, 
> until someone clarifies this align stuff :)
> 

Thank you very much for these tests!!


Cheers,

-- 
Antonio Quartulli

..each of us alone is worth nothing..
Ernesto "Che" Guevara

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]