arm: reduce .bss section clear time

For ARM architecture, enable the CONFIG_USE_ARCH_MEMSET/MEMCPY, will highly increase the memset/memcpy performance. This is able thanks to the ARM multiple register instructions.

Unfortunatelly the relocation is done without the cache enabled, so it takes some time, but zeroing the BSS memory takes much more longer, especially for the configs with big static buffers.

A quick test confirms, that the boot time improvement after using the arch memcpy for relocation has no significant meaning. The same test confirms that enable the memset for zeroing BSS, reduces the boot time.

So this patch enables the arch memset for zeroing the BSS after the relocation process. For ARM boards, this can be enabled in board configs by defining: 'CONFIG_USE_ARCH_MEMSET'.

Quoted 20 lines · albert.u.boot at aribaud.net

Hello Przemyslaw,

Quoted 16 lines · Przemyslaw Marczak

For ARM architecture, enable the CONFIG_USE_ARCH_MEMSET/MEMCPY, will highly increase the memset/memcpy performance. This is able thanks to the ARM multiple register instructions.

Unfortunatelly the relocation is done without the cache enabled, so it takes some time, but zeroing the BSS memory takes much more longer, especially for the configs with big static buffers.

A quick test confirms, that the boot time improvement after using the arch memcpy for relocation has no significant meaning. The same test confirms that enable the memset for zeroing BSS, reduces the boot time.

So this patch enables the arch memset for zeroing the BSS after the relocation process. For ARM boards, this can be enabled in board configs by defining: 'CONFIG_USE_ARCH_MEMSET'.

Quoted 5 lines

Since the issue is that zeroing is done one word at a time, could we not simply clear r3 as well as r2 (possibly even r4 and r5 too) and do a double (possibly quadruple) write loop? That would avoid calling a libc routine from the almost sole file in U-Boot where a C environment is not necessarily granted.

arch/arm/lib/crt0.S index renamed from arch/arm/lib/crt0.S +0 -0

22df3e5..fab3d2c 100644 --- a/arch/arm/lib/crt0.S +++ b/arch/arm/lib/crt0.S @@ -115,14 +115,22 @@ here: bl c_runtime_cpu_setup /* we still call old routine here */

	ldr	r0, =__bss_start	/* this is auto-relocated! */

arch/arm/lib/crt0.S index renamed from arch/arm/lib/crt0.S +8 -1

-	ldr	r1, =__bss_end		/* this is auto-relocated! */
 
+#ifdef CONFIG_USE_ARCH_MEMSET
+	ldr	r3, =__bss_end		/* this is auto-relocated! */
+	mov	r1, #0x00000000		/* prepare zero to clear BSS */
+
+	subs	r2, r3, r0		/* r2 = memset len */
+	bl	memset
+#else
+	ldr	r1, =__bss_end		/* this is auto-relocated! */

	mov	r2, #0x00000000		/* prepare zero to clear BSS */

clbss_l:cmp r0, r1 /* while not at end of BSS */

	strlo	r2, [r0]		/* clear 32-bit BSS word */
	addlo	r0, r0, #4		/* move to next */
	blo	clbss_l

+#endif

Hello Przemyslaw,

Quoted 8 lines · Przemyslaw Marczak

For ARM architecture, enable the CONFIG_USE_ARCH_MEMSET/MEMCPY, will highly increase the memset/memcpy performance. This is able thanks to the ARM multiple register instructions.

Unfortunatelly the relocation is done without the cache enabled, so it takes some time, but zeroing the BSS memory takes much more longer, especially for the configs with big static buffers.

Quoted 8 lines

A quick test confirms, that the boot time improvement after using the arch memcpy for relocation has no significant meaning. The same test confirms that enable the memset for zeroing BSS, reduces the boot time.

So this patch enables the arch memset for zeroing the BSS after the relocation process. For ARM boards, this can be enabled in board configs by defining: 'CONFIG_USE_ARCH_MEMSET'.

Since the issue is that zeroing is done one word at a time, could we not simply clear r3 as well as r2 (possibly even r4 and r5 too) and do a double (possibly quadruple) write loop? That would avoid calling a libc routine from the almost sole file in U-Boot where a C environment is not necessarily granted.

Quoted 21 lines · Albert ARIBAUD

Hello Przemyslaw,

Quoted 8 lines · Przemyslaw Marczak

