Hi Mark, On Tue, Sep 8, 2026 at 5:16 PM Mark Wielaard <[email protected]> wrote: > > Hi Aaron, > > On Mon, Aug 31, 2026 at 06:46:46PM -0400, Aaron Merey wrote: > > Remove the rdlock held throughout elf_getscn and replace it with a wrlock > > that covers only the one-time initialization of section zero. This > > reduces rwlock overhead when on the hot path and also fixes a race > > condition where runp->cnt could be updated while the rdlock was being > > held. > > > > In both elf_getscn and elf32_offscn, section zero and runp->cnt are now > > accessed atomically when no lock is held. > > So outside the Elf lock reading of the Elf state scns cnt and (if cnt > > 0) data[0].shdr should be done atomically and if that shows section > zero isn't initialized yet we should call getshdr on section zero to > load all section headers. > > getshdr then gets the Elf rd lock, recheck (not atomically) the shdr > is still not available, gets the wr lock, re-rechecks (again not > atomically) the shdr is still not available and then loads all shdrs > (and writes them non-atomically). So this patch relies on the next > patch to introduce the corresponding atomic stores?
That's right. In v2 I can separate the elf32_offscn.c changes into its own patch that makes this more clear. > > Note that there is a similar construct in elf_strptr. Yes we should be able to remove hot path locks from that function similar to this patch. Right now I'm prioritizing library functions that were prominent during lock overhead benchmarking but later I will address elf_strptr. > > Some questions below. > > > Signed-off-by: Aaron Merey <[email protected]> > > --- > > libelf/elf32_offscn.c | 5 ++-- > > libelf/elf_getscn.c | 57 ++++++++++++++++++++++++------------------- > > 2 files changed, 35 insertions(+), 27 deletions(-) > > > > diff --git a/libelf/elf32_offscn.c b/libelf/elf32_offscn.c > > index 1a9a3b0a..5ddb3b58 100644 > > --- a/libelf/elf32_offscn.c > > +++ b/libelf/elf32_offscn.c > > @@ -58,8 +58,9 @@ elfw2(LIBELFBITS,offscn) (Elf *elf, ElfW2(LIBELFBITS,Off) > > offset) > > > > /* If we have not looked at section headers before, > > we might need to read them in first. */ > > - if (runp->cnt > 0 > > - && unlikely (runp->data[0].shdr.ELFW(e,LIBELFBITS) == NULL) > > + if (atomic_load_acquire (&runp->cnt) > 0 > > + && unlikely (atomic_load_acquire > > (&runp->data[0].shdr.ELFW(e,LIBELFBITS)) > > + == NULL) > > && unlikely (elfw2(LIBELFBITS,getshdr) (&runp->data[0]) == NULL)) > > return NULL; > > > > diff --git a/libelf/elf_getscn.c b/libelf/elf_getscn.c > > index be9c76f0..8b268f7b 100644 > > --- a/libelf/elf_getscn.c > > +++ b/libelf/elf_getscn.c > > @@ -50,8 +50,6 @@ elf_getscn (Elf *elf, size_t idx) > > return NULL; > > } > > > > - rwlock_rdlock (elf->lock); > > - > > Elf_Scn *result = NULL; > > > > /* Find the section in the list. */ > > @@ -63,39 +61,51 @@ elf_getscn (Elf *elf, size_t idx) > > /* Section zero is special. It always exists even if there is no > > "first" section. And it is needed to store "overflow" values > > from the Elf header. */ > > - if (idx == 0 && runp->cnt == 0 && runp->max > 0) > > + if (idx == 0 && atomic_load_acquire (&runp->cnt) == 0 && runp->max > 0) > > { > > - Elf_Scn *scn0 = &runp->data[0]; > > - if (elf->class == ELFCLASS32) > > + rwlock_wrlock (elf->lock); > > + > > + /* Check whether section zero was set up before this thread acquired > > + the wrlock. */ > > + if (runp->cnt == 0) > > { > > So because you got the elf wrlock you don't need an atomic read here? That's right, the wrlock being held plus the THREAD-SAFETY contract together mean that no writes to runp->cnt can race with this read. > > > - scn0->shdr.e32 = calloc (1, sizeof (Elf32_Shdr)); > > - if (scn0->shdr.e32 == NULL) > > + Elf_Scn *scn0 = &runp->data[0]; > > + if (elf->class == ELFCLASS32) > > { > > - __libelf_seterrno (ELF_E_NOMEM); > > - goto out; > > + scn0->shdr.e32 = calloc (1, sizeof (Elf32_Shdr)); > > + if (scn0->shdr.e32 == NULL) > > + { > > + __libelf_seterrno (ELF_E_NOMEM); > > + rwlock_unlock (elf->lock); > > + return NULL; > > + } > > } > > - } > > - else > > - { > > - scn0->shdr.e64 = calloc (1, sizeof (Elf64_Shdr)); > > - if (scn0->shdr.e64 == NULL) > > + else > > { > > - __libelf_seterrno (ELF_E_NOMEM); > > - goto out; > > + scn0->shdr.e64 = calloc (1, sizeof (Elf64_Shdr)); > > + if (scn0->shdr.e64 == NULL) > > + { > > + __libelf_seterrno (ELF_E_NOMEM); > > + rwlock_unlock (elf->lock); > > + return NULL; > > + } > > } > > + > > + scn0->elf = elf; > > + scn0->shdr_flags = ELF_F_DIRTY | ELF_F_MALLOCED; > > + scn0->list = elf->state.elf.scns_last; > > + scn0->data_read = 1; > > + atomic_store_release (&runp->cnt, 1); > > But if you don't need the atomic load above, why do you need the > atomic store here (while still holding the wr lock)? The atomic store prevents a race between this store and loads that occur without the wrlock being held. I didn't use an atomic load when the wrlock is held because two threads reading runp->cnt simultaneously does not cause a data race. Aaron > > > } > > - scn0->elf = elf; > > - scn0->shdr_flags = ELF_F_DIRTY | ELF_F_MALLOCED; > > - scn0->list = elf->state.elf.scns_last; > > - scn0->data_read = 1; > > - runp->cnt = 1; > > + > > + rwlock_unlock (elf->lock); > > } > > > > while (1) > > { > > if (idx < runp->max) > > { > > - if (idx < runp->cnt) > > + if (idx < atomic_load_acquire (&runp->cnt)) > > OK, reading of cnt outside lock neds atomic load. > > > result = &runp->data[idx]; > > else > > __libelf_seterrno (ELF_E_INVALID_INDEX); > > @@ -112,9 +122,6 @@ elf_getscn (Elf *elf, size_t idx) > > } > > } > > > > - out: > > - rwlock_unlock (elf->lock); > > - > > return result; > > } > > INTDEF(elf_getscn) > > -- > > 2.55.0 > > >
