svn commit: r1527103 - /subversion/trunk/subversion/libsvn_subr/cache_config.c

[st...@apache.org]

Author: stefan2
Date: Fri Sep 27 22:12:15 2013
New Revision: 1527103

URL: http://svn.apache.org/r1527103
Log:
Make the membuffer cache initialization code atomic and being executed
at most once.

Previously, multiple theads could be trying to create the cache
simultaneously.  The code dealt with that after the fact safely,
hence this is not a correctness issue.  However, it causes a spike
in memory consumption likely followed by an OOM error or the server
process being killed by the OS.

* subversion/libsvn_subr/cache_config.c
  (initialize_cache): actual cache creation code factored out and
                      simplified from ...
  (svn_cache__get_global_membuffer_cache): ... this; use atomic init.

Modified:
    subversion/trunk/subversion/libsvn_subr/cache_config.c

Modified: subversion/trunk/subversion/libsvn_subr/cache_config.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/libsvn_subr/cache_config.c?rev=1527103&r1=1527102&r2=1527103&view=diff
==============================================================================
--- subversion/trunk/subversion/libsvn_subr/cache_config.c (original)
+++ subversion/trunk/subversion/libsvn_subr/cache_config.c Fri Sep 27 22:12:15 2013
@@ -23,6 +23,7 @@
 #include <apr_atomic.h>
 
 #include "svn_cache_config.h"
+#include "private/svn_atomic.h"
 #include "private/svn_cache.h"
 
 #include "svn_pools.h"
@@ -69,30 +70,27 @@ svn_cache_config_get(void)
   return &cache_settings;
 }
 
-/* Access the process-global (singleton) membuffer cache. The first call
- * will automatically allocate the cache using the current cache config.
- * NULL will be returned if the desired cache size is 0 or if the cache
- * could not be created for some reason.
+/* Initializer function as required by svn_atomic__init_once.  Allocate
+ * the process-global (singleton) membuffer cache and return it in the
+ * svn_membuffer_t * in *BATON.  POOL is unused and should be NULL.
  */
-svn_membuffer_t *
-svn_cache__get_global_membuffer_cache(void)
+static svn_error_t *
+initialize_cache(void *baton, apr_pool_t *pool)
 {
-  static svn_membuffer_t * volatile cache = NULL;
+  svn_membuffer_t **cache_p = baton;
+  svn_membuffer_t *cache = NULL;
 
   apr_uint64_t cache_size = cache_settings.cache_size;
-  if (!cache && cache_size)
+  if (cache_size)
     {
       svn_error_t *err;
 
-      svn_membuffer_t *old_cache = NULL;
-      svn_membuffer_t *new_cache = NULL;
-
       /* auto-allocate cache */
       apr_allocator_t *allocator = NULL;
       apr_pool_t *pool = NULL;
 
       if (apr_allocator_create(&allocator))
-        return NULL;
+        return SVN_NO_ERROR;
 
       /* Ensure that we free partially allocated data if we run OOM
        * before the cache is complete: If the cache cannot be allocated
@@ -112,11 +110,11 @@ svn_cache__get_global_membuffer_cache(vo
        */
       apr_pool_create_ex(&pool, NULL, NULL, allocator);
       if (pool == NULL)
-        return NULL;
+        return SVN_NO_ERROR;
       apr_allocator_owner_set(allocator, pool);
 
       err = svn_cache__membuffer_cache_create(
-          &new_cache,
+          &cache,
           (apr_size_t)cache_size,
           (apr_size_t)(cache_size / 5),
           0,
@@ -129,33 +127,40 @@ svn_cache__get_global_membuffer_cache(vo
        */
       if (err)
         {
-          /* Memory and error cleanup */
-          svn_error_clear(err);
+          /* Memory cleanup */
           svn_pool_destroy(pool);
 
-          /* Prevent future attempts to create the cache. However, an
-           * existing cache instance (see next comment) remains valid.
-           */
+          /* Document that we actually don't have a cache. */
           cache_settings.cache_size = 0;
 
-          /* The current caller won't get the cache object.
-           * However, a concurrent call might have succeeded in creating
-           * the cache object. That call and all following ones will then
-           * use the successfully created cache instance.
-           */
-          return NULL;
+          return svn_error_trace(err);
         }
 
-      /* Handle race condition: if we are the first to create a
-       * cache object, make it our global singleton. Otherwise,
-       * discard the new cache and keep the existing one.
-       *
-       * Cast is necessary because of APR bug:
-       * https://issues.apache.org/bugzilla/show_bug.cgi?id=50731
-       */
-      old_cache = apr_atomic_casptr((volatile void **)&cache, new_cache, NULL);
-      if (old_cache != NULL)
-        svn_pool_destroy(pool);
+      /* done */
+      *cache_p = cache;
+    }
+
+  return SVN_NO_ERROR;
+}
+
+/* Access the process-global (singleton) membuffer cache. The first call
+ * will automatically allocate the cache using the current cache config.
+ * NULL will be returned if the desired cache size is 0 or if the cache
+ * could not be created for some reason.
+ */
+svn_membuffer_t *
+svn_cache__get_global_membuffer_cache(void)
+{
+  static svn_membuffer_t *cache = NULL;
+  static svn_atomic_t initialized = 0;
+
+  svn_error_t *err
+    = svn_atomic__init_once(&initialized, initialize_cache, &cache, NULL);
+  if (err)
+    {
+      /* no caches today ... */
+      svn_error_clear(err);
+      return NULL;
     }
 
   return cache;