Re: (ITS#4943) tpool.c pause vs. finish

[hyc@symas.com]

h.b.furuseth@usit.uio.no wrote:
> Howard Chu writes:
>> Hallvard B Furuseth wrote:
>>> hyc@symas.com writes:
>>>> h.b.furuseth@usit.uio.no wrote:
>>>>> * When a thread is finishing, make it go active for context_reset().
>>>>>   Should make slapi module destructors safer, and I don't see that it
>>>>>   can hurt.
>>>> I don't understand this.
>>> Let me dig a bit back in the ITS...
>>>   "context_reset() can call connection_fake_destroy(), which via
>>>   slapi_int_free_object_extensions() calls slapi module destructors,
>>>   which can do who-knows-what."
>> context_reset() can only be called from the main thread. It can (must)
>> never be called by anything running from the pool.
> 
> It's called from pool_wrapper().  pool_wrapper() could go active so
> there can be no pause while that context_reset() call is running.

Oh, ok. This is pretty much a non-issue because threads in the pool stay live 
until slapd shuts down. The one exception is for a cn=config modify that 
decreases the size of the pool to below the number of currently active threads, 
in which case some number of threads will exit immediately. I don't see any 
potential for conflict here.

>>>>> * Scheduling: If several threads call pool_pause(), (...)
> 
> (need to check out how it should work...)
> 
>>>>>   - pool_context() breaks if several ldap_pvt_thread_t bit patterns can
>>>>>     represent the same thread ID: TID_HASH() would give different hashes
>>>>>     for the same thread, and pool_context() stops searching when it hits
>>>>>     a slot with ctx == NULL.  (My previous bug report was a bit wrong.)
>>>> How can a single thread ID be represented by multiple bit patterns?
>>> A struct/union with padding bytes seems the least unlikely possibility.
> 
> Note that I did say "least unlikely" rather than "most likely":
> It's not that I expect to encounter it, more that I'd prefer not to be
> surprised in the form of a slapd malfuntion.
> 
> So in that respect, my preference (not for RE23) is to cast to an
> integer for the hash, and also to add pthread_<set/get>specific() so
> thread_keys[] and its hash will only be a fallback solution.

Casting to an int only works with simple types, the point of using the hash was 
to provide a solution that works for both simple and structured thread IDs. If 
you want to #ifdef the code so the two cases are handled differently, then just 
get rid of TID_HASH for the integral case.

I'm still leary of the pthread TLS support, and making that change will require 
us to update all of the other thread package support, some of which I don't 
have access to build/test any more (e.g. SunOS4 LWP).

>>>> The best fix may be to just use the address of the thread's stack as the
>>>> thread ID. That's actually perfect for our purpose.
>>> How does a function called from somewhere inside the thread know that
>>> address?  That's almost what the user context is, and thus what
>>> thread_keys[] maps the thread ID to.
>> It requires some degree of machine-dependence, which is why I never
>> bothered to code it. The principle is simple though - all of our thread
>> stacks are some multiple of 1MB in size. Take the address of any local
>> variable, mask off the low 20 bits, and you have a unique thread ID (+/-
>> some additional masking/shifting based on the actual thread stack size).
> 
> I'm not sure how to determine _which_ multiple of 1MB - start two
> threads and compare their stack addresses, maybe?  In any case, I
> agree it sounds rather more machine-dependent than the current code.

That's what the LDAP_PVT_THREAD_STACK_SIZE macro tells you. It's really not 
hard. If we were to disallow increasing the number of threads at runtime, we 
could simply allocate a block of thread stacks all at once, which would then 
allow our thread IDs to be simple integers from 0-N.

-- 
   -- Howard Chu
   Chief Architect, Symas Corp.  http://www.symas.com
   Director, Highland Sun        http://highlandsun.com/hyc/
   Chief Architect, OpenLDAP     http://www.openldap.org/project/