[Date Prev][Date Next]
[Chronological]
[Thread]
[Top]
Re: (ITS#4943) tpool.c pause vs. finish
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/