msg115742 - (view) |
Author: Kristján Valur Jónsson (kristjan.jonsson) * ![Python committer (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
Date: 2010-09-08 14:50 |
Ah, good to hear.
|
msg116179 - (view) |
Author: Martin v. Löwis (loewis) * ![Python committer (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
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 (Python committer)](@@file/committer.png) |
Date: 2010-09-19 12:29 |
Patch looks good to me.
|
msg116905 - (view) |
Author: Kristján Valur Jónsson (kristjan.jonsson) * ![Python committer (Python committer)](@@file/committer.png) |
Date: 2010-09-20 02:12 |
Committed as revision 84914
|
|
Date |
User |
Action |
Args |
2022-04-11 14:57:06 | admin | set | github: 53995 |
2010-09-20 02:12:18 | kristjan.jonsson | set | status: open -> closed resolution: accepted messages:
+ msg116905
|
2010-09-19 12:29:16 | pitrou | set | messages:
+ msg116868 |
2010-09-17 06:27:22 | kristjan.jonsson | set | files:
+ pthread_tls.patch
messages:
+ msg116646 |
2010-09-13 20:04:47 | loewis | set | messages:
+ msg116345 |
2010-09-13 14:30:38 | pitrou | set | messages:
+ msg116309 |
2010-09-13 08:35:30 | loewis | set | messages:
+ msg116292 |
2010-09-13 08:00:29 | kristjan.jonsson | set | files:
+ pthread_tls.patch
messages:
+ msg116288 |
2010-09-13 06:47:24 | loewis | set | messages:
+ msg116284 |
2010-09-13 03:30:14 | kristjan.jonsson | set | files:
+ pthread_tls.patch
messages:
+ msg116275 |
2010-09-13 03:15:45 | kristjan.jonsson | set | messages:
+ msg116272 |
2010-09-12 12:30:58 | loewis | set | keywords:
patch, patch, needs review nosy:
+ loewis messages:
+ msg116179
|
2010-09-08 18:21:47 | giampaolo.rodola | set | keywords:
patch, patch, needs review nosy:
+ giampaolo.rodola
|
2010-09-08 14:50:38 | kristjan.jonsson | set | keywords:
patch, patch, needs review
messages:
+ msg115877 |
2010-09-08 11:12:37 | pitrou | set | keywords:
patch, patch, needs review
dependencies:
+ wrong assumption in pystate.c messages:
+ msg115859 |
2010-09-08 06:03:44 | kristjan.jonsson | set | keywords:
patch, patch, needs review
messages:
+ msg115851 |
2010-09-08 05:15:15 | kristjan.jonsson | set | keywords:
patch, patch, needs review
messages:
+ msg115849 |
2010-09-08 01:10:39 | kristjan.jonsson | set | keywords:
patch, patch, needs review
messages:
+ msg115836 |
2010-09-07 13:42:19 | pitrou | set | keywords:
patch, patch, needs review nosy:
+ pitrou messages:
+ msg115760
|
2010-09-07 10:54:51 | amaury.forgeotdarc | set | keywords:
patch, patch, needs review nosy:
+ amaury.forgeotdarc messages:
+ msg115748
|
2010-09-07 05:28:55 | kristjan.jonsson | create | |