Hello list, Time for a noob discussion, When I was building the kernel, I found a warning from drivers/mfd/abx500-core.c, that the Frame size is larger than 1024 bytes. Apparently the stack frame size can be changed from the config, but my question is, whether 1024 bytes low ? I am on an x86_64 (core i3). abx500-core.c had an object of struct device being allocated on stack. So dynamically allocating it makes the warning go away. Are there any implications on using dynamic allocation on this particular code? I mean, could there be some reason why the original developer went with static option ? --- drivers/mfd/abx500-core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/mfd/abx500-core.c b/drivers/mfd/abx500-core.c index f3a15aa..709a84f 100644 --- a/drivers/mfd/abx500-core.c +++ b/drivers/mfd/abx500-core.c @@ -154,15 +154,17 @@ EXPORT_SYMBOL(abx500_startup_irq_enabled); void abx500_dump_all_banks(void) { struct abx500_ops *ops; - struct device dummy_child = {NULL}; + struct device *dummy_child; struct abx500_device_entry *dev_entry; + dummy_child = kzalloc(sizeof(struct device),GFP_KERNEL); + list_for_each_entry(dev_entry, &abx500_list, list) { - dummy_child.parent = dev_entry->dev; + dummy_child->parent = dev_entry->dev; ops = &dev_entry->ops; if ((ops != NULL) && (ops->dump_all_banks != NULL)) - ops->dump_all_banks(&dummy_child); + ops->dump_all_banks(dummy_child); } } EXPORT_SYMBOL(abx500_dump_all_banks); -- 1.9.1 -- Thanks and Regards, Jay Aurabind
On Wed, 07 May 2014 22:06:14 +0530, Jay Aurabind said:
When I was building the kernel, I found a warning from drivers/mfd/ abx500-core.c, that the Frame size is larger than 1024 bytes. Apparently the stack frame size can be changed from the config, but my question is, whether 1024 bytes low ? I am on an x86_64 (core i3).
Allocating 1K on the stack is indeed evil.
abx500-core.c had an object of struct device being allocated on stack. So dynamically allocating it makes the warning go away. Are there any implications on using dynamic allocation on this particular code?
He probably didn't realize or didn't know better. Having said that:
+ dummy_child = kzalloc(sizeof(struct device),GFP_KERNEL);
There's no kfree() for this. So you introduced a memory leak.
list_for_each_entry(dev_entry, &abx500_list, list) { - dummy_child.parent = dev_entry->dev; + dummy_child->parent = dev_entry->dev; ops = &dev_entry->ops;
if ((ops != NULL) && (ops->dump_all_banks != NULL)) - ops->dump_all_banks(&dummy_child); + ops->dump_all_banks(dummy_child); }
kfree(dummy_child); /* should go here... */
} EXPORT_SYMBOL(abx500_dump_all_banks);
The weird part is that the entries on abx500_list apparently don't have valid ->parent pointers already, so we have have to invent dummy ones. Anybody understand why that's the case? This smells like we're not fixing the actual problem here, just changing the way we paper it over to be a less ugly papering over....
Allocating 1K on the stack is indeed evil.
Why would you say evil ? I didnt quite get why you meant by that. Is it at some extremes ? 1K is from the default ubuntu 14.04 config.
abx500-core.c had an object of struct device being allocated on stack. So dynamically allocating it makes the warning go away. Are there any implications on using dynamic allocation on this particular code?
He probably didn't realize or didn't know better.
Having said that:
+ dummy_child = kzalloc(sizeof(struct device),GFP_KERNEL);
There's no kfree() for this. So you introduced a memory leak.
My bad! "I successfully operated the patient, but forgot the scissors inside", :D Thanks for pointing out! :)
list_for_each_entry(dev_entry, &abx500_list, list) { - dummy_child.parent = dev_entry->dev; + dummy_child->parent = dev_entry->dev; ops = &dev_entry->ops;
if ((ops != NULL) && (ops->dump_all_banks != NULL)) - ops->dump_all_banks(&dummy_child); + ops->dump_all_banks(dummy_child); }
kfree(dummy_child); /* should go here... */
} EXPORT_SYMBOL(abx500_dump_all_banks);
The weird part is that the entries on abx500_list apparently don't have valid ->parent pointers already, so we have have to invent dummy ones.
Anybody understand why that's the case? This smells like we're not fixing the actual problem here, just changing the way we paper it over to be a less ugly papering over....
Waiting to hear from the experts!
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
On Thu, 08 May 2014 09:24:38 +0530, Jay Aurabind said:
Allocating 1K on the stack is indeed evil.
Why would you say evil ? I didnt quite get why you meant by that. Is it at some extremes ? 1K is from the default ubuntu 14.04 config.
Some paths in the kernel get very deep stacks (for instance, nfs reading an xfs file that's on an lvm partition on a dm-crypt target is famous for blowing the stack without any further help). On my laptop, the low-water mark is already pretty low: % dmesg | grep 'stack depth' [ 2.094319] cryptomgr_test (42) used greatest stack depth: 6456 bytes left [ 2.952135] modprobe (91) used greatest stack depth: 5904 bytes left [ 2.955593] modprobe (93) used greatest stack depth: 5848 bytes left [ 2.955953] cryptomgr_probe (94) used greatest stack depth: 5520 bytes left [ 4.145573] systemd-cgroups (135) used greatest stack depth: 5440 bytes left [ 4.859260] udevadm (255) used greatest stack depth: 4920 bytes left [ 4.966646] ata_id (268) used greatest stack depth: 4088 bytes left [ 145.528777] dhclient (1306) used greatest stack depth: 4072 bytes left [ 226.188576] ip (2232) used greatest stack depth: 3344 bytes left [ 300.303981] ip (2468) used greatest stack depth: 3152 bytes left So in the first 5 minutes, I already was down to 3K of stack. All it takes is that code interacting with another code that allocates 1K popping when I was at that 3K low water mark, and my kernel is in deep juju. The fact the 1K is in the default Ubuntu config just means that the kernel as distributed has bad code in it. :)
On Thu, May 08, 2014 at 11:46:43AM -0400, Valdis.Kletnieks@vt.edu wrote:
On Thu, 08 May 2014 09:24:38 +0530, Jay Aurabind said:
Allocating 1K on the stack is indeed evil.
Why would you say evil ? I didnt quite get why you meant by that. Is it at some extremes ? 1K is from the default ubuntu 14.04 config.
Some paths in the kernel get very deep stacks (for instance, nfs reading an xfs file that's on an lvm partition on a dm-crypt target is famous for blowing the stack without any further help). On my laptop, the low-water mark is already pretty low:
% dmesg | grep 'stack depth' [ 2.094319] cryptomgr_test (42) used greatest stack depth: 6456 bytes left [ 2.952135] modprobe (91) used greatest stack depth: 5904 bytes left [ 2.955593] modprobe (93) used greatest stack depth: 5848 bytes left [ 2.955953] cryptomgr_probe (94) used greatest stack depth: 5520 bytes left [ 4.145573] systemd-cgroups (135) used greatest stack depth: 5440 bytes left [ 4.859260] udevadm (255) used greatest stack depth: 4920 bytes left [ 4.966646] ata_id (268) used greatest stack depth: 4088 bytes left [ 145.528777] dhclient (1306) used greatest stack depth: 4072 bytes left [ 226.188576] ip (2232) used greatest stack depth: 3344 bytes left [ 300.303981] ip (2468) used greatest stack depth: 3152 bytes left
So in the first 5 minutes, I already was down to 3K of stack. All it takes is that code interacting with another code that allocates 1K popping when I was at that 3K low water mark, and my kernel is in deep juju.
The fact the 1K is in the default Ubuntu config just means that the kernel as distributed has bad code in it. :)
Thank you Valdis for the explaination. My stack depth is also similar to yours: [ 1.567353] cryptomgr_test (52) used greatest stack depth: 6656 bytes left [ 2.871152] modprobe (85) used greatest stack depth: 5624 bytes left [ 2.894478] modprobe (93) used greatest stack depth: 5552 bytes left [ 4.212907] all_generic_ide (127) used greatest stack depth: 5448 bytes left [ 4.440752] udevadm (134) used greatest stack depth: 4488 bytes left [ 4.571276] ata_id (165) used greatest stack depth: 4448 bytes left [ 7.661284] btrfs (183) used greatest stack depth: 4424 bytes left [ 7.859625] systemd-udevd (133) used greatest stack depth: 4392 bytes left [ 8.898118] hostname (208) used greatest stack depth: 4208 bytes left [ 9.341028] sh (209) used greatest stack depth: 4088 bytes left [ 48.353719] systemd-udevd (506) used greatest stack depth: 3768 bytes left [ 304.162368] kworker/u8:0 (6) used greatest stack depth: 3656 bytes left Apparently no other activity is happening which is decreasing the depth further, even after running some applications like firefox. So shouldnt we assume that initial value (somewhere around 6K) should be enough since the maximum it went down is only till its 50% mark ?
On Fri, 09 May 2014 17:39:21 +0530, Jay Aurabind said:
So shouldnt we assume that initial value (somewhere around 6K) should be enough since the maximum it went down is only till its 50% mark ?
Depends. Are you of the "we haven't seen it before, so it can't happen" school of programming, or the "it could plausibly happen, so we should guard against it" school? Consider you get down to that 6K mark - and now you hit that 1K allocation that you didn't bother cleaning up because "we've never seen it before". And then you hit a hardIRQ that *also* didn't bother cleaning up their 1K allocation that *that* kernel hacker "had never seen it before". What happens to your system? And how long is it going to take for you to figure out why every several weeks, your system dies with a totally different random memory overlay?
On Friday 09 May 2014 09:00 PM, Valdis.Kletnieks@vt.edu wrote:
On Fri, 09 May 2014 17:39:21 +0530, Jay Aurabind said:
So shouldnt we assume that initial value (somewhere around 6K) should be enough since the maximum it went down is only till its 50% mark ?
Depends. Are you of the "we haven't seen it before, so it can't happen" school of programming, or the "it could plausibly happen, so we should guard against it" school?
Consider you get down to that 6K mark - and now you hit that 1K allocation that you didn't bother cleaning up because "we've never seen it before". And then you hit a hardIRQ that *also* didn't bother cleaning up their 1K allocation that *that* kernel hacker "had never seen it before". What happens to your system? And how long is it going to take for you to figure out why every several weeks, your system dies with a totally different random memory overlay?
Point taken. Thank you Valdis, Martin, Paul and Frank for sharing your thoughts. Cheers, Jay
Valdis.Kletnieks@vt.edu writes:
[...] Some paths in the kernel get very deep stacks (for instance, nfs reading an xfs file that's on an lvm partition on a dm-crypt target is famous for blowing the stack without any further help). On my laptop, the low-water mark is already pretty low:
% dmesg | grep 'stack depth' [ 2.094319] cryptomgr_test (42) used greatest stack depth: 6456 bytes left [...] [ 145.528777] dhclient (1306) used greatest stack depth: 4072 bytes left [ 226.188576] ip (2232) used greatest stack depth: 3344 bytes left [ 300.303981] ip (2468) used greatest stack depth: 3152 bytes left
So in the first 5 minutes, I already was down to 3K of stack. All it takes is that code interacting with another code that allocates 1K popping when I was at that 3K low water mark, and my kernel is in deep juju.
Note that the kernel stack is not a monotonically exhausted resource. Each task has its own. It's freed up gradually as internal functions return, and fully as control returns to the user thread. The reported numbers decrease monotonically only to show "worst-case-until-now". It's not "only this much space remains, reboot soon" kind of thing. - FChE
Am 2014-05-07 18:36, schrieb Jay Aurabind:
Hello list, Time for a noob discussion,
When I was building the kernel, I found a warning from drivers/mfd/abx500-core.c, that the Frame size is larger than 1024 bytes. Apparently the stack frame size can be changed from the config, but my question is, whether 1024 bytes low ? I am on an x86_64 (core i3).
abx500-core.c had an object of struct device being allocated on stack. So dynamically allocating it makes the warning go away. Are there any implications on using dynamic allocation on this particular code? I mean, could there be some reason why the original developer went with static option ?
kmalloc may sleep and is therefore sometimes not recommended to be used.
--- drivers/mfd/abx500-core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/abx500-core.c b/drivers/mfd/abx500-core.c index f3a15aa..709a84f 100644 --- a/drivers/mfd/abx500-core.c +++ b/drivers/mfd/abx500-core.c @@ -154,15 +154,17 @@ EXPORT_SYMBOL(abx500_startup_irq_enabled); void abx500_dump_all_banks(void) { struct abx500_ops *ops; - struct device dummy_child = {NULL}; + struct device *dummy_child; struct abx500_device_entry *dev_entry;
+ dummy_child = kzalloc(sizeof(struct device),GFP_KERNEL); + list_for_each_entry(dev_entry, &abx500_list, list) { - dummy_child.parent = dev_entry->dev; + dummy_child->parent = dev_entry->dev; ops = &dev_entry->ops;
if ((ops != NULL) && (ops->dump_all_banks != NULL)) - ops->dump_all_banks(&dummy_child); + ops->dump_all_banks(dummy_child); } } EXPORT_SYMBOL(abx500_dump_all_banks);
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
On Thu, May 08, 2014 at 02:38:24PM +0200, Martin Kepplinger wrote:
Am 2014-05-07 18:36, schrieb Jay Aurabind:
Hello list, Time for a noob discussion,
When I was building the kernel, I found a warning from drivers/mfd/abx500-core.c, that the Frame size is larger than 1024 bytes. Apparently the stack frame size can be changed from the config, but my question is, whether 1024 bytes low ? I am on an x86_64 (core i3).
abx500-core.c had an object of struct device being allocated on stack. So dynamically allocating it makes the warning go away. Are there any implications on using dynamic allocation on this particular code? I mean, could there be some reason why the original developer went with static option ?
kmalloc may sleep and is therefore sometimes not recommended to be used.
Sure, but this code doesnt appear to be holding any locks. So it should be okay to call kmalloc here I guess.
--- drivers/mfd/abx500-core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/abx500-core.c b/drivers/mfd/abx500-core.c index f3a15aa..709a84f 100644 --- a/drivers/mfd/abx500-core.c +++ b/drivers/mfd/abx500-core.c @@ -154,15 +154,17 @@ EXPORT_SYMBOL(abx500_startup_irq_enabled); void abx500_dump_all_banks(void) { struct abx500_ops *ops; - struct device dummy_child = {NULL}; + struct device *dummy_child; struct abx500_device_entry *dev_entry;
+ dummy_child = kzalloc(sizeof(struct device),GFP_KERNEL); + list_for_each_entry(dev_entry, &abx500_list, list) { - dummy_child.parent = dev_entry->dev; + dummy_child->parent = dev_entry->dev; ops = &dev_entry->ops;
if ((ops != NULL) && (ops->dump_all_banks != NULL)) - ops->dump_all_banks(&dummy_child); + ops->dump_all_banks(dummy_child); } } EXPORT_SYMBOL(abx500_dump_all_banks);
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
On Friday 09 May 2014 05:43 PM, Jay Aurabind wrote:
On Thu, May 08, 2014 at 02:38:24PM +0200, Martin Kepplinger wrote:
Am 2014-05-07 18:36, schrieb Jay Aurabind:
Hello list, Time for a noob discussion,
When I was building the kernel, I found a warning from drivers/mfd/abx500-core.c, that the Frame size is larger than 1024 bytes. Apparently the stack frame size can be changed from the config, but my question is, whether 1024 bytes low ? I am on an x86_64 (core i3).
abx500-core.c had an object of struct device being allocated on stack. So dynamically allocating it makes the warning go away. Are there any implications on using dynamic allocation on this particular code? I mean, could there be some reason why the original developer went with static option ? kmalloc may sleep and is therefore sometimes not recommended to be used.
Sure, but this code doesnt appear to be holding any locks. So it should be okay to call kmalloc here I guess. You can use kmalloc even if you are holding a lock. Care must be taken to pass GFP_ATOMIC flag to the kmalloc, so that it does not sleep.
--- drivers/mfd/abx500-core.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/abx500-core.c b/drivers/mfd/abx500-core.c index f3a15aa..709a84f 100644 --- a/drivers/mfd/abx500-core.c +++ b/drivers/mfd/abx500-core.c @@ -154,15 +154,17 @@ EXPORT_SYMBOL(abx500_startup_irq_enabled); void abx500_dump_all_banks(void) { struct abx500_ops *ops; - struct device dummy_child = {NULL}; + struct device *dummy_child; struct abx500_device_entry *dev_entry;
+ dummy_child = kzalloc(sizeof(struct device),GFP_KERNEL); + list_for_each_entry(dev_entry, &abx500_list, list) { - dummy_child.parent = dev_entry->dev; + dummy_child->parent = dev_entry->dev; ops = &dev_entry->ops;
if ((ops != NULL) && (ops->dump_all_banks != NULL)) - ops->dump_all_banks(&dummy_child); + ops->dump_all_banks(dummy_child); } } EXPORT_SYMBOL(abx500_dump_all_banks);
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
-- Paul Davies C, CSE B.Tech. student, Govt. Engineering College, Thrissur.
participants (5)
-
fche＠redhat.com -
Jay Aurabind -
Martin Kepplinger -
Paul Davies C -
Valdis.Kletnieks＠vt.edu