[PATCH 3/4] gelf_getshdr.c, elfNN_getshdr.c: Remove hot path locks

[Mark Wielaard]

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 <amerey@redhat.com>
> ---
>  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