Philip Martin wrote:
>Philip Martin <philip@codematters.co.uk> writes:
>
>
>
>>brane@tigris.org writes:
>>
>>
>>>+ const char **dir_name;
>>>+ for (dir_name = valid_dir_names; *dir_name; ++dir_name)
>>>+ if (0 == strcmp (name, *dir_name))
>>>+ {
>>>+ void *new_adm_dir_name = (void*) *dir_name;
>>>+ void *old_adm_dir_name = adm_dir_name;
>>>
>>>
>>Perhaps 'const void *' and lose the cast?
>>
>>
>>
>>>+ if (old_adm_dir_name != apr_atomic_casptr (&adm_dir_name,
>>>+ new_adm_dir_name,
>>>+ old_adm_dir_name))
>>>+ {
>>>+ /* Another thread won the race to change the name. */
>>>+ return svn_error_create
>>>+ (APR_EAGAIN, NULL,
>>>+ _("Could not set the administrative directory name"));
>>>+ }
>>>
>>>
>
>This mail might be about to make me look stupid :)
>
>Examining this a bit more closely I'm not sure what problem you are
>trying to solve with that CAS.
>
>With the CAS there is a race between the line:
> old_adm_dir_name = adm_dir_name
>and the "pseudo line":
> adm_dir_name = new_adm_dir_name
>and the CAS will detect that race if it occurs. However the function
>doesn't really care about the original value of adm_dir_name, all it
>wants to do is set the new value. So that race only exists because
>you are using CAS, if the code simply did:
> adm_dir_name = new_adm_dir_name
>there would be no race.
>
>Another reason for using CAS is that the adm_dir_name type might not
>be atomic, and using CAS means that simultaneous writes won't leave
>adm_dir_name in an invalid state. However, looking at the generic CAS
>implementation in apr_atomic.c it appears that CAS won't help readers
>while the write is in progress, since the readers don't lock the
>mutex. I suppose I could use APR's --enable-nonportable-atomics to
>avoid the generic implementation, is that recommended? What about
>machines that have to use the generic implementation? Any machine
>where void* is non-atomic will probably have to use the generic
>implementation. I can't see from the documentation what guarantees
>APR's CAS is meant to provide.
>
>Another reason for using CAS would be to ensure that values written in
>one thread become visible in another thread. However, if threads are
>not otherwise syncronised but nevertheless expect svn_wc_set_adm_dir
>to affect each other then that looks like an application design error.
>Once the threads do synchronise then the data will become visible
>without any need for the CAS.
>
>
Ah-ha. Yes, we have to use CAS because void* may not be atomic. But,
what's more, we have to use CAS _everywhere_, not just in
svn_wc_set_adm_dir. Using a NULL comparator and exchange value will
effectively change the CAS to an atomic read. That won't avoid the race,
but it will address your other concerns. (No, I'm not prepared to assume
that void* is atomic, _especially_ not on MP machines.)
-- Brane
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Mon Sep 26 08:04:23 2005