Re: volatile/atomic issues in the new BDB cache

Branko Èibej <brane@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