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

[hyc@symas.com]

h.b.furuseth@usit.uio.no wrote:
> hyc@symas.com writes:
>> h.b.furuseth@usit.uio.no wrote:
>>> 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.
> 
> Eh?  Yes, that's what I was talking about.  But I'm not sure what you
> mean with them exiting immediately - or how they could.  Also they stay
> around until there are no pending requests.  A pause request could
> arrive more or less at any time, I presume.  (I don't know if there is a
> pause while the max threads number is being modified.)

Yes. As stated back in the distant past, cn=config fully serializes all write 
operations. The pool is paused for each write op. When the op completes and the 
pool is resumed, the condition variable is broadcasted, so all the other 
threads will wake up, and the first to do so will see that they're above the 
max and exit, before any other tasks are started.

>> 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.
> 
> Yes, except it's not guaranteed to work.  Which is why I'm thinking it
> may be better to fail to compile and provide a fix if/when someone
> complains.  We did have code which compared thread IDs with '==' for
> quite some time.  Do you remember if anyone complained, or if the
> switch to using ldap_pvt_thread_equal() was a "just in case" fix?

Yes, I needed to use pthread_equal() on OS/390 since their thread type was a 
structure.

> However note that we do
>   #define ldap_int_thread_equal(a, b) ((a) == (b))
> for all our supported thread implementations except pthreads, and
> for pthreads we can test pthread_<set/get>specific() just fine
> (as a replacement for thread_keys[]).
> 
> OTOH, '==' also works for pointers that are wider than any integer type
> (and for floating-point thread IDs:-) so I guess I'm arguing against my
> own suggestion on that one if we add pthread_<set/get>specific()
> support.
> 
>> 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.
> 
> No, sould still hash to avoid clustering.
> 
>> I'm still leary of the pthread TLS support,
> 
> Me too.  But the old code was broken on real-life systems I have access
> to, not just on theoretical ones.  Still waiting to hear from Kurt if my
> fix to the first tls.c change fixed it for FreeBSD.
> 
>> 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).
> 
> What do you mean?  The TLS problem is to extract an unsigned long from a
> ldap_pvt_thread_t.  That needs no support in the thr_*.c files, only to
> know what kind of type ldap_pvt_thread_t is.  Once configure knows what
> ldap_pvt_thread_t will be typedeffed to it can test if that's a pointer,
> integer or something worse - and whether it's too wide for a long.
> 
> BTW I don't know if the unsigned long needs to be the thread ID or just
> _some_ unique integer.  A question to openssl-users@openssl.org got no
> answer.

It just needs to be a unique integer. I've already had this conversation with 
the OpenSSL folks...

>>>>>> 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.
>>> (...)
>>> 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.
> 
> Hmm.  I asked the wrong question, should have read some man pages first.
> Still, thr_cthreads.c & thr_lwp.c don't use LDAP_PVT_THREAD_STACK_SIZE.
> man pthread_attr_getstacksize says it sets the _minimum_ stack size, but
> perhaps several M is large enough that one can expect it won't get
> multiples of that.

That kinda implies that no one has been using the cthreads or lwp support 
recently, since I doubt their default stack size would work with back-bdb's 
filterindex demands.
-- 
   -- 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/