[PATCH RFC] introduce dl_iterate_phdr_parallel
Gleb Natapov
gleb@scylladb.com
Sun Jul 31 13:51:00 GMT 2016
Sorry, I missed your answer since I am not subscribed and the reply was
sent to the mailing list only.
On Thu, Jul 28, 2016 at 05:47:16PM -0300, Adhemerval Zanella wrote:
>
>
> On 25/07/2016 11:23, Gleb Natapov wrote:
> > Problem: exception handling is not scalable. Attached program mt.cc
> > demonstrates this easily. It runs around 1 seconds with 1 thread, but
> > with only 4 threads it takes 4.5 seconds to complete. The reason is
> > locks that are taken on unwind path.
> >
> > There are two locks right now.
> >
> > Fist is in libgcc's _Unwind_Find_registered_FDE and it is eliminated by
> > https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01629.html (for dynamic
> > executables only, but this is common case).
> >
> > Second one is dl_load_write_lock in __dl_iterate_phdr. It serves dual
> > purpose: it stops the list of loaded objects from been modified while
> > iterating over it and it makes sure that more than one callback will
> > not run in parallel. This means that even if we will find a cleaver way
> > to make __dl_iterate_phdr iterate over the objects list locklessly the
> > lock will still have to be taken around the call to callback to preserve
> > second guaranty.
> >
> > This patch here propose to introduce another API: dl_iterate_phdr_parallel
> > which does exactly same thing as dl_iterate_phdr but may run more than
> > one provided callback in parallel. And to make it more scalable it
> > breaks single dl_load_write_lock into arrays of locks. Reader takes only
> > one lock, but writer has to lock all of them to proceed with modifying
> > the list.
>
> Wouldn't a read-write lock solve the performance problem at dl_iterate_phdr?
> It should scale better than current code, since dl_iterate_phdr should act
> just a reader on link_map structures and multiple readers should only incur
> in a lll_lock/lll_unlock on rwlock->__data.__lock. I am assuming that
> we set it to prefer readers and use a read-lock on dl_iterate_phdr.
>
I did try read-write lock first and it did not improved the test case
at all. The reason is (as I was very surprised to discover) that rwlock
takes a lock even if there are only readers. Though the lock is taken
for a short period of time when it becomes congested scalability drops
to the floor since all new waiters go to sleep in the kernel.
Attached is a, rather hacky, patch that I used for testing.
> I see this slight better than trading memory and a limited scalability
> (the _DL_LOCKS value) and also avoid adds another exported API.
The memory tradeoff is rather small. Using rwlock will not avoid
API addition though since two callbacks will be able to run in
parallel and this is user visible difference in behaviour from current
dl_iterate_phdr. C++ exception handling code, for instance, relies on the
fact that callbacks do not run in parallel. They use global cache which
I made to be per thread in the libgcc patch.
--
Gleb.
-------------- next part --------------
commit a8c321ce2d8c3afe9043ad514e89eccda2e88802
Author: Gleb Natapov <gleb@scylladb.com>
Date: Sun Jul 24 11:25:07 2016 +0300
use rwlock in dl-iteratephdr.c
diff --git a/elf/dl-close.c b/elf/dl-close.c
index 687d7de..0440fb9 100644
--- a/elf/dl-close.c
+++ b/elf/dl-close.c
@@ -535,7 +535,8 @@ _dl_close_worker (struct link_map *map, bool force)
tls_free_start = tls_free_end = NO_TLS_OFFSET;
/* We modify the list of loaded objects. */
- __rtld_lock_lock_recursive (GL(dl_load_write_lock));
+// __rtld_lock_lock_recursive (GL(dl_load_write_lock));
+ __rtld_rwlock_wrlock (GL(dl_load_write_lock));
/* Check each element of the search list to see if all references to
it are gone. */
@@ -748,7 +749,8 @@ _dl_close_worker (struct link_map *map, bool force)
}
}
- __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+// __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+ __rtld_rwlock_unlock (GL(dl_load_write_lock));
/* If we removed any object which uses TLS bump the generation counter. */
if (any_tls)
diff --git a/elf/dl-iteratephdr.c b/elf/dl-iteratephdr.c
index 1cb6e26..aa4d0a5 100644
--- a/elf/dl-iteratephdr.c
+++ b/elf/dl-iteratephdr.c
@@ -25,7 +25,8 @@
static void
cancel_handler (void *arg __attribute__((unused)))
{
- __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+ //__rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+ __rtld_rwlock_unlock (GL(dl_load_write_lock));
}
hidden_proto (__dl_iterate_phdr)
@@ -38,7 +39,8 @@ __dl_iterate_phdr (int (*callback) (struct dl_phdr_info *info,
int ret = 0;
/* Make sure nobody modifies the list of loaded objects. */
- __rtld_lock_lock_recursive (GL(dl_load_write_lock));
+// __rtld_lock_lock_recursive (GL(dl_load_write_lock));
+ __rtld_rwlock_rdlock (GL(dl_load_write_lock));
__libc_cleanup_push (cancel_handler, NULL);
/* We have to determine the namespace of the caller since this determines
@@ -80,7 +82,8 @@ __dl_iterate_phdr (int (*callback) (struct dl_phdr_info *info,
/* Release the lock. */
__libc_cleanup_pop (0);
- __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+// __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+ __rtld_rwlock_unlock (GL(dl_load_write_lock));
return ret;
}
diff --git a/elf/dl-object.c b/elf/dl-object.c
index 362992b..c7952a1 100644
--- a/elf/dl-object.c
+++ b/elf/dl-object.c
@@ -31,7 +31,8 @@ internal_function
_dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
{
/* We modify the list of loaded objects. */
- __rtld_lock_lock_recursive (GL(dl_load_write_lock));
+// __rtld_lock_lock_recursive (GL(dl_load_write_lock));
+ __rtld_rwlock_wrlock (GL(dl_load_write_lock));
if (GL(dl_ns)[nsid]._ns_loaded != NULL)
{
@@ -48,7 +49,8 @@ _dl_add_to_namespace_list (struct link_map *new, Lmid_t nsid)
new->l_serial = GL(dl_load_adds);
++GL(dl_load_adds);
- __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+// __rtld_lock_unlock_recursive (GL(dl_load_write_lock));
+ __rtld_rwlock_unlock (GL(dl_load_write_lock));
}
diff --git a/elf/dl-support.c b/elf/dl-support.c
index c30194c..f05d341 100644
--- a/elf/dl-support.c
+++ b/elf/dl-support.c
@@ -213,8 +213,8 @@ __rtld_lock_define_initialized_recursive (, _dl_load_lock)
/* This lock is used to keep __dl_iterate_phdr from inspecting the
list of loaded objects while an object is added to or removed from
that list. */
-__rtld_lock_define_initialized_recursive (, _dl_load_write_lock)
-
+//__rtld_lock_define_initialized_recursive (, _dl_load_write_lock)
+__libc_rwlock_define_initialized (,_dl_load_write_lock)
#ifdef HAVE_AUX_VECTOR
int _dl_clktck;
diff --git a/elf/rtld.c b/elf/rtld.c
index 647661c..da91c4e 100644
--- a/elf/rtld.c
+++ b/elf/rtld.c
@@ -128,7 +128,8 @@ struct rtld_global _rtld_global =
._dl_stack_flags = DEFAULT_STACK_PERMS,
#ifdef _LIBC_REENTRANT
._dl_load_lock = _RTLD_LOCK_RECURSIVE_INITIALIZER,
- ._dl_load_write_lock = _RTLD_LOCK_RECURSIVE_INITIALIZER,
+// ._dl_load_write_lock = _RTLD_LOCK_RECURSIVE_INITIALIZER,
+ ._dl_load_write_lock = PTHREAD_RWLOCK_INITIALIZER,
#endif
._dl_nns = 1,
._dl_ns =
@@ -693,6 +694,22 @@ rtld_lock_default_unlock_recursive (void *lock)
{
__rtld_lock_default_unlock_recursive (lock);
}
+
+static void
+rtld_rwlock_default_wrlock (void *lock)
+{
+}
+
+static void
+rtld_rwlock_default_rdlock (void *lock)
+{
+}
+
+static void
+rtld_rwlock_default_unlock (void *lock)
+{
+}
+
#endif
@@ -763,6 +780,9 @@ dl_main (const ElfW(Phdr) *phdr,
&& defined __rtld_lock_default_lock_recursive
GL(dl_rtld_lock_recursive) = rtld_lock_default_lock_recursive;
GL(dl_rtld_unlock_recursive) = rtld_lock_default_unlock_recursive;
+ GL(dl_rtld_rwlock_wrlock) = rtld_rwlock_default_wrlock;
+ GL(dl_rtld_rwlock_rdlock) = rtld_rwlock_default_rdlock;
+ GL(dl_rtld_rwlock_unlock) = rtld_rwlock_default_unlock;
#endif
/* The explicit initialization here is cheaper than processing the reloc
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index bdbdfed..a53f914 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -475,6 +475,9 @@ __pthread_initialize_minimal_internal (void)
keep the lock count from the ld.so implementation. */
GL(dl_rtld_lock_recursive) = (void *) __pthread_mutex_lock;
GL(dl_rtld_unlock_recursive) = (void *) __pthread_mutex_unlock;
+ GL(dl_rtld_rwlock_wrlock) = (void *) __pthread_rwlock_wrlock;
+ GL(dl_rtld_rwlock_rdlock) = (void *) __pthread_rwlock_rdlock;
+ GL(dl_rtld_rwlock_unlock) = (void *) __pthread_rwlock_unlock;
unsigned int rtld_lock_count = GL(dl_load_lock).mutex.__data.__count;
GL(dl_load_lock).mutex.__data.__count = 0;
while (rtld_lock_count-- > 0)
diff --git a/sysdeps/generic/ldsodefs.h b/sysdeps/generic/ldsodefs.h
index f68fdf4..b80c7eb 100644
--- a/sysdeps/generic/ldsodefs.h
+++ b/sysdeps/generic/ldsodefs.h
@@ -334,7 +334,8 @@ struct rtld_global
/* This lock is used to keep __dl_iterate_phdr from inspecting the
list of loaded objects while an object is added to or removed
from that list. */
- __rtld_lock_define_recursive (EXTERN, _dl_load_write_lock)
+ //__rtld_lock_define_recursive (EXTERN, _dl_load_write_lock)
+ __libc_rwlock_define (EXTERN, _dl_load_write_lock)
/* Incremented whenever something may have been added to dl_loaded. */
EXTERN unsigned long long _dl_load_adds;
@@ -372,6 +373,9 @@ struct rtld_global
&& defined __rtld_lock_default_lock_recursive
EXTERN void (*_dl_rtld_lock_recursive) (void *);
EXTERN void (*_dl_rtld_unlock_recursive) (void *);
+ EXTERN void (*_dl_rtld_rwlock_wrlock) (void *);
+ EXTERN void (*_dl_rtld_rwlock_rdlock) (void *);
+ EXTERN void (*_dl_rtld_rwlock_unlock) (void *);
#endif
/* If loading a shared object requires that we make the stack executable
diff --git a/sysdeps/nptl/libc-lockP.h b/sysdeps/nptl/libc-lockP.h
index 50b86d2..e91e021 100644
--- a/sysdeps/nptl/libc-lockP.h
+++ b/sysdeps/nptl/libc-lockP.h
@@ -236,6 +236,28 @@ typedef pthread_key_t __libc_key_t;
__libc_maybe_call (__pthread_mutex_unlock, (&(NAME).mutex), 0)
#endif
+#ifdef SHARED
+# define __rtld_rwlock_wrlock(NAME) \
+ GL(dl_rtld_rwlock_wrlock) (&(NAME))
+
+# define __rtld_rwlock_rdlock(NAME) \
+ GL(dl_rtld_rwlock_rdlock) (&(NAME))
+
+# define __rtld_rwlock_unlock(NAME) \
+ GL(dl_rtld_rwlock_unlock) (&(NAME))
+
+#else
+# define __rtld_rwlock_wrlock(NAME) \
+ __libc_maybe_call (__pthread_rwlock_wrlock, (&(NAME)), 0)
+
+# define __rtld_rwlock_rdlock(NAME) \
+ __libc_maybe_call (__pthread_rwlock_rdlock, (&(NAME)), 0)
+
+# define __rtld_rwlock_unlock(NAME) \
+ __libc_maybe_call (__pthread_rwlock_unlock, (&(NAME)), 0)
+
+#endif
+
/* Define once control variable. */
#if PTHREAD_ONCE_INIT == 0
/* Special case for static variables where we can avoid the initialization
More information about the Libc-alpha
mailing list