For ARM architecture, enable the CONFIG_USE_ARCH_MEMSET/MEMCPY, will highly increase the memset/memcpy performance. This is able thanks to the ARM multiple register instructions.

Unfortunatelly the relocation is done without the cache enabled, so it takes some time, but zeroing the BSS memory takes much more longer, especially for the configs with big static buffers.

Quoted 8 lines

A quick test confirms, that the boot time improvement after using the arch memcpy for relocation has no significant meaning. The same test confirms that enable the memset for zeroing BSS, reduces the boot time.

So this patch enables the arch memset for zeroing the BSS after the relocation process. For ARM boards, this can be enabled in board configs by defining: 'CONFIG_USE_ARCH_MEMSET'.

Quoted 5 lines

Since the issue is that zeroing is done one word at a time, could we not simply clear r3 as well as r2 (possibly even r4 and r5 too) and do a double (possibly quadruple) write loop? That would avoid calling a libc routine from the almost sole file in U-Boot where a C environment is not necessarily granted.

So this brings up something I've wondered about for a long while. We have arch/arm/lib/mem{set,cpy}.S which are old copies from the linux kernel. The kernel uses them for all ARM platforms. Why do we not always use these functions? I have a very vague notion it was a size thing?

Signature · 1 line

Tom

Hi Tom,

Quoted 30 lines · Tom Rini

Quoted 21 lines · Albert ARIBAUD

Hello Przemyslaw,

Quoted 8 lines · Przemyslaw Marczak

For ARM architecture, enable the CONFIG_USE_ARCH_MEMSET/MEMCPY, will highly increase the memset/memcpy performance. This is able thanks to the ARM multiple register instructions.

Unfortunatelly the relocation is done without the cache enabled, so it takes some time, but zeroing the BSS memory takes much more longer, especially for the configs with big static buffers.

Quoted 8 lines

A quick test confirms, that the boot time improvement after using the arch memcpy for relocation has no significant meaning. The same test confirms that enable the memset for zeroing BSS, reduces the boot time.

So this patch enables the arch memset for zeroing the BSS after the relocation process. For ARM boards, this can be enabled in board configs by defining: 'CONFIG_USE_ARCH_MEMSET'.

Quoted 5 lines

Since the issue is that zeroing is done one word at a time, could we not simply clear r3 as well as r2 (possibly even r4 and r5 too) and do a double (possibly quadruple) write loop? That would avoid calling a libc routine from the almost sole file in U-Boot where a C environment is not necessarily granted.

Quoted 5 lines

So this brings up something I've wondered about for a long while. We have arch/arm/lib/mem{set,cpy}.S which are old copies from the linux kernel. The kernel uses them for all ARM platforms. Why do we not always use these functions? I have a very vague notion it was a size thing?

That is a good question. Are we being hobbled cause of MLO? If so we can use the short (and slow) methods in that case and use the fast methods in the normal case. It seems that this is warranted in this case.

However in the particular case of dfu I think it?s best to avoid the large static buffers. Or if we do use the large buffers let?s put them in a linker segment that does not get zeroed on start.

Hello Przemyslaw,

Quoted 8 lines · Przemyslaw Marczak

For ARM architecture, enable the CONFIG_USE_ARCH_MEMSET/MEMCPY, will highly increase the memset/memcpy performance. This is able thanks to the ARM multiple register instructions.

Unfortunatelly the relocation is done without the cache enabled, so it takes some time, but zeroing the BSS memory takes much more longer, especially for the configs with big static buffers.

Quoted 8 lines

A quick test confirms, that the boot time improvement after using the arch memcpy for relocation has no significant meaning. The same test confirms that enable the memset for zeroing BSS, reduces the boot time.

So this patch enables the arch memset for zeroing the BSS after the relocation process. For ARM boards, this can be enabled in board configs by defining: 'CONFIG_USE_ARCH_MEMSET'.

Since the issue is that zeroing is done one word at a time, could we not simply clear r3 as well as r2 (possibly even r4 and r5 too) and do a double (possibly quadruple) write loop? That would avoid calling a libc routine from the almost sole file in U-Boot where a C environment is not necessarily granted.

Quoted 21 lines · Albert ARIBAUD

Hello Przemyslaw,

Quoted 8 lines · Przemyslaw Marczak

For ARM architecture, enable the CONFIG_USE_ARCH_MEMSET/MEMCPY, will highly increase the memset/memcpy performance. This is able thanks to the ARM multiple register instructions.

