volatile/atomic issues in the new BDB cache

First issue: the new "spinlock" appears to use CAS more than
necessary, I think it can be simplified to this:

Index: subversion/libsvn_fs_base/bdb/env.c
===================================================================
--- subversion/libsvn_fs_base/bdb/env.c (revision 18266)
+++ subversion/libsvn_fs_base/bdb/env.c (working copy)
@@ -362,17 +362,13 @@
       if (apr_err)
         {
           /* Tell other threads that the initialisation failed. */
- svn__atomic_cas(&bdb_cache_state,
- BDB_CACHE_INIT_FAILED,
- BDB_CACHE_START_INIT);
+ svn__atomic_set(&bdb_cache_state, BDB_CACHE_INIT_FAILED);
           return svn_error_create(apr_err, NULL,
                                   "Couldn't initialize the cache of"
                                   " Berkeley DB environment descriptors");
         }
 
- svn__atomic_cas(&bdb_cache_state,
- BDB_CACHE_INITIALIZED,
- BDB_CACHE_START_INIT);
+ svn__atomic_set(&bdb_cache_state, BDB_CACHE_INITIALIZED);
 #endif /* APR_HAS_THREADS */
     }
 #if APR_HAS_THREADS
@@ -387,9 +383,7 @@
                                 " Berkeley DB environment descriptors");
 
       apr_sleep(APR_USEC_PER_SEC / 1000);
- cache_state = svn__atomic_cas(&bdb_cache_state,
- BDB_CACHE_UNINITIALIZED,
- BDB_CACHE_UNINITIALIZED);
+ cache_state = svn__atomic_read(&bdb_cache_state);
     }
 #endif /* APR_HAS_THREADS */
 

Second issue: bdb_cache_state is declared as "volatile svn__atomic_t"
and always accessed through the svn__atomic_xxx functions, and that
combination is obviously intended to provide "thread safety". I guess
it's reasonable in practice :) However, the svn__atomic_xxx functions
are also used to access the the panic flag in bdb_env_t and yet that
flag is declared as plain svn_boolean_t, that's confusing.

If the svn__atomic_xxx calls are necessary then I think the panic flag
should be "volatile svn__atomic_t", however I don't really see why the
svn__atomic_xxx calls are needed or even why the flag is needed. In
bdb_cache_get the flag is redundant, it serves to avoid a BDB
get_flags() call but only when the flag is TRUE and there is little
point in such micro-optimisations in the "recovery needed" path. The
other uses of the panic flag are in the code used to close the
database, again it seems unnecessary to avoid a get_flags() call in
that code. I think we should remove the panic flag and substitute
get_flags() calls in the code to close the database.

-- 
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org