talloc: Other minor issues/queries

[Arran Cudbard-Bell]

>> 
>> I'm happy to put together patches...  Recording const vs non-const
>> wouldn't seem to be that contentious.  Preserving explicitly set names/
>> types seems like it'd need more discussion.
> 
> That seems reasonable to me, but I'd rather Metze also
> comment on this. It is a behavior change.

OK.

>>> Second, I'd like to remove the memlimit code. Can you be
>>> *really* clear as to what you are using them for, and
>>> can we find some other way to do this ?
>> 
>> We want to ensure heap allocations can't occur when a pool is full.
>> 
>> a. In cases where these pools are composed of mprotected pages,
>>    or pages that will be mprotected.
>> b. Where we want to trigger allocation of an additional "slab" of memory
>>    when an existing pool is used up.  This is used where we're performing
>>    configuration parsing on startup.
>> 
>> That's it, that's really our only use cases for memlimit. We never use 
>> them on any other type of chunk.
>> 
>> A flag saying "don't allow heap allocations" would be absolutely fine
>> for us in all cases, and much simpler to implement.
> 
> OK - that sounds *much* easier. An additional 'uint32_t flags'
> argument passed into talloc_pool() and talloc_pooled_object()
> with a 'CONFINE_TO_POOL' flags being the only one defined
> at present would be the way to go IMHO.

Sure, or talloc_set_pool_confined(TALLOC_CTX *ctx, bool) would 
maintain backwards compatibility/consistency, and allow the flag
to be flipped.

I guess it really depends on whether there's a desire to keep the 
current API style or move to something new.

> This would have to be a new major rev of the library
> as we'd be removing the memlimit code as well in order
> to make the internals of talloc understandable again :-).

Indeed :)

Would references would be on the chopping block for a new major
release also? I have to say that's one of the features we also use
but one we could easily replicate internally and wouldn't really mourn.

-

Now we're discussing major revs, there are a couple of other issues 
we've run into that it'd be good to get comment on.

First, is that the lack of parent pointer for the majority of chunks makes
any call that needs to access the parent ctx, very slow, when there 
are a large number of chunks at the same level.

I'm assuming it's a trade off between cheap reallocs and cheap
talloc_steal/talloc_parent_*.  Is allocing a bunch of chunks off of a 
context that gets realloced a common pattern for Samba? Am I 
missing something else that makes recording the parent in each chunk
painful?

I mention this because we had to strip out almost all calls to
talloc_steal and talloc_parent*, because they featured so heavily
in code profiling.  If it really is just a tradeoff between realloc and
talloc_parent* it might be good to revisit how parents are 
recorded.

Second, is expandable pools.

In FreeRADIUS the instance data for modules is only protected 
when a user sets a special environmental variable to specify the pool
size for all instance data memory. The user has to guess at how 
big this pool needs to be (which is pretty terrible) but it's the only way 
to handle this with the current version of talloc.

When we're building up the dictionaries we have a similar problem
in that we need to guess at how big the pool needs to be because
we can't expand it.  This leaves the pool massively oversized in the
majority of cases.

Having talloc automatically expand pools would be amazing 
(imagining a linked list of fixed size malloced chunks).  I know there's
additional complexity here, but it does seem to be limited to very
specific areas of the code.
Would this functionality ever be considered?

-Arran