[XrdTls] Make the thread-id returned to libcrypto appear more uniform for openssl 1.0 #2085
Conversation
src/XrdTls/XrdTlsContext.cc
Outdated
| @@ -366,9 +378,11 @@ const char *sslCiphers = "ALL:!LOW:!EXP:!MD5:!MD2"; | |||
|
|
|||
| XrdSysMutex ctxMutex; | |||
| #if __cplusplus >= 201103L | |||
| std::atomic<bool> initDone( false ); | |||
| std::atomic<int> initTlsDone( 0 ); | |||
There was a problem hiding this comment.
So, what is wrong with using an atomic bool?
There was a problem hiding this comment.
Ah, that was because I was changing that area because to not lose the debug initialisation even if Init() had already been called from elsewhere, and I noticed that our AtomicGet may be defined "__sync_fetch_and_or" and on the gcc docs here: https://gcc.gnu.org/onlinedocs/gcc/_005f_005fsync-Builtins.html it mentioned "The object pointed to by the first argument must be of integer or pointer type. It must not be a boolean type."; so I changed it.
There was a problem hiding this comment.
See my comment on the underlying issue here.
smithdh
force-pushed
the
xrdtls_tid_mixing
branch
3 times, most recently
from
7a5cc77
to
40fa6f6
Compare
There was a problem hiding this comment.
So, all of this looks fine. Last question (promise) as noted in the comment. I ask simply because I haven't looked at how OpenSSL handles its hash table so you can easily say that isn't a good solution; which is fine but at least we will have considered all the options.
| // So mix bits so that the table's hash function gives | ||
| // better distribution. | ||
|
|
||
| unsigned long x = (unsigned long)XrdSysThread::ID(); |
There was a problem hiding this comment.
So, all of this looks fine. Last question (promise) would using the LWP (i.e. process number) be better than the manipulation of the pthread_id which is somewhat not so random pointer. I ask because if this is used as an index to a hash table of size n then you would be best suited with a index value known to be unique and increases by 1 within a well defined small range so n%x fans out as much as possible.; which one would think using the LWP as x would accomplish that. If you think that would be the case then instead of using XrdSysThread::ID() you should use XrdSysThread::Num() here and like dispense with the "mixer".
|
Thanks; I like that idea. I looked, and the actual hash that the openssl table is using is h=key*13 (where key is e.g. XrdSysThread::Num()); but indeed the Num() based id should be much better, and then no need for 'mixer'. I'll revise the PR to use the Num(). |
smithdh
force-pushed
the
xrdtls_tid_mixing
branch
from
40fa6f6
to
69f3d32
Compare
|
gettid (used by Num()) is a system call, whereas the ID() (using pthread_self()) is a singe instruction [on linux]. so possibly there's a bit more overhead. |
|
Hi @abh3 I did a quick test just now: On a single thread test program on an centos7 machine, single threaded repeated syscall with gettid gave about 2MHz. Instead repeated pthread_self() + mix was about 100 the rate. My best guess at the frequency we're calling the sslTLS_id_callback() function in production was about ~1kHz, that was just based on the number of retrieves recorded in the openssl hash-table statistics and assuming there was 1 call to the callback for each. But 100 times is quite a bit slower: I think for now I'll leave the PR with the XrdSysThread::ID() + mix. I believe Guilherme was thinking of tagging 5.6.2 tomorrow, what do you think about including the PR still with ID()+mix, but possibly subject to later re-visit (and perhaps we anyway want to revisit to include ERR_remove_thread_state() in a future change, anyway)? Or would you favor holding this now, to give more time to consider, and maybe measure more carefully? |
smithdh
force-pushed
the
xrdtls_tid_mixing
branch
from
69f3d32
to
5112542
Compare
|
Hi @smithdh good to know. I was not aware that it was that much slower. Then we are all set. |
Connected to issue #2084