Hi Mark,
On Wed, Sep 9, 2026 at 3:57 PM Mark Wielaard <[email protected]> wrote:
>
> Hi Aaron,
>
> On Mon, Aug 31, 2026 at 06:46:47PM -0400, Aaron Merey wrote:
> > Atomically check whether the target shdr has already been loaded.
> > This reduces scn->elf->lock overhead. If the shdr is not loaded,
> > then use scn->elf->lock only for the one-time lazy loading of the
> > shdr.
>
> OK, so this and the previous patch work together.
>
> > Signed-off-by: Aaron Merey <[email protected]>
> > ---
> > libelf/elf32_getshdr.c | 15 ++++++++++-----
> > libelf/gelf_getshdr.c | 33 ++++++++++++++++++++-------------
> > 2 files changed, 30 insertions(+), 18 deletions(-)
> >
> > diff --git a/libelf/elf32_getshdr.c b/libelf/elf32_getshdr.c
> > index e4bebe18..c3d46671 100644
> > --- a/libelf/elf32_getshdr.c
> > +++ b/libelf/elf32_getshdr.c
> > @@ -198,8 +198,9 @@ load_shdr_wrlock (Elf_Scn *scn)
> >
> > /* Set the pointers in the `scn's. */
> > for (size_t cnt = 0; cnt < shnum; ++cnt)
> > - elf->state.ELFW(elf,LIBELFBITS).scns.data[cnt].shdr.ELFW(e,LIBELFBITS)
> > - = &elf->state.ELFW(elf,LIBELFBITS).shdr[cnt];
> > + atomic_store_release
> > +
> > (&elf->state.ELFW(elf,LIBELFBITS).scns.data[cnt].shdr.ELFW(e,LIBELFBITS),
> > + &elf->state.ELFW(elf,LIBELFBITS).shdr[cnt]);
>
> So you need to store
> elf->state.ELFW(elf,LIBELFBITS).scns.data[0].shdr... atomicly, but do
> you need to set the others also atomic?
Yes because one thread may try to load particular shdr from the
gelf_getshdr hot path while another thread is performing this store.
>
> Would it make sense to set scns.data[0] last? So another check doesn't see
> it set before all other shdrs have been set?
That would be better, I'll add it to v2.
>
> Where in the code/call path is scns.cnt set?
> Asking this question might show I don't fully grok what is going on.
scns.cnt is set during elf_begin and can be modified by elf_newscn,
elfNN_newphdr and elf_getscn. elf_begin runs before the relevant elf
descriptor can be used by multiple threads. The THREAD-SAFETY doc
lists elf_newscn and elfNN_newphdr as caller-serialized functions so
they must not run concurrently with elfNN/gelf_getshdr. elf_getscn
uses atomics to avoid races between stores and loads (added in the
previous patch in this series). There is one non-atomic load of cnt in
elf_getscn with elf->lock wrlock held but there is no store to cnt
that can occur concurrently (either the store happens in a
caller-serialized function or when elf->lock wrlock is held or in
elf_begin when the descriptor hasn't yet been shared across threads).
Aaron
>
> > result = scn->shdr.ELFW(e,LIBELFBITS);
> > assert (result != NULL);
> > @@ -275,9 +276,13 @@ elfw2(LIBELFBITS,getshdr) (Elf_Scn *scn)
> > if (!scn_valid (scn))
> > return NULL;
> >
> > - rwlock_rdlock (scn->elf->lock);
> > - result = __elfw2(LIBELFBITS,getshdr_rdlock) (scn);
> > - rwlock_unlock (scn->elf->lock);
> > + result = atomic_load_acquire (&scn->shdr.ELFW(e,LIBELFBITS));
> > + if (result == NULL)
> > + {
> > + rwlock_wrlock (scn->elf->lock);
> > + result = __elfw2(LIBELFBITS,getshdr_wrlock) (scn);
> > + rwlock_unlock (scn->elf->lock);
> > + }
> >
> > return result;
> > }
>
> OK, the same result check is then done in getshdr_wrlock to make sure
> some other thread didn't race past us.
>
> > diff --git a/libelf/gelf_getshdr.c b/libelf/gelf_getshdr.c
> > index 3858c8e1..2b16e502 100644
> > --- a/libelf/gelf_getshdr.c
> > +++ b/libelf/gelf_getshdr.c
> > @@ -51,18 +51,22 @@ gelf_getshdr (Elf_Scn *scn, GElf_Shdr *dst)
> > return NULL;
> > }
> >
> > - rwlock_rdlock (scn->elf->lock);
> > -
> > if (scn->elf->class == ELFCLASS32)
> > {
> > /* Copy the elements one-by-one. */
> > - Elf32_Shdr *shdr
> > - = scn->shdr.e32 ?: __elf32_getshdr_rdlock (scn);
> > + Elf32_Shdr *shdr = atomic_load_acquire (&scn->shdr.e32);
> >
> > if (shdr == NULL)
> > {
> > - __libelf_seterrno (ELF_E_INVALID_OPERAND);
> > - goto out;
> > + rwlock_wrlock (scn->elf->lock);
> > + shdr = __elf32_getshdr_wrlock (scn);
> > + rwlock_unlock (scn->elf->lock);
>
> OK, like above, 32bit case.
>
> > + if (shdr == NULL)
> > + {
> > + __libelf_seterrno (ELF_E_INVALID_OPERAND);
> > + return NULL;
> > + }
> > }
>
> OK, as done above (but without goto out, lock already dropped).
>
> > #define COPY(name) \
> > @@ -82,22 +86,25 @@ gelf_getshdr (Elf_Scn *scn, GElf_Shdr *dst)
> > }
> > else
> > {
> > - Elf64_Shdr *shdr
> > - = scn->shdr.e64 ?: __elf64_getshdr_rdlock (scn);
> > + Elf64_Shdr *shdr = atomic_load_acquire (&scn->shdr.e64);
> >
> > if (shdr == NULL)
> > {
> > - __libelf_seterrno (ELF_E_INVALID_OPERAND);
> > - goto out;
> > + rwlock_wrlock (scn->elf->lock);
> > + shdr = __elf64_getshdr_wrlock (scn);
> > + rwlock_unlock (scn->elf->lock);
>
> OK, like above, 32bit case.
>
> > + if (shdr == NULL)
> > + {
> > + __libelf_seterrno (ELF_E_INVALID_OPERAND);
> > + return NULL;
> > + }
> > }
>
> OK, as done above (but without goto out, lock already dropped).
>
> >
> > /* We only have to copy the data. */
> > result = memcpy (dst, shdr, sizeof (GElf_Shdr));
> > }
> >
> > - out:
> > - rwlock_unlock (scn->elf->lock);
> > -
> > return result;
> > }
> > INTDEF(gelf_getshdr)
>
> Ack, lock/unlock pairs are much smaller now.
>
> Cheers,
>
> Mark
>
