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

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

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.

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

> Any implementation that leaves uninitialized padding bytes would be a
> bug.  Tools like valgrind would barf all over them.

No.  Valid compiler, valid code.  Therefore valgrind is silent about
copying uninitialized values, it only complains when you use them for
something - like write() or arithmetic.  So a valgrind complaint about
an uninitialized _value_ isn't necessary at the point the uninitialized
_variable_ is read.  E.g. this complains in printf(), not in foo():
  int foo()
  {
    struct { char c; /*maybe padding here*/ int j; } s;
    s.c = 1;
    s.j = 2;
    return ((unsigned char *)&s)[1]; /* maybe padding byte */
  }
  int main()
  {
    printf("%d\n", foo());
  }

>> A pointer in hardware where several address representations map to the
>> same physical address, and the compiler handles that by normalizing
>> pointers when they are compared/subtracted.  Like DOS "huge" pointers
>> would have been if the compiler normalized them when comparing them
>> instead of when incrementing/decrementing them.  20-bit address bus,
>> 32-bit pointers, physical address = 16 * <segment half of pointer> +
>> <offset half of pointer>.
>
> Nobody voluntarily uses such memory models today. The very mention of "DOS"
> invalidates this discussion since there is no threading environment there.

Right, at this point I answered how it _can_ happen, not why I expect to
encounter it.  I could see the demands of hardware design cause strange
memory models to proliferate again, but I'm not holding my breath.

I'd be a bit less surprised to encounter integers with padding bits
again, but not for a simple ID type, and hashing an integer instead of
its bytes would fix it anyway.

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

-- 
Regards,
Hallvard