large frame size warning when compiling

large frame size warning when compiling

[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 ?


---
 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 213 bytes
Desc: Digital signature
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140507/a7231430/attachment.bin 

large frame size warning when compiling

[Valdis.Kletnieks at vt.edu]

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....

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140507/de99ca63/attachment.bin 

large frame size warning when compiling

[Jay Aurabind]

> 
> 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 at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 278 bytes
Desc: OpenPGP digital signature
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140508/53b0070f/attachment.bin 

large frame size warning when compiling

[Valdis.Kletnieks at vt.edu]

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. :)

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140508/bea0e8aa/attachment.bin 

large frame size warning when compiling

[Jay Aurabind]

On Thu, May 08, 2014 at 11:46:43AM -0400, Valdis.Kletnieks at 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 ?

large frame size warning when compiling

[Valdis.Kletnieks at vt.edu]

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?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140509/e2d5874a/attachment.bin 

large frame size warning when compiling

[Jay Aurabind]

On Friday 09 May 2014 09:00 PM, Valdis.Kletnieks at 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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 278 bytes
Desc: OpenPGP digital signature
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20140510/a8dc2df7/attachment.bin 

large frame size warning when compiling

[Frank Ch. Eigler]

Valdis.Kletnieks at 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

large frame size warning when compiling

[Martin Kepplinger]

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 at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
> 

large frame size warning when compiling

[Jay Aurabind]

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 at kernelnewbies.org
> > http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
> > 
> 

large frame size warning when compiling

[Paul Davies C]

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 at kernelnewbies.org
>>> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
>>>
> _______________________________________________
> Kernelnewbies mailing list
> Kernelnewbies at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies


-- 
Paul Davies C,
CSE B.Tech. student,
Govt. Engineering College, Thrissur.