Re: commit: ldap/libraries/libldap_r rmutex.c threads.c

[Hallvard B Furuseth <h.b.furuseth@usit.uio.no>]

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