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

[h.b.furuseth@usit.uio.no]

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.)

>>>>> How can a single thread ID be represented by multiple bit patterns?
>>>> A struct/union with padding bytes seems the least unlikely possibility.
>> (...)
>> 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.

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?

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.

>>>>> 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.

-- 
Regards,
Hallvard