[PATCH] new macro: talloc_ctxdup

[Pavel Březina]

On 05/01/2012 09:31 PM, Volker Lendecke wrote:
> On Tue, May 01, 2012 at 07:40:33AM -0400, Stephen Gallagher wrote:
>> On Tue, 2012-05-01 at 21:26 +1000, Andrew Bartlett wrote:
>>> On Tue, 2012-05-01 at 07:12 -0400, Stephen Gallagher wrote:
>> ...
>>>> This patch came out of a discussion that Pavel and I were having about
>>>> copying memory out of a talloc pool into a new context. This seemed an
>>>> expedient way to do it. Obviously as Andrew points out, you need to also
>>>> be aware of whether the children of this context are also allocated from
>>>> the pool.
>>>>
>>>> Perhaps it would be better to design a talloc_steal_ext() that could be
>>>> configured to recursively move child memory out of a pool, where
>>>> appropriate.
>>>
>>> I'm a little confused, as talloc_steal() already moves the context and
>>> any child memory to the new parent.  What would your talloc_steal_ext()
>>> do?
>>
>> Actually, that's a false assumption. In the following construct:
>>
>> TALLOC_CTX *tmp_ctx = talloc_pool(NULL, 65536);
>> struct user *user = talloc(tmp_ctx, struct user);
>> user->username = talloc_strdup(user, "user1");
>>
>> Ok, so now we have 64k of memory allocated to a pool. We've taken some
>> portion of it for use with user and its username.
>>
>> Now:
>> new_user = talloc_steal(mem_ctx, user);
>> This reassigns the parent to the mem_ctx, but the memory is still
>> allocated in the pool.
>>
>> talloc_free(tmp_ctx); /* This doesn't do what you think it does */
>>
>> The memory has its PARENTAGE as mem_ctx now, but the pool itself will
>> not actually be freed until such time as the new_user is freed (and by
>> extension new_user->username). So in the meantime, we're holding on to
>> the entire 64k pool memory.
>>
>> The idea behind talloc_steal_ext() would be that under the hood it would
>> iterate through and determine if any part of the memory was in a
>> talloc_pool. If so, it would perform a talloc_ctxdup() to reallocate it
>> on the new context hierarchy (which may or may not be a NEW
>> talloc_pool). The end result would be that the original talloc_pool
>> could then be freed immediately, instead of hanging on to potentially
>> large amounts of memory that would look like a memory leak without being
>> detectable as one by valgrind.
>
> Just don't allocate long-living memory from a potentially
> short-lived talloc_pool. The talloc_pool API was written
> because during the pstring removal for file names we had
> measurably lost performance, and malloc was very high on the
> profiles. talloc_pool brought that back, so I think for the
> normal SMB use it is a valid optimization. But you have to
> be careful not to allocate memory you later talloc_steal to
> some long-living context. It does work (if it doesn't please
> tell me!), but it has the drawback you describe.

Thank you for the explanation. I'm just curious - do you have some
numbers that would show the performance of Samba before and after the
talloc_pool?

Regards,
Pavel.