| Message ID | 20100506.220221.90798296.davem@davemloft.net |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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 ? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 > > > > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */