I sent this same message to LKML a while ago, but thought I''d get it a
more targeted audience:
========================Noticed today that the combination of 4KSTACKS and
DEBUG_STACKOVERFLOW
config options is a bit deadly.
DEBUG_STACKOVERFLOW warns in do_IRQ if we''re within THREAD_SIZE/8 of
the
end of useable stack space, or 512 bytes on a 4k stack.
If we are, then it goes down the dump_stack path, which uses most, if
not all, of the remaining stack, thereby turning a well-intentioned
warning into a full-blown catastrophe.
The callchain from the warning looks something like this, with stack
usage shown as found on my x86 box:
4 dump_stack
4 show_trace
8 show_trace_log_lvl
4 dump_trace
print_context_stack
12 print_trace_address
print_symbol
232 __print_symbol
164 sprint_symbol
20 printk
___
448
448 bytes to tell us that we''re within 512 bytes (or less) of certain
doom... and I think there''s call overhead on top of that?
The large stack usage in those 2 functions is due to big char arrays, of
size KSYM_NAME_LEN (128 bytes) and KSYM_SYMBOL_LEN (223 bytes).
IOW, the stack warning effectively reduces useful stack left in our itty
bitty 4k stacks by over 10%.
...
========================
In light of this, I''d like to propose that we turn off
DEBUG_STACKOVERFLOW in Fedora, at least on x86/4KSTACKS. I think it
does more harm than good; the warning is going to turn deadly most of
the time.
I also had sent a patch to LKML to print whether or not the stack was
overflowing, or had ever overflowed, on a kernel panic. It''s not yet
been merged.
... any comments? I can file a bug but thought some discussion might be
in order.
Thanks,
-Eric
On Tue, Sep 11, 2007 at 03:05:52PM -0500, Eric Sandeen wrote: > In light of this, I''d like to propose that we turn off > DEBUG_STACKOVERFLOW in Fedora, at least on x86/4KSTACKS. I think it > does more harm than good; the warning is going to turn deadly most of > the time. > > I also had sent a patch to LKML to print whether or not the stack was > overflowing, or had ever overflowed, on a kernel panic. It''s not yet > been merged. > > ... any comments? I can file a bug but thought some discussion might be > in order. We could turn it off everywhere expect the -debug kernels and just have XFS + -debug be a ''dont do that'' thing. Dave -- http://www.codemonkey.org.uk
Dave Jones wrote:> On Tue, Sep 11, 2007 at 03:05:52PM -0500, Eric Sandeen wrote: > > > In light of this, I''d like to propose that we turn off > > DEBUG_STACKOVERFLOW in Fedora, at least on x86/4KSTACKS. I think it > > does more harm than good; the warning is going to turn deadly most of > > the time. > > > > I also had sent a patch to LKML to print whether or not the stack was > > overflowing, or had ever overflowed, on a kernel panic. It''s not yet > > been merged. > > > > ... any comments? I can file a bug but thought some discussion might be > > in order. > > We could turn it off everywhere expect the -debug kernels and > just have XFS + -debug be a ''dont do that'' thing.Well, I didn''t actually *say* xfs ... *cough* ;-) There have been reports of mount.nfs, for example, using within 600 bytes of the end of the stack... if they''d caught an interrupt at the end of whatever callchain that was, they''de be uncomfortably close to the edge too. And - what is the use of keeping the warning turned on *anywhere* when really, all it does is turn a near-miss into the real thing? If you actually overrun the stack, you''re probably going to oops anyway, and get about the same backtrace - except, hopefully less often, w/o the warning going off. I see this almost entirely as statistically shaving another ~500 bytes off the usable stack, without generating much useful information in return. Or am I missing some other usefulness of this? -Eric
On Tue, Sep 11, 2007 at 04:05:09PM -0500, Eric Sandeen wrote: > Dave Jones wrote: > > On Tue, Sep 11, 2007 at 03:05:52PM -0500, Eric Sandeen wrote: > > > > > In light of this, I''d like to propose that we turn off > > > DEBUG_STACKOVERFLOW in Fedora, at least on x86/4KSTACKS. I think it > > > does more harm than good; the warning is going to turn deadly most of > > > the time. > > > > > > I also had sent a patch to LKML to print whether or not the stack was > > > overflowing, or had ever overflowed, on a kernel panic. It''s not yet > > > been merged. > > > > > > ... any comments? I can file a bug but thought some discussion might be > > > in order. > > > > We could turn it off everywhere expect the -debug kernels and > > just have XFS + -debug be a ''dont do that'' thing. > > Well, I didn''t actually *say* xfs ... *cough* ;-) > > There have been reports of mount.nfs, for example, using within 600 > bytes of the end of the stack... if they''d caught an interrupt at the > end of whatever callchain that was, they''de be uncomfortably close to > the edge too. > > And - what is the use of keeping the warning turned on *anywhere* when > really, all it does is turn a near-miss into the real thing? If you > actually overrun the stack, you''re probably going to oops anyway, and > get about the same backtrace - except, hopefully less often, w/o the > warning going off. > > I see this almost entirely as statistically shaving another ~500 bytes > off the usable stack, without generating much useful information in return. > > Or am I missing some other usefulness of this? Hmm, good point. Dave -- http://www.codemonkey.org.uk
On Tue, 2007-09-11 at 15:05 -0500, Eric Sandeen wrote:> I sent this same message to LKML a while ago, but thought I''d get it a > more targeted audience: > > ========================> Noticed today that the combination of 4KSTACKS and DEBUG_STACKOVERFLOW > config options is a bit deadly. > > DEBUG_STACKOVERFLOW warns in do_IRQ if we''re within THREAD_SIZE/8 of the > end of useable stack space, or 512 bytes on a 4k stack. > > If we are, then it goes down the dump_stack path, which uses most, if > not all, of the remaining stack, thereby turning a well-intentioned > warning into a full-blown catastrophe. > > The callchain from the warning looks something like this, with stack > usage shown as found on my x86 box: > > 4 dump_stack > 4 show_trace > 8 show_trace_log_lvl > 4 dump_trace > print_context_stack > 12 print_trace_address > print_symbol > 232 __print_symbol > 164 sprint_symbol > 20 printk > ___ > 448 > > 448 bytes to tell us that we''re within 512 bytes (or less) of certain > doom... and I think there''s call overhead on top of that? > > The large stack usage in those 2 functions is due to big char arrays, of > size KSYM_NAME_LEN (128 bytes) and KSYM_SYMBOL_LEN (223 bytes). > > IOW, the stack warning effectively reduces useful stack left in our itty > bitty 4k stacks by over 10%. > > ... > > ========================> > > In light of this, I''d like to propose that we turn off > DEBUG_STACKOVERFLOW in Fedora, at least on x86/4KSTACKS. I think it > does more harm than good; the warning is going to turn deadly most of > the time. > > I also had sent a patch to LKML to print whether or not the stack was > overflowing, or had ever overflowed, on a kernel panic. It''s not yet > been merged. > > ... any comments? I can file a bug but thought some discussion might be > in order. > > Thanks, >Sorry for butting in... but isn''t disabling STACKOVERFLOW the wrong answer to this problem? Does anyone see any reason why both sprint_symbol and __print_symbol shouldn''t use dynamically allocated buffers instead of wasting stack space? * - GIlboa * If performance is an issue, memory can be statically allocated per CPU with additional locking in dump_trace.
On Sat, Sep 15, 2007 at 01:34:24AM +0300, Gilboa Davara wrote: > > In light of this, I''d like to propose that we turn off > > DEBUG_STACKOVERFLOW in Fedora, at least on x86/4KSTACKS. I think it > > does more harm than good; the warning is going to turn deadly most of > > the time. > > > Sorry for butting in... but isn''t disabling STACKOVERFLOW the wrong > answer to this problem? > Does anyone see any reason why both sprint_symbol and __print_symbol > shouldn''t use dynamically allocated buffers instead of wasting stack > space? * I think the reason is that kmalloc can sleep, and those functions may be called from code that can''t sleep. Dave -- http://www.codemonkey.org.uk
On Fri, 2007-09-14 at 18:42 -0400, Dave Jones wrote:> On Sat, Sep 15, 2007 at 01:34:24AM +0300, Gilboa Davara wrote: > > > > In light of this, I''d like to propose that we turn off > > > DEBUG_STACKOVERFLOW in Fedora, at least on x86/4KSTACKS. I think it > > > does more harm than good; the warning is going to turn deadly most of > > > the time. > > > > > Sorry for butting in... but isn''t disabling STACKOVERFLOW the wrong > > answer to this problem? > > Does anyone see any reason why both sprint_symbol and __print_symbol > > shouldn''t use dynamically allocated buffers instead of wasting stack > > space? * > > I think the reason is that kmalloc can sleep, and those functions > may be called from code that can''t sleep. > > Dave >kmalloc(GFP_ATOMIC) should not sleep. ... I wonder if there''s a way, with no local_irq_disable/enable to use some pre-allocated per_cpu buffer. .. Either way, if such a patch is required, I should really propose and push it upstream. - Gilboa
Gilboa Davara wrote:> Sorry for butting in... but isn''t disabling STACKOVERFLOW the wrong > answer to this problem? > Does anyone see any reason why both sprint_symbol and __print_symbol > shouldn''t use dynamically allocated buffers instead of wasting stack > space? * > > - GIlboa > * If performance is an issue, memory can be statically allocated per CPU > with additional locking in dump_trace.Well, I agree that the dump_stack path should be lightened up stack-wise; and I don''t think performance should be an issue (dump_stack is used when something has gone wrong, probably not going to be performance critical?) Locked global buffers may be just fine (we did this for xfs error messages, I remember...) I was looking at this from a slightly different angle, which is that the stack overflow warning is largely pointless - no matter how much you lighten up the dump_stack path, it will add something to the stack depth of the current process, effectively *reducing* the available stack for all processes, and increasing the risk that you''ll actually overflow. (if you take an interrupt towards the end of the stack, the warning will go off and use the last bit - so you can''t count on that stack space to be available). And, if you overflow the stack, you''ll almost certainly get an oops and a backtrace anyway - usually thread_info gets overwritten and you BUG because it looks like you sleep in an interrupt, or somesuch. So, what''s the point of the IRQ stack-depth check, again? Especially with 4k stacks and separate IRQ stacks? And the more deterministic max-stack-depth excursion checker (CONFIG_DEBUG_STACK_USAGE) as well... Finally, the patch I sent upstream would clearly show on an oops whether or not the stack was currently overflowing, or whether the stack had ever overflowed prior to the oops. Seemed useful to me. -Eric
On Fri, 2007-09-14 at 22:07 -0500, Eric Sandeen wrote:> Gilboa Davara wrote: > > > Sorry for butting in... but isn''t disabling STACKOVERFLOW the wrong > > answer to this problem? > > Does anyone see any reason why both sprint_symbol and __print_symbol > > shouldn''t use dynamically allocated buffers instead of wasting stack > > space? * > > > > - GIlboa > > * If performance is an issue, memory can be statically allocated per CPU > > with additional locking in dump_trace. > > Well, I agree that the dump_stack path should be lightened up > stack-wise; and I don''t think performance should be an issue (dump_stack > is used when something has gone wrong, probably not going to be > performance critical?) Locked global buffers may be just fine (we did > this for xfs error messages, I remember...)I chose the easy wait out and generated a simple, alloc-by-demand patch. http://lkml.org/lkml/2007/9/15/69> > I was looking at this from a slightly different angle, which is that the > stack overflow warning is largely pointless - no matter how much you > lighten up the dump_stack path, it will add something to the stack depth > of the current process, effectively *reducing* the available stack for > all processes, and increasing the risk that you''ll actually overflow. > (if you take an interrupt towards the end of the stack, the warning will > go off and use the last bit - so you can''t count on that stack space to > be available).While it is true, A. If adding ~40 bytes to the kernel''s stack usage is critical, we''re already passed the all-doom-and-gloom-point. B. We can always calculate the available stack size, and if stack_remain is bigger then say, 80 bytes, call dump_stack.> > And, if you overflow the stack, you''ll almost certainly get an oops and > a backtrace anyway - usually thread_info gets overwritten and you BUG > because it looks like you sleep in an interrupt, or somesuch.Yeah, but at least to me, as a developer, having a warning before all-hell-breaks-lose, is a good thing (tm). Though, one can always argue that people who play around with kernel development can build their own kernel with STACKOVERFLOW enabled.> So, > what''s the point of the IRQ stack-depth check, again? Especially with > 4k stacks and separate IRQ stacks? And the more deterministic > max-stack-depth excursion checker (CONFIG_DEBUG_STACK_USAGE) as well... > > Finally, the patch I sent upstream would clearly show on an oops whether > or not the stack was currently overflowing, or whether the stack had > ever overflowed prior to the oops. Seemed useful to me.Just to satisfy my curiosity, can you post a link to the patch? - Gilboa
Gilboa Davara wrote:>> So, >> what''s the point of the IRQ stack-depth check, again? Especially with >> 4k stacks and separate IRQ stacks? And the more deterministic >> max-stack-depth excursion checker (CONFIG_DEBUG_STACK_USAGE) as well...>> Finally, the patch I sent upstream would clearly show on an oops whether >> or not the stack was currently overflowing, or whether the stack had >> ever overflowed prior to the oops. Seemed useful to me. > > Just to satisfy my curiosity, can you post a link to the patch?Sure: http://lkml.org/lkml/2007/8/31/28 -Eric
Gilboa Davara wrote:>> I was looking at this from a slightly different angle, which is that the >> stack overflow warning is largely pointless - no matter how much you >> lighten up the dump_stack path, it will add something to the stack depth >> of the current process, effectively *reducing* the available stack for >> all processes, and increasing the risk that you''ll actually overflow. >> (if you take an interrupt towards the end of the stack, the warning will >> go off and use the last bit - so you can''t count on that stack space to >> be available). > > While it is true, > A. If adding ~40 bytes to the kernel''s stack usage is critical, we''re > already passed the all-doom-and-gloom-point.Though, just to play devil''s advocate, say your absolute worst case stack depth goes to within 35 bytes of the end. The warning (even if trimmed down to 40 bytes) now renders your system unstable in the long run. Why waste it?> B. We can always calculate the available stack size, and if stack_remain > is bigger then say, 80 bytes, call dump_stack.That seems reasonable. Today, the dump_stack depth ~= the warning threshold, so it''s just broken and pointless as it stands.>> And, if you overflow the stack, you''ll almost certainly get an oops and >> a backtrace anyway - usually thread_info gets overwritten and you BUG >> because it looks like you sleep in an interrupt, or somesuch. > > Yeah, but at least to me, as a developer, having a warning before > all-hell-breaks-lose, is a good thing (tm).Perhaps. The current warning is fairly random, anyway, since it only fires on an IRQ. If you randomly get the warning at your max stack excursion, but never go so far as to actually blow the stack, then you really didn''t need the warning, did you? If you get the warning at maybe 85% of your stack excursion, then your thread continues post-IRQ and blows the stack, you get a nice backtrace anyway and the warning didn''t help much. I''m still not convinced that it''s that useful. I like the CONFIG_DEBUG_STACKUSAGE which accurately tells you what your max stack excursions have been. I just wish it could tell you what the callchain *was* (not really possible, as it''s written) - and making it resettable would be nice too (easy). -Eric
On Sat, 2007-09-15 at 17:45 -0500, Eric Sandeen wrote:> Gilboa Davara wrote: > > >> I was looking at this from a slightly different angle, which is that the > >> stack overflow warning is largely pointless - no matter how much you > >> lighten up the dump_stack path, it will add something to the stack depth > >> of the current process, effectively *reducing* the available stack for > >> all processes, and increasing the risk that you''ll actually overflow. > >> (if you take an interrupt towards the end of the stack, the warning will > >> go off and use the last bit - so you can''t count on that stack space to > >> be available). > > > > While it is true, > > A. If adding ~40 bytes to the kernel''s stack usage is critical, we''re > > already passed the all-doom-and-gloom-point. > > Though, just to play devil''s advocate, say your absolute worst case > stack depth goes to within 35 bytes of the end. The warning (even if > trimmed down to 40 bytes) now renders your system unstable in the long > run. Why waste it?True. ... Though being the devil''s advocate, if you don''t have enough stack space to dump_strace, do_IRQ should -really- call BUG() and halt everything before things turn really ugly.> > > B. We can always calculate the available stack size, and if stack_remain > > is bigger then say, 80 bytes, call dump_stack. > > That seems reasonable. Today, the dump_stack depth ~= the warning > threshold, so it''s just broken and pointless as it stands.Hopefully my patch will get admitted, reducing the stack usage to ~<80 bytes. (I need to recalculate the callstack usage) Speaking of which, who''s the maintainer of kernel/*syms*? The (second) patch [1] seemed to have passed the LKML without any (negative) comments, so I''ll try my luck with the actual code maintainer. (I couldn''t find anyone in MAINTAINERS)> > > Yeah, but at least to me, as a developer, having a warning before > > all-hell-breaks-lose, is a good thing (tm). > > Perhaps. The current warning is fairly random, anyway, since it only > fires on an IRQ. If you randomly get the warning at your max stack > excursion, but never go so far as to actually blow the stack, then you > really didn''t need the warning, did you? If you get the warning at > maybe 85% of your stack excursion, then your thread continues post-IRQ > and blows the stack, you get a nice backtrace anyway and the warning > didn''t help much. I''m still not convinced that it''s that useful.I''m usually afraid of the randomness of things. A minor stack over run can only show it''s nasty head -long- -long- after the actual trash took place making it next to impossible to pin-point the original source of the problem.> > I like the CONFIG_DEBUG_STACKUSAGE which accurately tells you what your > max stack excursions have been. I just wish it could tell you what the > callchain *was* (not really possible, as it''s written) - and making it > resettable would be nice too (easy)./me adds a note to self to lookup the CONFIG_DEBUG_STACKUSAGE code so I''ll know what you''re talking about ;)> > -Eric- Gilboa [1] http://lkml.org/lkml/2007/9/15/159
Gilboa Davara wrote:>> I like the CONFIG_DEBUG_STACKUSAGE which accurately tells you what your >> max stack excursions have been. I just wish it could tell you what the >> callchain *was* (not really possible, as it''s written) - and making it >> resettable would be nice too (easy). > > /me adds a note to self to lookup the CONFIG_DEBUG_STACKUSAGE code so > I''ll know what you''re talking about ;)Hm it''d be easier to find if I spelled it right: CONFIG_DEBUG_STACK_USAGE :) It zeros the whole stack when it''s allocated. When a thread exits, it finds the first non-zero value on the stack, and from that calculates the max stack excursion. See check_stack_usage() called from do_exit(). -Eric