talloc: Other minor issues/queries

[Arran Cudbard-Bell]

> On Oct 20, 2020, at 7:11 PM, Jeremy Allison <jra at samba.org> wrote:
> 
> On Tue, Oct 20, 2020 at 03:10:14PM -0500, Arran Cudbard-Bell via samba-technical wrote:
>> Just to keep this separate from the memlimit/pools discussion.
>> 
>> - I've noticed talloc_realloc_size clears the name/type of the chunk be 
>> allocated.  This isn't mentioned in the documentation.  Is clearing the 
>> name/type of the chunk expected behaviour?
> 
> Doesn't it add the name of the program text that called the realloc ?

Sorry yes, I should have been more specific.  It clears the the existing
name, and sets the name to be the program text.

There's a number of places where this happens. All the *_append_* 
functions drop the name of the chunk and set it it to be the string 
content.

For buffers its less relevant, but for structures... Well it just seems
like a rather unfriendly thing for a type validation system to do.
It seems like talloc_realloc* was originally envisioned primarily as a way
of extending arrays or buffers, and not working with flexible array 
members in structs, or struct "compositing"
(https://github.com/FreeRADIUS/freeradius-server/blob/master/src/lib/util/ext.c).

The cost to fix this would appear to be two bits in the flags field.
One to record whether the name was explicitly specified, one to
record whether the name was originally const or not.

There'd actually be some efficiency benefits to recording const vs not 
const in talloc_free_children. talloc_free_children currently walks 
over the list of children searching for a chunk that provides the name
for the context.

If we knew the context had a const name, then we could avoid that 
additional O(n) operation in the majority of cases.

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.

>> - talloc_set_memlimit is marked as deprecated in the
>>  current HEAD version of talloc, with a note to use cgroups.
>>  cgroups wouldn't offer equivalent functionality for us.  Could I humbly
>>  request memlimits not be deprecated? :)
> 
> OK, I think memlimits add horrible extra complexity to
> save applications some bookkeeping.
> 
> I want to *reduce* complexity in talloc.
> 
> First fix I want to do after the memlimit changes and tests
> have gone in is to remove the code paths around ALWAYS_REALLOC.
> We never compile with it in test, and we have tests that
> depend on it *not* being set, so it really needs to go.

Sounds good.

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

> If we can do that, then.....
> 
>> - Is there any appetite for functions which return chunks of a specific
>>  alignment?  i.e. talloc_aligned, talloc_aligned_array,
>>  talloc_aligned_pool.
>>  I've had to fudge some horrible variants of the above for local use.
>>  We use aligned memory for the following purposes:
>>  - Page aligned pools for protecting memory areas with mprotect
>>    to prevent runtime modifications.
>>  - 8 byte aligned buffers that can have the start and end regions
>>    easily poisoned with ASAN.
>>  - Cache line aligned ring buffers, with the cache line 
>>    alignment done to reduce contention where the producer and
>>    consumer are close.
> 
> Adding this, more interesting functionality becomes
> a lot easier to handle and manage complexity wise.

Agreed.

-Arran