classification
Title: Native TLS support for pthreads
Type: performance Stage:
Components: Interpreter Core Versions: Python 3.2
process
Status: closed Resolution: accepted
Dependencies: 9797 Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, giampaolo.rodola, kristjan.jonsson, loewis, pitrou
Priority: normal Keywords: needs review, patch

Created on 2010-09-07 05:28 by kristjan.jonsson, last changed 2010-09-20 02:12 by kristjan.jonsson. This issue is now closed.

Files
File name Uploaded Description Edit
pthread_tls.patch kristjan.jonsson, 2010-09-07 05:28
pthread_tls.patch kristjan.jonsson, 2010-09-13 03:30
pthread_tls.patch kristjan.jonsson, 2010-09-13 08:00
pthread_tls.patch kristjan.jonsson, 2010-09-17 06:27
Messages (19)
msg115742 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-07 05:28
The following patch adds native TLS implementation for pthreads, avoiding the relatively slow and clunky default tls implemented in thread.c
msg115748 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2010-09-07 10:54
The patch looks good.
Apart from PyGILState_Ensure(), are there other parts of the code that use these functions?
msg115760 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-07 13:42
Just tried to apply the patch:
- test_capi now freezes in auto-thread-state
- test_threading now freezes in test_finalize_runnning_thread
msg115836 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-08 01:10
The code may need a little bit of extra work:  The pthreads machine that I have to work with is a PS3 :).
And, despite the documentation not saying, I suspect that pthread_getspecific() actually does mess with "errno", making it necessary to preserve "errno" when calling PyThread_get_key_value().  (The reason for this is documented in thread_nt.h)

I'll do some tests and let you know.
msg115849 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-08 05:15
"errno" is preserved by PyEval_RestoreThread(), so this isn't the issue.
What you are probably seeing is latent race conditions in the test suite, made apparent by a non-locking TLS implementation.  The test suite isn't free from those, see for example issue 8799
msg115851 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-08 06:03
Hm, both the test you mention are using the (non-recursive) lock to synchronize threads.  I can't see anything wrong there.

Could you please try to replace the cod in pthread_getspecific() with this:

int err = errno
void *result = pthread_getspecific(key);
errno = err;
return result;

If this fixes those cases, then there is code somewhere that relies on errno being maintained across these calls.
msg115859 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-08 11:12
The problem turns out to be in pystate.c (my system returns 0 as a valid key number). See issue #9797.
msg115877 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-08 14:50
Ah, good to hear.
msg116179 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2010-09-12 12:30
The ifdef should go; pthreads always support TLS (since XPG5, 1997).
msg116272 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-13 03:15
I left the ifdef in for a quick and easy way to disable this code for those interested, but I'm happy to remove it if it makes for greater synergy.
msg116275 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-13 03:30
Added a new patch with #ifdef remvoved, for greater harmony.
msg116284 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2010-09-13 06:47
It seems we need to clarify the return value of PyThread_create_key. The patch returns 0 in case of failure, which is clearly wrong as 0 is also a valid key.

I think it's safe to return -1: TlsAlloc returns TLS_OUT_OF_INDEXES, which is 0xFFFFFFFF, i.e. -1 on a 32-bit system.
msg116288 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-13 08:00
I've changed the function as you suggest, although there are in fact no failure detection semantics defined for PyThread_create_key().  See e.g. thread.c:294

/* Return a new key.  This must be called before any other functions in
 * this family, and callers must arrange to serialize calls to this
 * function.  No violations are detected.
 */
int
PyThread_create_key(void)
msg116292 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2010-09-13 08:35
Am 13.09.2010 10:00, schrieb Kristján Valur Jónsson:
> 
> Kristján Valur Jónsson <kristjan@ccpgames.com> added the comment:
> 
> I've changed the function as you suggest, although there are in fact no failure detection semantics defined for PyThread_create_key().  

Right. That's why I'm thinking that we should introduce some. TLS
creation *can* fail, and it is unfortunate that the original
introduction of this API forgot to consider that. Your patch did
(de facto) introduce an error return code (originally 0, now -1).
So it would be good if this was documented, and the NT version adjusted.

In principle, this is really difficult to get right. AFAICT,
pthread_key_t doesn't even have to be an integral type, and even
if it is, it may well become -1. However, we can probably worry
about this when a system comes along where this implementation
breaks.
msg116309 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-13 14:30
> Right. That's why I'm thinking that we should introduce some. TLS
> creation *can* fail, and it is unfortunate that the original
> introduction of this API forgot to consider that. Your patch did
> (de facto) introduce an error return code (originally 0, now -1).

Perhaps we can simply call Py_FatalError() instead? There's not much we
can do when such a failure occurs anyway.
msg116345 - (view) Author: Martin v. Löwis (loewis) * (Python committer) Date: 2010-09-13 20:04
> Perhaps we can simply call Py_FatalError() instead? There's not much we
> can do when such a failure occurs anyway.

Sounds fine as well. Python's own usage definitely shouldn't fail. If
extension modules use this in an aggressive manner, they may actually
exhaust TLS keys (at least on Windows, where old versions had only
64 slots). Having the interpreter fail then sounds reasonable.
msg116646 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-17 06:27
Ok, here is a patch.  key creation returns -1 on error, and the caller can detect this and raise a fatal error.
msg116868 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-19 12:29
Patch looks good to me.
msg116905 - (view) Author: Kristján Valur Jónsson (kristjan.jonsson) * (Python committer) Date: 2010-09-20 02:12
Committed as revision 84914
History
Date User Action Args
2010-09-20 02:12:18kristjan.jonssonsetstatus: open -> closed
resolution: accepted
messages: + msg116905
2010-09-19 12:29:16pitrousetmessages: + msg116868
2010-09-17 06:27:22kristjan.jonssonsetfiles: + pthread_tls.patch

messages: + msg116646
2010-09-13 20:04:47loewissetmessages: + msg116345
2010-09-13 14:30:38pitrousetmessages: + msg116309
2010-09-13 08:35:30loewissetmessages: + msg116292
2010-09-13 08:00:29kristjan.jonssonsetfiles: + pthread_tls.patch

messages: + msg116288
2010-09-13 06:47:24loewissetmessages: + msg116284
2010-09-13 03:30:14kristjan.jonssonsetfiles: + pthread_tls.patch

messages: + msg116275
2010-09-13 03:15:45kristjan.jonssonsetmessages: + msg116272
2010-09-12 12:30:58loewissetkeywords: patch, patch, needs review
nosy: + loewis
messages: + msg116179

2010-09-08 18:21:47giampaolo.rodolasetkeywords: patch, patch, needs review
nosy: + giampaolo.rodola
2010-09-08 14:50:38kristjan.jonssonsetkeywords: patch, patch, needs review

messages: + msg115877
2010-09-08 11:12:37pitrousetkeywords: patch, patch, needs review

dependencies: + wrong assumption in pystate.c
messages: + msg115859
2010-09-08 06:03:44kristjan.jonssonsetkeywords: patch, patch, needs review

messages: + msg115851
2010-09-08 05:15:15kristjan.jonssonsetkeywords: patch, patch, needs review

messages: + msg115849
2010-09-08 01:10:39kristjan.jonssonsetkeywords: patch, patch, needs review

messages: + msg115836
2010-09-07 13:42:19pitrousetkeywords: patch, patch, needs review
nosy: + pitrou
messages: + msg115760

2010-09-07 10:54:51amaury.forgeotdarcsetkeywords: patch, patch, needs review
nosy: + amaury.forgeotdarc
messages: + msg115748

2010-09-07 05:28:55kristjan.jonssoncreate