Unfortunatelly the relocation is done without the cache enabled, so it takes some time, but zeroing the BSS memory takes much more longer, especially for the configs with big static buffers.

Quoted 8 lines

A quick test confirms, that the boot time improvement after using the arch memcpy for relocation has no significant meaning. The same test confirms that enable the memset for zeroing BSS, reduces the boot time.

So this patch enables the arch memset for zeroing the BSS after the relocation process. For ARM boards, this can be enabled in board configs by defining: 'CONFIG_USE_ARCH_MEMSET'.

Quoted 5 lines

Since the issue is that zeroing is done one word at a time, could we not simply clear r3 as well as r2 (possibly even r4 and r5 too) and do a double (possibly quadruple) write loop? That would avoid calling a libc routine from the almost sole file in U-Boot where a C environment is not necessarily granted.

I want to jump up here again. Note that the arch memset/memcpy routines are in asm and I don't belive require a C environment. Why don't we simply use the asm versions for everyone and backport whatever we need from the kernel to re-sync there as it's not a choice there and it's a performance win too?

This patchset reduces the boot time for ARM architecture, Exynos boards, and boards with DFU enabled(ARM).

For tested Trats2 device, this was done in three steps.

First was enable the arch memcpy and memset.
The second step was enable memset for .bss clear.
The third step for reduce this operation is to keep .bss section small as possible.

The .bss section will grow if we have a lot of static variables. This section is cleared before jump to the relocated U-Boot, and it's done word by word. To reduce the time for this step, we can enable arch memset, which uses multiple ARM registers.

For configs with DFU enabled, we can find the dfu buffer in this section, which has at least 8MB (32MB for trats2). This is a lot of useless data, which is not required for standard boot. So this buffer should be dynamic allocated.

Przemyslaw Marczak (3):
exynos: config: enable arch memcpy and arch memset
arm: relocation: clear .bss section with arch memset if defined
dfu: mmc: file buffer: remove static allocation

  arch/arm/lib/crt0.S             | 10 +++++++++-
  drivers/dfu/dfu_mmc.c           | 25 ++++++++++++++++++++++---
  include/configs/exynos-common.h |  3 +++
  3 files changed, 34 insertions(+), 4 deletions(-)

Hi Przemyslaw,

Quoted 25 lines · Przemyslaw Marczak

This patchset reduces the boot time for ARM architecture, Exynos boards, and boards with DFU enabled(ARM).

For tested Trats2 device, this was done in three steps.

First was enable the arch memcpy and memset.
The second step was enable memset for .bss clear.
The third step for reduce this operation is to keep .bss section small as possible.

The .bss section will grow if we have a lot of static variables. This section is cleared before jump to the relocated U-Boot, and it's done word by word. To reduce the time for this step, we can enable arch memset, which uses multiple ARM registers.

For configs with DFU enabled, we can find the dfu buffer in this section, which has at least 8MB (32MB for trats2). This is a lot of useless data, which is not required for standard boot. So this buffer should be dynamic allocated.

Przemyslaw Marczak (3):
exynos: config: enable arch memcpy and arch memset
arm: relocation: clear .bss section with arch memset if defined
dfu: mmc: file buffer: remove static allocation

Quoted 4 lines

  arch/arm/lib/crt0.S             | 10 +++++++++-
  drivers/dfu/dfu_mmc.c           | 25 ++++++++++++++++++++++---
  include/configs/exynos-common.h |  3 +++
  3 files changed, 34 insertions(+), 4 deletions(-)

Looking at the commit messages of this patchset I can conclude that your overall boot time reduction is:

from ~1527ms
to ~464ms

This is amazing! Congrats. :)

We really should in general make more use of the optimized functions and take care that the buffers (e.g. the DFU buffer in this case) are used in a sane way.

Thanks,
Stefan

Hello Stefan,

Quoted 33 lines · Stefan Roese

Hi Przemyslaw,

Quoted 25 lines · Przemyslaw Marczak

This patchset reduces the boot time for ARM architecture, Exynos boards, and boards with DFU enabled(ARM).

For tested Trats2 device, this was done in three steps.

First was enable the arch memcpy and memset.
The second step was enable memset for .bss clear.
The third step for reduce this operation is to keep .bss section small as possible.

The .bss section will grow if we have a lot of static variables. This section is cleared before jump to the relocated U-Boot, and it's done word by word. To reduce the time for this step, we can enable arch memset, which uses multiple ARM registers.

For configs with DFU enabled, we can find the dfu buffer in this section, which has at least 8MB (32MB for trats2). This is a lot of useless data, which is not required for standard boot. So this buffer should be dynamic allocated.

Przemyslaw Marczak (3):
exynos: config: enable arch memcpy and arch memset
arm: relocation: clear .bss section with arch memset if defined
dfu: mmc: file buffer: remove static allocation

Quoted 4 lines

 arch/arm/lib/crt0.S             | 10 +++++++++-
 drivers/dfu/dfu_mmc.c           | 25 ++++++++++++++++++++++---
 include/configs/exynos-common.h |  3 +++
 3 files changed, 34 insertions(+), 4 deletions(-)

Quoted 8 lines

Looking at the commit messages of this patchset I can conclude that your overall boot time reduction is:

from ~1527ms
to ~464ms

This is amazing! Congrats. :)

Thank you. I was also amazed.

The time results are taken with from the clock cycle counter, I think it's reliable. Some day I would like to check it using the oscilloscope.

Quoted 4 lines

We really should in general make more use of the optimized functions and take care that the buffers (e.g. the DFU buffer in this case) are used in a sane way.

Quoted 4 lines

Thanks,
Stefan

Yes you're right, I thought that Exynos config has enabled arch memcpy/set lib, before I checked this?

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Hi Przemyslaw,

Quoted 60 lines · Przemyslaw Marczak

Hello Stefan,

Quoted 33 lines · Stefan Roese

Hi Przemyslaw,

Quoted 25 lines · Przemyslaw Marczak

This patchset reduces the boot time for ARM architecture, Exynos boards, and boards with DFU enabled(ARM).

For tested Trats2 device, this was done in three steps.

First was enable the arch memcpy and memset.
The second step was enable memset for .bss clear.
The third step for reduce this operation is to keep .bss section small as possible.

The .bss section will grow if we have a lot of static variables. This section is cleared before jump to the relocated U-Boot, and it's done word by word. To reduce the time for this step, we can enable arch memset, which uses multiple ARM registers.

For configs with DFU enabled, we can find the dfu buffer in this section, which has at least 8MB (32MB for trats2). This is a lot of useless data, which is not required for standard boot. So this buffer should be dynamic allocated.

Przemyslaw Marczak (3):
exynos: config: enable arch memcpy and arch memset
arm: relocation: clear .bss section with arch memset if defined
dfu: mmc: file buffer: remove static allocation

Quoted 4 lines

  arch/arm/lib/crt0.S             | 10 +++++++++-
  drivers/dfu/dfu_mmc.c           | 25 ++++++++++++++++++++++---
  include/configs/exynos-common.h |  3 +++
  3 files changed, 34 insertions(+), 4 deletions(-)

Quoted 8 lines

Looking at the commit messages of this patchset I can conclude that your overall boot time reduction is:

from ~1527ms
to ~464ms

This is amazing! Congrats. :)

Thank you. I was also amazed.

The time results are taken with from the clock cycle counter, I think it's reliable. Some day I would like to check it using the oscilloscope.

Quoted 8 lines

We really should in general make more use of the optimized functions and take care that the buffers (e.g. the DFU buffer in this case) are used in a sane way.

Thanks,
Stefan

Quoted 2 lines

Yes you're right, I thought that Exynos config has enabled arch memcpy/set lib, before I checked this?

Those numbers are indeed incredible; I suppose the caches are disabled?

Quoted 6 lines

Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Regards

? Pantelis

Hello,

Quoted 62 lines · Pantelis Antoniou

Hi Przemyslaw,

Quoted 60 lines · Przemyslaw Marczak

Hello Stefan,

Quoted 33 lines · Stefan Roese

Hi Przemyslaw,

Quoted 25 lines · Przemyslaw Marczak

This patchset reduces the boot time for ARM architecture, Exynos boards, and boards with DFU enabled(ARM).

For tested Trats2 device, this was done in three steps.

First was enable the arch memcpy and memset.
The second step was enable memset for .bss clear.
The third step for reduce this operation is to keep .bss section small as possible.

The .bss section will grow if we have a lot of static variables. This section is cleared before jump to the relocated U-Boot, and it's done word by word. To reduce the time for this step, we can enable arch memset, which uses multiple ARM registers.

For configs with DFU enabled, we can find the dfu buffer in this section, which has at least 8MB (32MB for trats2). This is a lot of useless data, which is not required for standard boot. So this buffer should be dynamic allocated.

Przemyslaw Marczak (3):
exynos: config: enable arch memcpy and arch memset
arm: relocation: clear .bss section with arch memset if defined
dfu: mmc: file buffer: remove static allocation

Quoted 4 lines

 arch/arm/lib/crt0.S             | 10 +++++++++-
 drivers/dfu/dfu_mmc.c           | 25 ++++++++++++++++++++++---
 include/configs/exynos-common.h |  3 +++
 3 files changed, 34 insertions(+), 4 deletions(-)

Quoted 8 lines

Looking at the commit messages of this patchset I can conclude that your overall boot time reduction is:

from ~1527ms
to ~464ms

This is amazing! Congrats. :)

Thank you. I was also amazed.

The time results are taken with from the clock cycle counter, I think it's reliable. Some day I would like to check it using the oscilloscope.

Quoted 8 lines

We really should in general make more use of the optimized functions and take care that the buffers (e.g. the DFU buffer in this case) are used in a sane way.

Thanks,
Stefan

Quoted 6 lines

Quoted 2 lines

Yes you're right, I thought that Exynos config has enabled arch memcpy/set lib, before I checked this?

Those numbers are indeed incredible; I suppose the caches are disabled?

The caches are enabled after the relocation, in one of board_init_r calls.

Quoted 7 lines

Quoted 6 lines

Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Quoted 5 lines

Regards

? Pantelis

Best regards,

Signature · 4 lines

Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Hi Przemyslaw,

Quoted 74 lines · Przemyslaw Marczak

Hello,

Quoted 68 lines · Pantelis Antoniou

Hi Przemyslaw,

Quoted 55 lines · Przemyslaw Marczak

Hello Stefan,

Quoted 33 lines · Stefan Roese

Hi Przemyslaw,

Quoted 25 lines · Przemyslaw Marczak

This patchset reduces the boot time for ARM architecture, Exynos boards, and boards with DFU enabled(ARM).

For tested Trats2 device, this was done in three steps.

First was enable the arch memcpy and memset.
The second step was enable memset for .bss clear.
The third step for reduce this operation is to keep .bss section small as possible.

The .bss section will grow if we have a lot of static variables. This section is cleared before jump to the relocated U-Boot, and it's done word by word. To reduce the time for this step, we can enable arch memset, which uses multiple ARM registers.

For configs with DFU enabled, we can find the dfu buffer in this section, which has at least 8MB (32MB for trats2). This is a lot of useless data, which is not required for standard boot. So this buffer should be dynamic allocated.

Przemyslaw Marczak (3):
exynos: config: enable arch memcpy and arch memset
arm: relocation: clear .bss section with arch memset if defined
dfu: mmc: file buffer: remove static allocation

Quoted 4 lines

  arch/arm/lib/crt0.S             | 10 +++++++++-
  drivers/dfu/dfu_mmc.c           | 25 ++++++++++++++++++++++---
  include/configs/exynos-common.h |  3 +++
  3 files changed, 34 insertions(+), 4 deletions(-)

Quoted 8 lines

Looking at the commit messages of this patchset I can conclude that your overall boot time reduction is:

from ~1527ms
to ~464ms

This is amazing! Congrats. :)

Thank you. I was also amazed.

The time results are taken with from the clock cycle counter, I think it's reliable. Some day I would like to check it using the oscilloscope.

Quoted 4 lines

We really should in general make more use of the optimized functions and take care that the buffers (e.g. the DFU buffer in this case) are used in a sane way.

Quoted 7 lines

Quoted 4 lines

Thanks,
Stefan

Yes you're right, I thought that Exynos config has enabled arch memcpy/set lib, before I checked this?

Those numbers are indeed incredible; I suppose the caches are disabled?

Quoted 2 lines

The caches are enabled after the relocation, in one of board_init_r calls.

How big is this .bss section? We?re talking about something that takes 1.5secs to clear a few MBs of memory? This is horrible.

Even at the optimized case .5secs is too much.

Quoted 13 lines

Quoted 7 lines

Quoted 6 lines

Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Quoted 5 lines

Regards

? Pantelis

Quoted 6 lines

Best regards,
--
Przemyslaw Marczak
Samsung R&D Institute Poland
Samsung Electronics
p.marczak at samsung.com

Regards

? Pantelis

This patchset reduces the boot time for ARM architecture, Exynos boards, and boards with DFU enabled(ARM).

For tested Trats2 device, this was done in three steps.

First was enable the arch memcpy and memset.
The second step was enable memset for .bss clear.
The third step for reduce this operation is to keep .bss section small as possible.

The .bss section will grow if we have a lot of static variables. This section is cleared before jump to the relocated U-Boot, and it's done word by word. To reduce the time for this step, we can enable arch memset, which uses multiple ARM registers.

For configs with DFU enabled, we can find the dfu buffer in this section, which has at least 8MB (32MB for trats2). This is a lot of useless data, which is not required for standard boot. So this buffer should be dynamic allocated.

Przemyslaw Marczak (3):
exynos: config: enable arch memcpy and arch memset
arm: relocation: clear .bss section with arch memset if defined
dfu: mmc: file buffer: remove static allocation

  arch/arm/lib/crt0.S             | 10 +++++++++-
  drivers/dfu/dfu_mmc.c           | 25 ++++++++++++++++++++++---
  include/configs/exynos-common.h |  3 +++
  3 files changed, 34 insertions(+), 4 deletions(-)

And the next is interesting.
odroid_defconfig has more than 80MB for malloc (we need about 64mb
for the DFU now, to be able write 32MB file).

This is the CONFIG_SYS_MALLOC_LEN. And the memory area for malloc is set to 0 in function mem_malloc_init(). So for this config that function sets more than 80MB to zero.

This is not good, because we shouldn't expect zeroed memory returned by malloc pointer. This is a job for calloc.

Especially if some command expects zeroed memory after malloc, probably after few next calls - it can crash...

Dear All,

Quoted 8 lines

And the next is interesting.
odroid_defconfig has more than 80MB for malloc (we need about 64mb
for the DFU now, to be able write 32MB file).

This is the CONFIG_SYS_MALLOC_LEN. And the memory area for malloc is set to 0 in function mem_malloc_init(). So for this config that function sets more than 80MB to zero.

Quoted 5 lines

This is not good, because we shouldn't expect zeroed memory returned by malloc pointer. This is a job for calloc.

Especially if some command expects zeroed memory after malloc, probably after few next calls - it can crash...

I think that the above excerpt is _really_ important and should be discussed.

I've "cut" it from the original post, so it won't get lost between the lines.

It seems really strange, that malloc() area is cleared after relocation. Which means that all "first" malloc'ed buffers get implicitly zeroed.

Przemek is right here that this zeroing shouldn't be performed.

I'm also concerned about potential bugs, which show up (or even worse - won't show up soon) after this change.

Hence, I would like to ask directly the community about the possible solutions.

Please look at: ./common/dlmalloc.c mem_alloc_init() function [1].

On the one hand removing memset() at [1] speeds up booting time and makes malloc() doing what is is supposed to do.

On the other hand there might be in space some boards, which rely on this memset and without it some wired things may start to happening.

Hi Lukasz,

Quoted 39 lines · Lukasz Majewski

Dear All,

Quoted 8 lines

And the next is interesting.
odroid_defconfig has more than 80MB for malloc (we need about
64mb for the DFU now, to be able write 32MB file).

This is the CONFIG_SYS_MALLOC_LEN. And the memory area for malloc is set to 0 in function mem_malloc_init(). So for this config that function sets more than 80MB to zero.

Quoted 5 lines

This is not good, because we shouldn't expect zeroed memory returned by malloc pointer. This is a job for calloc.

Especially if some command expects zeroed memory after malloc, probably after few next calls - it can crash...

I think that the above excerpt is _really_ important and should be discussed.

I've "cut" it from the original post, so it won't get lost between the lines.

It seems really strange, that malloc() area is cleared after relocation. Which means that all "first" malloc'ed buffers get implicitly zeroed.

Przemek is right here that this zeroing shouldn't be performed.

I'm also concerned about potential bugs, which show up (or even worse - won't show up soon) after this change.

Hence, I would like to ask directly the community about the possible solutions.

Please look at: ./common/dlmalloc.c mem_alloc_init() function [1].

On the one hand removing memset() at [1] speeds up booting time and makes malloc() doing what is is supposed to do.

