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? 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? Where in the code/call path is scns.cnt set? Asking this question might show I don't fully grok what is going on. > 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
