Message ID | 20111102224317.GM31337@atomide.com |
---|---|
State | Changes Requested, archived |
Delegated to: | David Miller |
Headers | show |
On 02/11/2011 23:43, Tony Lindgren wrote: > Commit 87fb4b7b533073eeeaed0b6bf7c2328995f6c075 (net: more > accurate skb truesize) changed the alignment of size. This > can cause problems at least on some machines with NFS root: > > Unhandled fault: alignment exception (0x801) at 0xc183a43a > Internal error: : 801 [#1] PREEMPT > Modules linked in: > CPU: 0 Not tainted (3.1.0-08784-g5eeee4a #733) > pc : [<c02fbba0>] lr : [<c02fbb9c>] psr: 60000013 > sp : c180fef8 ip : 00000000 fp : c181f580 > r10: 00000000 r9 : c044b28c r8 : 00000001 > r7 : c183a3a0 r6 : c1835be0 r5 : c183a412 r4 : 000001f2 > r3 : 00000000 r2 : 00000000 r1 : ffffffe6 r0 : c183a43a > Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel > Control: 0005317f Table: 10004000 DAC: 00000017 > Process swapper (pid: 1, stack limit = 0xc180e270) > Stack: (0xc180fef8 to 0xc1810000) > fee0: 00000024 00000000 > ff00: 00000000 c183b9c0 c183b8e0 c044b28c c0507ccc c019dfc4 c180ff2c c0503cf8 > ff20: c180ff4c c180ff4c 00000000 c1835420 c182c740 c18349c0 c05233c0 00000000 > ff40: 00000000 c00e6bb8 c180e000 00000000 c04dd82c c0507e7c c050cc18 c183b9c0 > ff60: c05233c0 00000000 00000000 c01f34f4 c0430d70 c019d364 c04dd898 c04dd898 > ff80: c04dd82c c0507e7c c180e000 00000000 c04c584c c01f4918 c04dd898 c04dd82c > ffa0: c04ddd28 c180e000 00000000 c0008758 c181fa60 3231d82c 00000037 00000000 > ffc0: 00000000 c04dd898 c04dd82c c04ddd28 00000013 00000000 00000000 00000000 > ffe0: 00000000 c04b2224 00000000 c04b21a0 c001056c c001056c 00000000 00000000 > Function entered at [<c02fbba0>] from [<c019dfc4>] > Function entered at [<c019dfc4>] from [<c01f34f4>] > Function entered at [<c01f34f4>] from [<c01f4918>] > Function entered at [<c01f4918>] from [<c0008758>] > Function entered at [<c0008758>] from [<c04b2224>] > Function entered at [<c04b2224>] from [<c001056c>] > Code: e1a00005 e3a01028 ebfa7cb0 e35a0000 (e5858028) > > Here PC is at __alloc_skb and &shinfo->dataref is unaligned because > skb->end can be unaligned without this patch. > > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: Tony Lindgren <tony@atomide.com> > > --- a/net/core/skbuff.c > +++ b/net/core/skbuff.c > @@ -189,6 +189,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, > * aligned memory blocks, unless SLUB/SLAB debug is enabled. > * Both skb->head and skb_shared_info are cache line aligned. > */ > + size = SKB_DATA_ALIGN(size); > size += SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > data = kmalloc_node_track_caller(size, gfp_mask, node); > if (!data) Hmm, I dont think this is the right way to fix the bug. There is a problem with your kmalloc() or alignments on your architecture. What is the SMP_CACHE_BYTES value ? -- 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: Eric Dumazet <eric.dumazet@gmail.com> Date: Wed, 02 Nov 2011 23:55:11 +0100 > There is a problem with your kmalloc() or alignments on your architecture. > > What is the SMP_CACHE_BYTES value ? kmalloc() behavior doesn't have anything to do with this bug. The issue is calculation of skb->end, which is based upon calculated 'size' variable. skb->end determines alignment of skb_shared_info, which is where the alignment problem is occuring for Tony. -- 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: David Miller <davem@davemloft.net> Date: Wed, 02 Nov 2011 19:09:55 -0400 (EDT) > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Wed, 02 Nov 2011 23:55:11 +0100 > >> There is a problem with your kmalloc() or alignments on your architecture. >> >> What is the SMP_CACHE_BYTES value ? > > kmalloc() behavior doesn't have anything to do with this bug. Sorry, I take that back, I see now how ksize() and friends are being used here. -- 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 03/11/2011 00:09, David Miller wrote: > From: Eric Dumazet <eric.dumazet@gmail.com> > Date: Wed, 02 Nov 2011 23:55:11 +0100 > >> There is a problem with your kmalloc() or alignments on your architecture. >> >> What is the SMP_CACHE_BYTES value ? > > kmalloc() behavior doesn't have anything to do with this bug. > > The issue is calculation of skb->end, which is based upon calculated > 'size' variable. > > skb->end determines alignment of skb_shared_info, which is where the > alignment problem is occuring for Tony. > I understood that David, but check the code, and you can see that exact skb->end depends also on ksize() So maybe the right fix is to make sure skb->end is properly aligned, say with SLOB So a more generic fix is welcomed. -- 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
c> > The issue is calculation of skb->end, which is based upon calculated > 'size' variable. > > skb->end determines alignment of skb_shared_info, which is where the > alignment problem is occuring for Tony. Right, and SMP_CACHE_BYTES setting should save us in any case. For ARM, SMP_CACHE_BYTES seems to be set to L1_CACHE_BYTES which in turn is set via ARM_L1_CACHE_SHIFT which can be set seemingly to any value but the defaults are 5 and 6 which should be OK. So unless Tony is using a non-standard setting of ARM_L1_CACHE_SHIFT, this report is a bit mysterious. -- 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 <davem@davemloft.net> [111102 15:42]: > c> > > The issue is calculation of skb->end, which is based upon calculated > > 'size' variable. > > > > skb->end determines alignment of skb_shared_info, which is where the > > alignment problem is occuring for Tony. > > Right, and SMP_CACHE_BYTES setting should save us in any case. > > For ARM, SMP_CACHE_BYTES seems to be set to L1_CACHE_BYTES which in > turn is set via ARM_L1_CACHE_SHIFT which can be set seemingly to any > value but the defaults are 5 and 6 which should be OK. > > So unless Tony is using a non-standard setting of ARM_L1_CACHE_SHIFT, > this report is a bit mysterious. This is happening at least with omap1_defconfig. In that case we have ARM_L1_CACHE_SHIFT=5. Regards, Tony -- 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 03/11/2011 00:19, Tony Lindgren wrote: > * David Miller <davem@davemloft.net> [111102 15:42]: >> c> >>> The issue is calculation of skb->end, which is based upon calculated >>> 'size' variable. >>> >>> skb->end determines alignment of skb_shared_info, which is where the >>> alignment problem is occuring for Tony. >> >> Right, and SMP_CACHE_BYTES setting should save us in any case. >> >> For ARM, SMP_CACHE_BYTES seems to be set to L1_CACHE_BYTES which in >> turn is set via ARM_L1_CACHE_SHIFT which can be set seemingly to any >> value but the defaults are 5 and 6 which should be OK. >> >> So unless Tony is using a non-standard setting of ARM_L1_CACHE_SHIFT, >> this report is a bit mysterious. > > This is happening at least with omap1_defconfig. In that case we have > ARM_L1_CACHE_SHIFT=5. > > Regards, > > Tony Do you use SLOB, SLUB or SLAB ? 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
From: Tony Lindgren <tony@atomide.com> Date: Wed, 2 Nov 2011 16:19:17 -0700 > * David Miller <davem@davemloft.net> [111102 15:42]: >> c> >> > The issue is calculation of skb->end, which is based upon calculated >> > 'size' variable. >> > >> > skb->end determines alignment of skb_shared_info, which is where the >> > alignment problem is occuring for Tony. >> >> Right, and SMP_CACHE_BYTES setting should save us in any case. >> >> For ARM, SMP_CACHE_BYTES seems to be set to L1_CACHE_BYTES which in >> turn is set via ARM_L1_CACHE_SHIFT which can be set seemingly to any >> value but the defaults are 5 and 6 which should be OK. >> >> So unless Tony is using a non-standard setting of ARM_L1_CACHE_SHIFT, >> this report is a bit mysterious. > > This is happening at least with omap1_defconfig. In that case we have > ARM_L1_CACHE_SHIFT=5. Ok, so ksize(x) gives an odd value for whichever allocator you are using. Which is the point Eric was trying to make. It becomes clearernow. -- 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 <eric.dumazet@gmail.com> [111102 15:46]: > On 03/11/2011 00:19, Tony Lindgren wrote: > > > * David Miller <davem@davemloft.net> [111102 15:42]: > >> c> > >>> The issue is calculation of skb->end, which is based upon calculated > >>> 'size' variable. > >>> > >>> skb->end determines alignment of skb_shared_info, which is where the > >>> alignment problem is occuring for Tony. > >> > >> Right, and SMP_CACHE_BYTES setting should save us in any case. > >> > >> For ARM, SMP_CACHE_BYTES seems to be set to L1_CACHE_BYTES which in > >> turn is set via ARM_L1_CACHE_SHIFT which can be set seemingly to any > >> value but the defaults are 5 and 6 which should be OK. > >> > >> So unless Tony is using a non-standard setting of ARM_L1_CACHE_SHIFT, > >> this report is a bit mysterious. > > > > This is happening at least with omap1_defconfig. In that case we have > > ARM_L1_CACHE_SHIFT=5. > > > > Regards, > > > > Tony > > > Do you use SLOB, SLUB or SLAB ? Seems to be SLOB for omap1_defconfig. Tony -- 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 03/11/2011 00:24, Tony Lindgren wrote: > > Seems to be SLOB for omap1_defconfig. > > Tony OK this makes sense now Your patch is absolutely needed, I completely forgot about SLOB :( since, kmalloc(386) on SLOB gives exactly ksize=386 bytes, not nearest power of two. [ 60.305763] malloc(size=385)->ffff880112c11e38 ksize=386 -> nsize=2 [ 60.305921] malloc(size=385)->ffff88007c92ce28 ksize=386 -> nsize=2 [ 60.306898] malloc(size=656)->ffff88007c44ad28 ksize=656 -> nsize=272 [ 60.325385] malloc(size=656)->ffff88007c575868 ksize=656 -> nsize=272 [ 60.325531] malloc(size=656)->ffff88011c777230 ksize=656 -> nsize=272 [ 60.325701] malloc(size=656)->ffff880114011008 ksize=656 -> nsize=272 [ 60.346716] malloc(size=385)->ffff880114142008 ksize=386 -> nsize=2 [ 60.346900] malloc(size=385)->ffff88011c777690 ksize=386 -> nsize=2 Acked-by: Eric Dumazet <eric.dumazet@gmail.com> 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
--- a/net/core/skbuff.c +++ b/net/core/skbuff.c @@ -189,6 +189,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask, * aligned memory blocks, unless SLUB/SLAB debug is enabled. * Both skb->head and skb_shared_info are cache line aligned. */ + size = SKB_DATA_ALIGN(size); size += SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); data = kmalloc_node_track_caller(size, gfp_mask, node); if (!data)
Commit 87fb4b7b533073eeeaed0b6bf7c2328995f6c075 (net: more accurate skb truesize) changed the alignment of size. This can cause problems at least on some machines with NFS root: Unhandled fault: alignment exception (0x801) at 0xc183a43a Internal error: : 801 [#1] PREEMPT Modules linked in: CPU: 0 Not tainted (3.1.0-08784-g5eeee4a #733) pc : [<c02fbba0>] lr : [<c02fbb9c>] psr: 60000013 sp : c180fef8 ip : 00000000 fp : c181f580 r10: 00000000 r9 : c044b28c r8 : 00000001 r7 : c183a3a0 r6 : c1835be0 r5 : c183a412 r4 : 000001f2 r3 : 00000000 r2 : 00000000 r1 : ffffffe6 r0 : c183a43a Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment kernel Control: 0005317f Table: 10004000 DAC: 00000017 Process swapper (pid: 1, stack limit = 0xc180e270) Stack: (0xc180fef8 to 0xc1810000) fee0: 00000024 00000000 ff00: 00000000 c183b9c0 c183b8e0 c044b28c c0507ccc c019dfc4 c180ff2c c0503cf8 ff20: c180ff4c c180ff4c 00000000 c1835420 c182c740 c18349c0 c05233c0 00000000 ff40: 00000000 c00e6bb8 c180e000 00000000 c04dd82c c0507e7c c050cc18 c183b9c0 ff60: c05233c0 00000000 00000000 c01f34f4 c0430d70 c019d364 c04dd898 c04dd898 ff80: c04dd82c c0507e7c c180e000 00000000 c04c584c c01f4918 c04dd898 c04dd82c ffa0: c04ddd28 c180e000 00000000 c0008758 c181fa60 3231d82c 00000037 00000000 ffc0: 00000000 c04dd898 c04dd82c c04ddd28 00000013 00000000 00000000 00000000 ffe0: 00000000 c04b2224 00000000 c04b21a0 c001056c c001056c 00000000 00000000 Function entered at [<c02fbba0>] from [<c019dfc4>] Function entered at [<c019dfc4>] from [<c01f34f4>] Function entered at [<c01f34f4>] from [<c01f4918>] Function entered at [<c01f4918>] from [<c0008758>] Function entered at [<c0008758>] from [<c04b2224>] Function entered at [<c04b2224>] from [<c001056c>] Code: e1a00005 e3a01028 ebfa7cb0 e35a0000 (e5858028) Here PC is at __alloc_skb and &shinfo->dataref is unaligned because skb->end can be unaligned without this patch. Cc: Eric Dumazet <eric.dumazet@gmail.com> Signed-off-by: Tony Lindgren <tony@atomide.com> -- 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