[PATCH] new macro: talloc_ctxdup

[Pavel Březina]

Hi,
I'm sending a small patch that adds talloc_ctxdup(parent, context). It
is an equivalent to the existing talloc_memdup() but it duplicates a
talloc context and keeps its name.

Regards,
Pavel.

[Jeremy Allison]

On Mon, Apr 30, 2012 at 07:08:14PM +0200, Pavel Březina wrote:
> Hi,
> I'm sending a small patch that adds talloc_ctxdup(parent, context). It
> is an equivalent to the existing talloc_memdup() but it duplicates a
> talloc context and keeps its name.

What use do you have for such a thing ?

[Andrew Bartlett]

On Mon, 2012-04-30 at 14:11 -0700, Jeremy Allison wrote:
> On Mon, Apr 30, 2012 at 07:08:14PM +0200, Pavel Březina wrote:
> > Hi,
> > I'm sending a small patch that adds talloc_ctxdup(parent, context). It
> > is an equivalent to the existing talloc_memdup() but it duplicates a
> > talloc context and keeps its name.
>
> What use do you have for such a thing ?

Additionally, I'm concerned that doing this in talloc (rather than in
local macros with clear purpose) may be confusing with regards to the
talloc tree.  This new talloc object would be a duplicate in name, and
in memory, but would not have duplicate children - which if this is a
struct, may be pointer elements in that struct.

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Stephen Gallagher]

On Tue, 2012-05-01 at 15:24 +1000, Andrew Bartlett wrote:

> On Mon, 2012-04-30 at 14:11 -0700, Jeremy Allison wrote:
> > On Mon, Apr 30, 2012 at 07:08:14PM +0200, Pavel Březina wrote:
> > > Hi,
> > > I'm sending a small patch that adds talloc_ctxdup(parent, context). It
> > > is an equivalent to the existing talloc_memdup() but it duplicates a
> > > talloc context and keeps its name.
> >
> > What use do you have for such a thing ?
>
> Additionally, I'm concerned that doing this in talloc (rather than in
> local macros with clear purpose) may be confusing with regards to the
> talloc tree.  This new talloc object would be a duplicate in name, and
> in memory, but would not have duplicate children - which if this is a
> struct, may be pointer elements in that struct.

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.

[Andrew Bartlett]

On Tue, 2012-05-01 at 07:12 -0400, Stephen Gallagher wrote:

> On Tue, 2012-05-01 at 15:24 +1000, Andrew Bartlett wrote:
> > On Mon, 2012-04-30 at 14:11 -0700, Jeremy Allison wrote:
> > > On Mon, Apr 30, 2012 at 07:08:14PM +0200, Pavel Březina wrote:
> > > > Hi,
> > > > I'm sending a small patch that adds talloc_ctxdup(parent, context). It
> > > > is an equivalent to the existing talloc_memdup() but it duplicates a
> > > > talloc context and keeps its name.
> > >
> > > What use do you have for such a thing ?
> >
> > Additionally, I'm concerned that doing this in talloc (rather than in
> > local macros with clear purpose) may be confusing with regards to the
> > talloc tree.  This new talloc object would be a duplicate in name, and
> > in memory, but would not have duplicate children - which if this is a
> > struct, may be pointer elements in that struct.
>
>
> 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?

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[Stephen Gallagher]

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.

[Andrew Bartlett]

On Tue, 2012-05-01 at 07:40 -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.

Indeed, this is the fundamental issue with talloc_pool().  If any memory
is kept longer than the desired pool life, this happens.  

I don't see a good solution however, unlike talloc_steal(), and more
like talloc_realloc() you would get a new pointer.  Nothing can
recursively fill in those pointers into the C struct, except by scanning
memory.

For this reason, either we should decide that talloc_pool() isn't the
right solution for this kind of memory, or if you know that this memory
is needed outside the pool, that you never put it on the pool.

talloc_pool() is a great performance hack, but like all performance
hacks, it works in it's optimal case, but has costs in other areas.  

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org

[simo]

On Tue, 2012-05-01 at 07:40 -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.

In this case I would rather either do that by default or maybe
distinguish between talloc_steal() (does not copy enything out of the
pool) and talloc_move() (where I would do the actual move of memory out
of the pool).

However what I need to understand is where is the problem in keeping
around a talloc_pool. The point of talloc_pool is to make
allocations/deallocation faster, and should be used in fast paths, not
everywhere.
My sense is that if you need to use talloc_steal() it means that you
shouldn't use a pool.

FWIW: I also agree that talloc_dupctx() is a confusing name and most
iomportantly and easy way to fail to copy around memory properly, and
because we cannot easily cioy a hierarchy w/o being aware of the actual
structure used I thik I would rather not introduce this function, not
with this name at least,
If you want to call it talloc_named_memdup() I's be ok with it though.

Simo.

--
Simo Sorce
Samba Team GPL Compliance Officer <[hidden email]>
Principal Software Engineer at Red Hat, Inc. <[hidden email]>

[Volker Lendecke]

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.

With best regards,

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: [phone], fax: [phone]
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]

[Pavel Březina]

On 05/01/2012 02:54 PM, simo wrote:

> On Tue, 2012-05-01 at 07:40 -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.
>
> In this case I would rather either do that by default or maybe
> distinguish between talloc_steal() (does not copy enything out of the
> pool) and talloc_move() (where I would do the actual move of memory out
> of the pool).
>
> However what I need to understand is where is the problem in keeping
> around a talloc_pool. The point of talloc_pool is to make
> allocations/deallocation faster, and should be used in fast paths, not
> everywhere.
> My sense is that if you need to use talloc_steal() it means that you
> shouldn't use a pool.

Sounds reasonable.

> FWIW: I also agree that talloc_dupctx() is a confusing name and most
> iomportantly and easy way to fail to copy around memory properly, and
> because we cannot easily cioy a hierarchy w/o being aware of the actual
> structure used I thik I would rather not introduce this function, not
> with this name at least,
> If you want to call it talloc_named_memdup() I's be ok with it though.

Patch is attached.

Pavel.

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

[Volker Lendecke]

On Fri, May 04, 2012 at 01:51:04PM +0200, Pavel Březina wrote:
> 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?

No, I don't. That was before 3.2.0, which was released more
than 4 years ago. Sorry.

Volker

--
SerNet GmbH, Bahnhofsallee 1b, 37081 Göttingen
phone: [phone], fax: [phone]
AG Göttingen, HRB 2816, GF: Dr. Johannes Loxen
http://www.sernet.de, mailto:[hidden email]