Hi Tobias,
On 04/17/2014 06:09 PM, Tobias Brunner wrote:
Hi Christoph,
At least we should make sure the read is atomic. Something like this
perhaps?
if (ref_cur(&this->max_level[group]) < level &&
ref_cur(&this->max_vlevel[group]) < level)
{
return;
}
Yes, right. Even if in most cases, reading an enum is atomic, some
platforms do not guarantee it.
In this case, we should perform atomic operation wherever these levels
are read or manipulated, even under read or write lock.
I pushed a commit [1] to the atomic-ref branch that implements this
using the two flavors of GCC atomic built-ins directly.
It's unfortunate that reading the value with the __sync built-ins
requires such an expensive operation (and that GCC seems unable to
optimize it even with the second argument set to 0). Well, better safe
than sorry, I guess, and in your case it is probably still faster than
having to acquire the read lock. Plus with GCC 4.7 or newer you get the
faster __atomic built-ins.
I reviewed the code and tested it, it's great. There is a little impact
on performance, but the additional cost of atomic reading remains
negligible compared to the gain of not locking.
The check is disabled completely if these built-ins are not available,
because fallbacks (like those for ref_*) would not improve the
performance. But since even the atomic implementation allows races an
alternative approach would be to use __atomic* if available and just
directly read and write if not (thus avoiding the __sync penalty). We
could make max_[v]level an int32_t array to be reasonably safe.
This sounds reasonable to simply read and write when atomic macros are
not available. I may be wrong, but I think that modern processors always
read or write an int32_t atomically, provided it is aligned on a 32 bit
boundary.
Let me know what you think.
We should probably also add support for clang's __c11 built-ins (and
check for stdatomic.h). So I guess adding a more generic abstraction
layer for this would make sense.
I also think it makes sense.
Thanks and regards,
Christophe
Regards,
Tobias
[1] http://git.strongswan.org/?p=strongswan.git;a=commitdiff;h=f2b930bc1
_______________________________________________
Dev mailing list
[email protected]
https://lists.strongswan.org/mailman/listinfo/dev