Quoted 2 lines

On the other hand there might be in space some boards, which rely on this memset and without it some wired things may start to happening.

I think removing it is a good idea. It was one optimisation that I did for boot time in the Chromium tree. If you do it now (and Tom agrees) then there is plenty of time to test for this release cycle. You could go further and add a test CONFIG which fills it with some other non-zero value.

Regards,
Simon

Hi Simon,

Quoted 46 lines

Hi Lukasz,

Quoted 41 lines · Lukasz Majewski

Dear All,

Quoted 8 lines

And the next is interesting.
odroid_defconfig has more than 80MB for malloc (we need about
64mb for the DFU now, to be able write 32MB file).

This is the CONFIG_SYS_MALLOC_LEN. And the memory area for malloc is set to 0 in function mem_malloc_init(). So for this config that function sets more than 80MB to zero.

Quoted 5 lines

This is not good, because we shouldn't expect zeroed memory returned by malloc pointer. This is a job for calloc.

Especially if some command expects zeroed memory after malloc, probably after few next calls - it can crash...

I think that the above excerpt is _really_ important and should be discussed.

I've "cut" it from the original post, so it won't get lost between the lines.

It seems really strange, that malloc() area is cleared after relocation. Which means that all "first" malloc'ed buffers get implicitly zeroed.

Przemek is right here that this zeroing shouldn't be performed.

I'm also concerned about potential bugs, which show up (or even worse - won't show up soon) after this change.

Hence, I would like to ask directly the community about the possible solutions.

Please look at: ./common/dlmalloc.c mem_alloc_init() function [1].

On the one hand removing memset() at [1] speeds up booting time and makes malloc() doing what is is supposed to do.

On the other hand there might be in space some boards, which rely on this memset and without it some wired things may start to happening.

Quoted 5 lines

I think removing it is a good idea. It was one optimisation that I did for boot time in the Chromium tree. If you do it now (and Tom agrees) then there is plenty of time to test for this release cycle. You could go further and add a test CONFIG which fills it with some other non-zero value.

Tom, is such approach acceptable for you?

Quoted 48 lines · Lukasz Majewski

Hi Simon,

Quoted 46 lines

Hi Lukasz,

Quoted 41 lines · Lukasz Majewski

Dear All,

Quoted 8 lines

And the next is interesting.
odroid_defconfig has more than 80MB for malloc (we need about
64mb for the DFU now, to be able write 32MB file).

This is the CONFIG_SYS_MALLOC_LEN. And the memory area for malloc is set to 0 in function mem_malloc_init(). So for this config that function sets more than 80MB to zero.

Quoted 5 lines

This is not good, because we shouldn't expect zeroed memory returned by malloc pointer. This is a job for calloc.

Especially if some command expects zeroed memory after malloc, probably after few next calls - it can crash...

I think that the above excerpt is _really_ important and should be discussed.

I've "cut" it from the original post, so it won't get lost between the lines.

It seems really strange, that malloc() area is cleared after relocation. Which means that all "first" malloc'ed buffers get implicitly zeroed.

Przemek is right here that this zeroing shouldn't be performed.

I'm also concerned about potential bugs, which show up (or even worse - won't show up soon) after this change.

Hence, I would like to ask directly the community about the possible solutions.

Please look at: ./common/dlmalloc.c mem_alloc_init() function [1].

On the one hand removing memset() at [1] speeds up booting time and makes malloc() doing what is is supposed to do.

On the other hand there might be in space some boards, which rely on this memset and without it some wired things may start to happening.

Quoted 7 lines

Quoted 5 lines

I think removing it is a good idea. It was one optimisation that I did for boot time in the Chromium tree. If you do it now (and Tom agrees) then there is plenty of time to test for this release cycle. You could go further and add a test CONFIG which fills it with some other non-zero value.

Tom, is such approach acceptable for you?

I was thinking at first we should default to a poisoned value. But given what we're seeing with generic board updates (lots of boards aren't even build-tested at every release which isn't really a surprise), I think the "funky" boards which may exist are probably not going to be seen for a while anyhow so we'd have to default to a poison for a long while. So yes, lets just add a CONFIG option (and Kconfig line) to optionally do it and default to no memset.

... but I just audited everyone doing "malloc (" and found a few things to fixup so we really do want to take a poke around.

Hello,

