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

[hyc@symas.com]

h.b.furuseth@usit.uio.no wrote:
> I wonder if we've been waiting for each other...  Anyway, I've committed
> some pending fixes now that HEAD passes all tests for me.  I've some
> more cleanup, but if we are close to a freeze I don't know if that means
> I should rush it in or wait until afterwards.

Cleanup as in tidying the code, or as in fixing functionality?
> 
>>> 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.
> 
> I'm not sure, but I gather that's derived from the DCE threads reference
> implementation where pthread_t has three members 'field1', 'field2' and
> 'field3'.  tls.c can't stuff all of them in a long, so the old one may
> not get a unique ID.  OTOH DCE has a pthread_getunique_np() call which
> uses just the 2nd field as a unique integer thread ID, I expect we could
> use that.  Except I don't have a host to test such a change on.  Dunno
> why pthread_equal checks all three fields.  I've asked on
> comp.programming.threads.

I don't remember which version of the spec OS/390 implemented, it was certainly 
old but it wasn't DCE. Might have been Draft 6 IIRC.

> Also I note that the DCE struct will get four padding bytes on a 64-bit
> host which aligns the struct at 8*n addresses, and the compiler is not
> obliged to preserve these during struct copy.
> 
> On the other hand DCE threads are based on an extremely outdated Posix
> draft, so I don't know how relevant it is now.

Yes, Draft 4, and I don't know of any systems that still use that exclusively.

> Another time when tls.c code loses information is if pointers are wider
> than long, which does happen.  (Don't know how it is nowadays, but I
> remember plenty of postings about it some years ago.)

Probably the OpenSSL API should have used a void * here. Too bad.

> [Using address of the thread's stack]
>>>> That's what the LDAP_PVT_THREAD_STACK_SIZE macro tells you.
> 
> As modified by PTHREAD_STACK_MIN and pthread_attr_getguardsize().  And
> slight other variations, apparently.  I tried on a few hosts and Solaris
> would a few times use some extra space near the stack of a thread or two.
> 
> OTOH we could allocate stack memory ourselves and then use
> pthread_attr_setstack() to put the stack just where we want it.
> 
> However this doesn't help tls.c with threads that have been created by
> _other_ packages.  And the SSL id_callback function apparently has no
> way to return failure:-(
> 
> All in all I still like a thread-specific key better - they are the
> standard way of doing things like this.  We only need it for pthreads
> since the other thread types as far as I can tell have integer IDs.

OK, suppose we use a pthread thread-specific key here. What are you proposing 
for the unique value?

>>>> 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.
> 
> IIRC disallowing that would fix some other tpool problem too, but I
> don't remember what.

I'm not really keen on that since I like having the ability to both increase 
and decrease the setting dynamically.

> Which led me to notice that the setstacksize call now _decreases_ the
> stack size on some hosts.  I don't suppose that was intentional, so I
> guess it should be
>     size_t cursize;
>     if( pthread_attr_getstacksize( &attr, &cursize )
>         || cursize < LDAP_PVT_THREAD_STACK_SIZE )
>         pthread_attr_setstacksize( &attr, LDAP_PVT_THREAD_STACK_SIZE );
> in thr_posix and I don't know what in other thr_* files.  A RE24 change
> I guess, since reduces the max number of threads before slapd gets into
> trouble.

I don't consider this a bug. If someone wants a larger thread stack they can 
recompile to get it. Or perhaps we should make it a tunable variable and add a 
config keyword for it.

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