diff mbox series

malloc: Correct the documentation of the top_pad default

Message ID 87wnbojswh.fsf@oldenburg.str.redhat.com
State New
Headers show
Series malloc: Correct the documentation of the top_pad default | expand

Commit Message

Florian Weimer Aug. 4, 2022, 8:48 a.m. UTC
DEFAULT_TOP_PAD is defined as 131072 in
sysdeps/generic/malloc-machine.h.

---
 elf/dl-tunables.list           | 1 +
 elf/tst-rtld-list-tunables.exp | 2 +-
 manual/tunables.texi           | 2 +-
 3 files changed, 3 insertions(+), 2 deletions(-)

Comments

Siddhesh Poyarekar Aug. 4, 2022, 2:51 p.m. UTC | #1
On 2022-08-04 04:48, Florian Weimer via Libc-alpha wrote:
> DEFAULT_TOP_PAD is defined as 131072 in
> sysdeps/generic/malloc-machine.h.

LGTM, but I wonder if we should tighten this so that it doesn't appear 
that DEFAULT_TOP_PAD could be 0 in any of our build configurations. 
e.g. we could replace the definition with a #error "define 
DEFAULT_TOP_PAD" or something like that to ensure that it's always 
defined through malloc-machine.h.  What do you think?

Thanks,
Sid

> 
> ---
>   elf/dl-tunables.list           | 1 +
>   elf/tst-rtld-list-tunables.exp | 2 +-
>   manual/tunables.texi           | 2 +-
>   3 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
> index e6a56b3070..e925590c36 100644
> --- a/elf/dl-tunables.list
> +++ b/elf/dl-tunables.list
> @@ -42,6 +42,7 @@ glibc {
>         type: SIZE_T
>         env_alias: MALLOC_TOP_PAD_
>         security_level: SXID_IGNORE
> +      default: 131072
>       }
>       perturb {
>         type: INT_32
> diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp
> index cdfdb56a94..2233ea9c7c 100644
> --- a/elf/tst-rtld-list-tunables.exp
> +++ b/elf/tst-rtld-list-tunables.exp
> @@ -9,7 +9,7 @@ glibc.malloc.perturb: 0 (min: 0, max: 255)
>   glibc.malloc.tcache_count: 0x0 (min: 0x0, max: 0x[f]+)
>   glibc.malloc.tcache_max: 0x0 (min: 0x0, max: 0x[f]+)
>   glibc.malloc.tcache_unsorted_limit: 0x0 (min: 0x0, max: 0x[f]+)
> -glibc.malloc.top_pad: 0x0 (min: 0x0, max: 0x[f]+)
> +glibc.malloc.top_pad: 0x20000 (min: 0x0, max: 0x[f]+)
>   glibc.malloc.trim_threshold: 0x0 (min: 0x0, max: 0x[f]+)
>   glibc.rtld.dynamic_sort: 2 (min: 1, max: 2)
>   glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10)
> diff --git a/manual/tunables.texi b/manual/tunables.texi
> index 83cdcdac6d..ffd50a3fe2 100644
> --- a/manual/tunables.texi
> +++ b/manual/tunables.texi
> @@ -143,7 +143,7 @@ number of bytes to retain when shrinking any of the arenas.  This provides the
>   necessary hysteresis in heap size such that excessive amounts of system calls
>   can be avoided.
>   
> -The default value of this tunable is @samp{0}.
> +The default value of this tunable is @samp{131072} (128 KB).
>   @end deftp
>   
>   @deftp Tunable glibc.malloc.perturb
>
Florian Weimer Aug. 4, 2022, 3:14 p.m. UTC | #2
* Siddhesh Poyarekar:

> On 2022-08-04 04:48, Florian Weimer via Libc-alpha wrote:
>> DEFAULT_TOP_PAD is defined as 131072 in
>> sysdeps/generic/malloc-machine.h.
>
> LGTM, but I wonder if we should tighten this so that it doesn't appear
> that DEFAULT_TOP_PAD could be 0 in any of our build configurations. 
> e.g. we could replace the definition with a #error "define
> DEFAULT_TOP_PAD" or something like that to ensure that it's always 
> defined through malloc-machine.h.  What do you think?

We could just remove the various #ifdefs, I guess.  The other question
is whether we can use the constant in the tunable definition file.  I
haven't investigate any of this.

Should I push my change in the meantime?

Thanks,
Florian
Siddhesh Poyarekar Aug. 4, 2022, 3:15 p.m. UTC | #3
On 2022-08-04 11:14, Florian Weimer wrote:
> * Siddhesh Poyarekar:
> 
>> On 2022-08-04 04:48, Florian Weimer via Libc-alpha wrote:
>>> DEFAULT_TOP_PAD is defined as 131072 in
>>> sysdeps/generic/malloc-machine.h.
>>
>> LGTM, but I wonder if we should tighten this so that it doesn't appear
>> that DEFAULT_TOP_PAD could be 0 in any of our build configurations.
>> e.g. we could replace the definition with a #error "define
>> DEFAULT_TOP_PAD" or something like that to ensure that it's always
>> defined through malloc-machine.h.  What do you think?
> 
> We could just remove the various #ifdefs, I guess.  The other question
> is whether we can use the constant in the tunable definition file.  I
> haven't investigate any of this.
> 
> Should I push my change in the meantime?

