Hi Aaron, On Tue, 2026-10-27 at 20:34 -0400, Aaron Merey wrote: > Add load_lock to Dwfl for serializing module loading, including loading > of module's elf, dwarf, symbols as well as performing relocations.
Looks like this patch and the previous patch (which already uses the mutex) were swapped. > load_lock is used only for lazy loading of these components. Hot paths > are kept lock-free using atomics. New Dwfl_Module fields elf_loaded, > dwarf_loaded and symbols_loaded have been added. These are set to > true when lazy loading of elf, dwarf and symbols is completed. They are > also set to true when initialization has been attempted and failed. So we are using slightly different patterns here. e.g. for the build_id_len we use the field itself, set to NULL for not initialized, (void *) -1 for tried but failed. > This commit serializes only the lazy loading entry points. > libdwfl/ > > * dwfl_begin.c (dwfl_begin): Initialize load_lock. Also move > the debuginfod_lock init to avoid NULL dereference. > * dwfl_end.c (dwfl_end): Tear down load_lock. > * dwfl_module_build_id.c (found_build_id): Store build_id_len > atomically. > (dwfl_module_build_id): Attempt to read build_id_len > atomically. If it hasn't been loaded, acquire load_lock > first. > * dwfl_module_getdwarf.c (find_symtab_locked): Renamed from > find_symtab. > (find_symtab): Wrapper for find_symtab_locked that handles > locking and checking symbols_loaded. > (get_dwarf_locked): Renamed from dwfl_module_getdwarf. > (dwfl_module_getdwarf): Wrapper for get_dwarf_locked that > handles locking and checking dwarf_loaded. > * dwfl_module_getelf.c (get_elf_locked): Renamed from > dwfl_module_getelf. > (dwfl_module_getelf): Wrapper for get_elf_locked that handles > locking and checking elf_loaded. > * dwfl_module_getsym.c (__libdwfl_getsym): Check symbols_loaded > before calling dwfl_module_getsymtab. > * libdwflP.h (struct Dwfl): Add load_lock. > (struct Dwfl_Module): Add bools symbols_loaded, dwarf_loaded > and elf_loaded. > > Signed-off-by: Aaron Merey <[email protected]> > --- > libdwfl/dwfl_begin.c | 4 +-- > libdwfl/dwfl_end.c | 1 + > libdwfl/dwfl_module_build_id.c | 31 +++++++++++----- > libdwfl/dwfl_module_getdwarf.c | 64 ++++++++++++++++++++++++---------- > libdwfl/dwfl_module_getelf.c | 33 +++++++++++++++--- > libdwfl/dwfl_module_getsym.c | 3 +- > libdwfl/libdwflP.h | 8 +++++ > 7 files changed, 109 insertions(+), 35 deletions(-) > > diff --git a/libdwfl/dwfl_begin.c b/libdwfl/dwfl_begin.c > index 835de8e3..2b029426 100644 > --- a/libdwfl/dwfl_begin.c > +++ b/libdwfl/dwfl_begin.c > @@ -48,10 +48,10 @@ dwfl_begin (const Dwfl_Callbacks *callbacks) > { > dwfl->callbacks = callbacks; > dwfl->offline_next_address = OFFLINE_REDZONE; > + mutex_init (dwfl->debuginfod_lock); > + mutex_init (dwfl->load_lock); > } > > - mutex_init (dwfl->debuginfod_lock); > - > return dwfl; > } O, that dwfl->debuginfod_lock would be a NULL pointer derefence if allocation ever failed. Nice catch. > INTDEF (dwfl_begin) > diff --git a/libdwfl/dwfl_end.c b/libdwfl/dwfl_end.c > index c82d83fb..97516a01 100644 > --- a/libdwfl/dwfl_end.c > +++ b/libdwfl/dwfl_end.c > @@ -54,6 +54,7 @@ dwfl_end (Dwfl *dwfl) > free (dwfl->lookup_segndx); > free (dwfl->sysroot); > mutex_fini (dwfl->debuginfod_lock); > + mutex_fini (dwfl->load_lock); > > Dwfl_Module *next = dwfl->modulelist; > while (next != NULL) OK, finit new lock. > diff --git a/libdwfl/dwfl_module_build_id.c b/libdwfl/dwfl_module_build_id.c > index 0c198f23..c3ff08f8 100644 > --- a/libdwfl/dwfl_module_build_id.c > +++ b/libdwfl/dwfl_module_build_id.c > @@ -52,7 +52,9 @@ found_build_id (Dwfl_Module *mod, bool set, > > mod->build_id_bits = memcpy (copy, bits, len); > mod->build_id_vaddr = vaddr; > - mod->build_id_len = len; > + > + /* Build id readers check len to tell if bits and vaddr are also loaded. > */ > + atomic_store_release (&mod->build_id_len, len); > return len; > } OK, use atomic to set the field. > @@ -85,23 +87,34 @@ dwfl_module_build_id (Dwfl_Module *mod, > if (mod == NULL) > return -1; > > - if (mod->build_id_len == 0 && mod->main.elf != NULL) > + int len = atomic_load_acquire (&mod->build_id_len); > + if (len == 0) > { > - /* We have the file, but have not examined it yet. */ > - int result = __libdwfl_find_build_id (mod, true, mod->main.elf); > - if (result <= 0) > + mutex_lock (mod->dwfl->load_lock); > + len = mod->build_id_len; > + if (len == 0 && mod->main.elf != NULL) > { OK, use atomic load, get lock, recheck field to see if anymore raced passed us. > - mod->build_id_len = -1; /* Cache negative result. */ > - return result; > + /* We have the file, but have not examined it yet. */ > + len = __libdwfl_find_build_id (mod, true, mod->main.elf); > + if (len <= 0) > + { > + /* Cache negative result. */ > + atomic_store_release (&mod->build_id_len, -1); > + mutex_unlock (mod->dwfl->load_lock); > + return len; > + } > } OK, if lookup failed, atomic store -1 in field. unlock and return negative len. > + > + mutex_unlock (mod->dwfl->load_lock); > } > - if (mod->build_id_len <= 0) > + if (len <= 0) > return 0; I think this is correct, this happens when we already had a negative (- 1) cached len? > *bits = mod->build_id_bits; > *vaddr = mod->build_id_vaddr; > - return mod->build_id_len; > + > + return len; > } > NEW_INTDEF (dwfl_module_build_id) Have to double check. We set/store the mod->build_id_len before we set the bits and vaddr. Is that correct? Shouldn't they be set before len is set? > diff --git a/libdwfl/dwfl_module_getdwarf.c b/libdwfl/dwfl_module_getdwarf.c > index 430c3236..247fbbd2 100644 > --- a/libdwfl/dwfl_module_getdwarf.c > +++ b/libdwfl/dwfl_module_getdwarf.c > @@ -1075,12 +1075,8 @@ find_aux_sym (Dwfl_Module *mod __attribute__ > ((unused)), > > /* Try to find a symbol table in either MOD->main.elf or MOD->debug.elf. */ > static void > -find_symtab (Dwfl_Module *mod) > +find_symtab_locked (Dwfl_Module *mod) > { > - if (mod->symdata != NULL || mod->aux_symdata != NULL /* Already > done. */ > - || mod->symerr != DWFL_E_NOERROR) /* Cached previous failure. */ > - return; > - > __libdwfl_getelf (mod); > mod->symerr = mod->elferr; > if (mod->symerr != DWFL_E_NOERROR) > @@ -1325,6 +1321,24 @@ find_symtab (Dwfl_Module *mod) > } > } OK, renamed to _locked and remove the early sanity check that callers now have to do. > +/* Load the symtab if it isn't already loaded. */ > +static void > +find_symtab (Dwfl_Module *mod) > +{ > + if (likely (atomic_load_acquire (&mod->symbols_loaded))) > + return; > + > + mutex_lock (mod->dwfl->load_lock); > + if (!mod->symbols_loaded && mod->symdata == NULL > + && mod->aux_symdata == NULL && mod->symerr == DWFL_E_NOERROR) > + { > + find_symtab_locked (mod); > + mod->symerr = __libdwfl_canon_error (mod->symerr); > + atomic_store_release (&mod->symbols_loaded, true); > + } > + > + mutex_unlock (mod->dwfl->load_lock); > +} OK. Do the atomic check dance, get lock, sanity check and call find_symtab_locked, then store loaded field. > /* Try to open a libebl backend for MOD. */ > Dwfl_Error > @@ -1411,10 +1425,6 @@ load_dw (Dwfl_Module *mod, struct dwfl_file *debugfile) > static void > find_dw (Dwfl_Module *mod) > { > - if (mod->dw != NULL /* Already done. */ > - || mod->dwerr != DWFL_E_NOERROR) /* Cached previous failure. */ > - return; > - > __libdwfl_getelf (mod); > mod->dwerr = mod->elferr; > if (mod->dwerr != DWFL_E_NOERROR) > @@ -1471,12 +1481,9 @@ find_dw (Dwfl_Module *mod) > mod->dwerr = __libdwfl_canon_error (mod->dwerr); > } OK. Like above. > -Dwarf * > -dwfl_module_getdwarf (Dwfl_Module *mod, Dwarf_Addr *bias) > +static void > +get_dwarf_locked (Dwfl_Module *mod) > { > - if (mod == NULL) > - return NULL; > - > find_dw (mod); > if (mod->dwerr == DWFL_E_NOERROR) > { > @@ -1489,13 +1496,34 @@ dwfl_module_getdwarf (Dwfl_Module *mod, Dwarf_Addr > *bias) > if (mod->debug.elf != mod->main.elf) > (void) __libdwfl_relocate (mod, mod->debug.elf, false); > } > + } > + > + atomic_store_release (&mod->dwarf_loaded, true); > +} > > - *bias = dwfl_adjusted_dwarf_addr (mod, 0); > - return mod->dw; OK, like above. Except mod->dwarf_loaded is stored in the _locked variant. Is that intentional? The pattern above is take lock, call _locked function variant (which sets field), on return store loaded field, unlock. > +Dwarf * > +dwfl_module_getdwarf (Dwfl_Module *mod, Dwarf_Addr *bias) > +{ > + if (mod == NULL) > + return NULL; > + > + if (unlikely (!atomic_load_acquire (&mod->dwarf_loaded))) > + { > + mutex_lock (mod->dwfl->load_lock); > + if (!mod->dwarf_loaded && mod->dw == NULL && mod->dwerr == > DWFL_E_NOERROR) > + get_dwarf_locked (mod); > + > + mutex_unlock (mod->dwfl->load_lock); > + } > + > + if (mod->dwerr != DWFL_E_NOERROR) > + { > + __libdwfl_seterrno (mod->dwerr); > + return NULL; > } > > - __libdwfl_seterrno (mod->dwerr); > - return NULL; > + *bias = dwfl_adjusted_dwarf_addr (mod, 0); > + return mod->dw; > } > INTDEF (dwfl_module_getdwarf) OK. > diff --git a/libdwfl/dwfl_module_getelf.c b/libdwfl/dwfl_module_getelf.c > index 6358de4c..e3a0197b 100644 > --- a/libdwfl/dwfl_module_getelf.c > +++ b/libdwfl/dwfl_module_getelf.c > @@ -32,12 +32,9 @@ > > #include "libdwflP.h" > > -Elf * > -dwfl_module_getelf (Dwfl_Module *mod, GElf_Addr *loadbase) > +static Elf * > +get_elf_locked (Dwfl_Module *mod, GElf_Addr *loadbase) > { > - if (mod == NULL) > - return NULL; > - > __libdwfl_getelf (mod); > if (mod->elferr == DWFL_E_NOERROR) > { > @@ -68,4 +65,30 @@ dwfl_module_getelf (Dwfl_Module *mod, GElf_Addr *loadbase) > __libdwfl_seterrno (mod->elferr); > return NULL; > } > + > +Elf * > +dwfl_module_getelf (Dwfl_Module *mod, GElf_Addr *loadbase) > +{ > + if (mod == NULL) > + return NULL; > + > + if (likely (atomic_load_acquire (&mod->elf_loaded))) > + { > + if (mod->elferr != DWFL_E_NOERROR) > + { > + __libdwfl_seterrno (mod->elferr); > + return NULL; > + } > + > + *loadbase = dwfl_adjusted_address (mod, 0); > + return mod->main.elf; > + } > + > + mutex_lock (mod->dwfl->load_lock); > + Elf *elf = get_elf_locked (mod, loadbase); > + atomic_store_release (&mod->elf_loaded, true); > + mutex_unlock (mod->dwfl->load_lock); > + > + return elf; > +} > INTDEF (dwfl_module_getelf) OK. Like the dwarf variant, but with the store in the caller of the _locked function. > diff --git a/libdwfl/dwfl_module_getsym.c b/libdwfl/dwfl_module_getsym.c > index 8de9a3eb..2bb07fd8 100644 > --- a/libdwfl/dwfl_module_getsym.c > +++ b/libdwfl/dwfl_module_getsym.c > @@ -41,7 +41,8 @@ __libdwfl_getsym (Dwfl_Module *mod, int ndx, GElf_Sym *sym, > GElf_Addr *addr, > if (unlikely (mod == NULL)) > return NULL; > > - if (unlikely (mod->symdata == NULL)) > + if (unlikely (!atomic_load_acquire (&mod->symbols_loaded) > + || mod->symdata == NULL)) > { > int result = INTUSE(dwfl_module_getsymtab) (mod); > if (result < 0) I assume this dwfl_module_getsymtab ends up calling find_symtab? Which then does the whole symbols_loaded dance again? > diff --git a/libdwfl/libdwflP.h b/libdwfl/libdwflP.h > index 56014ff6..3b4df7e8 100644 > --- a/libdwfl/libdwflP.h > +++ b/libdwfl/libdwflP.h > @@ -144,6 +144,10 @@ struct Dwfl > > /* Serialize debuginfod_client usage. */ > mutex_define (, debuginfod_lock); > + > + /* Serialize loading of ELF/DWARF, relocations, symbols as well as > + Dwfl_Callbacks.*/ > + mutex_define (, load_lock); > }; OK, define new lock. > > #define OFFLINE_REDZONE 0x10000 > @@ -206,6 +210,10 @@ struct Dwfl_Module > Dwfl_Error symerr; /* Previous failure to load symbols. */ > Dwfl_Error dwerr; /* Previous failure to load DWARF. */ > > + bool symbols_loaded; /* True if symbols loaded or failed to > load. */ > + bool dwarf_loaded; /* True if dwarf loaded or failed to load. */ > + bool elf_loaded; /* True if elf loaded or failed to load. */ > + > /* Known CU's in this module. */ > struct dwfl_cu *first_cu, **cu; > OK, new fields. Might use a bitfield. But probably not worth it (and might not work with atomic operations). Given that you probably won't have lots of Dwfl_Modules. I am not 100% sure this is all correct. Could you review my review? It probably is all fine. Just want to make sure we didn't miss anything. Thanks, Mark
