env: reduce the stack footprint for the env buf

Original env buf directly locate at stack lead large stack footprint when call those env functions. It is not good when the system memory is critical or only want the uboot run at restrict range, that is not to touch the memory of other place at its best.

So now this patch move the env buf to the heap area, which reduce the area uboot need to touch.

Signed-off-by Lei Wen <[email protected]>

---
 common/env_dataflash.c |    8 +++++++-
 common/env_eeprom.c    |    8 +++++++-
 common/env_mgdisk.c    |   10 +++++++++-
 common/env_mmc.c       |    9 ++++++++-
 common/env_nand.c      |   10 +++++++++-
 common/env_nvram.c     |    9 ++++++++-
 common/env_sf.c        |    9 ++++++++-
 7 files changed, 56 insertions(+), 7 deletions(-)

Original env buf directly locate at stack lead large stack footprint when call those env functions. It is not good when the system memory is critical or only want the uboot run at restrict range, that is not to touch the memory of other place at its best.

So now this patch move the env buf to the heap area, which reduce the area uboot need to touch.

Dear Lei Wen,

Quoted 5 lines · In message <[email protected]> you wrote:

Original env buf directly locate at stack lead large stack footprint when call those env functions. It is not good when the system memory is critical or only want the uboot run at restrict range, that is not to touch the memory of other place at its best.

Quoted 2 lines

So now this patch move the env buf to the heap area, which reduce the area uboot need to touch.

In which way do you think this will save any memory?

Your switch from automatic variables to manually allocated ones has, in my opinion several disadvantages:

1) It increases the code size.

2) It increases the probability of bugs like memory leaks etc.

3) Instead of using the stack, which gets allocated fully dynamically ? and gets freed guaranteed completely when the function returns, ? you now need at least the same space in the malloc arena - where ? you have to allocate it statically, and where allocations of ? buffers like that can easily lead to memory fragmentation, ? increasing the memory footprint further.

Did you do any specific measurements of memory footprint, or what makes you believe your code is preferrable?

Quoted 2 lines

In which way do you think this will save any memory?

This patch is not intend to save memory...

One of our project need to confine the ddr usage of uboot in the smallest case, not to pollute other area. So for us, the small stack is good one...

For now the uboot is relocated to the end of the dram, and malloc area is almost a fix value, uboot would live happily in this area. But for env case, it allocate a range which could be large, due to the CONFIG_ENV_SIZE could be a big one, in the stack range. Because the stack is grown downwards, so it takes more memory range than it is allocated in the malloc method.

Dear Lei Wen,

Quoted 4 lines · In message <[email protected]> you…

Quoted 2 lines

In which way do you think this will save any memory?

This patch is not intend to save memory...

Then I don't understand at all what the benefit of that patch would be.

Quoted 2 lines

One of our project need to confine the ddr usage of uboot in the smallest case, not to pollute other area. So for us, the small stack is good one...

So it is still about saving memory...

Quoted 5 lines

For now the uboot is relocated to the end of the dram, and malloc area is almost a fix value, uboot would live happily in this area. But for env case, it allocate a range which could be large, due to the CONFIG_ENV_SIZE could be a big one, in the stack range. Because the stack is grown downwards, so it takes more memory range than it is allocated in the malloc method.

malloc arena and stack are adjacent. If you have enough free room in the malloc arena for the environment buffers, but cannot afford to have them on your stack, then this means your malloc arena has lots of unused space. Decrease the size of your malloc arena by the size of the environment buffer, and you reach the same goal as with your patch - actually you save more memory, as the code size of u-boot shrinks.

