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

[Alan Modra <amodra@gmail.com>]

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.

>     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?

>  				   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

> +{
> +  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.

-- 
Alan Modra
Australia Development Lab, IBM