apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brian Pane <brian.p...@cnet.com>
Subject Re: Showstopper ... was: Tagged the tree
Date Wed, 08 Jan 2003 16:16:34 GMT
On Wed, 2003-01-08 at 07:15, William A. Rowe, Jr. wrote:

> Just an observation reviewing the apr/includes/ changes... I don't like the
> look of this code;
> 
> +#define apr_atomic_casptr(mem,with,cmp) (void*)atomic_cmpxchg((unsigned long *)(mem),(unsigned
long)(cmp),(unsigned long)(with))
> 
> Very simply, this is a very easily broken macro... it is too easy to silently 
> pass the wrong pointer arg to apr_atomic_casptr and explode.
> 
> We need some type safety here, and this macro (I believe) destroys
> all hope of that.  It seems this needs to become a function.  Macro to
> function should be a safe change, since code compiled to an earlier
> apr will continue to use this unsafe but otherwise usable macro.

There's a good reason why the atomic CAS can't be a function.
Its most common use is in spinlocks, where the probability of
having to retry the CAS increases with every clock cycle you
spend in the "if (cas failed) {retry;}" loop.  Adding a function
call around the CAS would double the length of the code path
between the lookup of the old value and the compare-and-swap
of the new value, which in turn doubles the probability that
the CAS will fail and require a retry.

As for type safety, keep in mind that the role of
apr_atomic_casptr is to atomically swap any pointer to
an object.  If we were to turn it into a function, its first
argument would have to be a void**.


> To the dev@apr members, anyone feel like calling this 0.9.2 in conjunction
> with httpd's next release and pushing on to 0.9.3-dev???

Okay with me.

Brian



Mime
View raw message