Yes please.

Thanks,
Sid
Alejandro Colomar Aug. 4, 2022, 3:46 p.m. UTC | #4
Hi Florian,

On 8/4/22 10:48, Florian Weimer via Libc-alpha wrote:
> DEFAULT_TOP_PAD is defined as 131072 in
> sysdeps/generic/malloc-machine.h.
> 
> ---
>   elf/dl-tunables.list           | 1 +
>   elf/tst-rtld-list-tunables.exp | 2 +-
>   manual/tunables.texi           | 2 +-
>   3 files changed, 3 insertions(+), 2 deletions(-)
> 

[...]

> diff --git a/manual/tunables.texi b/manual/tunables.texi
> index 83cdcdac6d..ffd50a3fe2 100644
> --- a/manual/tunables.texi
> +++ b/manual/tunables.texi
> @@ -143,7 +143,7 @@ number of bytes to retain when shrinking any of the arenas.  This provides the
>   necessary hysteresis in heap size such that excessive amounts of system calls
>   can be avoided.
>   
> -The default value of this tunable is @samp{0}.
> +The default value of this tunable is @samp{131072} (128 KB).

I don't know how much glibc uses KB vs KiB, but I'd like to be pedantic 
here and suggest using KiB (maybe this requires a global change to keep 
consistency).  See units(7).

Cheers,

Alex

>   @end deftp
>   
>   @deftp Tunable glibc.malloc.perturb
>
Florian Weimer Aug. 4, 2022, 3:54 p.m. UTC | #5
* Alejandro Colomar:

> Hi Florian,
>
> On 8/4/22 10:48, Florian Weimer via Libc-alpha wrote:
>> DEFAULT_TOP_PAD is defined as 131072 in
>> sysdeps/generic/malloc-machine.h.
>> ---
>>   elf/dl-tunables.list           | 1 +
>>   elf/tst-rtld-list-tunables.exp | 2 +-
>>   manual/tunables.texi           | 2 +-
>>   3 files changed, 3 insertions(+), 2 deletions(-)
>> 
>
> [...]
>
>> diff --git a/manual/tunables.texi b/manual/tunables.texi
>> index 83cdcdac6d..ffd50a3fe2 100644
>> --- a/manual/tunables.texi
>> +++ b/manual/tunables.texi
>> @@ -143,7 +143,7 @@ number of bytes to retain when shrinking any of the arenas.  This provides the
>>   necessary hysteresis in heap size such that excessive amounts of system calls
>>   can be avoided.
>>   -The default value of this tunable is @samp{0}.
>> +The default value of this tunable is @samp{131072} (128 KB).
>
> I don't know how much glibc uses KB vs KiB, but I'd like to be
> pedantic here and suggest using KiB (maybe this requires a global
> change to keep consistency).  See units(7).

I checked the chapter used KB, but not KiB before this change, so I kept
using KB.  Maybe we should change all occurrences to KiB.

Thanks,
Florian
diff mbox series

Patch

diff --git a/elf/dl-tunables.list b/elf/dl-tunables.list
index e6a56b3070..e925590c36 100644
--- a/elf/dl-tunables.list
+++ b/elf/dl-tunables.list
@@ -42,6 +42,7 @@  glibc {
       type: SIZE_T
       env_alias: MALLOC_TOP_PAD_
       security_level: SXID_IGNORE
+      default: 131072
     }
     perturb {
       type: INT_32
diff --git a/elf/tst-rtld-list-tunables.exp b/elf/tst-rtld-list-tunables.exp
index cdfdb56a94..2233ea9c7c 100644
--- a/elf/tst-rtld-list-tunables.exp
+++ b/elf/tst-rtld-list-tunables.exp
@@ -9,7 +9,7 @@  glibc.malloc.perturb: 0 (min: 0, max: 255)
 glibc.malloc.tcache_count: 0x0 (min: 0x0, max: 0x[f]+)
 glibc.malloc.tcache_max: 0x0 (min: 0x0, max: 0x[f]+)
 glibc.malloc.tcache_unsorted_limit: 0x0 (min: 0x0, max: 0x[f]+)
-glibc.malloc.top_pad: 0x0 (min: 0x0, max: 0x[f]+)
+glibc.malloc.top_pad: 0x20000 (min: 0x0, max: 0x[f]+)
 glibc.malloc.trim_threshold: 0x0 (min: 0x0, max: 0x[f]+)
 glibc.rtld.dynamic_sort: 2 (min: 1, max: 2)
 glibc.rtld.nns: 0x4 (min: 0x1, max: 0x10)
diff --git a/manual/tunables.texi b/manual/tunables.texi
index 83cdcdac6d..ffd50a3fe2 100644
--- a/manual/tunables.texi
+++ b/manual/tunables.texi
@@ -143,7 +143,7 @@  number of bytes to retain when shrinking any of the arenas.  This provides the
 necessary hysteresis in heap size such that excessive amounts of system calls
 can be avoided.
 
-The default value of this tunable is @samp{0}.
+The default value of this tunable is @samp{131072} (128 KB).
 @end deftp
 
 @deftp Tunable glibc.malloc.perturb