You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@apr.apache.org by sf...@apache.org on 2014/05/09 22:14:01 UTC
svn commit: r1593614 - in /apr/apr/trunk: CHANGES configure.in
memory/unix/apr_pools.c
Author: sf
Date: Fri May 9 20:14:01 2014
New Revision: 1593614
URL: http://svn.apache.org/r1593614
Log:
Option to detect concurrent accesses to pools
Enabled by new configure option --enable-pool-concurrency-check.
Compared to pool-owner-debugging, this only detects cases where there is actual
contention between accesses. The advantage is that runtime costs should be
relatively low.
The diagnostic messages could still use some improvement.
Modified:
apr/apr/trunk/CHANGES
apr/apr/trunk/configure.in
apr/apr/trunk/memory/unix/apr_pools.c
Modified: apr/apr/trunk/CHANGES
URL: http://svn.apache.org/viewvc/apr/apr/trunk/CHANGES?rev=1593614&r1=1593613&r2=1593614&view=diff
==============================================================================
--- apr/apr/trunk/CHANGES [utf-8] (original)
+++ apr/apr/trunk/CHANGES [utf-8] Fri May 9 20:14:01 2014
@@ -1,6 +1,10 @@
-*- coding: utf-8 -*-
Changes for APR 2.0.0
+ *) Add new --enable-pool-concurrency-check configure option to detect
+ thread-unsafe concurrent accesses to pools. Runtime costs should be
+ relatively low. [Stefan Fritsch]
+
*) Add apr_sockaddr_info_copy(), for making a deep copy of an
apr_sockaddr_t into a specified pool. [Yann Ylavic
<ylavic.dev gmail.com>]
Modified: apr/apr/trunk/configure.in
URL: http://svn.apache.org/viewvc/apr/apr/trunk/configure.in?rev=1593614&r1=1593613&r2=1593614&view=diff
==============================================================================
--- apr/apr/trunk/configure.in (original)
+++ apr/apr/trunk/configure.in Fri May 9 20:14:01 2014
@@ -1492,6 +1492,14 @@ AC_ARG_ENABLE(allocator-uses-mmap,
fi ]
)
+AC_ARG_ENABLE(pool-concurrency-check,
+ [ --enable-pool-concurrency-check Check for concurrent usage of memory pools],
+ [ if test "$enableval" = "yes"; then
+ AC_DEFINE(APR_POOL_CONCURRENCY_CHECK, 1,
+ [Define if pool functions should abort if concurrent usage is detected])
+ fi ]
+)
+
dnl ----------------------------- Checks for standard typedefs
AC_TYPE_OFF_T
AC_TYPE_PID_T
Modified: apr/apr/trunk/memory/unix/apr_pools.c
URL: http://svn.apache.org/viewvc/apr/apr/trunk/memory/unix/apr_pools.c?rev=1593614&r1=1593613&r2=1593614&view=diff
==============================================================================
--- apr/apr/trunk/memory/unix/apr_pools.c (original)
+++ apr/apr/trunk/memory/unix/apr_pools.c Fri May 9 20:14:01 2014
@@ -49,6 +49,11 @@
int apr_running_on_valgrind = 0;
#endif
+#if APR_POOL_CONCURRENCY_CHECK && !APR_HAS_THREADS
+#error pool-concurrency-check does not make sense without threads
+#endif
+
+
/*
* Magic numbers
*/
@@ -540,6 +545,10 @@ struct apr_pool_t {
apr_os_proc_t owner_proc;
#endif /* defined(NETWARE) */
cleanup_t *pre_cleanups;
+#if APR_POOL_CONCURRENCY_CHECK
+ volatile apr_uint32_t in_use;
+ apr_os_thread_t in_use_by;
+#endif /* APR_POOL_CONCURRENCY_CHECK */
};
#define SIZEOF_POOL_T APR_ALIGN_DEFAULT(sizeof(apr_pool_t))
@@ -673,6 +682,65 @@ APR_DECLARE(void) apr_pool_terminate(voi
#define node_free_space(node_) ((apr_size_t)(node_->endp - node_->first_avail))
/*
+ * Helpers to mark pool as in-use/free. Used for finding thread-unsafe
+ * concurrent accesses from different threads.
+ */
+#if APR_POOL_CONCURRENCY_CHECK
+static void pool_concurrency_abort(apr_pool_t *pool, const char *func, apr_uint32_t old)
+{
+ fprintf(stderr, "%s: previous state %d in_use_by/cur %lx/%lx pool %p(%s)\n",
+ func, old,
+ (unsigned long)pool->in_use_by,
+ (unsigned long)apr_os_thread_current(),
+ pool, pool->tag);
+ abort();
+}
+
+static APR_INLINE void pool_concurrency_set_used(apr_pool_t *pool)
+{
+ apr_uint32_t old;
+
+ old = apr_atomic_cas32(&pool->in_use, 1, 0);
+
+ if (old == 2 && pool->in_use_by == apr_os_thread_current()) {
+ /* cleanups may use the pool */
+ return;
+ }
+
+ if (old != 0)
+ pool_concurrency_abort(pool, __func__, old);
+
+ pool->in_use_by = apr_os_thread_current();
+}
+
+static APR_INLINE void pool_concurrency_set_idle(apr_pool_t *pool)
+{
+ apr_uint32_t old;
+
+ old = apr_atomic_cas32(&pool->in_use, 0, 1);
+
+ if (old != 1)
+ pool_concurrency_abort(pool, __func__, old);
+}
+
+static APR_INLINE void pool_concurrency_init(apr_pool_t *pool)
+{
+ pool->in_use = 0;
+}
+
+static APR_INLINE void pool_concurrency_set_destroyed(apr_pool_t *pool)
+{
+ apr_atomic_set32(&pool->in_use, 3);
+ pool->in_use_by = apr_os_thread_current();
+}
+#else
+static APR_INLINE void pool_concurrency_init(apr_pool_t *pool) { }
+static APR_INLINE void pool_concurrency_set_used(apr_pool_t *pool) { }
+static APR_INLINE void pool_concurrency_set_idle(apr_pool_t *pool) { }
+static APR_INLINE void pool_concurrency_set_destroyed(apr_pool_t *pool) { }
+#endif /* APR_POOL_CONCURRENCY_CHECK */
+
+/*
* Memory allocation
*/
@@ -682,12 +750,14 @@ APR_DECLARE(void *) apr_palloc(apr_pool_
void *mem;
apr_size_t size, free_index;
+ pool_concurrency_set_used(pool);
size = APR_ALIGN_DEFAULT(in_size);
#if HAVE_VALGRIND
if (apr_running_on_valgrind)
size += 2 * REDZONE;
#endif
if (size < in_size) {
+ pool_concurrency_set_idle(pool);
if (pool->abort_fn)
pool->abort_fn(APR_ENOMEM);
@@ -708,6 +778,7 @@ APR_DECLARE(void *) apr_palloc(apr_pool_
}
else {
if ((node = allocator_alloc(pool->allocator, size)) == NULL) {
+ pool_concurrency_set_idle(pool);
if (pool->abort_fn)
pool->abort_fn(APR_ENOMEM);
@@ -743,14 +814,17 @@ APR_DECLARE(void *) apr_palloc(apr_pool_
have_mem:
#if HAVE_VALGRIND
if (!apr_running_on_valgrind) {
+ pool_concurrency_set_idle(pool);
return mem;
}
else {
mem = (char *)mem + REDZONE;
VALGRIND_MEMPOOL_ALLOC(pool, mem, in_size);
+ pool_concurrency_set_idle(pool);
return mem;
}
#else
+ pool_concurrency_set_idle(pool);
return mem;
#endif
}
@@ -786,7 +860,10 @@ APR_DECLARE(void) apr_pool_clear(apr_poo
/* Run pre destroy cleanups */
run_cleanups(&pool->pre_cleanups);
+
+ pool_concurrency_set_used(pool);
pool->pre_cleanups = NULL;
+ pool_concurrency_set_idle(pool);
/* Destroy the subpools. The subpools will detach themselves from
* this pool thus this loop is safe and easy.
@@ -796,6 +873,8 @@ APR_DECLARE(void) apr_pool_clear(apr_poo
/* Run cleanups */
run_cleanups(&pool->cleanups);
+
+ pool_concurrency_set_used(pool);
pool->cleanups = NULL;
pool->free_cleanups = NULL;
@@ -814,13 +893,17 @@ APR_DECLARE(void) apr_pool_clear(apr_poo
APR_IF_VALGRIND(VALGRIND_MEMPOOL_TRIM(pool, pool, 1));
- if (active->next == active)
+ if (active->next == active) {
+ pool_concurrency_set_idle(pool);
return;
+ }
*active->ref = NULL;
allocator_free(pool->allocator, active->next);
active->next = active;
active->ref = &active->next;
+
+ pool_concurrency_set_idle(pool);
}
APR_DECLARE(void) apr_pool_destroy(apr_pool_t *pool)
@@ -830,7 +913,10 @@ APR_DECLARE(void) apr_pool_destroy(apr_p
/* Run pre destroy cleanups */
run_cleanups(&pool->pre_cleanups);
+
+ pool_concurrency_set_used(pool);
pool->pre_cleanups = NULL;
+ pool_concurrency_set_idle(pool);
/* Destroy the subpools. The subpools will detach themselve from
* this pool thus this loop is safe and easy.
@@ -840,6 +926,7 @@ APR_DECLARE(void) apr_pool_destroy(apr_p
/* Run cleanups */
run_cleanups(&pool->cleanups);
+ pool_concurrency_set_destroyed(pool);
/* Free subprocesses */
free_proc_chain(pool->subprocesses);
@@ -985,6 +1072,8 @@ APR_DECLARE(apr_status_t) apr_pool_creat
pool->ref = NULL;
}
+ pool_concurrency_init(pool);
+
*newpool = pool;
return APR_SUCCESS;
@@ -1050,6 +1139,8 @@ APR_DECLARE(apr_status_t) apr_pool_creat
#endif /* defined(NETWARE) */
if (!allocator)
pool_allocator->owner = pool;
+
+ pool_concurrency_init(pool);
*newpool = pool;
return APR_SUCCESS;
@@ -1195,6 +1286,7 @@ APR_DECLARE(char *) apr_pvsprintf(apr_po
apr_memnode_t *active, *node;
apr_size_t free_index;
+ pool_concurrency_set_used(pool);
ps.node = active = pool->active;
ps.pool = pool;
ps.vbuff.curpos = ps.node->first_avail;
@@ -1257,8 +1349,10 @@ APR_DECLARE(char *) apr_pvsprintf(apr_po
/*
* Link the node in if it's a new one
*/
- if (!ps.got_a_new_node)
+ if (!ps.got_a_new_node) {
+ pool_concurrency_set_idle(pool);
return strp;
+ }
active = pool->active;
node = ps.node;
@@ -1275,8 +1369,10 @@ APR_DECLARE(char *) apr_pvsprintf(apr_po
active->free_index = free_index;
node = active->next;
- if (free_index >= node->free_index)
+ if (free_index >= node->free_index) {
+ pool_concurrency_set_idle(pool);
return strp;
+ }
do {
node = node->next;
@@ -1286,9 +1382,11 @@ APR_DECLARE(char *) apr_pvsprintf(apr_po
list_remove(active);
list_insert(active, node);
+ pool_concurrency_set_idle(pool);
return strp;
error:
+ pool_concurrency_set_idle(pool);
if (pool->abort_fn)
pool->abort_fn(APR_ENOMEM);
if (ps.got_a_new_node) {
Re: svn commit: r1593614 - in /apr/apr/trunk: CHANGES configure.in memory/unix/apr_pools.c
Posted by Stefan Fritsch <sf...@sfritsch.de>.
On Thursday 15 May 2014 10:03:25, Ruediger Pluem wrote:
> > + if (old == 2 && pool->in_use_by == apr_os_thread_current()) {
>
> How can old ever be 2?
That was a leftover from a version with a separate state for the pool
being cleared.
>
> > + /* cleanups may use the pool */
> > + return;
> > + }
> > +
> > + if (old != 0)
> > + pool_concurrency_abort(pool, __func__, old);
>
> Isn't __func__ C99?
Thanks for the review. Both issues fixed in r1595549.
Stefan
Re: svn commit: r1593614 - in /apr/apr/trunk: CHANGES configure.in
memory/unix/apr_pools.c
Posted by Ruediger Pluem <rp...@apache.org>.
sf@apache.org wrote:
> Author: sf
> Date: Fri May 9 20:14:01 2014
> New Revision: 1593614
>
> URL: http://svn.apache.org/r1593614
> Log:
> Option to detect concurrent accesses to pools
>
> Enabled by new configure option --enable-pool-concurrency-check.
>
> Compared to pool-owner-debugging, this only detects cases where there is actual
> contention between accesses. The advantage is that runtime costs should be
> relatively low.
>
> The diagnostic messages could still use some improvement.
>
>
> Modified:
> apr/apr/trunk/CHANGES
> apr/apr/trunk/configure.in
> apr/apr/trunk/memory/unix/apr_pools.c
>
> Modified: apr/apr/trunk/memory/unix/apr_pools.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/memory/unix/apr_pools.c?rev=1593614&r1=1593613&r2=1593614&view=diff
> ==============================================================================
> --- apr/apr/trunk/memory/unix/apr_pools.c (original)
> +++ apr/apr/trunk/memory/unix/apr_pools.c Fri May 9 20:14:01 2014
> @@ -673,6 +682,65 @@ APR_DECLARE(void) apr_pool_terminate(voi
> #define node_free_space(node_) ((apr_size_t)(node_->endp - node_->first_avail))
>
> /*
> + * Helpers to mark pool as in-use/free. Used for finding thread-unsafe
> + * concurrent accesses from different threads.
> + */
> +#if APR_POOL_CONCURRENCY_CHECK
> +static void pool_concurrency_abort(apr_pool_t *pool, const char *func, apr_uint32_t old)
> +{
> + fprintf(stderr, "%s: previous state %d in_use_by/cur %lx/%lx pool %p(%s)\n",
> + func, old,
> + (unsigned long)pool->in_use_by,
> + (unsigned long)apr_os_thread_current(),
> + pool, pool->tag);
> + abort();
> +}
> +
> +static APR_INLINE void pool_concurrency_set_used(apr_pool_t *pool)
> +{
> + apr_uint32_t old;
> +
> + old = apr_atomic_cas32(&pool->in_use, 1, 0);
> +
> + if (old == 2 && pool->in_use_by == apr_os_thread_current()) {
How can old ever be 2?
> + /* cleanups may use the pool */
> + return;
> + }
> +
> + if (old != 0)
> + pool_concurrency_abort(pool, __func__, old);
Isn't __func__ C99?
Regards
Rüdiger