[strongSwan-dev] [PATCH] log: do not acquire lock if nobody is listening

[Christophe Gouault]

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
>