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: 3488 bytes --]

Hi all,

On Thu, May 23, 2013 at 09:29:42AM +0200, Antonio Quartulli wrote:
> > 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.
> 

I did some tests on my MIPS routers and the result is that with Martin's
code skb->data points to unaligned memory after reserving 14 + 20
bytes and, of course, also after pushing 20 (as we expected).

I tested it with netdev_alloc_skb_ip_align() and the result was good: skb->data
is not unaligned anymore.

However, this is completely useless.

skb->data is accessed only by means of memcpy and no direct access to
frag_header members is done anywhere. This means that aligned or unaligned
memory does not matter at all, because memcpy always accesses byte by byte.

So the code is good as it is, there is no need for netdev_alloc_skb_ip_align().

Thanks again Martin for you 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 --]