Can you please provide exact numbers? How big is your RAM? What is the debug output of arch/*/lib/board.c without and with this patch on your system? How big is your environment resp. the environment sectors?

Sounds like you have allocated too small a region for your stack - try increasing CONFIG_SYS_STACK_SIZE and decreasing CONFIG_SYS_MALLOC_LEN

The only other thing I could think of that would make sense would be having the malloc heap in a completely different memory segment to the stack - odd, but possible

Dear Lei Wen,

Quoted 4 lines · In message <[email protected]> you…

Quoted 2 lines

In which way do you think this will save any memory?

This patch is not intend to save memory...

Then I don't understand at all what the benefit of that patch would be.

Quoted 2 lines

One of our project need to confine the ddr usage of uboot in the smallest case, not to pollute other area. So for us, the small stack is good one...

So it is still about saving memory...

Quoted 6 lines

For now the uboot is relocated to the end of the dram, and malloc area is almost a fix value, uboot would live happily in this area. But for env case, it allocate a range which could be large, due to the CONFIG_ENV_SIZE could be a big one, in the stack range. Because the stack is grown downwards, so it takes more memory range than it is allocated in the malloc method.

malloc arena and stack are adjacent. ?If you have enough free room in the malloc arena for the environment buffers, but cannot afford to have them on your stack, then this means your malloc arena has lots of unused space. ?Decrease the size of your malloc arena by the size of the environment buffer, and you reach the same goal as with your patch - actually you save more memory, as the code size of u-boot shrinks.

Can you please provide exact numbers? ?How big is your RAM? What is the debug output of arch/*/lib/board.c without and with this patch on your system? ?How big is your environment resp. the environment sectors?

Quoted 8 lines

malloc arena and stack are adjacent. ??If you have enough free room in the malloc arena for the environment buffers, but cannot afford to have them on your stack, then this means your malloc arena has lots of unused space. Decrease the size of your malloc arena by the size of the environment buffer, and you reach the same goal as with your patch - actually you save more memory, as the code size of u-boot shrinks.

Em, what you said is also right for my case.
But I think making the malloc heap area large while limit the stack usage could benefit those function use malloc, while keep user in mind how many memory he is using on that board. For now, uboot just print uboot self-reserved, malloc size, board info, global data size for the uesr. For the stack usage, user is not quite aware of.

Just like the env case, if that ENV_SIZE goes too large, user may not notice the memory usage just from the debug info.

My ram is big, 512MB. But for my case, I need uboot use the minimum memory as it can. The debug output is same, for this patch don't change the

Dear Lei Wen,

Quoted 16 lines · In message <[email protected]> you…

Quoted 8 lines

malloc arena and stack are adjacent. ?If you have enough free room in the malloc arena for the environment buffers, but cannot afford to have them on your stack, then this means your malloc arena has lots of unused space. ?Decrease the size of your malloc arena by the size of the environment buffer, and you reach the same goal as with your patch - actually you save more memory, as the code size of u-boot shrinks.

Em, what you said is also right for my case.
But I think making the malloc heap area large while limit the stack usage could benefit those function use malloc, while keep user in mind how many memory he is using on that board. For now, uboot just print uboot self-reserved, malloc size, board info, global data size for the uesr. For the stack usage, user is not quite aware of.

Quoted 2 lines

Just like the env case, if that ENV_SIZE goes too large, user may not notice the memory usage just from the debug info.

We should pay attention to use the correct terms here. The "user", i. e. the end user running the U-Boot image on some system, will most probably not bother at all wether you use space on the stack or in the malloc arena, or how much. ?The only thing that concerns him is that the thing "just works".

It is our, the developer's, task, to make sure his expectations are met. ?And from the robustness and reliability, but also from the memory footprint point of view, may favours go clearly with automatic variables on the stack over manual allocation.

I mentioned this before: the stack can temporarily use arbitrary amounts of storage, whil still allowing you to use a maximum of free RAM at other times.

I don't really care how much memory is needed on the stack for environment buffers when importing or exporting the environment from/to external storage, because this memory is freed again and usable for other purposes when I need it, for example to load some big OS and/or file system images. ?With malloc(), such memory is permantly reseverd, and thus permantly lost.

In my opinion this is a clear disadvantage of the malloc() based approach.

Quoted 2 lines

My ram is big, 512MB. But for my case, I need uboot use the minimum memory as it can. The debug output is same, for this patch don't change the

Can you please explain your requirements in a bit more detail, and especially, how your patch is supposed to help? ?I would expect that the time when the environment buffers need space on the stack is NOT the time when you need so much of memory?

Hi Wolfgang,

Quoted 4 lines · Wolfgang Denk

Dear Lei Wen,

Quoted 1 line · In message <[email protected]> you…

My current implementation is to do a memory dump by using the uboot, while uboot's only task is to compress and write the memory to the sd card. For the compress and write part need a lot of heap, 350k+, if still keep the env buf in the stack, it would increase the total memory touched by uboot...

As dump usage need uboot touch the least memory, so this is the purpose I submit this patch... Reuse the heap area at its best, and don't increase stack much in the runtime...

Quoted 6 lines

My current implementation is to do a memory dump by using the uboot, while uboot's only task is to compress and write the memory to the sd card. For the compress and write part need a lot of heap, 350k+, if still keep the env buf in the stack, it would increase the total memory touched by uboot...

Quoted 3 lines

As dump usage need uboot touch the least memory, so this is the purpose I submit this patch... Reuse the heap area at its best, and don't increase stack much in the runtime...

Ah, so with the env buffer on the stack, the bottom of the stack is being pushed into the memory you want to dump and corrupting it - Makes sense to me now that you would resort to moving the env buffer to the heap.

There are probably a number of other areas in u-boot where large buffers are put on the stack while the malloc pool is relatively empty with the end result being that u-boot is using more memory than strictly required.

It would be an interesting profiling excercise

Quoted 11 lines

I mentioned this before: the stack can temporarily use arbitrary amounts of storage, whil still allowing you to use a maximum of free RAM at other times.

I don't really care how much memory is needed on the stack for environment buffers when importing or exporting the environment from/to external storage, because this memory is freed again and usable for other purposes when I need it, for example to load some big
OS and/or file system images. With malloc(), such memory is permantly
reseverd, and thus permantly lost.

I am a little confused here... Why the malloc would make the memory permantly reserved and lost... In this patch, I just temp malloc one buffer, then free it after I use it...

My current implementation is to do a memory dump by using the uboot, while uboot's only task is to compress and write the memory to the sd card. For the compress and write part need a lot of heap, 350k+, if still keep the env buf in the stack, it would increase the total memory touched by uboot...

As dump usage need uboot touch the least memory, so this is the purpose I submit this patch... Reuse the heap area at its best, and don't increase stack much in the runtime...

Dear Lei Wen,

Quoted 12 lines · In message <[email protected]> you…

Quoted 11 lines

I mentioned this before: the stack can temporarily use arbitrary amounts of storage, whil still allowing you to use a maximum of free RAM at other times.

I don't really care how much memory is needed on the stack for environment buffers when importing or exporting the environment from/to external storage, because this memory is freed again and usable for other purposes when I need it, for example to load some big OS and/or file system images. ? With malloc(), such memory is permantly reseverd, and thus permantly lost.

Quoted 3 lines

I am a little confused here... Why the malloc would make the memory permantly reserved and lost... In this patch, I just temp malloc one buffer, then free it after I use it...

Yes, you free it, so it becomes usable for other malloc() calls (eventually - in real life fragmentation might play a role, too), but you sneed to reserve the full emount of buffer space in the malloc arena, for the whole like time of U-Boot.

