[strongSwan-dev] [PATCH] log: do not acquire lock if nobody is listening
christophe.gouault at 6wind.com
Fri Apr 18 10:42:12 CEST 2014
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
>>> if (ref_cur(&this->max_level[group]) < level &&
>>> ref_cur(&this->max_vlevel[group]) < level)
>> 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  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
> 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,
>  http://git.strongswan.org/?p=strongswan.git;a=commitdiff;h=f2b930bc1
More information about the Dev