Re: [PATCH v6 4/5] elf: Don't cache symbol nor relocation tables with mmap

[Sam James <sam@gentoo.org>]

"H.J. Lu" <hjl.tools@gmail.com> writes:

> On Sat, Mar 9, 2024 at 4:44 PM Sam James <sam@gentoo.org> wrote:
>>
>> "H.J. Lu" <hjl.tools@gmail.com> writes:
>>
>> > During a "-j 8" LLVM 17 debug build on a machine with 32GB RAM and 16GB
>> > swap, ld was killed by kernel because of out of memory:
>> >
>> > [79437.949336] oom-kill:constraint=CONSTRAINT_NONE,nodemask=(null),cpuset=/,mems_allowed=0,global_oom,task_memcg=/user.slice/user-1000.slice/session-9.scope,task=ld,pid=797431,uid=1000
>> > [79437.949349] Out of memory: Killed process 797431 (ld) total-vm:9219600kB, anon-rss:6558156kB, file-rss:1792kB, shmem-rss:0kB, UID:1000 pgtables:17552kB oom_score_adj:0
>> >
>> > Don't cache symbol nor relocation tables if they are mapped in.  Data to
>> > link the 3.5GB clang executable in LLVM 17 debug build on Linux/x86-64
>> > with 32GB RAM is:
>> >
>> >               stdio           mmap            improvement
>> > user          86.73           87.02           -0.3%
>> > system                9.55            9.21            3.6%
>> > total         100.40          97.66           0.7%
>> > maximum set(GB)       17.34           13.14           24%
>> > page faults   4047667         3042877         25%
>> >
>> > and data to link the 275M cc1plus executable in GCC 14 stage 1 build is:
>> >
>> > user          5.41            5.44            -0.5%
>> > system                0.80            0.76            5%
>> > total         6.25            6.26            -0.2%
>> > maximum set(MB)       1323            968             27%
>> > page faults   323451          236371          27%
>> >
>> > These improve the overall system performance for parallel build by
>> > reducing memory usage and page faults.
>> >
>> > Also rename _bfd_link_keep_memory to _bfd_elf_link_keep_memory.  Since
>> > the --no-keep-memory linker option causes:
>> >
>> > https://sourceware.org/bugzilla/show_bug.cgi?id=31458
>> >
>> > this is opt-in by each backend.  Add the --keep-memory linker option to
>> > always cache the symbol and relocation tables to mitigate --no-keep-memory
>> > bugs like:
>> >
>> > https://sourceware.org/bugzilla/show_bug.cgi?id=31466
>> >
>> > bfd/
>> >
>> >       * elf-bfd.h (_bfd_elf_link_keep_memory): New.
>> >       * elf32-i386.c (elf_i386_scan_relocs): Replace
>> >       _bfd_link_keep_memory with _bfd_elf_link_keep_memory.
>> >       * elf64-x86-64.c (elf_x86_64_scan_relocs): Likewise.
>> >       * elflink.c (_bfd_elf_link_iterate_on_relocs): Likewise.
>> >       (elf_link_add_object_symbols): Likewise.
>> >       (init_reloc_cookie): Likewise.
>> >       (init_reloc_cookie_rels): Likewise.
>> >       (_bfd_elf_link_keep_memory): New.
>> >       * libbfd-in.h (_bfd_link_keep_memory): Removed.
>> >       * linker.c (_bfd_link_keep_memory): Likewise.
>> >       * libbfd.h: Regenerated.
>> >
>> > include/
>> >
>> >       * bfdlink.h (bfd_link_info): Add keep_memory_is_set.
>> >
>> > ld/
>> >
>> >       * NEWS: Mention --keep-memory.
>> >       * ld.texi: Document --keep-memory.
>> >       * ldlex.h (option_values): Add OPTION_KEEP_MEMORY.
>> >       * lexsup.c (ld_options): Add --keep-memory.
>> >       (parse_args): Handle OPTION_KEEP_MEMORY and set
>> >       link_info.keep_memory_is_set for OPTION_NO_KEEP_MEMORY.
>> > ---
>> >  bfd/elf-bfd.h      |  3 ++
>> >  bfd/elf32-i386.c   |  2 +-
>> >  bfd/elf64-x86-64.c |  2 +-
>> >  bfd/elflink.c      | 72 ++++++++++++++++++++++++++++++++++++++--------
>> >  bfd/libbfd-in.h    |  3 --
>> >  bfd/libbfd.h       |  3 --
>> >  bfd/linker.c       | 35 ----------------------
>> >  include/bfdlink.h  |  3 ++
>> >  ld/NEWS            |  2 ++
>> >  ld/ld.texi         | 14 +++++----
>> >  ld/ldlex.h         |  3 +-
>> >  ld/lexsup.c        | 11 +++++--
>> >  12 files changed, 90 insertions(+), 63 deletions(-)
>> >
>> > diff --git a/bfd/elf-bfd.h b/bfd/elf-bfd.h
>> > index e7c2cd19bed..b9ec590cd4e 100644
>> > --- a/bfd/elf-bfd.h
>> > +++ b/bfd/elf-bfd.h
>> > @@ -3144,6 +3144,9 @@ extern bool _bfd_elf_mmap_section_contents
>> >  extern void _bfd_elf_munmap_section_contents
>> >    (asection *, void *);
>> >
>> > +extern bool _bfd_elf_link_keep_memory
>> > +  (struct bfd_link_info *);
>> > +
>> >  /* Large common section.  */
>> >  extern asection _bfd_elf_large_com_section;
>> >
>> > diff --git a/bfd/elf32-i386.c b/bfd/elf32-i386.c
>> > index 0f963c373be..d1bd11094de 100644
>> > --- a/bfd/elf32-i386.c
>> > +++ b/bfd/elf32-i386.c
>> > @@ -1932,7 +1932,7 @@ elf_i386_scan_relocs (bfd *abfd,
>> >
>> >    if (elf_section_data (sec)->this_hdr.contents != contents)
>> >      {
>> > -      if (!converted && !_bfd_link_keep_memory (info))
>> > +      if (!converted && !_bfd_elf_link_keep_memory (info))
>> >       _bfd_elf_munmap_section_contents (sec, contents);
>> >        else
>> >       {
>> > diff --git a/bfd/elf64-x86-64.c b/bfd/elf64-x86-64.c
>> > index 2e3f86840e5..5e1fad069d1 100644
>> > --- a/bfd/elf64-x86-64.c
>> > +++ b/bfd/elf64-x86-64.c
>> > @@ -2590,7 +2590,7 @@ elf_x86_64_scan_relocs (bfd *abfd, struct bfd_link_info *info,
>> >
>> >    if (elf_section_data (sec)->this_hdr.contents != contents)
>> >      {
>> > -      if (!converted && !_bfd_link_keep_memory (info))
>> > +      if (!converted && !_bfd_elf_link_keep_memory (info))
>> >       _bfd_elf_munmap_section_contents (sec, contents);
>> >        else
>> >       {
>> > diff --git a/bfd/elflink.c b/bfd/elflink.c
>> > index 2a0586722e8..e5f2c75cb74 100644
>> > --- a/bfd/elflink.c
>> > +++ b/bfd/elflink.c
>> > @@ -4205,10 +4205,9 @@ _bfd_elf_link_iterate_on_relocs
>> >             || bfd_is_abs_section (o->output_section))
>> >           continue;
>> >
>> > -       internal_relocs = _bfd_elf_link_info_read_relocs (abfd, info,
>> > -                                                         o, NULL,
>> > -                                                         NULL,
>> > -                                                         _bfd_link_keep_memory (info));
>> > +       internal_relocs = _bfd_elf_link_info_read_relocs
>> > +         (abfd, info, o, NULL, NULL,
>> > +          _bfd_elf_link_keep_memory (info));
>> >         if (internal_relocs == NULL)
>> >           return false;
>> >
>> > @@ -5574,10 +5573,9 @@ elf_link_add_object_symbols (bfd *abfd, struct bfd_link_info *info)
>> >                 && (s->flags & SEC_DEBUGGING) != 0))
>> >           continue;
>> >
>> > -       internal_relocs = _bfd_elf_link_info_read_relocs (abfd, info,
>> > -                                                         s, NULL,
>> > -                                                         NULL,
>> > -                                                         _bfd_link_keep_memory (info));
>> > +       internal_relocs = _bfd_elf_link_info_read_relocs
>> > +         (abfd, info, s, NULL, NULL,
>> > +          _bfd_elf_link_keep_memory (info));
>> >         if (internal_relocs == NULL)
>> >           goto error_free_vers;
>> >
>> > @@ -13630,7 +13628,7 @@ init_reloc_cookie (struct elf_reloc_cookie *cookie,
>> >         info->callbacks->einfo (_("%P%X: can not read symbols: %E\n"));
>> >         return false;
>> >       }
>> > -      if (_bfd_link_keep_memory (info) )
>> > +      if (_bfd_elf_link_keep_memory (info) )
>> >       {
>> >         symtab_hdr->contents = (bfd_byte *) cookie->locsyms;
>> >         info->cache_size += (cookie->locsymcount
>> > @@ -13667,9 +13665,9 @@ init_reloc_cookie_rels (struct elf_reloc_cookie *cookie,
>> >      }
>> >    else
>> >      {
>> > -      cookie->rels = _bfd_elf_link_info_read_relocs (abfd, info, sec,
>> > -                                                  NULL, NULL,
>> > -                                                  _bfd_link_keep_memory (info));
>> > +      cookie->rels = _bfd_elf_link_info_read_relocs
>> > +     (abfd, info, sec, NULL, NULL,
>> > +      _bfd_elf_link_keep_memory (info));
>> >        if (cookie->rels == NULL)
>> >       return false;
>> >        cookie->rel = cookie->rels;
>> > @@ -15599,3 +15597,53 @@ _bfd_elf_add_dynamic_tags (bfd *output_bfd, struct bfd_link_info *info,
>> >
>> >    return true;
>> >  }
>> > +
>> > +/* Return false if linker should avoid caching relocation information
>> > +   and symbol tables of input files in memory.  */
>> > +
>> > +bool
>> > +_bfd_elf_link_keep_memory (struct bfd_link_info *info)
>> > +{
>> > +#ifdef USE_MMAP
>> > +  /* Don't cache symbol nor relocation tables if they are mapped in.
>> > +     NB: Since the --no-keep-memory linker option causes:
>> > +
>> > +     https://sourceware.org/bugzilla/show_bug.cgi?id=31458
>> > +
>> > +     this is opt-in by each backend.  */
>> > +  if (!info->keep_memory_is_set)
>> > +    {
>> > +      const struct elf_backend_data *bed
>> > +     = get_elf_backend_data (info->output_bfd);
>> > +      if (bed->use_mmap)
>> > +     return false;
>> > +    }
>> > +#endif
>> > +  bfd *abfd;
>> > +  bfd_size_type size;
>> > +
>> > +  if (!info->keep_memory)
>> > +    return false;
>> > +
>> > +  if (info->max_cache_size == (bfd_size_type) -1)
>> > +    return true;
>> > +
>> > +  abfd = info->input_bfds;
>> > +  size = info->cache_size;
>> > +  do
>> > +    {
>> > +      if (size >= info->max_cache_size)
>> > +     {
>> > +       /* Over the limit.  Reduce the memory usage.  */
>> > +       info->keep_memory = false;
>> > +       return false;
>> > +     }
>> > +      if (!abfd)
>> > +     break;
>> > +      size += abfd->alloc_size;
>> > +      abfd = abfd->link.next;
>> > +    }
>> > +  while (1);
>> > +
>> > +  return true;
>> > +}
>> > diff --git a/bfd/libbfd-in.h b/bfd/libbfd-in.h
>> > index 9d5396a0354..8ad9d36e396 100644
>> > --- a/bfd/libbfd-in.h
>> > +++ b/bfd/libbfd-in.h
>> > @@ -848,9 +848,6 @@ extern bfd_byte * _bfd_write_unsigned_leb128
>> >  extern struct bfd_link_info *_bfd_get_link_info (bfd *)
>> >    ATTRIBUTE_HIDDEN;
>> >
>> > -extern bool _bfd_link_keep_memory (struct bfd_link_info *)
>> > -  ATTRIBUTE_HIDDEN;
>> > -
>> >  extern uintptr_t _bfd_pagesize ATTRIBUTE_HIDDEN;
>> >  extern uintptr_t _bfd_pagesize_m1 ATTRIBUTE_HIDDEN;
>> >  extern uintptr_t _bfd_minimum_mmap_size ATTRIBUTE_HIDDEN;
>> > diff --git a/bfd/libbfd.h b/bfd/libbfd.h
>> > index 4b605c4ce6c..17a6095ba00 100644
>> > --- a/bfd/libbfd.h
>> > +++ b/bfd/libbfd.h
>> > @@ -854,9 +854,6 @@ extern bfd_byte * _bfd_write_unsigned_leb128
>> >  extern struct bfd_link_info *_bfd_get_link_info (bfd *)
>> >    ATTRIBUTE_HIDDEN;
>> >
>> > -extern bool _bfd_link_keep_memory (struct bfd_link_info *)
>> > -  ATTRIBUTE_HIDDEN;
>> > -
>> >  extern uintptr_t _bfd_pagesize ATTRIBUTE_HIDDEN;
>> >  extern uintptr_t _bfd_pagesize_m1 ATTRIBUTE_HIDDEN;
>> >  extern uintptr_t _bfd_minimum_mmap_size ATTRIBUTE_HIDDEN;
>> > diff --git a/bfd/linker.c b/bfd/linker.c
>> > index 36cca9624c2..eb42a78b622 100644
>> > --- a/bfd/linker.c
>> > +++ b/bfd/linker.c
>> > @@ -3556,38 +3556,3 @@ _bfd_nolink_bfd_define_start_stop (struct bfd_link_info *info ATTRIBUTE_UNUSED,
>> >  {
>> >    return (struct bfd_link_hash_entry *) _bfd_ptr_bfd_null_error (sec->owner);
>> >  }
>> > -
>> > -/* Return false if linker should avoid caching relocation infomation
>> > -   and symbol tables of input files in memory.  */
>> > -
>> > -bool
>> > -_bfd_link_keep_memory (struct bfd_link_info * info)
>> > -{
>> > -  bfd *abfd;
>> > -  bfd_size_type size;
>> > -
>> > -  if (!info->keep_memory)
>> > -    return false;
>> > -
>> > -  if (info->max_cache_size == (bfd_size_type) -1)
>> > -    return true;
>> > -
>> > -  abfd = info->input_bfds;
>> > -  size = info->cache_size;
>> > -  do
>> > -    {
>> > -      if (size >= info->max_cache_size)
>> > -     {
>> > -       /* Over the limit.  Reduce the memory usage.  */
>> > -       info->keep_memory = false;
>> > -       return false;
>> > -     }
>> > -      if (!abfd)
>> > -     break;
>> > -      size += abfd->alloc_size;
>> > -      abfd = abfd->link.next;
>> > -    }
>> > -  while (1);
>> > -
>> > -  return true;
>> > -}
>> > diff --git a/include/bfdlink.h b/include/bfdlink.h
>> > index eac07d78364..9397032c6dd 100644
>> > --- a/include/bfdlink.h
>> > +++ b/include/bfdlink.h
>> > @@ -580,6 +580,9 @@ struct bfd_link_info
>> >    /* TRUE if commonpagesize is set on command-line.  */
>> >    unsigned int commonpagesize_is_set : 1;
>> >
>> > +  /* TRUE if keep_memory is set on command-line.  */
>> > +  unsigned int keep_memory_is_set : 1;
>> > +
>> >    /* Char that may appear as the first char of a symbol, but should be
>> >       skipped (like symbol_leading_char) when looking up symbols in
>> >       wrap_hash.  Used by PowerPC Linux for 'dot' symbols.  */
>> > diff --git a/ld/NEWS b/ld/NEWS
>> > index f70d2157339..8751b177329 100644
>> > --- a/ld/NEWS
>> > +++ b/ld/NEWS
>> > @@ -1,5 +1,7 @@
>> >  -*- text -*-
>> >
>> > +* Add --keep-memory to always cache the symbol and relocation tables.
>> > +
>> >  * Add -plugin-save-temps to store plugin intermediate files permanently.
>> >
>> >  Changes in 2.42:
>> > diff --git a/ld/ld.texi b/ld/ld.texi
>> > index 6f234752278..792fdf1c2f3 100644
>> > --- a/ld/ld.texi
>> > +++ b/ld/ld.texi
>> > @@ -2140,13 +2140,17 @@ If the map file already exists then it will be overwritten by this
>> >  operation.
>> >
>> >  @cindex memory usage
>> > +@kindex --keep-memory
>> >  @kindex --no-keep-memory
>> > -@item --no-keep-memory
>> > +@item --keep-memory
>> >  @command{ld} normally optimizes for speed over memory usage by caching the
>> > -symbol tables of input files in memory.  This option tells @command{ld} to
>> > -instead optimize for memory usage, by rereading the symbol tables as
>> > -necessary.  This may be required if @command{ld} runs out of memory space
>> > -while linking a large executable.
>> > +symbol and relocation tables of input files in memory if @samp{mmap} isn't
>> > +used to map them in.  The @option{--no-keep-memory} option tells @command{ld}
>> > +to instead optimize for memory usage, by rereading the symbol and
>> > +relocation tables as necessary.  This may be required if @command{ld}
>> > +runs out of memory space while linking a large executable.
>> > +The @option{--keep-memory} option tells @command{ld} to always cache the
>> > +symbol and relocation tables.
>>
>> Sorry, one more thing: this is tri-state now, right?
>>
>> i.e. there's three different behaviours:
>> 1) --keep-memory -> we cache more
>
> Correct.
>
>> 2) nothing/default -> old behaviour
>
> Default depends on use_mmap.   If use_mmap is true,
> it is the same as --no-keep-memory.   Otherwise, it
> is the same as --keep-memory.  For x86, the default is
> --no-keep-memory.

Oh, duh. OK, thanks, I'm happy then.

>
>> 3) --no-keep-memory -> we cache less, as before
>
> Yes.
>
>> If that's right, maybe explicitly say "The default is a compromise
>> between explicit --no-keep-memory and --keep-memory. --keep-memory
>> behavior is different." or similar?
>>
>> Right now, it might sound like --keep-memory does the same as not
>> passing it at all, unless overridden later.
>>
>> >
>> >  @kindex --no-undefined
>> >  @kindex -z defs
>> > diff --git a/ld/ldlex.h b/ld/ldlex.h
>> > index d575562a357..49ac8552e1f 100644
>> > --- a/ld/ldlex.h
>> > +++ b/ld/ldlex.h
>> > @@ -43,9 +43,10 @@ enum option_values
>> >    OPTION_NO_EXPORT_DYNAMIC,
>> >    OPTION_HELP,
>> >    OPTION_IGNORE,
>> > +  OPTION_KEEP_MEMORY,
>> > +  OPTION_NO_KEEP_MEMORY,
>> >    OPTION_MAP,
>> >    OPTION_NO_DEMANGLE,
>> > -  OPTION_NO_KEEP_MEMORY,
>> >    OPTION_NO_WARN_MISMATCH,
>> >    OPTION_NO_WARN_SEARCH_MISMATCH,
>> >    OPTION_NOINHIBIT_EXEC,
>> > diff --git a/ld/lexsup.c b/ld/lexsup.c
>> > index dad3b6059ed..96d37ca3093 100644
>> > --- a/ld/lexsup.c
>> > +++ b/ld/lexsup.c
>> > @@ -383,14 +383,16 @@ static const struct ld_option ld_options[] =
>> >      '\0', NULL, N_("Print option help"), TWO_DASHES },
>> >    { {"init", required_argument, NULL, OPTION_INIT},
>> >      '\0', N_("SYMBOL"), N_("Call SYMBOL at load-time"), ONE_DASH },
>> > +  { {"keep-memory", no_argument, NULL, OPTION_KEEP_MEMORY},
>> > +    '\0', NULL, N_("Use more memory and less disk I/O"), TWO_DASHES },
>> > +  { {"no-keep-memory", no_argument, NULL, OPTION_NO_KEEP_MEMORY},
>> > +    '\0', NULL, N_("Use less memory and more disk I/O"), TWO_DASHES },
>> >    { {"Map", required_argument, NULL, OPTION_MAP},
>> >      '\0', N_("FILE/DIR"), N_("Write a linker map to FILE or DIR/<outputname>.map"), ONE_DASH },
>> >    { {"no-define-common", no_argument, NULL, OPTION_NO_DEFINE_COMMON},
>> >      '\0', NULL, N_("Do not define Common storage"), TWO_DASHES },
>> >    { {"no-demangle", no_argument, NULL, OPTION_NO_DEMANGLE },
>> >      '\0', NULL, N_("Do not demangle symbol names"), TWO_DASHES },
>> > -  { {"no-keep-memory", no_argument, NULL, OPTION_NO_KEEP_MEMORY},
>> > -    '\0', NULL, N_("Use less memory and more disk I/O"), TWO_DASHES },
>> >    { {"no-undefined", no_argument, NULL, OPTION_NO_UNDEFINED},
>> >      '\0', NULL, N_("Do not allow unresolved references in object files"),
>> >      TWO_DASHES },
>> > @@ -1091,8 +1093,13 @@ parse_args (unsigned argc, char **argv)
>> >       case OPTION_NO_PRINT_GC_SECTIONS:
>> >         link_info.print_gc_sections = false;
>> >         break;
>> > +     case OPTION_KEEP_MEMORY:
>> > +       link_info.keep_memory = true;
>> > +       link_info.keep_memory_is_set = true;
>> > +       break;
>> >       case OPTION_NO_KEEP_MEMORY:
>> >         link_info.keep_memory = false;
>> > +       link_info.keep_memory_is_set = true;
>> >         break;
>> >       case OPTION_NO_UNDEFINED:
>> >         link_info.unresolved_syms_in_objects = RM_DIAGNOSE;