On the othern hand, when using buffers on the stack, then these buffers are only temporarily used, when the functions are actually being called. In this specific case (keep in mind that we are talking about env_relocate_spec() this happens exactlky once, very early in the system initialization, i. e. long before you are able to run any user commands.

Quoted 4 lines

My current implementation is to do a memory dump by using the uboot, while uboot's only task is to compress and write the memory to the sd card. For the compress and write part need a lot of heap, 350k+, if still keep the env buf in the stack, it would increase the total memory touched by uboot...

This statement makes no sense at all. ?The environment buffers are only used once, long before you are able to run any user commands. Wehn you can start doing your stuff, they are long gone, and all of it's memory has been reclaimed.

Quoted 3 lines

As dump usage need uboot touch the least memory, so this is the purpose I submit this patch... Reuse the heap area at its best, and don't increase stack much in the runtime...

But it statically increases the size of the malloc areane, so you are just shifting the memory from location A to B.

For the last time I repeat my question: can you please explain how your patch is supposed to reduce the memory footprint of your system, and document your acchievments with actual measurements? I would expect that the time when the environment buffers need space on the stack is NOT the time when you need so much of memory?

The measurement is simple, for I don't change other part, the heap size and etc part
size, but only move the temp buffer to the heap area instead of stack area. So this reduce the large memory need in the env_relocate_spec() function. So that the stack would not touch those memory since most function don't cost so much
stack as env_relocate_spec() does...

The heap area is already enough for the env_relocate_spec() in our platform. Maybe
you concern is for those platform don't allocate enough heap size for the env?...
Also fragmentation problem I am not figure out clearly... Temporarily use large heap then free
would make uboot unable to malloc enough memory in latter allocate?...

Dear Lei Wen,

Quoted 8 lines · In message <[email protected]> you…

The measurement is simple, for I don't change other part, the heap size and etc part
size, but only move the temp buffer to the heap area instead of stack area. So this reduce the large memory need in the env_relocate_spec() function. So that the stack would not touch those memory since most function don't cost so much
stack as env_relocate_spec() does...

You don't provide any real measurement results here. ?Also, you did not report what happens when you - for example - reduce the size of your malloc arena by the size...

Quoted 7 lines

The heap area is already enough for the env_relocate_spec() in our platform. Maybe
you concern is for those platform don't allocate enough heap size for the env?...
Also fragmentation problem I am not figure out clearly... Temporarily use large heap then free
would make uboot unable to malloc enough memory in latter allocate?...

To sum it up:

- Your patch increases the code size, and the code complexity, and ?increases the risk of memory leaks.

- Your patch requires on many systems to increase the size of the ?malloc arena, with the result that the available memory at runtim ?becomes lower.

- Your patch fails to adjust the malloc arena size on the affected ?boards (and eventually any board may be affected, so all will have ?to be checked and verified).

- You claim that this patch helps to use less memory on your system, ?but you fail to prove this with actual numbers.

So we have a number of disadvantages for all users, a lot of effort for all users (they all need to re-test their boards), and a claimed but otherwise unproven advantage for a simgle user (you).

I hereby reject your patch. ?If you really think it, you can still maintain it as a local, out-of-tree patch.

If I compile the same uboot with gcc 4.5.2, binutils 2.11 and glibc 2.13 the uboot compiles also successfully but it didn't work on the taget system. The uboot don't have any outputs on the debug console.

Dear =?UTF-8?B?TWFpayBIw6RuaWc=?=,

Quoted 4 lines · In message<[email protected]> you wrote:

If I compile the same uboot with gcc 4.5.2, binutils 2.11 and glibc 2.13 the uboot compiles also successfully but it didn't work on the taget system. The uboot don't have any outputs on the debug console.

There are a number of known (and probably more unknown) compiler bugs in GCC 4.5.x.

Workarounds for some of them have been added recently in mainline, so you might try switching to current top of tree instead (especially if you're on an ARM system).

Best regards,

Wolfgang Denk

should I better use gcc 4.4?

And yes I'm working on ARM.

Dear =?ISO-8859-1?Q?Maik_H=E4nig?=,

Quoted 2 lines · In message<[email protected]> you wrote:

should I better use gcc 4.4?

No, you should go forward, i. e. rather use current U-Boot code that is supposed to fix these problems (at last the most obvious ones - and if there are any left, you'd help to detec and eventually fix these, too).

Quoted 1 line

And yes I'm working on ARM.

Thought so from the symptoms you reported...

Best regards,

Wolfgang Denk

I tried the uboot version 2010.12 also with the eldk but I didn't get it running.

Here is my patch. It seems there is a problem in the lolevel_init. But I didn't flound it. At the moment I have no debugger.

So I have two problems the version 2010.12 didn't work with eldk and the version 2010.09 works with eldk but not with gcc 4.5.2.