Hi Mark, On Wed, Oct 28, 2026 at 11:13 AM Mark Wielaard <[email protected]> wrote: > > 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.
That's right, I shouldn't have swapped them. Will fix in v2. > > > 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. The reason for these new _loaded flags is that a module's elf, dw and symbol fields themselves are ambiguous as completion indicators. They can be set to non-NULL before initialization is complete. Also a NULL elf, dw or symbol field does not distinguish between whether loading hasn't been attempted vs. loading was attempted and failed. We could restructure elf, dw and symbol initialization so that these new flags aren't needed but that would require a more complex refactor. > > > 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? That's right, this preserves the existing behaviour for returning 0 for a -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? found_build_id sets the module buildid bits and vaddr before atomic storing the len. These particular bits and vaddr stores update arguments passed by the caller. > > > 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. For consistency I'll move dwarf_loaded store to the wrapper in v2. > > > +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? That's right. __libdwfl_getsym can skip dwfl_module_getsymtab when symbols_loading indicates initialization finished. Unless mod->symdata == NULL, in which case dwfl_module_getsymtab is called and sets libdwfl's errno if needed. > > > 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. Right, I don't think that will work with atomics which require a pointer to the field's address. > > 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. While doing some further testing I found that mod->build_id_len can be reset in __libdwfl_getelf and this should be an atomic store. Will update in v2. > > Thanks, > > Mark >
