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

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

hyc@symas.com wrote:
>h.b.furuseth@usit.uio.no wrote:
>> (...)
>> But it would cleaner to ch_malloc an integer instead of the (void*)
>> cast.  Then it won't depend on integer->pointer->integer conversions
>> preserving the value either.
>
> Ugh. That's just silly. We should just typedef a union that's big enough
> to hold a pointer or an integer...

Eh?  Then we'd need to change every setkey/getkey call to use set/get
that union instead of a simple pointer.

> (...)
>> Was that an instruction/permission to axe it or not?
>
> Yes, go ahead.

Too late, I #ifdeffed instead:-)

>> (...)
>> I do not understand the calls to ldap_pvt_thread_pool_context_reset()
>> in slapd (init.c and bconfig.c).  That frees the thread's sl_malloc
>> memory with slap_sl_mem_destroy(), which seems rather hazardous until
>> the thread is really, really done.
>
> Those calls are from the main thread, and only occur during startup.

And in slap_destroy().

> The point is to free up that memory since the main thread will never
> need it again.

Ah.  I guess I need to figure out just what is allowed to use
slap_sl_malloc() and what isn't.

>> For that matter, I guess that means context_reset() had better release
>> keys in the reverse order of how they were set.  That usually happened,
>> but not always, and my clear_key_idx() made the "not always" more
>> common.  So to close the gap from a deleted key I'll make it move all
>> later keys forward, instead of just moving the latest key to the gap.
>
> I don't think the order is really important... ??

Well, I was thinking if some key could manage to get in front of the
sl_malloc key and then be deleted, and then some key which sl_malloced
memory ended up before it.  But as noted above I don't have a clue:-)

Also see tpool.c 1.30.2.17: "ITS#4805 free thread keys in reverse
order".  However I don't see from the ITS notes how it is related.

>> Bdb uses the database environment address as a key to pool_setkey().
>> So there can be at most ~30 BDB databases with an active key at the
>> same time in the same thread.  What does that mean - 30 overlays
>> all using bdb?  Access control groups spanning 30 databases?
>
> Seems so. We could raise this limit if it's a problem, but I don't see
> why anyone would use 30 separate databases instead of just one.
> Configuring the caches would be a real bear...

Well, it's the most natural way to set up an LDAP hotel - 1 database
per hosted organization, where each org can configure its own indexes
etc.  But cache config is definitely an argument against it.

It's no less strange to have e.g. 100 databases than 30, so I'm not
suggesting to raise the limit in OpenLDAP.  Though I suppose we could
make it a default, overridden by CPPFLAGS=-DLDAP_THREAD_MAXKEYS=<num>.

>> Nitpick: Slapd uses both function pointers and data pointers as keys to
>> pool_[sg]etkey().  That makes it possible for two "different" keys to
>> compare equal, on a host where functions and data live in different
>> segments that otherwise share the same address space.  So I guess I
>> should clean up and replace (void*)<function> keys with the address of
>>     static char <function>_key;
>
> Hosts such as ... ? Not even OS/390 is so strange.

Don't know, but it wouldn't surprise me if SINTRAN on ND-500 worked that
way.  What I do remember is that a program or library consisted of 2-3
files: program segment, data segment and optionally link segment.

But it's likely less uncommon for function pointers to be wider than
data pointers, which OpenLDAP doesn't support anyway.

> The use of function pointers was just a convenience, since the address
> is guaranteed to be a unique value within the process and easily
> recognizable in a debugger. We could easily use addresses of static
> variables instead but it's silly to introduce new variables just for
> this purpose.

Someday I want to revisit this and use OS-supported thread-specific
keys, then it'll change anyway.

-- 
Regards,
Hallvard