Re: [PATCH v4 5/6] elf: Use mmap to map in symbol and relocation tables

["H.J. Lu" <hjl.tools@gmail.com>]

On Fri, Mar 8, 2024 at 7:36 AM H.J. Lu <hjl.tools@gmail.com> wrote:
>
> On Thu, Mar 7, 2024 at 4:42 PM Alan Modra <amodra@gmail.com> wrote:
> >
> > On Wed, Mar 06, 2024 at 03:24:00PM -0800, H.J. Lu wrote:
> > > Add _bfd_mmap_read_untracked to mmap in symbol tables and relocations
> > > whose sizes >= 4 * page size.  Don't cache external relocations when
> > > mmap is used.
> > >
> > > When mmap is used to map in all ELF sections, 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          84.28           85.04           -0.9%
> > > system                12.46           10.16           14%
> > > total         96              95.35           0.7%
> > > page faults   4837944         4047667         16%
> > >
> > > and data to link the 275M cc1plus executable in GCC 14 stage 1 build
> > > is:
> > >
> > > user          5.22            5.27            -1%
> > > system                0.94            0.84            11%
> > > total         6.20            6.13            0.7%
> > > page faults   361272          323377          10%
> > >
> > >       * elf.c (bfd_elf_get_elf_syms): Replace bfd_read with
> > >       _bfd_mmap_read_untracked.
> > >       * elflink.c (elf_link_read_relocs_from_section): Add 2 arguments
> > >       to return mmap memory address and size.
> > >       (_bfd_elf_link_info_read_relocs); Replace bfd_read with
> > >       _bfd_mmap_read_untracked.
> > >       (bfd_elf_final_link): Don't cache external relocations when mmap
> > >       is used.
> > >       * libbfd.c (_bfd_mmap_read_untracked ): New.
> > >       * libbfd-in.h (_bfd_mmap_read_untracked): Likewise.
> > >       * libbfd.h: Regenerated.
> > > ---
> > >  bfd/elf.c       | 47 +++++++++++++++++++++++++++++++++++------------
> > >  bfd/elflink.c   | 44 ++++++++++++++++++++++++++++++++------------
> > >  bfd/libbfd-in.h |  3 +++
> > >  bfd/libbfd.c    | 33 +++++++++++++++++++++++++++++++++
> > >  bfd/libbfd.h    |  3 +++
> > >  5 files changed, 106 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/bfd/elf.c b/bfd/elf.c
> > > index e2e31a93950..7427dca2ba0 100644
> > > --- a/bfd/elf.c
> > > +++ b/bfd/elf.c
> > > @@ -464,19 +464,30 @@ bfd_elf_get_elf_syms (bfd *ibfd,
> > >        goto out;
> > >      }
> > >    pos = symtab_hdr->sh_offset + symoffset * extsym_size;
> > > +  size_t alloc_ext_size = amt;
> > >    if (extsym_buf == NULL)
> > >      {
> > > -      alloc_ext = bfd_malloc (amt);
> > > -      extsym_buf = alloc_ext;
> > > +#ifdef USE_MMAP
> > > +      if ((ibfd->flags & BFD_PLUGIN) != 0
> > > +       || amt < _bfd_minimum_mmap_size)
> > > +     {
> > > +#endif
> > > +       alloc_ext = bfd_malloc (amt);
> > > +       extsym_buf = alloc_ext;
> > > +#ifdef USE_MMAP
> > > +     }
> > > +#endif
> > >      }
> > > -  if (extsym_buf == NULL
> > > -      || bfd_seek (ibfd, pos, SEEK_SET) != 0
> > > -      || bfd_read (extsym_buf, amt, ibfd) != amt)
> > > +
> > > +  if (bfd_seek (ibfd, pos, SEEK_SET) != 0
> > > +      || !_bfd_mmap_read_untracked (&extsym_buf, &alloc_ext_size,
> > > +                                 &alloc_ext, ibfd))
> > >      {
> > >        intsym_buf = NULL;
> > >        goto out;
> > >      }
> > >
> > > +  size_t alloc_extshndx_size = 0;
> > >    if (shndx_hdr == NULL || shndx_hdr->sh_size == 0)
> > >      extshndx_buf = NULL;
> > >    else
> > > @@ -487,15 +498,27 @@ bfd_elf_get_elf_syms (bfd *ibfd,
> > >         intsym_buf = NULL;
> > >         goto out;
> > >       }
> > > +      alloc_extshndx_size = amt;
> > >        pos = shndx_hdr->sh_offset + symoffset * sizeof (Elf_External_Sym_Shndx);
> > >        if (extshndx_buf == NULL)
> > >       {
> > > -       alloc_extshndx = (Elf_External_Sym_Shndx *) bfd_malloc (amt);
> > > -       extshndx_buf = alloc_extshndx;
> > > +#ifdef USE_MMAP
> > > +       if ((ibfd->flags & BFD_PLUGIN) != 0
> > > +           || amt < _bfd_minimum_mmap_size)
> > > +         {
> > > +#endif
> > > +           alloc_extshndx
> > > +             = (Elf_External_Sym_Shndx *) bfd_malloc (amt);
> > > +           extshndx_buf = alloc_extshndx;
> > > +#ifdef USE_MMAP
> > > +         }
> > > +#endif
> > >       }
> > > -      if (extshndx_buf == NULL
> > > -       || bfd_seek (ibfd, pos, SEEK_SET) != 0
> > > -       || bfd_read (extshndx_buf, amt, ibfd) != amt)
> > > +      if (bfd_seek (ibfd, pos, SEEK_SET) != 0
> > > +       || !_bfd_mmap_read_untracked ((void **) &extshndx_buf,
> > > +                                     &alloc_extshndx_size,
> > > +                                     (void **) &alloc_extshndx,
> > > +                                     ibfd))
> > >       {
> > >         intsym_buf = NULL;
> > >         goto out;
> > > @@ -534,8 +557,8 @@ bfd_elf_get_elf_syms (bfd *ibfd,
> > >        }
> > >
> > >   out:
> > > -  free (alloc_ext);
> > > -  free (alloc_extshndx);
> > > +  _bfd_munmap_readonly_untracked (alloc_ext, alloc_ext_size);
> > > +  _bfd_munmap_readonly_untracked (alloc_extshndx, alloc_extshndx_size);
> > >
> > >    return intsym_buf;
> > >  }
> > > diff --git a/bfd/elflink.c b/bfd/elflink.c
> > > index 47fb890f94f..4602fb3d10f 100644
> > > --- a/bfd/elflink.c
> > > +++ b/bfd/elflink.c
> > > @@ -2644,8 +2644,11 @@ _bfd_elf_link_assign_sym_version (struct elf_link_hash_entry *h, void *data)
> > >     may be either a REL or a RELA section.  The relocations are
> > >     translated into RELA relocations and stored in INTERNAL_RELOCS,
> > >     which should have already been allocated to contain enough space.
> > > -   The EXTERNAL_RELOCS are a buffer where the external form of the
> > > -   relocations should be stored.
> > > +   The *EXTERNAL_RELOCS_P are a buffer where the external form of the
> > > +   relocations should be stored.  If *EXTERNAL_RELOCS_P is NULL,
> > > +   *EXTERNAL_RELOCS_P and *EXTERNAL_RELOCS_SIZE_P returns the mmap
> > > +   memory address and size.  Otherwise, *EXTERNAL_RELOCS_SIZE_P is
> > > +   unchanged and EXTERNAL_RELOCS_SIZE_P returns 0.
> >
> > Comment doesn't match function params.
>
> Fixed.
>
> > >     Returns FALSE if something goes wrong.  */
> > >
> > > @@ -2653,7 +2656,8 @@ static bool
> > >  elf_link_read_relocs_from_section (bfd *abfd,
> > >                                  asection *sec,
> > >                                  Elf_Internal_Shdr *shdr,
> > > -                                void *external_relocs,
> > > +                                void **external_relocs_addr,
> > > +                                size_t *external_relocs_size_addr,
> >
> > I think external_relocs_size is a better name.  Why the _addr suffix?
>
> Fixed.
>
> > >                                  Elf_Internal_Rela *internal_relocs)
> > >  {
> > >    const struct elf_backend_data *bed;
> > > @@ -2663,13 +2667,17 @@ elf_link_read_relocs_from_section (bfd *abfd,
> > >    Elf_Internal_Rela *irela;
> > >    Elf_Internal_Shdr *symtab_hdr;
> > >    size_t nsyms;
> > > +  void *external_relocs = *external_relocs_addr;
> > >
> > >    /* Position ourselves at the start of the section.  */
> > >    if (bfd_seek (abfd, shdr->sh_offset, SEEK_SET) != 0)
> > >      return false;
> > >
> > >    /* Read the relocations.  */
> > > -  if (bfd_read (external_relocs, shdr->sh_size, abfd) != shdr->sh_size)
> > > +  *external_relocs_size_addr = shdr->sh_size;
> > > +  if (!_bfd_mmap_read_untracked (&external_relocs,
> > > +                              external_relocs_size_addr,
> > > +                              external_relocs_addr, abfd))
> > >      return false;
> > >
> > >    symtab_hdr = &elf_tdata (abfd)->symtab_hdr;
> > > @@ -2754,6 +2762,7 @@ _bfd_elf_link_info_read_relocs (bfd *abfd,
> > >                               bool keep_memory)
> > >  {
> > >    void *alloc1 = NULL;
> > > +  size_t alloc1_size;
> > >    Elf_Internal_Rela *alloc2 = NULL;
> > >    const struct elf_backend_data *bed = get_elf_backend_data (abfd);
> > >    struct bfd_elf_section_data *esdo = elf_section_data (o);
> > > @@ -2791,17 +2800,26 @@ _bfd_elf_link_info_read_relocs (bfd *abfd,
> > >        if (esdo->rela.hdr)
> > >       size += esdo->rela.hdr->sh_size;
> > >
> > > -      alloc1 = bfd_malloc (size);
> > > -      if (alloc1 == NULL)
> > > -     goto error_return;
> > > -      external_relocs = alloc1;
> > > +#ifdef USE_MMAP
> > > +      if (size < _bfd_minimum_mmap_size)
> > > +     {
> > > +#endif
> > > +       alloc1 = bfd_malloc (size);
> > > +       if (alloc1 == NULL)
> > > +         goto error_return;
> > > +       external_relocs = alloc1;
> > > +#ifdef USE_MMAP
> > > +     }
> > > +#endif
> > >      }
> > > +  else
> > > +    alloc1 = external_relocs;
> > >
> > >    internal_rela_relocs = internal_relocs;
> > >    if (esdo->rel.hdr)
> > >      {
> > >        if (!elf_link_read_relocs_from_section (abfd, o, esdo->rel.hdr,
> > > -                                           external_relocs,
> > > +                                           &alloc1, &alloc1_size,
> > >                                             internal_relocs))
> > >       goto error_return;
> > >        external_relocs = (((bfd_byte *) external_relocs)
> > > @@ -2812,7 +2830,7 @@ _bfd_elf_link_info_read_relocs (bfd *abfd,
> > >
> > >    if (esdo->rela.hdr
> > >        && (!elf_link_read_relocs_from_section (abfd, o, esdo->rela.hdr,
> > > -                                           external_relocs,
> > > +                                           &alloc1, &alloc1_size,
> > >                                             internal_rela_relocs)))
> > >      goto error_return;
> > >
> > > @@ -2820,7 +2838,7 @@ _bfd_elf_link_info_read_relocs (bfd *abfd,
> > >    if (keep_memory)
> > >      esdo->relocs = internal_relocs;
> > >
> > > -  free (alloc1);
> > > +  _bfd_munmap_readonly_untracked (alloc1, alloc1_size);
> > >
> > >    /* Don't free alloc2, since if it was allocated we are passing it
> > >       back (under the name of internal_relocs).  */
> > > @@ -2828,7 +2846,7 @@ _bfd_elf_link_info_read_relocs (bfd *abfd,
> > >    return internal_relocs;
> > >
> > >   error_return:
> > > -  free (alloc1);
> > > +  _bfd_munmap_readonly_untracked (alloc1, alloc1_size);
> > >    if (alloc2 != NULL)
> > >      {
> > >        if (keep_memory)
> > > @@ -12741,12 +12759,14 @@ bfd_elf_final_link (bfd *abfd, struct bfd_link_info *info)
> > >       goto error_return;
> > >      }
> > >
> > > +#ifndef USE_MMAP
> > >    if (max_external_reloc_size != 0)
> > >      {
> > >        flinfo.external_relocs = bfd_malloc (max_external_reloc_size);
> > >        if (flinfo.external_relocs == NULL)
> > >       goto error_return;
> > >      }
> > > +#endif
> > >
> > >    if (max_internal_reloc_count != 0)
> > >      {
> > > diff --git a/bfd/libbfd-in.h b/bfd/libbfd-in.h
> > > index 7887fad9c92..effe1b86b53 100644
> > > --- a/bfd/libbfd-in.h
> > > +++ b/bfd/libbfd-in.h
> > > @@ -905,6 +905,9 @@ extern void _bfd_munmap_readonly_untracked
> > >  #define _bfd_munmap_readonly_untracked(ptr, rsize) free (ptr)
> > >  #endif
> > >
> > > +extern bool _bfd_mmap_read_untracked
> > > +  (void **, size_t *, void **, bfd *) ATTRIBUTE_HIDDEN;
> > > +
> > >  static inline void *
> > >  _bfd_malloc_and_read (bfd *abfd, bfd_size_type asize, bfd_size_type rsize)
> > >  {
> > > diff --git a/bfd/libbfd.c b/bfd/libbfd.c
> > > index c847c4f0180..0a31f113999 100644
> > > --- a/bfd/libbfd.c
> > > +++ b/bfd/libbfd.c
> > > @@ -1180,6 +1180,39 @@ _bfd_mmap_readonly_tracked (bfd *abfd, size_t rsize)
> > >  }
> > >  #endif
> > >
> > > +/* Attempt to read *SIZE_ADDR bytes from ABFD's iostream to *PTR_P.
> > > +   Return true if the full the amount has been read.  If *PTR_P is
> > > +   NULL, mmap should be used, return the memory address at the
> > > +   current offset in *PTR_P as well as return mmap address and size
> > > +   in *PTR_ADDR and *SIZE_ADDR.  Otherwise, return NULL in *PTR_ADDR
> > > +   and 0 in *SIZE_ADDR.  */
> > > +
> > > +bool
> > > +_bfd_mmap_read_untracked (void **ptr_p, size_t *size_addr,
> > > +                       void **ptr_addr, bfd *abfd)
> >
> > I find these parameter names confusing.  Perhaps
> > void **data, size_t *size, void **mmap_base, bfd *abfd
>
> Fixed.
>
> > > +{
> > > +  void *ptr = *ptr_p;
> > > +  size_t size = *size_addr;
> > > +
> > > +#ifdef USE_MMAP
> > > +  if (ptr == NULL)
> > > +    {
> > > +      ptr = _bfd_mmap_readonly_untracked (abfd, size, ptr_addr,
> > > +                                       size_addr);
> > > +      if (ptr == NULL || ptr == (void *) -1)
> > > +     abort ();
> > > +      *ptr_p = ptr;
> > > +      return true;
> > > +    }
> > > +  else
> > > +#endif
> > > +    {
> > > +      *ptr_addr = NULL;
> > > +      *size_addr = 0;
> > > +      return bfd_read (ptr, size, abfd) == size;
> > > +    }
> > > +}
> > > +
> > >  /* Default implementation */
> > >
> > >  bool
> > > diff --git a/bfd/libbfd.h b/bfd/libbfd.h
> > > index 1515a03b093..c80f5a86ed1 100644
> > > --- a/bfd/libbfd.h
> > > +++ b/bfd/libbfd.h
> > > @@ -911,6 +911,9 @@ extern void _bfd_munmap_readonly_untracked
> > >  #define _bfd_munmap_readonly_untracked(ptr, rsize) free (ptr)
> > >  #endif
> > >
> > > +extern bool _bfd_mmap_read_untracked
> > > +  (void **, size_t *, void **, bfd *) ATTRIBUTE_HIDDEN;
> > > +
> > >  static inline void *
> > >  _bfd_malloc_and_read (bfd *abfd, bfd_size_type asize, bfd_size_type rsize)
> > >  {
> > > --
> > > 2.44.0
> >
> > OK with these fixed.
>
> Here is the v5 patch:
>
> https://patchwork.sourceware.org/project/binutils/list/?series=31726
>
> I will check it in tomorrow.
>

Here is the v6 patch:

https://patchwork.sourceware.org/project/binutils/list/?series=31752

with the fix for:

https://sourceware.org/bugzilla/show_bug.cgi?id=31466

-- 
H.J.