[02/15] Change type of dwarf_lock from rwlock to mutex
Commit Message
Change type of dwarf_lock to mutex in order to take advantage of
built-in support for recursive locking.
* lib/locks.h: Add macros for locking, unlocking, initializing
and destroying mutexes.
* libdw/dwarf_begin_elf.c (dwarf_end): Replace rwlock macro with
mutex macro.
* libdw/dwarf_formref_die.c (dwarf_formref_die): Ditto.
* libdw/dwarf_getalt.c (dwarf_getalt): Ditto.
* libdw/dwarf_setalt.c (dwarf_setalt): Ditto.
* libdw/libdwP.h (struct Dwarf): Ditto.
* libdw/libdw_findcu.c (__libdw_findcu): Ditto.
Signed-off-by: Aaron Merey <amerey@redhat.com>
---
If performance suffers too much from serializing all accesses to Dwarf
objects, we could instead consider using an rwlock with a custom wrapper
that implements recursive locking.
lib/locks.h | 16 ++++++++++++++++
libdw/dwarf_begin_elf.c | 2 +-
libdw/dwarf_end.c | 2 +-
libdw/dwarf_formref_die.c | 4 ++--
libdw/dwarf_getalt.c | 10 +++++-----
libdw/dwarf_setalt.c | 4 ++--
libdw/libdwP.h | 2 +-
libdw/libdw_findcu.c | 4 ++--
8 files changed, 30 insertions(+), 14 deletions(-)
Comments
Hi Aaron,
On Sun, Jan 19, 2025 at 10:20:28PM -0500, Aaron Merey wrote:
> Change type of dwarf_lock to mutex in order to take advantage of
> built-in support for recursive locking.
When/where do we need recursive locking?
> * lib/locks.h: Add macros for locking, unlocking, initializing
> and destroying mutexes.
> * libdw/dwarf_begin_elf.c (dwarf_end): Replace rwlock macro with
> mutex macro.
> * libdw/dwarf_formref_die.c (dwarf_formref_die): Ditto.
> * libdw/dwarf_getalt.c (dwarf_getalt): Ditto.
> * libdw/dwarf_setalt.c (dwarf_setalt): Ditto.
> * libdw/libdwP.h (struct Dwarf): Ditto.
> * libdw/libdw_findcu.c (__libdw_findcu): Ditto.
OK, this seems to work because we only use rwlocks and never rdlocks.
> Signed-off-by: Aaron Merey <amerey@redhat.com>
>
> ---
> If performance suffers too much from serializing all accesses to Dwarf
> objects, we could instead consider using an rwlock with a custom wrapper
> that implements recursive locking.
We could also use multiple locks for different parts of the
Dwarf. Like we already do for the trees.
Currently the lock is shared for accessing the
__libdw_intern_next_unit and the alt_dwarf.
> lib/locks.h | 16 ++++++++++++++++
> libdw/dwarf_begin_elf.c | 2 +-
> libdw/dwarf_end.c | 2 +-
> libdw/dwarf_formref_die.c | 4 ++--
> libdw/dwarf_getalt.c | 10 +++++-----
> libdw/dwarf_setalt.c | 4 ++--
> libdw/libdwP.h | 2 +-
> libdw/libdw_findcu.c | 4 ++--
> 8 files changed, 30 insertions(+), 14 deletions(-)
>
> diff --git a/lib/locks.h b/lib/locks.h
> index 90fe3f1b..9c17eca7 100644
> --- a/lib/locks.h
> +++ b/lib/locks.h
> @@ -43,6 +43,17 @@
> # define rwlock_rdlock(lock) RWLOCK_CALL (rdlock (&lock))
> # define rwlock_wrlock(lock) RWLOCK_CALL (wrlock (&lock))
> # define rwlock_unlock(lock) RWLOCK_CALL (unlock (&lock))
> +# define mutex_define(class,name) class pthread_mutex_t name
> +# define MUTEX_CALL(call) \
> + ({ int _err = pthread_mutex_ ## call; assert_perror (_err); })
> +# define mutex_init(lock) \
> + ({ pthread_mutexattr_t _attr; \
> + pthread_mutexattr_init (&_attr); \
> + pthread_mutexattr_settype (&_attr, PTHREAD_MUTEX_RECURSIVE); \
> + MUTEX_CALL (init (&lock, &_attr)); })
> +# define mutex_lock(_lock) MUTEX_CALL (lock (&_lock))
> +# define mutex_unlock(lock) MUTEX_CALL (unlock (&lock))
> +# define mutex_fini(lock) MUTEX_CALL (destroy (&lock))
> # define once(once_control, init_routine) \
> ONCE_CALL (once (&once_control, init_routine))
> #else
> @@ -55,6 +66,11 @@
> # define rwlock_rdlock(lock) ((void) (lock))
> # define rwlock_wrlock(lock) ((void) (lock))
> # define rwlock_unlock(lock) ((void) (lock))
> +# define mutex_define(class,name) class int name
> +# define mutex_init(lock) ((void) (lock))
> +# define mutex_lock(lock) ((void) (lock))
> +# define mutex_unlock(lock) ((void) (lock))
> +# define mutex_fini(lock) ((void) (lock))
> # define once_define(class,name)
> # define once(once_control, init_routine) init_routine()
> #endif /* USE_LOCKS */
> diff --git a/libdw/dwarf_begin_elf.c b/libdw/dwarf_begin_elf.c
> index c8292913..8a3a939b 100644
> --- a/libdw/dwarf_begin_elf.c
> +++ b/libdw/dwarf_begin_elf.c
> @@ -579,7 +579,7 @@ dwarf_begin_elf (Elf *elf, Dwarf_Cmd cmd, Elf_Scn *scngrp)
> __libdw_seterrno (DWARF_E_NOMEM); /* no memory. */
> return NULL;
> }
> - rwlock_init (result->dwarf_lock);
> + mutex_init (result->dwarf_lock);
> eu_search_tree_init (&result->cu_tree);
> eu_search_tree_init (&result->tu_tree);
> eu_search_tree_init (&result->split_tree);
> diff --git a/libdw/dwarf_end.c b/libdw/dwarf_end.c
> index ee3ed74e..92389c07 100644
> --- a/libdw/dwarf_end.c
> +++ b/libdw/dwarf_end.c
> @@ -129,7 +129,7 @@ dwarf_end (Dwarf *dwarf)
> if (dwarf->mem_tails != NULL)
> free (dwarf->mem_tails);
> pthread_rwlock_destroy (&dwarf->mem_rwl);
> - rwlock_fini (dwarf->dwarf_lock);
> + mutex_fini (dwarf->dwarf_lock);
>
> /* Free the pubnames helper structure. */
> free (dwarf->pubnames_sets);
> diff --git a/libdw/dwarf_formref_die.c b/libdw/dwarf_formref_die.c
> index 69d1bf1c..85c484f6 100644
> --- a/libdw/dwarf_formref_die.c
> +++ b/libdw/dwarf_formref_die.c
> @@ -92,9 +92,9 @@ dwarf_formref_die (Dwarf_Attribute *attr, Dwarf_Die *result)
> bool scan_debug_types = false;
> do
> {
> - rwlock_wrlock(attr->cu->dbg->dwarf_lock);
> + mutex_lock (attr->cu->dbg->dwarf_lock);
> cu = __libdw_intern_next_unit (attr->cu->dbg, scan_debug_types);
> - rwlock_unlock(attr->cu->dbg->dwarf_lock);
> + mutex_unlock (attr->cu->dbg->dwarf_lock);
>
> if (cu == NULL)
> {
> diff --git a/libdw/dwarf_getalt.c b/libdw/dwarf_getalt.c
> index 26433b81..f4b8ad02 100644
> --- a/libdw/dwarf_getalt.c
> +++ b/libdw/dwarf_getalt.c
> @@ -170,12 +170,12 @@ dwarf_getalt (Dwarf *main)
> if (main == NULL)
> return NULL;
>
> - rwlock_wrlock(main->dwarf_lock);
> + mutex_lock (main->dwarf_lock);
>
> /* Only try once. */
> if (main->alt_dwarf == (void *) -1)
> {
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
> return NULL;
> }
>
> @@ -185,7 +185,7 @@ dwarf_getalt (Dwarf *main)
> if (main->alt_dwarf != NULL)
> {
> result = main->alt_dwarf;
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
> return result;
> }
>
> @@ -195,12 +195,12 @@ dwarf_getalt (Dwarf *main)
> if (main->alt_dwarf == NULL)
> {
> main->alt_dwarf = (void *) -1;
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
> return NULL;
> }
>
> result = main->alt_dwarf;
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
>
> return result;
> }
> diff --git a/libdw/dwarf_setalt.c b/libdw/dwarf_setalt.c
> index f98a457c..d8790fd3 100644
> --- a/libdw/dwarf_setalt.c
> +++ b/libdw/dwarf_setalt.c
> @@ -35,7 +35,7 @@
> void
> dwarf_setalt (Dwarf *main, Dwarf *alt)
> {
> - rwlock_wrlock (main->dwarf_lock);
> + mutex_lock (main->dwarf_lock);
>
> if (main->alt_fd != -1)
> {
> @@ -45,6 +45,6 @@ dwarf_setalt (Dwarf *main, Dwarf *alt)
> }
>
> main->alt_dwarf = alt;
> - rwlock_unlock (main->dwarf_lock);
> + mutex_unlock (main->dwarf_lock);
> }
> INTDEF (dwarf_setalt)
> diff --git a/libdw/libdwP.h b/libdw/libdwP.h
> index d6bab606..db5abeec 100644
> --- a/libdw/libdwP.h
> +++ b/libdw/libdwP.h
> @@ -266,7 +266,7 @@ struct Dwarf
>
> /* The dwarf_lock is a read-write lock designed to ensure thread-safe access
> and modification of an entire Dwarf object. */
> - rwlock_define(, dwarf_lock);
> + mutex_define(, dwarf_lock);
>
> /* Internal memory handling. This is basically a simplified thread-local
> reimplementation of obstacks. Unfortunately the standard obstack
> diff --git a/libdw/libdw_findcu.c b/libdw/libdw_findcu.c
> index bbbbee5d..613f61c8 100644
> --- a/libdw/libdw_findcu.c
> +++ b/libdw/libdw_findcu.c
> @@ -249,7 +249,7 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
> if (found != NULL)
> return *found;
>
> - rwlock_wrlock (dbg->dwarf_lock);
> + mutex_lock (dbg->dwarf_lock);
>
> if (start < *next_offset)
> __libdw_seterrno (DWARF_E_INVALID_DWARF);
> @@ -276,7 +276,7 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
> }
> }
>
> - rwlock_unlock (dbg->dwarf_lock);
> + mutex_unlock (dbg->dwarf_lock);
> return result;
> }
This does seems to do what it says it does. I just don't know why yet.
Cheers,
Mark
Hi Mark,
On Wed, Jan 22, 2025 at 4:55 PM Mark Wielaard <mark@klomp.org> wrote:
>
> On Sun, Jan 19, 2025 at 10:20:28PM -0500, Aaron Merey wrote:
> > Change type of dwarf_lock to mutex in order to take advantage of
> > built-in support for recursive locking.
>
> When/where do we need recursive locking?
One example is dwarf_getsrcfiles, which contains a call to itself. A
dwarf_getsrcfiles lock is added in patch 04/15 in this series.
>
> > * lib/locks.h: Add macros for locking, unlocking, initializing
> > and destroying mutexes.
> > * libdw/dwarf_begin_elf.c (dwarf_end): Replace rwlock macro with
> > mutex macro.
> > * libdw/dwarf_formref_die.c (dwarf_formref_die): Ditto.
> > * libdw/dwarf_getalt.c (dwarf_getalt): Ditto.
> > * libdw/dwarf_setalt.c (dwarf_setalt): Ditto.
> > * libdw/libdwP.h (struct Dwarf): Ditto.
> > * libdw/libdw_findcu.c (__libdw_findcu): Ditto.
>
> OK, this seems to work because we only use rwlocks and never rdlocks.
>
> > Signed-off-by: Aaron Merey <amerey@redhat.com>
> >
> > ---
> > If performance suffers too much from serializing all accesses to Dwarf
> > objects, we could instead consider using an rwlock with a custom wrapper
> > that implements recursive locking.
>
> We could also use multiple locks for different parts of the
> Dwarf. Like we already do for the trees.
>
> Currently the lock is shared for accessing the
> __libdw_intern_next_unit and the alt_dwarf.
Yes but for now I'd prefer to keep the lock logic as simple as possible
and address lock performance issues when necessary. This should make it
easier to achieve full thread-safety.
Hi Aaron,
On Thu, 2025-01-23 at 21:13 -0500, Aaron Merey wrote:
> > > If performance suffers too much from serializing all accesses to Dwarf
> > > objects, we could instead consider using an rwlock with a custom wrapper
> > > that implements recursive locking.
> >
> > We could also use multiple locks for different parts of the
> > Dwarf. Like we already do for the trees.
> >
> > Currently the lock is shared for accessing the
> > __libdw_intern_next_unit and the alt_dwarf.
>
> Yes but for now I'd prefer to keep the lock logic as simple as possible
> and address lock performance issues when necessary. This should make it
> easier to achieve full thread-safety.
>
OK, but we should clearly document what field/function a lock
"protects". So please add a comment to the field/function to make clear
it shouldn't be accessed/called without holding the lock (and maybe to
the lock saying which fields/functions it covers).
Thanks,
Mark
@@ -43,6 +43,17 @@
# define rwlock_rdlock(lock) RWLOCK_CALL (rdlock (&lock))
# define rwlock_wrlock(lock) RWLOCK_CALL (wrlock (&lock))
# define rwlock_unlock(lock) RWLOCK_CALL (unlock (&lock))
+# define mutex_define(class,name) class pthread_mutex_t name
+# define MUTEX_CALL(call) \
+ ({ int _err = pthread_mutex_ ## call; assert_perror (_err); })
+# define mutex_init(lock) \
+ ({ pthread_mutexattr_t _attr; \
+ pthread_mutexattr_init (&_attr); \
+ pthread_mutexattr_settype (&_attr, PTHREAD_MUTEX_RECURSIVE); \
+ MUTEX_CALL (init (&lock, &_attr)); })
+# define mutex_lock(_lock) MUTEX_CALL (lock (&_lock))
+# define mutex_unlock(lock) MUTEX_CALL (unlock (&lock))
+# define mutex_fini(lock) MUTEX_CALL (destroy (&lock))
# define once(once_control, init_routine) \
ONCE_CALL (once (&once_control, init_routine))
#else
@@ -55,6 +66,11 @@
# define rwlock_rdlock(lock) ((void) (lock))
# define rwlock_wrlock(lock) ((void) (lock))
# define rwlock_unlock(lock) ((void) (lock))
+# define mutex_define(class,name) class int name
+# define mutex_init(lock) ((void) (lock))
+# define mutex_lock(lock) ((void) (lock))
+# define mutex_unlock(lock) ((void) (lock))
+# define mutex_fini(lock) ((void) (lock))
# define once_define(class,name)
# define once(once_control, init_routine) init_routine()
#endif /* USE_LOCKS */
@@ -579,7 +579,7 @@ dwarf_begin_elf (Elf *elf, Dwarf_Cmd cmd, Elf_Scn *scngrp)
__libdw_seterrno (DWARF_E_NOMEM); /* no memory. */
return NULL;
}
- rwlock_init (result->dwarf_lock);
+ mutex_init (result->dwarf_lock);
eu_search_tree_init (&result->cu_tree);
eu_search_tree_init (&result->tu_tree);
eu_search_tree_init (&result->split_tree);
@@ -129,7 +129,7 @@ dwarf_end (Dwarf *dwarf)
if (dwarf->mem_tails != NULL)
free (dwarf->mem_tails);
pthread_rwlock_destroy (&dwarf->mem_rwl);
- rwlock_fini (dwarf->dwarf_lock);
+ mutex_fini (dwarf->dwarf_lock);
/* Free the pubnames helper structure. */
free (dwarf->pubnames_sets);
@@ -92,9 +92,9 @@ dwarf_formref_die (Dwarf_Attribute *attr, Dwarf_Die *result)
bool scan_debug_types = false;
do
{
- rwlock_wrlock(attr->cu->dbg->dwarf_lock);
+ mutex_lock (attr->cu->dbg->dwarf_lock);
cu = __libdw_intern_next_unit (attr->cu->dbg, scan_debug_types);
- rwlock_unlock(attr->cu->dbg->dwarf_lock);
+ mutex_unlock (attr->cu->dbg->dwarf_lock);
if (cu == NULL)
{
@@ -170,12 +170,12 @@ dwarf_getalt (Dwarf *main)
if (main == NULL)
return NULL;
- rwlock_wrlock(main->dwarf_lock);
+ mutex_lock (main->dwarf_lock);
/* Only try once. */
if (main->alt_dwarf == (void *) -1)
{
- rwlock_unlock (main->dwarf_lock);
+ mutex_unlock (main->dwarf_lock);
return NULL;
}
@@ -185,7 +185,7 @@ dwarf_getalt (Dwarf *main)
if (main->alt_dwarf != NULL)
{
result = main->alt_dwarf;
- rwlock_unlock (main->dwarf_lock);
+ mutex_unlock (main->dwarf_lock);
return result;
}
@@ -195,12 +195,12 @@ dwarf_getalt (Dwarf *main)
if (main->alt_dwarf == NULL)
{
main->alt_dwarf = (void *) -1;
- rwlock_unlock (main->dwarf_lock);
+ mutex_unlock (main->dwarf_lock);
return NULL;
}
result = main->alt_dwarf;
- rwlock_unlock (main->dwarf_lock);
+ mutex_unlock (main->dwarf_lock);
return result;
}
@@ -35,7 +35,7 @@
void
dwarf_setalt (Dwarf *main, Dwarf *alt)
{
- rwlock_wrlock (main->dwarf_lock);
+ mutex_lock (main->dwarf_lock);
if (main->alt_fd != -1)
{
@@ -45,6 +45,6 @@ dwarf_setalt (Dwarf *main, Dwarf *alt)
}
main->alt_dwarf = alt;
- rwlock_unlock (main->dwarf_lock);
+ mutex_unlock (main->dwarf_lock);
}
INTDEF (dwarf_setalt)
@@ -266,7 +266,7 @@ struct Dwarf
/* The dwarf_lock is a read-write lock designed to ensure thread-safe access
and modification of an entire Dwarf object. */
- rwlock_define(, dwarf_lock);
+ mutex_define(, dwarf_lock);
/* Internal memory handling. This is basically a simplified thread-local
reimplementation of obstacks. Unfortunately the standard obstack
@@ -249,7 +249,7 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
if (found != NULL)
return *found;
- rwlock_wrlock (dbg->dwarf_lock);
+ mutex_lock (dbg->dwarf_lock);
if (start < *next_offset)
__libdw_seterrno (DWARF_E_INVALID_DWARF);
@@ -276,7 +276,7 @@ __libdw_findcu (Dwarf *dbg, Dwarf_Off start, bool v4_debug_types)
}
}
- rwlock_unlock (dbg->dwarf_lock);
+ mutex_unlock (dbg->dwarf_lock);
return result;
}