| Message ID | 4a613b26cc402fb3ed8130cd07a37e76f4c133ef.1342634029.git.tony.luck@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
* Tony Luck <tony.luck@intel.com> wrote: > The stack_not_used() function in <linux/sched.h> assumes that stacks > grow downwards. This is not true on IA64 or PARISC, so this function > would walk off in the wrong direction and into the weeds. > > Found on IA64 because of a compilation failure with recursive dependencies > on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE. > > Fixing the code is possible, but should be combined with other > infrastructure additions to set up the "canary" at the end of the stack. > > Reported-by: Fengguang Wu <fengguang.wu@intel.com> (failed allmodconfig build) > Signed-off-by: Tony Luck <tony.luck@intel.com> > --- > lib/Kconfig.debug | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > index ff5bdee..4a18650 100644 > --- a/lib/Kconfig.debug > +++ b/lib/Kconfig.debug > @@ -714,7 +714,7 @@ config STACKTRACE > > config DEBUG_STACK_USAGE > bool "Stack utilization instrumentation" > - depends on DEBUG_KERNEL > + depends on DEBUG_KERNEL && !IA64 && !PARISC The modern way of doing this is by adding an ARCH_SUPPORTS_ flag. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-07-25 at 09:45 +0200, Ingo Molnar wrote: > * Tony Luck <tony.luck@intel.com> wrote: > > > The stack_not_used() function in <linux/sched.h> assumes that stacks > > grow downwards. This is not true on IA64 or PARISC, so this function > > would walk off in the wrong direction and into the weeds. > > > > Found on IA64 because of a compilation failure with recursive dependencies > > on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE. > > > > Fixing the code is possible, but should be combined with other > > infrastructure additions to set up the "canary" at the end of the stack. > > > > Reported-by: Fengguang Wu <fengguang.wu@intel.com> (failed allmodconfig build) > > Signed-off-by: Tony Luck <tony.luck@intel.com> > > --- > > lib/Kconfig.debug | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > index ff5bdee..4a18650 100644 > > --- a/lib/Kconfig.debug > > +++ b/lib/Kconfig.debug > > @@ -714,7 +714,7 @@ config STACKTRACE > > > > config DEBUG_STACK_USAGE > > bool "Stack utilization instrumentation" > > - depends on DEBUG_KERNEL > > + depends on DEBUG_KERNEL && !IA64 && !PARISC > > The modern way of doing this is by adding an ARCH_SUPPORTS_ > flag. That's a bit daft, isn't it? We'd have to add ARCH_SUPPORTS_ flags to about 25 separate architectures just to get it not supported on these two. Since the problem is an invalid assumption about how the stack grows, why not just condition it on that. We actually have a config option for this: CONFIG_STACK_GROWSUP. But for some reason ia64 doesn't define this, why not, Tony? It looks deliberate because you have replaced a lot of #ifdef CONFIG_STACK_GROWSUP with #if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64) but not all of them. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PiBTaW5jZSB0aGUgcHJvYmxlbSBpcyBhbiBpbnZhbGlkIGFzc3VtcHRpb24gYWJvdXQgaG93IHRo ZSBzdGFjayBncm93cywNCj4gd2h5IG5vdCBqdXN0IGNvbmRpdGlvbiBpdCBvbiB0aGF0LiAgV2Ug YWN0dWFsbHkgaGF2ZSBhIGNvbmZpZyBvcHRpb24gZm9yDQo+IHRoaXM6IENPTkZJR19TVEFDS19H Uk9XU1VQLiAgQnV0IGZvciBzb21lIHJlYXNvbiBpYTY0IGRvZXNuJ3QgZGVmaW5lDQo+IHRoaXMs IHdoeSBub3QsIFRvbnk/ICBJdCBsb29rcyBkZWxpYmVyYXRlIGJlY2F1c2UgeW91IGhhdmUgcmVw bGFjZWQgYQ0KPiBsb3Qgb2YNCj4NCj4gI2lmZGVmIENPTkZJR19TVEFDS19HUk9XU1VQDQo+DQo+ IHdpdGgNCj4NCj4gI2lmIGRlZmluZWQoQ09ORklHX1NUQUNLX0dST1dTVVApIHx8IGRlZmluZWQo Q09ORklHX0lBNjQpDQo+DQo+IGJ1dCBub3QgYWxsIG9mIHRoZW0uDQoNCmlhNjQgaXMgc3BlY2lh bCAtIHdlIGhhdmUgc3RhY2tzIHRoYXQgZ3JvdyBib3RoIHVwd2FyZHMgYW5kIGRvd253YXJkcyEN Cg0KVGhlIHR5cGljYWwgIkMiIHN0YWNrIGZvciBsb2NhbCBmdW5jdGlvbiB2YXJpYWJsZXMgdGhh dCBuZWVkIHRvIGJlIGFsbG9jYXRlZCBpbg0KbWVtb3J5IChhcnJheXMsIHN0cnVjdHVyZXMsIHRo aW5ncyB3ZSB0YWtlIHRoZSBhZGRyZXNzIG9mLCB0aGluZ3MgdGhhdCBqdXN0DQpkb24ndCBmaXQg YmVjYXVzZSB3ZSBydW4gb3V0IG9mIHJlZ2lzdGVycykgZ3Jvd3MgZG93bndhcmRzLiBCdXQgbG9j YWwNCnZhcmlhYmxlcyBhc3NpZ25lZCB0byByZWdpc3RlcnMgZ2V0IHF1aWV0bHkgc2F2ZWQgYXdh eSB0byBhbiB1cHdhcmRzIGdyb3dpbmcNCnN0YWNrIGJ5IHRoZSByZWdpc3RlciBzdGFjayBlbmdp bmUgKHdvcmtpbmcgc29tZXdoYXQgYXN5bmNocm9ub3VzbHkgZnJvbQ0KdGhlIGNwdSkubA0KDQot VG9ueQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
* James Bottomley <James.Bottomley@HansenPartnership.com> wrote: > On Wed, 2012-07-25 at 09:45 +0200, Ingo Molnar wrote: > > * Tony Luck <tony.luck@intel.com> wrote: > > > > > The stack_not_used() function in <linux/sched.h> assumes that stacks > > > grow downwards. This is not true on IA64 or PARISC, so this function > > > would walk off in the wrong direction and into the weeds. > > > > > > Found on IA64 because of a compilation failure with recursive dependencies > > > on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE. > > > > > > Fixing the code is possible, but should be combined with other > > > infrastructure additions to set up the "canary" at the end of the stack. > > > > > > Reported-by: Fengguang Wu <fengguang.wu@intel.com> (failed allmodconfig build) > > > Signed-off-by: Tony Luck <tony.luck@intel.com> > > > --- > > > lib/Kconfig.debug | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug > > > index ff5bdee..4a18650 100644 > > > --- a/lib/Kconfig.debug > > > +++ b/lib/Kconfig.debug > > > @@ -714,7 +714,7 @@ config STACKTRACE > > > > > > config DEBUG_STACK_USAGE > > > bool "Stack utilization instrumentation" > > > - depends on DEBUG_KERNEL > > > + depends on DEBUG_KERNEL && !IA64 && !PARISC > > > > The modern way of doing this is by adding an ARCH_SUPPORTS_ > > flag. > > That's a bit daft, isn't it? [...] It's generally more maintainable than a random list of architecture exclusions because every (old or new) architecture can just grep for ARCH_SUPPORTS_ pattern and see whether they support everything that others support. The above exclusion list of architectures is much harder to find in a structured way. > [...] We'd have to add ARCH_SUPPORTS_ flags to about 25 > separate architectures just to get it not supported on these > two. That is one off overhead and it makes things easier to maintain going forward. Anyway, that's the current upstream technique and it's been in place for years. > Since the problem is an invalid assumption about how the stack > grows, why not just condition it on that. We actually have a > config option for this: CONFIG_STACK_GROWSUP. But for some > reason ia64 doesn't define this, why not, Tony? It looks > deliberate because you have replaced a lot of > > #ifdef CONFIG_STACK_GROWSUP > > with > > #if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64) > > but not all of them. Yes, that's another possible solution, assuming that it's really only about the up/down difference. Thanks, Ingo -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "Ingo" == Ingo Molnar <mingo@kernel.org> writes: Ingo> * James Bottomley <James.Bottomley@HansenPartnership.com> wrote: >> Since the problem is an invalid assumption about how the stack >> grows, why not just condition it on that. We actually have a >> config option for this: CONFIG_STACK_GROWSUP. But for some reason >> ia64 doesn't define this, why not, Tony? It looks deliberate >> because you have replaced a lot of >> >> #ifdef CONFIG_STACK_GROWSUP >> >> with >> >> #if defined(CONFIG_STACK_GROWSUP) || defined(CONFIG_IA64) >> >> but not all of them. Ingo> Yes, that's another possible solution, assuming that it's really Ingo> only about the up/down difference. Ingo> Thanks, IA64 has two stacks -- the standard one, that grows down, and the register stack engine backing store, that grows up. The usual mechanisms for stack growth are used, so only some of the bits predicated on `STACK_GROWSUP' are useful. Peter C -- Dr Peter Chubb peter.chubb AT nicta.com.au http://www.ssrg.nicta.com.au Software Systems Research Group/NICTA -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2012-07-18 at 10:35 -0700, Tony Luck wrote: > The stack_not_used() function in <linux/sched.h> assumes that stacks > grow downwards. This is not true on IA64 or PARISC, so this function > would walk off in the wrong direction and into the weeds. OK, so looking at all of this, that statement's not quite true ... at least for parisc, we begin the stack where end_of_stack() says the end should be and so the walker will likely find the next word after the canary skip occupied and terminate there, so we think the stack is larger than it really is. It gets the wrong value, but it will never even walk out of the stack area. > Found on IA64 because of a compilation failure with recursive dependencies > on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE. > > Fixing the code is possible, but should be combined with other > infrastructure additions to set up the "canary" at the end of the stack. I agree with this. Most of it looks easily fixable, but how would I enable the fix for ia64? For PA it's simple: I'll just use CONFIG_STACK_GROWSUP, but that won't work for you. James -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PiBJIGFncmVlIHdpdGggdGhpcy4gIE1vc3Qgb2YgaXQgbG9va3MgZWFzaWx5IGZpeGFibGUsIGJ1 dCBob3cgd291bGQgSQ0KPiBlbmFibGUgdGhlIGZpeCBmb3IgaWE2ND8gIEZvciBQQSBpdCdzIHNp bXBsZTogSSdsbCBqdXN0IHVzZQ0KPiBDT05GSUdfU1RBQ0tfR1JPV1NVUCwgYnV0IHRoYXQgd29u J3Qgd29yayBmb3IgeW91Lg0KDQppYTY0IGhhcyBhbiB1Z2x5IGNoaWNrZW4gdnMuIGVnZyBidWls ZCBkZXBlbmRlbmN5LiBXaGVuIHRyeWluZyB0byBidWlsZCBvdXIgYXNtLW9mZnNldHMuaA0KZmls ZSAodG8gZ2V0ICNkZWZpbmUgY29uc3RhbnRzIGZvciB2YXJpb3VzIHN0cnVjdHVyZSBzaXplcyBh bmQgb2Zmc2V0cyBpbiBhIGZvcm1hdCB0aGF0IGlzDQp1c2FibGUgaW4gYXNzZW1ibHkgY29kZSkg d2UgZ2V0Og0KDQppbmNsdWRlL2xpbnV4L3NjaGVkLmg6MjUzOTogZXJyb3I6ICdJQTY0X1RBU0tf U0laRScgdW5kZWNsYXJlZCAoZmlyc3QgdXNlIGluIHRoaXMgZnVuY3Rpb24pDQoNCldoaWNoIGlz IHNhZCBiZWNhdXNlIElBNjRfVEFTS19TSVpFIGlzIG9uZSBvZiB0aGUgI2RlZmluZXMgdGhhdCBh c20tb2Zmc2V0cy5oIGlzIHRyeWluZw0KdG8gcHJvZHVjZS4NCg0KV2hpY2ggaXMgd2h5IEkganVz dCB0aHJldyB1cCBteSBoYW5kcyBpbiBkZXNwYWlyIGFuZCBzYWlkICIhSUE2NCIgZm9yIHRoaXMg b3B0aW9uLg0KDQotVG9ueQ0K -- To unsubscribe from this list: send the line "unsubscribe linux-parisc" 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/lib/Kconfig.debug b/lib/Kconfig.debug index ff5bdee..4a18650 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -714,7 +714,7 @@ config STACKTRACE config DEBUG_STACK_USAGE bool "Stack utilization instrumentation" - depends on DEBUG_KERNEL + depends on DEBUG_KERNEL && !IA64 && !PARISC help Enables the display of the minimum amount of free stack which each task has ever had available in the sysrq-T and sysrq-P debug output.
The stack_not_used() function in <linux/sched.h> assumes that stacks grow downwards. This is not true on IA64 or PARISC, so this function would walk off in the wrong direction and into the weeds. Found on IA64 because of a compilation failure with recursive dependencies on IA64_TASKSIZE and IA64_THREAD_INFO_SIZE. Fixing the code is possible, but should be combined with other infrastructure additions to set up the "canary" at the end of the stack. Reported-by: Fengguang Wu <fengguang.wu@intel.com> (failed allmodconfig build) Signed-off-by: Tony Luck <tony.luck@intel.com> --- lib/Kconfig.debug | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)