[PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[Eric Dumazet]

eth_type_trans() & get_rps_cpus() currently need two 64bytes cache lines
in packet to compute rxhash.

Increasing NET_SKB_PAD from 32 to 64 reduces the need to one cache line
only, and makes RPS faster.

NET_IP_ALIGN(2) + ethernet_header(14) + IP_header(20/40) + ports(8)

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 746a652..fe5798b 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1356,9 +1356,12 @@ static inline int skb_network_offset(const struct sk_buff *skb)
  *
  * Various parts of the networking layer expect at least 32 bytes of
  * headroom, you should not reduce this.
+ * With RPS, we raised NET_SKB_PAD to 64 so that get_rps_cpus() fetches span
+ * a 64 bytes aligned block to fit modern (>= 64 bytes) cache line sizes
+ * NET_IP_ALIGN(2) + ethernet_header(14) + IP_header(20/40) + ports(8)
  */
 #ifndef NET_SKB_PAD
-#define NET_SKB_PAD	32
+#define NET_SKB_PAD	64
 #endif
 
 extern int ___pskb_trim(struct sk_buff *skb, unsigned int len);

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 05 May 2010 07:24:09 +0200

> eth_type_trans() & get_rps_cpus() currently need two 64bytes cache lines
> in packet to compute rxhash.
> 
> Increasing NET_SKB_PAD from 32 to 64 reduces the need to one cache line
> only, and makes RPS faster.
> 
> NET_IP_ALIGN(2) + ethernet_header(14) + IP_header(20/40) + ports(8)
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks Eric.

Seeing this made me go check who was overriding NET_IP_ALIGN or
NET_SKB_PAD.

The powerpc bits are legitimate, but the microblaze case is complete
bogosity.  It defines NET_IP_ALIGN to the default (2) and sets
NET_SKB_PAD to L1_CACHE_BYTES which on microblaze is 4 and
significantly smaller than the default.

So I'm going to delete them in net-next-2.6 like so:

--------------------
microblaze: Kill NET_SKB_PAD and NET_IP_ALIGN overrides.

NET_IP_ALIGN defaults to 2, no need to override.

NET_SKB_PAD is now 64, which is much larger than microblaze's
L1_CACHE_SIZE so no need to override that either.

Signed-off-by: David S. Miller <davem@davemloft.net>
---
 arch/microblaze/include/asm/system.h |   10 ----------
 1 files changed, 0 insertions(+), 10 deletions(-)

diff --git a/arch/microblaze/include/asm/system.h b/arch/microblaze/include/asm/system.h
index 48c4f03..b1e2f07 100644
--- a/arch/microblaze/include/asm/system.h
+++ b/arch/microblaze/include/asm/system.h
@@ -97,14 +97,4 @@ extern struct dentry *of_debugfs_root;
 
 #define arch_align_stack(x) (x)
 
-/*
- * MicroBlaze doesn't handle unaligned accesses in hardware.
- *
- * Based on this we force the IP header alignment in network drivers.
- * We also modify NET_SKB_PAD to be a cacheline in size, thus maintaining
- * cacheline alignment of buffers.
- */
-#define NET_IP_ALIGN	2
-#define NET_SKB_PAD	L1_CACHE_BYTES
-
 #endif /* _ASM_MICROBLAZE_SYSTEM_H */
-- 
1.7.0.4

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[Eric Dumazet]

Le jeudi 06 mai 2010 à 22:02 -0700, David Miller a écrit :

> Seeing this made me go check who was overriding NET_IP_ALIGN or
> NET_SKB_PAD.
> 
> The powerpc bits are legitimate, but the microblaze case is complete
> bogosity.  It defines NET_IP_ALIGN to the default (2) and sets
> NET_SKB_PAD to L1_CACHE_BYTES which on microblaze is 4 and
> significantly smaller than the default.
> 
> So I'm going to delete them in net-next-2.6 like so:
> 
> --------------------
> microblaze: Kill NET_SKB_PAD and NET_IP_ALIGN overrides.
> 
> NET_IP_ALIGN defaults to 2, no need to override.
> 
> NET_SKB_PAD is now 64, which is much larger than microblaze's
> L1_CACHE_SIZE so no need to override that either.
> 
> Signed-off-by: David S. Miller <davem@davemloft.net>
> ---
>  arch/microblaze/include/asm/system.h |   10 ----------
>  1 files changed, 0 insertions(+), 10 deletions(-)
> 
> diff --git a/arch/microblaze/include/asm/system.h b/arch/microblaze/include/asm/system.h
> index 48c4f03..b1e2f07 100644
> --- a/arch/microblaze/include/asm/system.h
> +++ b/arch/microblaze/include/asm/system.h
> @@ -97,14 +97,4 @@ extern struct dentry *of_debugfs_root;
>  
>  #define arch_align_stack(x) (x)
>  
> -/*
> - * MicroBlaze doesn't handle unaligned accesses in hardware.
> - *
> - * Based on this we force the IP header alignment in network drivers.
> - * We also modify NET_SKB_PAD to be a cacheline in size, thus maintaining
> - * cacheline alignment of buffers.
> - */
> -#define NET_IP_ALIGN	2
> -#define NET_SKB_PAD	L1_CACHE_BYTES
> -
>  #endif /* _ASM_MICROBLAZE_SYSTEM_H */

Yes, this seems strange it actually worked if L1_CACHE_BYTES = 4

Thanks

Re: [microblaze-uclinux] Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[John Williams]

On Fri, May 7, 2010 at 3:15 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le jeudi 06 mai 2010 à 22:02 -0700, David Miller a écrit :
>
>> Seeing this made me go check who was overriding NET_IP_ALIGN or
>> NET_SKB_PAD.
>>
>> The powerpc bits are legitimate, but the microblaze case is complete
>> bogosity.  It defines NET_IP_ALIGN to the default (2) and sets
>> NET_SKB_PAD to L1_CACHE_BYTES which on microblaze is 4 and
>> significantly smaller than the default.
>>
>> So I'm going to delete them in net-next-2.6 like so:
>>
>> --------------------
>> microblaze: Kill NET_SKB_PAD and NET_IP_ALIGN overrides.
>>
>> NET_IP_ALIGN defaults to 2, no need to override.
>>
>> NET_SKB_PAD is now 64, which is much larger than microblaze's
>> L1_CACHE_SIZE so no need to override that either.
>>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>> ---
>>  arch/microblaze/include/asm/system.h |   10 ----------
>>  1 files changed, 0 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/microblaze/include/asm/system.h b/arch/microblaze/include/asm/system.h
>> index 48c4f03..b1e2f07 100644
>> --- a/arch/microblaze/include/asm/system.h
>> +++ b/arch/microblaze/include/asm/system.h
>> @@ -97,14 +97,4 @@ extern struct dentry *of_debugfs_root;
>>
>>  #define arch_align_stack(x) (x)
>>
>> -/*
>> - * MicroBlaze doesn't handle unaligned accesses in hardware.
>> - *
>> - * Based on this we force the IP header alignment in network drivers.
>> - * We also modify NET_SKB_PAD to be a cacheline in size, thus maintaining
>> - * cacheline alignment of buffers.
>> - */
>> -#define NET_IP_ALIGN 2
>> -#define NET_SKB_PAD  L1_CACHE_BYTES
>> -
>>  #endif /* _ASM_MICROBLAZE_SYSTEM_H */
>
> Yes, this seems strange it actually worked if L1_CACHE_BYTES = 4

There will be some patches coming from Michal that cleans all of this
up - MicroBlaze has a configurable cacheline length, we have some
patches that set this to the longest possible (32 bytes) in a
conservative assumption.

John

Re: [microblaze-uclinux] Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[David Miller]

From: John Williams <john.williams@petalogix.com>
Date: Fri, 7 May 2010 15:28:08 +1000

> There will be some patches coming from Michal that cleans all of this
> up - MicroBlaze has a configurable cacheline length, we have some
> patches that set this to the longest possible (32 bytes) in a
> conservative assumption.

32 it the old default, the new one is 64.

So either way, these settings are completely bogus.

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[Michal Simek]

Eric Dumazet wrote:
> Le jeudi 06 mai 2010 à 22:02 -0700, David Miller a écrit :
> 
>> Seeing this made me go check who was overriding NET_IP_ALIGN or
>> NET_SKB_PAD.
>>
>> The powerpc bits are legitimate, but the microblaze case is complete
>> bogosity.  It defines NET_IP_ALIGN to the default (2) and sets
>> NET_SKB_PAD to L1_CACHE_BYTES which on microblaze is 4 and
>> significantly smaller than the default.
>>
>> So I'm going to delete them in net-next-2.6 like so:
>>
>> --------------------
>> microblaze: Kill NET_SKB_PAD and NET_IP_ALIGN overrides.
>>
>> NET_IP_ALIGN defaults to 2, no need to override.
>>
>> NET_SKB_PAD is now 64, which is much larger than microblaze's
>> L1_CACHE_SIZE so no need to override that either.
>>
>> Signed-off-by: David S. Miller <davem@davemloft.net>
>> ---
>>  arch/microblaze/include/asm/system.h |   10 ----------
>>  1 files changed, 0 insertions(+), 10 deletions(-)
>>
>> diff --git a/arch/microblaze/include/asm/system.h b/arch/microblaze/include/asm/system.h
>> index 48c4f03..b1e2f07 100644
>> --- a/arch/microblaze/include/asm/system.h
>> +++ b/arch/microblaze/include/asm/system.h
>> @@ -97,14 +97,4 @@ extern struct dentry *of_debugfs_root;
>>  
>>  #define arch_align_stack(x) (x)
>>  
>> -/*
>> - * MicroBlaze doesn't handle unaligned accesses in hardware.
>> - *
>> - * Based on this we force the IP header alignment in network drivers.
>> - * We also modify NET_SKB_PAD to be a cacheline in size, thus maintaining
>> - * cacheline alignment of buffers.
>> - */
>> -#define NET_IP_ALIGN	2
>> -#define NET_SKB_PAD	L1_CACHE_BYTES
>> -
>>  #endif /* _ASM_MICROBLAZE_SYSTEM_H */
> 
> Yes, this seems strange it actually worked if L1_CACHE_BYTES = 4

This was fault which I fixed. I sent pull request to Linus yesterday 
with contains patch which fix it.
L1_CACHE_BYTES was setup to 32 which is maximum cache line length on 
Microblaze.

I will add this Microblaze patch to my repo for testing and anyway 
should go through my repo.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[David Miller]

From: Michal Simek <monstr@monstr.eu>
Date: Fri, 07 May 2010 09:53:48 +0200

> I will add this Microblaze patch to my repo for testing and anyway
> should go through my repo.

It's already in the net-next-2.6 tree.

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[Michal Simek]

David Miller wrote:
> From: Michal Simek <monstr@monstr.eu>
> Date: Fri, 07 May 2010 09:53:48 +0200
> 
>> I will add this Microblaze patch to my repo for testing and anyway
>> should go through my repo.
> 
> It's already in the net-next-2.6 tree.

hmm. Not happy from it. I will test that patch from linux-next.

Michal



-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[Michal Simek]

David Miller wrote:
> From: Michal Simek <monstr@monstr.eu>
> Date: Fri, 07 May 2010 09:53:48 +0200
> 
>> I will add this Microblaze patch to my repo for testing and anyway
>> should go through my repo.
> 
> It's already in the net-next-2.6 tree.

Anyway.

I am ok with removing NET_IP_ALIGN because it is already defined in 
skbuff.h to 2.
But increasing NET_SKB_PAD to 64 caused that Microblaze extends skb 
buffers for some bytes.
I measured it by iperf and netperf and I see regression around 1-2Mbit/s 
that's why I would like to ask you to revert this patch or keep at least 
NET_SKB_PAD part.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[Eric Dumazet]

Le vendredi 07 mai 2010 à 11:48 +0200, Michal Simek a écrit :
> David Miller wrote:
> > From: Michal Simek <monstr@monstr.eu>
> > Date: Fri, 07 May 2010 09:53:48 +0200
> > 
> >> I will add this Microblaze patch to my repo for testing and anyway
> >> should go through my repo.
> > 
> > It's already in the net-next-2.6 tree.
> 
> Anyway.
> 
> I am ok with removing NET_IP_ALIGN because it is already defined in 
> skbuff.h to 2.
> But increasing NET_SKB_PAD to 64 caused that Microblaze extends skb 
> buffers for some bytes.
> I measured it by iperf and netperf and I see regression around 1-2Mbit/s 
> that's why I would like to ask you to revert this patch or keep at least 
> NET_SKB_PAD part.

Interesting.

Increasing NET_SKB_PAD to say 128 or 256 should not have performance
impact, but reserve a bit more ram. (truesize...)

Investigation is needed. Maybe your NIC now allocates high order pages ?

What driver are you using ?

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[Michal Simek]

Eric Dumazet wrote:
> Le vendredi 07 mai 2010 à 11:48 +0200, Michal Simek a écrit :
>> David Miller wrote:
>>> From: Michal Simek <monstr@monstr.eu>
>>> Date: Fri, 07 May 2010 09:53:48 +0200
>>>
>>>> I will add this Microblaze patch to my repo for testing and anyway
>>>> should go through my repo.
>>> It's already in the net-next-2.6 tree.
>> Anyway.
>>
>> I am ok with removing NET_IP_ALIGN because it is already defined in 
>> skbuff.h to 2.
>> But increasing NET_SKB_PAD to 64 caused that Microblaze extends skb 
>> buffers for some bytes.
>> I measured it by iperf and netperf and I see regression around 1-2Mbit/s 
>> that's why I would like to ask you to revert this patch or keep at least 
>> NET_SKB_PAD part.
> 
> Interesting.
> 
> Increasing NET_SKB_PAD to say 128 or 256 should not have performance
> impact, but reserve a bit more ram. (truesize...)

yes. Microblaze is very sensitive on it. I have spent a couple of days 
to find out why bandwidth is so bad and I found that truesize is the 
problem. Whole driver allocated skb buffer to max mtu size (9000).

But if mtu is 1500 the driver still worked with max skb buffers size
Please correct me if I am wrong but if truesize is setup to 9000 then 
net code is working with that size even if mtu is 1500.
The next thing is that cache operations take a lot of cpu cycles.

I create a patch which allocated aligned skb buffers where size depends 
on current mtu size and this change rapidly increase bandwidth for 
mtu=1500 from 8Mb/s to 35Mb/s (depends on setting). There is no 
difference if jumbo frames are used (I means for driver with/without my 
patch).

> 
> Investigation is needed. Maybe your NIC now allocates high order pages ?
> 
> What driver are you using ?
> 

I am using non-mainline ll_temac driver.
drivers/net/xilinx_lltemac/

http://developer.petalogix.com/git/gitweb.cgi?p=linux-2.6-microblaze.git;a=shortlog

Regards,
Michal




-- 
Michal Simek, Ing. (M.Eng)
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel 2.6 Microblaze Linux - http://www.monstr.eu/fdt/
Microblaze U-BOOT custodian

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[Eric Dumazet]

Le vendredi 07 mai 2010 à 12:09 +0200, Michal Simek a écrit :
> Eric Dumazet wrote:
> > Le vendredi 07 mai 2010 à 11:48 +0200, Michal Simek a écrit :
> >> David Miller wrote:
> >>> From: Michal Simek <monstr@monstr.eu>
> >>> Date: Fri, 07 May 2010 09:53:48 +0200
> >>>
> >>>> I will add this Microblaze patch to my repo for testing and anyway
> >>>> should go through my repo.
> >>> It's already in the net-next-2.6 tree.
> >> Anyway.
> >>
> >> I am ok with removing NET_IP_ALIGN because it is already defined in 
> >> skbuff.h to 2.
> >> But increasing NET_SKB_PAD to 64 caused that Microblaze extends skb 
> >> buffers for some bytes.
> >> I measured it by iperf and netperf and I see regression around 1-2Mbit/s 
> >> that's why I would like to ask you to revert this patch or keep at least 
> >> NET_SKB_PAD part.
> > 
> > Interesting.
> > 
> > Increasing NET_SKB_PAD to say 128 or 256 should not have performance
> > impact, but reserve a bit more ram. (truesize...)
> 
> yes. Microblaze is very sensitive on it. I have spent a couple of days 
> to find out why bandwidth is so bad and I found that truesize is the 
> problem. Whole driver allocated skb buffer to max mtu size (9000).
> 
> But if mtu is 1500 the driver still worked with max skb buffers size
> Please correct me if I am wrong but if truesize is setup to 9000 then 
> net code is working with that size even if mtu is 1500.
> The next thing is that cache operations take a lot of cpu cycles.

Allocating space <before> data doesnt mean we touch it. In fact we dont,
this is why I said increasing SKB_PAD from 32 to 64 should not make a
difference on perfs.

But yes, udp/tcp pacing is sensitive to truesize, since truesize is used
to limit socket receive buffer (bytes limit, not packets)

If you hit socket limit (or global limit), tcp will try to coalesce
buffers to reduce the 'truesize'. This involves very expensive
operations. UDP dont have such slow path, but might drop frames one
socket limit is reached.

So David patch (or mine if you prefer) only points an underlying problem
(truesize changed from 9056 to 9088, a very small increase...)

Using 9000 bytes buffers is very risky in the long term (if PAGE_SIZE is
4096), because after a while, your driver wont be able to allocate new
skbs.

> 
> I create a patch which allocated aligned skb buffers where size depends 
> on current mtu size and this change rapidly increase bandwidth for 
> mtu=1500 from 8Mb/s to 35Mb/s (depends on setting). There is no 
> difference if jumbo frames are used (I means for driver with/without my 
> patch).
> 
> > 
> > Investigation is needed. Maybe your NIC now allocates high order pages ?
> > 
> > What driver are you using ?
> > 
> 
> I am using non-mainline ll_temac driver.
> drivers/net/xilinx_lltemac/
> 
> http://developer.petalogix.com/git/gitweb.cgi?p=linux-2.6-microblaze.git;a=shortlog
> 
> Regards,
> Michal
> 
> 
> 
> 

Re: [PATCH net-next-2.6] net: Increase NET_SKB_PAD to 64 bytes

[David Miller]

From: Michal Simek <monstr@monstr.eu>
Date: Fri, 07 May 2010 11:48:06 +0200

> I would like to ask you to revert this patch

Done.