On Mon, Jul 23, 2007 at 10:38:39PM +0400, Alexey Dobriyan wrote:
quoted
Managed to hit BUG_ON() in kmap_atomic_prot() three times while doing
nothing unusual for this box (two times it was under X, so I can't
guarantee, one time while trying to reproduce via ./configure in gdb
tarball)
Yeah, I hit this several times a few days ago. Same story: it just
randomly went splat in response to no obvious stimulus. Reported it to
netdev, was greeted with stunned silence.
quoted
Box has 2.5G of RAM. 2.6.22 was OK.
[dives into framebuffer console setup for complete oops]
I had more complete info: http://article.gmane.org/gmane.linux.network/66966
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
On Mon, Jul 23, 2007 at 10:38:39PM +0400, Alexey Dobriyan wrote:
quoted
Managed to hit BUG_ON() in kmap_atomic_prot() three times while doing
nothing unusual for this box (two times it was under X, so I can't
guarantee, one time while trying to reproduce via ./configure in gdb
tarball)
Yeah, I hit this several times a few days ago. Same story: it just
randomly went splat in response to no obvious stimulus. Reported it to
netdev, was greeted with stunned silence.
quoted
quoted
Box has 2.5G of RAM. 2.6.22 was OK.
[dives into framebuffer console setup for complete oops]
I had more complete info: http://article.gmane.org/gmane.linux.network/66966
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
Ahh, you suspect networking.
Here, setup is 2 cheap-ass 100Mb realtek 8139 NICs, one to campus network
receiving ~20 junk packets per second, one gathering netconsole output
and ssh to it, no conntracks and fancy stuff.
[reboots with cables physically unplugged]
On Tue, Jul 24, 2007 at 12:40:45AM +0400, Alexey Dobriyan wrote:
quoted
I had more complete info: http://article.gmane.org/gmane.linux.network/66966
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
Ahh, you suspect networking.
Here, setup is 2 cheap-ass 100Mb realtek 8139 NICs, one to campus network
receiving ~20 junk packets per second, one gathering netconsole output
and ssh to it, no conntracks and fancy stuff.
[reboots with cables physically unplugged]
OK, I run gdb recompile, cat(1) every file in /usr/portage (shitload of
small files) with both cables unplugged. It all went fine for ~5 minutes
after that it crashed exactly same way after 10 secs after plugging one
of them.
On Tue, Jul 24, 2007 at 12:40:45AM +0400, Alexey Dobriyan wrote:
quoted
quoted
I had more complete info: http://article.gmane.org/gmane.linux.network/66966
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
Ahh, you suspect networking.
Here, setup is 2 cheap-ass 100Mb realtek 8139 NICs, one to campus network
receiving ~20 junk packets per second, one gathering netconsole output
and ssh to it, no conntracks and fancy stuff.
[reboots with cables physically unplugged]
OK, I run gdb recompile, cat(1) every file in /usr/portage (shitload of
small files) with both cables unplugged. It all went fine for ~5 minutes
after that it crashed exactly same way after 10 secs after plugging one
of them.
It'd be nice to get a clean trace. Are you able to obtain the full
trace with CONFIG_FRAME_POINTER=y?
It'd be nice to get a clean trace. Are you able to obtain the full
trace with CONFIG_FRAME_POINTER=y?
If you are talking about
http://userweb.kernel.org/~akpm/dsc03659.jpg
then I think that _is_ a full trace. It's certainly not very messy, and it
seems accurate. It's just that inlining makes it much harder to see the
call-graphs, but that's what inlining does..
For example, missing from the call graph is
get_page_from_freelist ->
buffered_rmqueue -> [ missing - inlined ]
prep_new_page -> [ missing - inlined ]
prep_zero_page -> [ missing - inlined ]
clear_highpage -> [ missing - inlined ]
kmap_atomic -> [ missing - tailcall ]
kmap_atomic_prot
but I'm pretty sure the call trace is good (and I'm also pretty sure gcc
is overly aggressive at inlining, and that it causes us pain for
debugging, but whatever)
The earlier part of the trace looks fine too.
The only odd part I see is the existence of "dput()" there, so maybe it's
not *quite* clean and enabling frame pointers might get rid of a few bogus
entries, but it looks pretty close to clean.
Linus
From: Sam Ravnborg <hidden> Date: 2007-07-23 21:36:21
For example, missing from the call graph is
get_page_from_freelist ->
buffered_rmqueue -> [ missing - inlined ]
prep_new_page -> [ missing - inlined ]
prep_zero_page -> [ missing - inlined ]
clear_highpage -> [ missing - inlined ]
kmap_atomic -> [ missing - tailcall ]
kmap_atomic_prot
(and I'm also pretty sure gcc
is overly aggressive at inlining, and that it causes us pain for
debugging, but whatever)
mm/page_alloc.c:static inline void prep_zero_page(struct page *page, int order, gfp_t gfp_flags)
include/linux/highmem.h:static inline void clear_highpage(struct page *page)
So at least two was explicit marked inline.
Now if that made I change i dunno.
Sam
On Tue, Jul 24, 2007 at 12:40:45AM +0400, Alexey Dobriyan wrote:
quoted
quoted
I had more complete info: http://article.gmane.org/gmane.linux.network/66966
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
Ahh, you suspect networking.
Here, setup is 2 cheap-ass 100Mb realtek 8139 NICs, one to campus network
receiving ~20 junk packets per second, one gathering netconsole output
and ssh to it, no conntracks and fancy stuff.
[reboots with cables physically unplugged]
OK, I run gdb recompile, cat(1) every file in /usr/portage (shitload of
small files) with both cables unplugged. It all went fine for ~5 minutes
after that it crashed exactly same way after 10 secs after plugging one
of them.
It'd be nice to get a clean trace. Are you able to obtain the full
trace with CONFIG_FRAME_POINTER=y?
Sorry, no camera shot, finding camera requires wakening up M. :)
It took longer that usual, but here it is
kmap_atomic
get_page_from_freelist
__alloc_pages
cache_alloc_refill
__alloc_pages
cache_alloc_refill
kmem_cache_alloc
dst_alloc
ip_route_input
ip_rcv
netif_receive_skb
rtl8139_poll
net_rx_action
__do_softirq
do_softirq
irq_exit
do_IRQ
common_interrupt
handle_mm_fault
do_page_fault
error_core
much more loaded x86_64 box near also running 2.6.23-rc1 with debugging
turned on, using atl1 driver doesn't experience any crashes.
And I found 2.6.22-b91cba52e9b7b3f1c0037908a192d93a869ca9e5-x entry on
top of grub config which means b91cba52e9b7b3f1c0037908a192d93a869ca9e5
_without_ any debugging was OK.
On Tue, Jul 24, 2007 at 12:40:45AM +0400, Alexey Dobriyan wrote:
quoted
quoted
I had more complete info: http://article.gmane.org/gmane.linux.network/66966
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
Ahh, you suspect networking.
Here, setup is 2 cheap-ass 100Mb realtek 8139 NICs, one to campus network
receiving ~20 junk packets per second, one gathering netconsole output
and ssh to it, no conntracks and fancy stuff.
[reboots with cables physically unplugged]
OK, I run gdb recompile, cat(1) every file in /usr/portage (shitload of
small files) with both cables unplugged. It all went fine for ~5 minutes
after that it crashed exactly same way after 10 secs after plugging one
of them.
It'd be nice to get a clean trace. Are you able to obtain the full
trace with CONFIG_FRAME_POINTER=y?
Sorry, no camera shot, finding camera requires wakening up M. :)
It took longer that usual, but here it is
kmap_atomic
get_page_from_freelist
__alloc_pages
cache_alloc_refill
__alloc_pages
cache_alloc_refill
kmem_cache_alloc
dst_alloc
ip_route_input
ip_rcv
netif_receive_skb
rtl8139_poll
net_rx_action
__do_softirq
do_softirq
irq_exit
do_IRQ
common_interrupt
handle_mm_fault
do_page_fault
error_core
much more loaded x86_64 box near also running 2.6.23-rc1 with debugging
turned on, using atl1 driver doesn't experience any crashes.
And I found 2.6.22-b91cba52e9b7b3f1c0037908a192d93a869ca9e5-x entry on
top of grub config which means b91cba52e9b7b3f1c0037908a192d93a869ca9e5
_without_ any debugging was OK.
I worked out that the crash I saw was in
BUG_ON(!pte_none(*(kmap_pte-idx)));
in the read of kmap_pte[idx]. Which would be weird as the caller is using
a literal KM_USER0.
So maybe I goofed, and that BUG_ON is triggering (it scrolled off, and I am
unable to reproduce it now).
If that BUG_ON _is_ triggering then it might indicate that someone is doing
a __GFP_HIGHMEM|__GFP_ZERO allocation while holding KM_USER0.
If they're holding an atomic kmap then they'll be running in_atomic so it
is unlikely that they accidentally added __GFP_WAIT because lots of people
would be getting lots of might_sleep() warnings.
Hence that first VM_BUG_ON in prep_zero_page() _should_ be triggering.
Do you have CONFIG_DEBUG_VM enabled?
Also, it might be useful to apply -mm's kmap_atomic-debugging.patch. it
will detect lots of abuse.
On Tue, Jul 24, 2007 at 12:40:45AM +0400, Alexey Dobriyan wrote:
quoted
quoted
I had more complete info: http://article.gmane.org/gmane.linux.network/66966
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
Ahh, you suspect networking.
Here, setup is 2 cheap-ass 100Mb realtek 8139 NICs, one to campus network
receiving ~20 junk packets per second, one gathering netconsole output
and ssh to it, no conntracks and fancy stuff.
[reboots with cables physically unplugged]
OK, I run gdb recompile, cat(1) every file in /usr/portage (shitload of
small files) with both cables unplugged. It all went fine for ~5 minutes
after that it crashed exactly same way after 10 secs after plugging one
of them.
It'd be nice to get a clean trace. Are you able to obtain the full
trace with CONFIG_FRAME_POINTER=y?
Sorry, no camera shot, finding camera requires wakening up M. :)
It took longer that usual, but here it is
kmap_atomic
get_page_from_freelist
__alloc_pages
cache_alloc_refill
__alloc_pages
cache_alloc_refill
kmem_cache_alloc
dst_alloc
ip_route_input
ip_rcv
netif_receive_skb
rtl8139_poll
net_rx_action
__do_softirq
do_softirq
irq_exit
do_IRQ
common_interrupt
handle_mm_fault
do_page_fault
error_core
much more loaded x86_64 box near also running 2.6.23-rc1 with debugging
turned on, using atl1 driver doesn't experience any crashes.
And I found 2.6.22-b91cba52e9b7b3f1c0037908a192d93a869ca9e5-x entry on
top of grub config which means b91cba52e9b7b3f1c0037908a192d93a869ca9e5
_without_ any debugging was OK.
I worked out that the crash I saw was in
BUG_ON(!pte_none(*(kmap_pte-idx)));
in the read of kmap_pte[idx]. Which would be weird as the caller is using
a literal KM_USER0.
So maybe I goofed, and that BUG_ON is triggering (it scrolled off, and I am
unable to reproduce it now).
If that BUG_ON _is_ triggering then it might indicate that someone is doing
a __GFP_HIGHMEM|__GFP_ZERO allocation while holding KM_USER0.
If they're holding an atomic kmap then they'll be running in_atomic so it
is unlikely that they accidentally added __GFP_WAIT because lots of people
would be getting lots of might_sleep() warnings.
Hence that first VM_BUG_ON in prep_zero_page() _should_ be triggering.
Do you have CONFIG_DEBUG_VM enabled?
Yes.
Also, it might be useful to apply -mm's kmap_atomic-debugging.patch. it
will detect lots of abuse.
I hit it only once with this patch applied, but there were no additional
warnings.
I worked out that the crash I saw was in
BUG_ON(!pte_none(*(kmap_pte-idx)));
in the read of kmap_pte[idx]. Which would be weird as the caller is using
a literal KM_USER0.
So maybe I goofed, and that BUG_ON is triggering (it scrolled off, and I am
unable to reproduce it now).
If that BUG_ON _is_ triggering then it might indicate that someone is doing
a __GFP_HIGHMEM|__GFP_ZERO allocation while holding KM_USER0.
Or doing double kunmaps, or doing a kunmap_atomic() on the page, not the
address. I've seen both of those end up triggering that BUG_ON() in a
later kmap.
Looking over the 2.6.22..2.6.23-rc1 diff, I found one such error in
ocfs2 at least. But you are probably not using that, so I'll keep
looking...
---
[PATCH] ocfs2: bad kunmap_atomic()
kunmap_atomic() takes the virtual address, not the mapped page as
argument.
Signed-off-by: Jens Axboe <redacted>
I worked out that the crash I saw was in
BUG_ON(!pte_none(*(kmap_pte-idx)));
in the read of kmap_pte[idx]. Which would be weird as the caller is using
a literal KM_USER0.
So maybe I goofed, and that BUG_ON is triggering (it scrolled off, and I am
unable to reproduce it now).
If that BUG_ON _is_ triggering then it might indicate that someone is doing
a __GFP_HIGHMEM|__GFP_ZERO allocation while holding KM_USER0.
Or doing double kunmaps, or doing a kunmap_atomic() on the page, not the
address. I've seen both of those end up triggering that BUG_ON() in a
later kmap.
Looking over the 2.6.22..2.6.23-rc1 diff, I found one such error in
ocfs2 at least. But you are probably not using that, so I'll keep
looking...
What about the new async crypto stuff? I've been looking, but is it
guarenteed that async_memcpy() runs in process context with interrupts
enabled always? If not, there's a km type bug there.
In general, I think the highmem stuff could do with more safety checks:
- People ALWAYS get the atomic unmaps wrong, passing in the page instead
of the address. I've seen tons of these. And since kunmap_atomic()
takes a void pointer, nobody notices until it goes boom.
- People easily get the km type wrong - they use KM_USERx in interrupt
context, or one of the irq variants without disabling interrupts.
If we could just catch these two types of bugs, we've got a lot of these
problems covered.
--
Jens Axboe
I worked out that the crash I saw was in
BUG_ON(!pte_none(*(kmap_pte-idx)));
in the read of kmap_pte[idx]. Which would be weird as the caller is using
a literal KM_USER0.
So maybe I goofed, and that BUG_ON is triggering (it scrolled off, and I am
unable to reproduce it now).
If that BUG_ON _is_ triggering then it might indicate that someone is doing
a __GFP_HIGHMEM|__GFP_ZERO allocation while holding KM_USER0.
Or doing double kunmaps, or doing a kunmap_atomic() on the page, not the
address. I've seen both of those end up triggering that BUG_ON() in a
later kmap.
Looking over the 2.6.22..2.6.23-rc1 diff, I found one such error in
ocfs2 at least. But you are probably not using that, so I'll keep
looking...
What about the new async crypto stuff? I've been looking, but is it
guarenteed that async_memcpy() runs in process context with interrupts
enabled always? If not, there's a km type bug there.
I think Shannon maintains that now.
In general, I think the highmem stuff could do with more safety checks:
- People ALWAYS get the atomic unmaps wrong, passing in the page instead
of the address. I've seen tons of these. And since kunmap_atomic()
takes a void pointer, nobody notices until it goes boom.
yeah, it's a real trap. For a while I had a patch which converted
kmap_atomic() to return a char*, and kunmap_atomic() to take a char*, so
misuse got compile warnings. But it was a pig to maintain so I tossed it.
It'd be somewhat easier to do now we've converted a lot of callers to
clear_user_highpage() and similar.
- People easily get the km type wrong - they use KM_USERx in interrupt
context, or one of the irq variants without disabling interrupts.
If we could just catch these two types of bugs, we've got a lot of these
problems covered.
Here's the -mm debug patch:
diff -puN arch/i386/mm/highmem.c~kmap_atomic-debugging arch/i386/mm/highmem.c
From: Mike Galbraith <hidden> Date: 2007-07-24 10:30:08
On Mon, 2007-07-23 at 13:24 -0700, Andrew Morton wrote:
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
My box bugged during boot the first time I booted 23-rc1, but nothing
made it to the console, and I didn't have a serial console running. I
didn't have DEBUG_PAGEALLOC or friends set.
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
From: Mike Galbraith <hidden> Date: 2007-07-24 10:38:07
On Tue, 2007-07-24 at 12:01 +0200, Mike Galbraith wrote:
On Mon, 2007-07-23 at 13:24 -0700, Andrew Morton wrote:
quoted
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
My box bugged during boot the first time I booted 23-rc1, but nothing
made it to the console, and I didn't have a serial console running. I
didn't have DEBUG_PAGEALLOC or friends set.
quoted
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
I just enabled all debug options, and was just rewarded with the below.
Hm. I just also experienced filesystem corruption when I tried to send
from that kernel, and it bugged in the process. My mount table ended up
in /etc/resolv.conf along with some binary goop, making nscd rather
unhappy after reboot. fsck time.
.Mike
From: Dan Williams <hidden> Date: 2007-07-24 13:55:49
On 7/24/07, Jens Axboe [off-list ref] wrote:
What about the new async crypto stuff? I've been looking, but is it
guarenteed that async_memcpy() runs in process context with interrupts
enabled always? If not, there's a km type bug there.
Currently the only user is the MD raid456 driver, and yes, it only
performs copies from the handle_stripe routine which is always run in
process context with interrupts enabled. However this is not
documented. Would it be advisable to add a WARN_ON for this
condition?
In general, I think the highmem stuff could do with more safety checks:
- People ALWAYS get the atomic unmaps wrong, passing in the page instead
of the address. I've seen tons of these. And since kunmap_atomic()
takes a void pointer, nobody notices until it goes boom.
- People easily get the km type wrong - they use KM_USERx in interrupt
context, or one of the irq variants without disabling interrupts.
If we could just catch these two types of bugs, we've got a lot of these
problems covered.
--
Jens Axboe
From: Dan Williams <hidden> Date: 2007-07-24 14:00:53
On 7/24/07, Andrew Morton [off-list ref] wrote:
[...]
quoted
What about the new async crypto stuff? I've been looking, but is it
guarenteed that async_memcpy() runs in process context with interrupts
enabled always? If not, there's a km type bug there.
I think Shannon maintains that now.
I am looking after the async_tx API, I will send a patch to update
MAINTAINERS shortly.
--
Dan
From: Andrew Morton <akpm@linux-foundation.org> Date: 2007-07-24 16:29:15
On Tue, 24 Jul 2007 12:01:09 +0200 Mike Galbraith [off-list ref] wrote:
On Mon, 2007-07-23 at 13:24 -0700, Andrew Morton wrote:
quoted
You're using DEBUG_PAGEALLOC, but I was not, so I think we can rule that out.
My box bugged during boot the first time I booted 23-rc1, but nothing
made it to the console, and I didn't have a serial console running. I
didn't have DEBUG_PAGEALLOC or friends set.
quoted
I haven't worked out where that kmap_atomic() call is coming from yet.
Both traces point up into the page allocator, but I _think_ that's stack
gunk.
I just enabled all debug options, and was just rewarded with the below.
See, networking's kmem_cache_alloc(..., __GFP_ZERO) ended up calling into
the page allocator with __GFP_ZERO. This is the bug - slab isn't supposed
to do that: the __GFP_ZERO is supposed to be removed.
Now, it's not a highmem page, so prep_zero_page() won't actually establish
a kmap, but it will check that the kmap slot is presently unused on this
CPU.
But networking calls in here from softirq context (illegal for KM_USER0)
and sometimes that KM_USER0 slot *will* be in use, so kmap_atomic_prot()
will go BUG.
I must say it's really really scary that such a low-level function as
prep_zero_page() is using KM_USER0. I don't think it has enough debugging
checks in there to prevent Bad Stuff from going undetected.
I guess this was the bug:
--- a/mm/slab.c~a+++ a/mm/slab.c
@@ -2776,7 +2776,7 @@ static int cache_grow(struct kmem_cache *'nodeid'.*/if(!objp)-objp=kmem_getpages(cachep,flags,nodeid);+objp=kmem_getpages(cachep,local_flags,nodeid);if(!objp)gotofailed;
_
I don't see why you later got fs corruption - afacit we won't actually
modify the KM_USER0 slot in this scenario.
262 /**
263 * kzalloc - allocate memory. The memory is set to zero.
(gdb) list *kmem_cache_alloc+0xa1
0xc0172e7a is in kmem_cache_alloc (mm/slab.c:3176).
3171 STATS_INC_ALLOCHIT(cachep);
3172 ac->touched = 1;
3173 objp = ac->entry[--ac->avail];
3174 } else {
3175 STATS_INC_ALLOCMISS(cachep);
3176 objp = cache_alloc_refill(cachep, flags);
3177 }
3178 return objp;
3179 }
3180
(gdb) list *cache_alloc_refill+0x380
0xc0172872 is in cache_alloc_refill (include/linux/gfp.h:154).
149
150 /* Unknown node is current node */
151 if (nid < 0)
152 nid = numa_node_id();
153
154 return __alloc_pages(gfp_mask, order,
155 NODE_DATA(nid)->node_zonelists + gfp_zone(gfp_mask));
156 }
157
158 #ifdef CONFIG_NUMA
(gdb) list *__alloc_pages+0x5b
0xc015b7fb is in __alloc_pages (mm/page_alloc.c:1248).
1243 if (unlikely(*z == NULL)) {
1244 /* Should this ever happen?? */
1245 return NULL;
1246 }
1247
1248 page = get_page_from_freelist(gfp_mask|__GFP_HARDWALL, order,
1249 zonelist, ALLOC_WMARK_LOW|ALLOC_CPUSET);
1250 if (page)
1251 goto got_pg;
1252
(gdb) list *get_page_from_freelist+0x39e
0xc015b6e0 is in get_page_from_freelist (include/linux/highmem.h:122).
117 return __alloc_zeroed_user_highpage(__GFP_MOVABLE, vma, vaddr);
118 }
119
120 static inline void clear_highpage(struct page *page)
121 {
122 void *kaddr = kmap_atomic(page, KM_USER0);
123 clear_page(kaddr);
124 kunmap_atomic(kaddr, KM_USER0);
125 }
126
From: Adrian Bunk <hidden> Date: 2007-07-24 18:00:41
On Mon, Jul 23, 2007 at 02:28:11PM -0700, Linus Torvalds wrote:
On Mon, 23 Jul 2007, Andrew Morton wrote:
quoted
It'd be nice to get a clean trace. Are you able to obtain the full
trace with CONFIG_FRAME_POINTER=y?
If you are talking about
http://userweb.kernel.org/~akpm/dsc03659.jpg
then I think that _is_ a full trace. It's certainly not very messy, and it
seems accurate. It's just that inlining makes it much harder to see the
call-graphs, but that's what inlining does..
For example, missing from the call graph is
get_page_from_freelist ->
buffered_rmqueue -> [ missing - inlined ]
prep_new_page -> [ missing - inlined ]
prep_zero_page -> [ missing - inlined ]
clear_highpage -> [ missing - inlined ]
kmap_atomic -> [ missing - tailcall ]
kmap_atomic_prot
but I'm pretty sure the call trace is good (and I'm also pretty sure gcc
is overly aggressive at inlining, and that it causes us pain for
debugging, but whatever)
...
For prep_zero_page() and clear_highpage() we can't blame gcc since we
force gcc to always inline them.
buffered_rmqueue() and prep_new_page() are static functions with only
one caller each, and for the normal non-debug case it's a really nice
optimization to have them inlined automatically. But it might make sense
to add -fno-inline-functions-called-once to the CFLAGS depending on some
debug option?
Linus
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
buffered_rmqueue() and prep_new_page() are static functions with only
one caller each, and for the normal non-debug case it's a really nice
optimization to have them inlined automatically.
I'm not at all sure I agree.
Inlining big functions doesn't actually tend to generally generate any
better code, so if gcc's logic is "single callsite - always inline", then
that logic is likely not right.
Linus
buffered_rmqueue() and prep_new_page() are static functions with only
one caller each, and for the normal non-debug case it's a really nice
optimization to have them inlined automatically.
I'm not at all sure I agree.
Inlining big functions doesn't actually tend to generally generate any
better code, so if gcc's logic is "single callsite - always inline", then
that logic is likely not right.
fwiw, -fno-inline-functions-called-once (who knew?) takes i386 allnoconfig
vmlinux .text from 928360 up to 955362 bytes (27k larger).
A surprisingly large increase - I wonder if it did something dumb. It
appears to still correctly inline those things which we've manually marked
inline. hm.
It would be nice to defeat the autoinlining for debug purposes though.
fwiw, -fno-inline-functions-called-once (who knew?) takes i386 allnoconfig
vmlinux .text from 928360 up to 955362 bytes (27k larger).
A surprisingly large increase - I wonder if it did something dumb. It
appears to still correctly inline those things which we've manually marked
inline. hm.
I think inlining small enough functions is worth it, and the thing is, the
kernel is actually pretty damn good at having lots of small functions.
It's one of the few things I really care about from a coding style
standpoint.
So I'm not surprised that "-fno-inline-functions-called-once" makes things
larger, because I think it's generally a good idea to inline things that
are just called once. But it does make things harder to debug, and the
performance advantages become increasingly small for bigger functions.
And that's a balancing act. Do we care about performance? Yes. But do we
care so much that it's worth inlining something like buffered_rmqueue()?
So I would not be surprised if "-fno-inline-functions-called-once" will
disable *all* the inlining heuristics, and say "oh, it's not an inline
function, and it's only called once, so we won't inline it at all".
So "called once" should probably make the inlining weight bigger (ie
inline *larger* functions than you would otherwise), it just shouldn't
make it "infinite". It's not worth it.
Linus
So "called once" should probably make the inlining weight bigger (ie
inline *larger* functions than you would otherwise), it just shouldn't
make it "infinite". It's not worth it.
There's probably a --param where it can be tweaked exactly. The
problem is that --params tend to be very gcc version specific
and might do something completely different on a newer or
older version. So it's better not to use them.
-Andi
From: Adrian Bunk <hidden> Date: 2007-07-24 19:41:37
On Tue, Jul 24, 2007 at 12:15:48PM -0700, Linus Torvalds wrote:
On Tue, 24 Jul 2007, Andrew Morton wrote:
quoted
fwiw, -fno-inline-functions-called-once (who knew?) takes i386 allnoconfig
vmlinux .text from 928360 up to 955362 bytes (27k larger).
A surprisingly large increase - I wonder if it did something dumb. It
appears to still correctly inline those things which we've manually marked
inline. hm.
I think inlining small enough functions is worth it, and the thing is, the
kernel is actually pretty damn good at having lots of small functions.
It's one of the few things I really care about from a coding style
standpoint.
So I'm not surprised that "-fno-inline-functions-called-once" makes things
larger, because I think it's generally a good idea to inline things that
are just called once. But it does make things harder to debug, and the
performance advantages become increasingly small for bigger functions.
And that's a balancing act. Do we care about performance? Yes.
When using CONFIG_CC_OPTIMIZE_FOR_SIZE=y we even actively tell gcc that
we only care about size and do not care about performance...
But do we
care so much that it's worth inlining something like buffered_rmqueue()?
...
Where is the problem with having buffered_rmqueue() inlined?
Linus
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
There's probably a --param where it can be tweaked exactly. The
problem is that --params tend to be very gcc version specific
and might do something completely different on a newer or
older version. So it's better not to use them.
I agree wholeheartedly with that sentiment. We've tried at times (because
some gcc snapshots made some *truly* insane choices for a while), and
maybe we still have some around. Not worth the pain.
Linus
But do we
care so much that it's worth inlining something like buffered_rmqueue()?
...
Where is the problem with having buffered_rmqueue() inlined?
In this case, it was a pain to just even try to find the call chain, or
read the asm.
I would encourage lots of kernel hackers to read the assembler code gcc
generates. I suspect people being aware of code generation issues (and
writing their code with that in mind) is a *much* bigger performance
impact than gcc inlining random functions.
So maybe I'm old-fashioned and crazy, but "readability of the asm result"
actually is a worthwhile goal. Not because we care directly, but because
I'd like to encourage people to do it, due to the *indirect* benefits.
Linus
On Tue, Jul 24, 2007 at 11:25:22AM -0700, Linus Torvalds wrote:
On Tue, 24 Jul 2007, Andrew Morton wrote:
quoted
I guess this was the bug:
Looks very likely to me. Mike, Alexey, does this fix things for you?
Yeah, box is running for more than hour, survived LTP, gdb testsuite,
portage sync and so on.
What new to me is that someone decided TSC is unstable but that's
probably OK on full debug kernel:
Clocksource tsc unstable (delta = 91724418 ns)
Time: pit clocksource has been installed.
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2007-07-26 06:16:50
Jeff Garzik wrote:
On Tue, 24 Jul 2007, Adrian Bunk wrote:
quoted
buffered_rmqueue() and prep_new_page() are static functions with only
one caller each, and for the normal non-debug case it's a really nice
optimization to have them inlined automatically.
I'm not at all sure I agree.
Inlining big functions doesn't actually tend to generally generate any
better code, so if gcc's logic is "single callsite - always inline", then
that logic is likely not right.
From: Adrian Bunk <hidden> Date: 2007-07-26 18:08:09
On Tue, Jul 24, 2007 at 12:48:13PM -0700, Linus Torvalds wrote:
On Tue, 24 Jul 2007, Adrian Bunk wrote:
quoted
quoted
But do we
care so much that it's worth inlining something like buffered_rmqueue()?
...
Where is the problem with having buffered_rmqueue() inlined?
In this case, it was a pain to just even try to find the call chain, or
read the asm.
Optimization versus debugging is a common issue...
As I said, it might make sense to disable this optimization depending on
some debugging option.
I would encourage lots of kernel hackers to read the assembler code gcc
generates. I suspect people being aware of code generation issues (and
writing their code with that in mind) is a *much* bigger performance
impact than gcc inlining random functions.
So maybe I'm old-fashioned and crazy, but "readability of the asm result"
actually is a worthwhile goal. Not because we care directly, but because
I'd like to encourage people to do it, due to the *indirect* benefits.
This would lead to people trying to optimize code for one gcc version -
and the code might stay this way for 10 years.
People should write readable C code. This also has the best chances of
resulting in good performance with the next gcc version on the next
generation hardware.
Linus
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
So maybe I'm old-fashioned and crazy, but "readability of the asm result"
actually is a worthwhile goal. Not because we care directly, but because
I'd like to encourage people to do it, due to the *indirect* benefits.
This would lead to people trying to optimize code for one gcc version -
and the code might stay this way for 10 years.
No. The fact is, code that is easy to optimize is easy to optimize.
It has _nothing_ to do with gcc versions.
Linus