[2/2] malloc: Use __libc_type to detect an inner libc

Message ID e661c73438b37a7c931089e6ba0421a5711d468d.1605283657.git.fweimer@redhat.com
State Superseded
Headers
Series [1/2] Replace __libc_multiple_libcs with tri-state __libc_type |

Commit Message

Florian Weimer Nov. 13, 2020, 4:14 p.m. UTC
  The inner libc must not use sbrk.  _dl_addr occasionally shows up
in profiles, but had to be used before because __libc_multiple_libs
was unreliable.
---
 malloc/arena.c  | 7 +------
 malloc/malloc.c | 2 ++
 2 files changed, 3 insertions(+), 6 deletions(-)
  

Comments

Adhemerval Zanella Nov. 13, 2020, 6:21 p.m. UTC | #1
On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
> The inner libc must not use sbrk.  _dl_addr occasionally shows up
> in profiles, but had to be used before because __libc_multiple_libs
> was unreliable.

LGTM.  Is the performance the only advantage of using sbrk on 
the inner libc? 

> ---
>  malloc/arena.c  | 7 +------
>  malloc/malloc.c | 2 ++
>  2 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/malloc/arena.c b/malloc/arena.c
> index 202daf15b0..865eaa1a84 100644
> --- a/malloc/arena.c
> +++ b/malloc/arena.c
> @@ -296,12 +296,7 @@ ptmalloc_init (void)
>  #ifdef SHARED
>    /* In case this libc copy is in a non-default namespace, never use brk.
>       Likewise if dlopened from statically linked program.  */
> -  Dl_info di;
> -  struct link_map *l;
> -
> -  if (_dl_open_hook != NULL
> -      || (_dl_addr (ptmalloc_init, &di, &l, NULL) != 0
> -          && l->l_ns != LM_ID_BASE))
> +  if (__libc_type == libc_type_secondary)
>      __morecore = __failing_morecore;
>  #endif
>  
> diff --git a/malloc/malloc.c b/malloc/malloc.c
> index 5b87bdb081..0834e4181a 100644
> --- a/malloc/malloc.c
> +++ b/malloc/malloc.c
> @@ -247,6 +247,8 @@
>  /* For SINGLE_THREAD_P.  */
>  #include <sysdep-cancel.h>
>  
> +#include <libc-internal.h>
> +
>  /*
>    Debugging:
>  
>
  
Florian Weimer Nov. 13, 2020, 7:05 p.m. UTC | #2
* Adhemerval Zanella via Libc-alpha:

> On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
>> The inner libc must not use sbrk.  _dl_addr occasionally shows up
>> in profiles, but had to be used before because __libc_multiple_libs
>> was unreliable.
>
> LGTM.  Is the performance the only advantage of using sbrk on 
> the inner libc?

Avoiding sbrk in the inner libc is required for correctness because sbrk
is not thread-safe, and there is no external synchronization.

The startup performance improvement comes from not calling _dl_addr.  On
POWER, it saves around 150,000 instructions, according to a quick test.

Thanks,
Florian
  
Adhemerval Zanella Nov. 16, 2020, 1:18 p.m. UTC | #3
On 13/11/2020 16:05, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
>>> The inner libc must not use sbrk.  _dl_addr occasionally shows up
>>> in profiles, but had to be used before because __libc_multiple_libs
>>> was unreliable.
>>
>> LGTM.  Is the performance the only advantage of using sbrk on 
>> the inner libc?
> 
> Avoiding sbrk in the inner libc is required for correctness because sbrk
> is not thread-safe, and there is no external synchronization.
> 
> The startup performance improvement comes from not calling _dl_addr.  On
> POWER, it saves around 150,000 instructions, according to a quick test.

In fact I meant what is the gain of using sbrk on outer libc.  I am asking
because maybe an option would be to use the same strategy regardless whether
it is a inner or outer libc.
  
Florian Weimer Nov. 24, 2020, 11:23 a.m. UTC | #4
* Adhemerval Zanella:

> On 13/11/2020 16:05, Florian Weimer wrote:
>> * Adhemerval Zanella via Libc-alpha:
>> 
>>> On 13/11/2020 13:14, Florian Weimer via Libc-alpha wrote:
>>>> The inner libc must not use sbrk.  _dl_addr occasionally shows up
>>>> in profiles, but had to be used before because __libc_multiple_libs
>>>> was unreliable.
>>>
>>> LGTM.  Is the performance the only advantage of using sbrk on 
>>> the inner libc?
>> 
>> Avoiding sbrk in the inner libc is required for correctness because sbrk
>> is not thread-safe, and there is no external synchronization.
>> 
>> The startup performance improvement comes from not calling _dl_addr.  On
>> POWER, it saves around 150,000 instructions, according to a quick test.
>
> In fact I meant what is the gain of using sbrk on outer libc.  I am asking
> because maybe an option would be to use the same strategy regardless whether
> it is a inner or outer libc.

Oh, that's totally unclear.  The main arena has a very different layout
from the other arenas.

The __morecore hook exposes the inner sbrk-based workings of our malloc
as part of the ABI.  I'm not sure if we can restructure the code so that
__morecore is never called, or if that would break existing users (such
as libhugetlbfs).

Thanks,
Florian
  

Patch

diff --git a/malloc/arena.c b/malloc/arena.c
index 202daf15b0..865eaa1a84 100644
--- a/malloc/arena.c
+++ b/malloc/arena.c
@@ -296,12 +296,7 @@  ptmalloc_init (void)
 #ifdef SHARED
   /* In case this libc copy is in a non-default namespace, never use brk.
      Likewise if dlopened from statically linked program.  */
-  Dl_info di;
-  struct link_map *l;
-
-  if (_dl_open_hook != NULL
-      || (_dl_addr (ptmalloc_init, &di, &l, NULL) != 0
-          && l->l_ns != LM_ID_BASE))
+  if (__libc_type == libc_type_secondary)
     __morecore = __failing_morecore;
 #endif
 
diff --git a/malloc/malloc.c b/malloc/malloc.c
index 5b87bdb081..0834e4181a 100644
--- a/malloc/malloc.c
+++ b/malloc/malloc.c
@@ -247,6 +247,8 @@ 
 /* For SINGLE_THREAD_P.  */
 #include <sysdep-cancel.h>
 
+#include <libc-internal.h>
+
 /*
   Debugging: