You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Philip Martin <ph...@codematters.co.uk> on 2006/01/28 00:21:19 UTC
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
Re: volatile/atomic issues in the new BDB cache
Posted by Branko Čibej <br...@xbc.nu>.
Branko Čibej wrote:
> Philip Martin wrote:
>> 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.
>>
> The real purpose of the panic flag is to avoid calls to DB_ENV->open
> if we know that the cached environment handle is useless. Once the
> environment is panicked, it's important to release all references to
> it quickly, so that the queue of processes waiting to do the recovery
> doesn't get too long.
>
> At first I'd considered removing a broken environment from the cache,
> but that created races that would've been really hard to avoid. So I
> left it in the cache and marked it useless instead. This also made the
> reference counting cleaner.
Oh yes, I almost forgot -- earlier BDB versions don't have the
DB_ENV->get_flags function, so the only way to notice a panicked
environment is by catching DB_RUNRECOVERY errors in DB->close (or
DB_ENV->close). So (once I commit the patch I asked Ben to test) that
micro-optimisation will only be used with BDB-4.4 and later, and that's
a good thing because it means that a broken environment will get
released that much sooner.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: volatile/atomic issues in the new BDB cache
Posted by Branko Čibej <br...@xbc.nu>.
Philip Martin wrote:
> Branko Čibej <br...@xbc.nu> writes:
>
>
>> Philip Martin wrote:
>>
>>> First issue: the new "spinlock" appears to use CAS more than
>>> necessary, I think it can be simplified to this:
>>>
>>>
>> Don't we all wish. Saith apr_atomic.h:
>>
>>> * @warning do not mix apr_atomic's with the CAS function.
>>> * on some platforms they may be implemented by different mechanisms
>>>
>
> I think we should add a similar warning to the svn__atomic macros.
>
I agree; I'll do this and the volatile thing while I'm fixing the
memory-free bug you found with valgrind.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: volatile/atomic issues in the new BDB cache
Posted by Philip Martin <ph...@codematters.co.uk>.
Branko Čibej <br...@xbc.nu> writes:
> Philip Martin wrote:
>> First issue: the new "spinlock" appears to use CAS more than
>> necessary, I think it can be simplified to this:
>>
> Don't we all wish. Saith apr_atomic.h:
>> * @warning do not mix apr_atomic's with the CAS function.
>> * on some platforms they may be implemented by different mechanisms
I think we should add a similar warning to the svn__atomic macros.
>> 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.
>>
> Really? Saith bdb_env_t in env.c:
>> svn__atomic_t panic;
Yes, I made a mistake there.
> It's not svn_boolean_t, but I agree it should be volatile.
Good.
> The return
> value of svn_fs_bdb__get_panic is boolean, but that's irrelevant.
Perhaps that's why I made my mistake; I agree it's OK.
--
Philip Martin
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: volatile/atomic issues in the new BDB cache
Posted by Branko Čibej <br...@xbc.nu>.
Philip Martin wrote:
> First issue: the new "spinlock" appears to use CAS more than
> necessary, I think it can be simplified to this:
>
Don't we all wish. Saith apr_atomic.h:
> * @warning do not mix apr_atomic's with the CAS function.
> * on some platforms they may be implemented by different mechanisms
> 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.
>
Really? Saith bdb_env_t in env.c:
> svn__atomic_t panic;
It's not svn_boolean_t, but I agree it should be volatile. The return
value of svn_fs_bdb__get_panic is boolean, but that's irrelevant.
> 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.
>
The real purpose of the panic flag is to avoid calls to DB_ENV->open if
we know that the cached environment handle is useless. Once the
environment is panicked, it's important to release all references to it
quickly, so that the queue of processes waiting to do the recovery
doesn't get too long.
At first I'd considered removing a broken environment from the cache,
but that created races that would've been really hard to avoid. So I
left it in the cache and marked it useless instead. This also made the
reference counting cleaner.
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org