Howard Chu writes: >Hallvard B Furuseth wrote: >> Opheader.oh_tid (alias Operation.o_tid) is never set, yet >> overlays/accesslog.c uses the value as the owner thread ID. >> (...) > > Hm, I don't know what happened with oh_tid, but I think it would be > better to actually set it, in case others need the thread ID in the > future. On some systems pthread_self() is an expensive call, best to > avoid calling it needlessly, especially since we have only a small > pool of threads; we can get the thread IDs once and never need to ask > again.
Actually, I don't see any code in OpenLDAP which needs the thread ID from ldap_pvt_thread_self(). What it it needs is _some_ value which is unique to the thread. In slapd, the operation's thread context will do nicely for that - including in your rmutex, since the caller passes it an ID. So I suggest we switch from thread ID to thread-specific variables. A thread implementation should have those, since errno is thread-specific. E.g. pthreads has pthread_key_create() & co. We can use libldap_r/tpool.c:ldap_pvt_thread_pool_context() as a fallback. That one uses the thread ID to implement one thread-specific variable as the "thread context". But its implementation can be inefficient if the thread ID type has padding bits or something like that. Some compilers also support thread-specific non-auto variables, e.g. extern <__thread or __declspec(thread)> char *foo; If I understand correctly that's unsafe in Microsoft's DLLs because an already loaded Windows program has a fixed number of thread-specific variables and a DLL can't introduce a new one. A compiler feature should be more efficient than library calls, but if that's the kind of troubles we can expect we should push it out quickly in a prerelase version so we have a chance to catch any similar weirdnesses in other systems as well. >> A few notes on rmutex: >> >> - I don't see why you wrap the struct it in a pointer. It does mean one >> can move an rmutex in memory, but it replaces a mutex variable which >> couldn't be moved moved. > > Just copying the approach from rdwr.c. In particular, this keeps the > rmutex itself an opaque structure, which I think is desirable overall, > without preventing its use in other structure declarations, etc. Shrug. People who insist on shooting themselves in the foot will succeed anyway. On my system (RedHat), the standard includes provide plenty of complete types, e.g. FILE, fpos_t, sigset_t, fd_set, pthread_mutex_t, pthread_cond_t. That's fine by me. It should be sufficient discouragement to make the OpenLDAP implementation a fallback implementation, and try to use one from the system if available. Though when I googled for recursive mutexes, the first seemingly relevant hit was a long rant from some Posix personality about why recursive mutexes are a bad idea, "you should fix your data structure instead". >> - In ldap_pvt_thread_rmutex_unlock(), since you do error checking here: >> ldap_pvt_thread_mutex_lock( &rm->ltrm_mutex ); >> if( !ldap_pvt_thread_equal( owner, rm->ltrm_owner )) { >> ldap_pvt_thread_mutex_unlock( &rm->ltrm_mutex ); >> return LDAP_PVT_THREAD_EINVAL; >> } >> I'd also do replace the ldap_pvt_thread_mutex_lock() with >> rc = ldap_pvt_thread_mutex_lock( &rm->ltrm_mutex ); >> if( rc ) >> return rc; >> and then maybe the other test can just as well be deleted, since >> slapd doesn't check the return value anyway:-) If so, the 'owner' >> argument to ...unlock() can be dropped. > > Removing the test makes no sense, ltrm_mutex is not held for the > duration of the rmutex. I don't understand. Is ldap_pvt_thread_rmutex_unlock(rmutex not held by the current thread); intended to be valid code? If so the ldap_pvt_thread_mutex_lock() call in that function is a bug. -- Hallvard