From: "H.J. Lu" <hjl.tools@gmail.com>
To: Alan Modra <amodra@gmail.com>
Cc: binutils@sourceware.org, goldstein.w.n@gmail.com
Subject: Re: [PATCH v4 5/6] elf: Use mmap to map in symbol and relocation tables
Date: Fri, 8 Mar 2024 07:36:13 -0800 [thread overview]
Message-ID: <CAMe9rOoXn05OO6X=DhE0pSDQJuRax-uf+qyhwim2tigBaydSAA@mail.gmail.com> (raw)
In-Reply-To: <Zepe/zTmP2dTGBYs@squeak.grove.modra.org>
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.
Thanks.
--
H.J.
next prev parent reply other threads:[~2024-03-08 15:36 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-06 23:23 [PATCH v4 0/6] elf: Use mmap to map in section contents H.J. Lu
2024-03-06 23:23 ` [PATCH v4 1/6] bfd: Don't hard-code BFD_JUMP_TABLE_COPY H.J. Lu
2024-03-07 22:46 ` Alan Modra
2024-03-06 23:23 ` [PATCH v4 2/6] bfd: Change the --with-mmap default to true H.J. Lu
2024-03-07 22:46 ` Alan Modra
2024-03-06 23:23 ` [PATCH v4 3/6] elf: Use mmap to map in read-only sections H.J. Lu
2024-03-08 0:40 ` Alan Modra
2024-03-08 15:25 ` H.J. Lu
2024-03-06 23:23 ` [PATCH v4 4/6] elf: Add _bfd_elf_mmap_section and _bfd_elf_munmap_section_contents H.J. Lu
2024-03-08 0:41 ` Alan Modra
2024-03-08 15:28 ` H.J. Lu
2024-03-08 17:03 ` H.J. Lu
2024-03-08 0:50 ` Sam James
2024-03-08 3:33 ` Alan Modra
2024-03-06 23:24 ` [PATCH v4 5/6] elf: Use mmap to map in symbol and relocation tables H.J. Lu
2024-03-08 0:42 ` Alan Modra
2024-03-08 15:36 ` H.J. Lu [this message]
2024-03-09 16:34 ` H.J. Lu
2024-03-06 23:24 ` [PATCH v4 6/6] elf: Don't cache symbol nor relocation tables with mmap H.J. Lu
2024-03-08 0:43 ` Alan Modra
2024-03-08 0:48 ` Sam James
2024-03-08 15:31 ` H.J. Lu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMe9rOoXn05OO6X=DhE0pSDQJuRax-uf+qyhwim2tigBaydSAA@mail.gmail.com' \
--to=hjl.tools@gmail.com \
--cc=amodra@gmail.com \
--cc=binutils@sourceware.org \
--cc=goldstein.w.n@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).