On Fri, Jul 12, 2013 at 11:16:16PM +0100, Daniel Stone wrote: > On 12 July 2013 22:40, Jamey Sharp <ja...@minilop.net> wrote: > > Hmm. XInitThreads wasn't designed to be used this way. For instance, > > initializing thread support isn't thread-safe, for fairly obvious > > reasons. > > > > This patch might mask more bugs than it causes, and thereby be a net > > win. But it seems equally likely to turn out the other way. > > > > I'd suggest an awful lot of caution here. > > Hmm. So the failure mode here would be n threads both calling > XOpenDisplay simultaneously, all using the Displays independently > (i.e. never mixing threads) and not having called XInitThreads. If > XInitThreads gets entered simultaneously between the test and > assignment of _Xglobal_lock, then we'll leak n - 1 copies of all the > mutexes. The real disaster is if pthread_mutex_init isn't thread-safe > (non-atomic pointer stores?), in which case the mutex pointers are > going to be half of one and half of the other. > > So there's a theoretical API break for that case, but that can also be > fixed by calling XInitThreads beforehand, which I think is acceptable.
I mostly agree with your reasoning, except the subtleties of memory ordering semantics make this even more murky. Calling XInitThreads first only helps if it's followed by a write barrier, and the later calls are preceded by read barriers. I think pthread_create should have the right barrier semantics in the parent and child threads, so if you call XInitThreads before pthread_create, it should be fine--but POSIX apparently doesn't specify anything about that. > The immediate provocation was the Mali GLES/EGL implementation, which > uses multiple threads itself, and thus relies on XInitThreads having > been called somewhere; so if you ever use that specific > implementation, every app has to call XInitThreads first to ensure it > doesn't die horribly. Yeah, I won't argue that XInitThreads was ever a good idea... :-/ For that matter, I'm not certain why --disable-xthreads is still allowed. In fact, I don't really mean to argue against the patch. I'm just proposing caution. Jamey
signature.asc
Description: Digital signature
_______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel
