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

[Christophe Gouault]

Hi Tobias,

Thanks for reviewing my patches.

On 04/14/2014 06:31 PM, Tobias Brunner wrote:
> Hi Christoph,
>
>> +	if (this->max_level[group] < level &&
>> +		this->max_vlevel[group] < level)
>> +		return;
>
> Unfortunately, this might not be thread-safe.  It's essentially the same
> problem I described in my response to your half-open IKE_SA patch.
> Because the log levels/logger registrations can be changed at runtime
> the threads that are calling vlog() might not be aware of these updates.

I agree it is not thread safe, but the only risk I suspect it that a log 
listener might lose a message at the time it registers or increases its 
loglevel (assuming that reading this->max_level[group] or 
this->max_vlevel[group] is atomic), in which case it does not seem to be 
a serious problem.

What is your opinion?

>> +  49.87%  charon  [kernel.kallsyms]                [k] do_raw_spin_lock
>
> I was not able to reproduce numbers that come even close on a x64 Ubuntu
> Linux system.  On what platform/architecture are you running this?  And
> how exactly did you determine the numbers?  For instance, what is charon
> doing during your perf record run?  And how is it configured (e.g.
> charon.threads)?

I was running charon on an x86_64, with 8 cores/16 hyperthreads, on a 
debian 7.4 system.

Here is my strongswan.conf:

charon {
	dos_protection=no
	install_routes=no
	ikesa_table_size = 65536
	ikesa_table_segments = 16384
	cookie_threshold = 0
	block_threshold = 0
	threads = 64

	syslog {
		daemon {
			default = -1
		}
	}
	filelog {
		stderr {
			default = -1
		}
	}
}

charon replies to IKEv2 negotiations initiated by a remote load-tester 
at a high throughput.

> It seems strange that acquiring an *uncontested* read lock (the write
> lock is only very rarely held, in your case probably never) can produce
> such a massive overhead.  Acquiring such a lock should be possible
> without syscall, just some atomic compare-and-swap operation should be
> sufficient.

I was also surprised by this behavior. I didn't expect spin_locks to be 
invoked on a read lock.

> Unless, of course, your architecture does not support such atomic
> operations, hence the library has to resort to using a regular
> (spin-)lock.  Or if the library just is not optimized very much and
> always does this.
>
> It might not be a bad idea to check if there is any potential in
> optimizing the pthread_rwlock_rdlock implementation in the pthread
> library you are using.

I am using debian's libpthread-2.13.so for x86_64. I would reasonably 
expect that it supports optimized read locks.

Regards,
Christophe

> Since this rwlock is not the only one in the
> strongSwan code base it might increase the overall performance (but this
> is definitely the lock that is most often acquired).
>
> To reduce the overhead somewhat you could also compile strongSwan with
> reduced log statements (see [1]).
>
> Regards,
> Tobias
>
> [1]
> http://wiki.strongswan.org/projects/strongswan/wiki/LoggerConfiguration#Compile-time-configuration
>