Quoted 65 lines · Tom Rini

Quoted 48 lines · Lukasz Majewski

Hi Simon,

Quoted 46 lines

Hi Lukasz,

Quoted 41 lines · Lukasz Majewski

Dear All,

Quoted 8 lines

And the next is interesting.
odroid_defconfig has more than 80MB for malloc (we need about
64mb for the DFU now, to be able write 32MB file).

This is the CONFIG_SYS_MALLOC_LEN. And the memory area for malloc is set to 0 in function mem_malloc_init(). So for this config that function sets more than 80MB to zero.

Quoted 5 lines

This is not good, because we shouldn't expect zeroed memory returned by malloc pointer. This is a job for calloc.

Especially if some command expects zeroed memory after malloc, probably after few next calls - it can crash...

I think that the above excerpt is _really_ important and should be discussed.

I've "cut" it from the original post, so it won't get lost between the lines.

It seems really strange, that malloc() area is cleared after relocation. Which means that all "first" malloc'ed buffers get implicitly zeroed.

Przemek is right here that this zeroing shouldn't be performed.

I'm also concerned about potential bugs, which show up (or even worse - won't show up soon) after this change.

Hence, I would like to ask directly the community about the possible solutions.

Please look at: ./common/dlmalloc.c mem_alloc_init() function [1].

On the one hand removing memset() at [1] speeds up booting time and makes malloc() doing what is is supposed to do.

On the other hand there might be in space some boards, which rely on this memset and without it some wired things may start to happening.

Quoted 7 lines

Quoted 5 lines

I think removing it is a good idea. It was one optimisation that I did for boot time in the Chromium tree. If you do it now (and Tom agrees) then there is plenty of time to test for this release cycle. You could go further and add a test CONFIG which fills it with some other non-zero value.

Tom, is such approach acceptable for you?

I was thinking at first we should default to a poisoned value. But given what we're seeing with generic board updates (lots of boards aren't even build-tested at every release which isn't really a surprise), I think the "funky" boards which may exist are probably not going to be seen for a while anyhow so we'd have to default to a poison for a long while. So yes, lets just add a CONFIG option (and Kconfig line) to optionally do it and default to no memset.

Quoted 3 lines

... but I just audited everyone doing "malloc (" and found a few things to fixup so we really do want to take a poke around.

The present malloc implementation, which includes the memset at init, could be little confusing. Because of this memset. the "few" first calls of malloc will always return a pointer to the zeroed memory region. Of course, the only calloc do the right job and set with zeros the memory region, which was used by any previous malloc call.

So, maybe some code expects zeroed memory also when using malloc.

In this case, I would like to skip this memset as an option.

This also requires skip some checking in calloc code.

I will send the patch with such option for Kconfig.

Hi Lukasz,

Quoted 34 lines · Lukasz Majewski

Dear All,

Quoted 8 lines

And the next is interesting.
odroid_defconfig has more than 80MB for malloc (we need about 64mb
for the DFU now, to be able write 32MB file).

This is the CONFIG_SYS_MALLOC_LEN. And the memory area for malloc is set to 0 in function mem_malloc_init(). So for this config that function sets more than 80MB to zero.

Quoted 5 lines

This is not good, because we shouldn't expect zeroed memory returned by malloc pointer. This is a job for calloc.

Especially if some command expects zeroed memory after malloc, probably after few next calls - it can crash...

I think that the above excerpt is _really_ important and should be discussed.

I've "cut" it from the original post, so it won't get lost between the lines.

It seems really strange, that malloc() area is cleared after relocation. Which means that all "first" malloc'ed buffers get implicitly zeroed.

Przemek is right here that this zeroing shouldn't be performed.

I'm also concerned about potential bugs, which show up (or even worse - won't show up soon) after this change.

Hence, I would like to ask directly the community about the possible solutions.

Quoted 7 lines

Please look at: ./common/dlmalloc.c mem_alloc_init() function [1].

On the one hand removing memset() at [1] speeds up booting time and makes malloc() doing what is is supposed to do.

On the other hand there might be in space some boards, which rely on this memset and without it some wired things may start to happening.

I think removing it is a good idea. It was one optimisation that I did for boot time in the Chromium tree. If you do it now (and Tom agrees) then there is plenty of time to test for this release cycle. You could go further and add a test CONFIG which fills it with some other non-zero value.

Regards,
Simon