[U-Boot] [PATCH v3 4/6] dlmalloc: add option for skip memset in malloc init

[Przemyslaw Marczak <p.marczak@samsung.com>]

Hello,

On 02/20/2015 01:52 PM, Masahiro Yamada wrote:
> Hi Przemyslaw,
>
>
> On Fri, 20 Feb 2015 12:06:17 +0100
> Przemyslaw Marczak <p.marczak@samsung.com> wrote:
>
>> This commit introduces new config: CONFIG_SYS_MALLOC_INIT_SKIP_ZEROING.
>>
>> Before this change, the all amount of memory reserved for the malloc,
>> was set to zero in mem_malloc_init(). When the malloc reserved memory
>> exceeds few MiB, then the boot process can slow down.
>>
>> So enabling this config, is an option to reduce the boot time.
>>
>> This option can be enabled by Kconfig.
>>
>> Note:
>> After enable this option, only calloc() will return the pointer to zeroed
>> memory area. Previously, without this option, the memory pointed to untouched
>> malloc memory region, was filled with zeros. So it means, that code with
>> malloc() calls should be reexamined.
>>
>> Signed-off-by: Przemyslaw Marczak <p.marczak@samsung.com>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>
>> ---
>> Changes v3:
>> - squash the commit with the Kconfig option
>> ---
>>   Kconfig           | 26 +++++++++++++++++++-------
>>   common/dlmalloc.c | 10 +++++++---
>>   2 files changed, 26 insertions(+), 10 deletions(-)
>>
>> diff --git a/Kconfig b/Kconfig
>> index 75bab7f..87d4daf 100644
>> --- a/Kconfig
>> +++ b/Kconfig
>> @@ -76,13 +76,25 @@ config SYS_MALLOC_F_LEN
>>   	  initial serial device and any others that are needed.
>>
>>   menuconfig EXPERT
>> -        bool "Configure standard U-Boot features (expert users)"
>> -        help
>> -          This option allows certain base U-Boot options and settings
>> -          to be disabled or tweaked. This is for specialized
>> -          environments which can tolerate a "non-standard" U-Boot.
>> -          Only use this if you really know what you are doing.
>> -
>> +	bool "Configure standard U-Boot features (expert users)"
>> +	help
>> +	  This option allows certain base U-Boot options and settings
>> +	  to be disabled or tweaked. This is for specialized
>> +	  environments which can tolerate a "non-standard" U-Boot.
>> +	  Only use this if you really know what you are doing.
>> +
>> +if EXPERT
>> +	config SYS_MALLOC_INIT_SKIP_ZEROING
>> +	bool "Skip memset at malloc init (reduce boot time)"
>> +	help
>> +	  This avoids zeroing memory reserved for malloc at malloc init.
>> +	  Significant boot time reduction is visible for configs in which
>> +	  CONFIG_SYS_MALLOC_LEN value, has more than few MiB.
>> +	  Useful for bzip2, bmp logo.
>> +	  Warning:
>> +	  When enabling this, please check if malloc calls, maybe
>> +	  should be replaced by calloc - if expects zeroed memory.
>> +endif
>>   endmenu		# General setup
>>
>>   menu "Boot images"
>> diff --git a/common/dlmalloc.c b/common/dlmalloc.c
>> index 6453ee9..63f68ed 100644
>> --- a/common/dlmalloc.c
>> +++ b/common/dlmalloc.c
>> @@ -1535,9 +1535,9 @@ void mem_malloc_init(ulong start, ulong size)
>>
>>   	debug("using memory %#lx-%#lx for malloc()\n", mem_malloc_start,
>>   	      mem_malloc_end);
>> -
>> -	memset((void *)mem_malloc_start, 0, size);
>> -
>> +#ifndef CONFIG_SYS_MALLOC_INIT_SKIP_ZEROING
>> +	memset((void *)mem_malloc_start, 0x0, size);
>> +#endif
>>   	malloc_bin_reloc();
>>   }
>>
>> @@ -2948,10 +2948,12 @@ Void_t* cALLOc(n, elem_size) size_t n; size_t elem_size;
>>
>>
>>     /* check if expand_top called, in which case don't need to clear */
>> +#ifndef CONFIG_SYS_MALLOC_INIT_SKIP_ZEROING
>>   #if MORECORE_CLEARS
>>     mchunkptr oldtop = top;
>>     INTERNAL_SIZE_T oldtopsize = chunksize(top);
>>   #endif
>> +#endif
>>     Void_t* mem = mALLOc (sz);
>>
>>     if ((long)n < 0) return NULL;
>> @@ -2977,6 +2979,7 @@ Void_t* cALLOc(n, elem_size) size_t n; size_t elem_size;
>>
>>       csz = chunksize(p);
>>
>> +#ifndef CONFIG_SYS_MALLOC_INIT_SKIP_ZEROING
>>   #if MORECORE_CLEARS
>>       if (p == oldtop && csz > oldtopsize)
>>       {
>> @@ -2984,6 +2987,7 @@ Void_t* cALLOc(n, elem_size) size_t n; size_t elem_size;
>>         csz = oldtopsize;
>>       }
>>   #endif
>> +#endif
>>
>>       MALLOC_ZERO(mem, csz - SIZE_SZ);
>>       return mem;
>
>
> You are adding only "ifndef" conditionals.
>
> IMHO,
> Isn't "#ifdef CONFIG_SYS_MALLOC_INIT_ZEROING" better?
> (Generally speaking, CONFIG options that disable a feature are not preferable.)
>
> You also need to add "default y" to the Kconfig
> and add "# CONIFG_SYS_MALLOC_INIT_ZEROING is not set" to your defconfig.
>
>
> Best Regards
> Masahiro Yamada
>

Ok, will change this to positive.

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