ctdb: Adding memory pool for queue callback

[Andrew Bartlett]

On Wed, 2018-11-07 at 17:26 +0100, Volker Lendecke via samba-technical
wrote:
> On Wed, Nov 07, 2018 at 05:03:42PM +0100, Swen Schillig wrote:
> > 
> > On Wed, 2018-11-07 at 16:48 +0100, Volker Lendecke wrote:
> > > 
> > > On Wed, Nov 07, 2018 at 04:30:58PM +0100, Swen Schillig wrote:
> > > > 
> > > > Oh, didn't answer your last question....
> > > > yes, it does match the CTDB use pattern where we fetch a pool
> > > > and
> > > > then
> > > > re-use that memory for as long as we use that queue/connection.
> > > Right, but the devil might be in the details: Do we have a
> > > hierarchy
> > > hanging off "data" allocated from the pool or not? And does this
> > > make
> > > a difference for overall performance?
> > > 
> > No.
> > The memory is used as the chunk as it was received and if anything
> > special needs to be done, the memory is getting copied into a new
> > structure and the old (pool-) memory is free'd.
> Just FYI: I get vastly different results depending on the compiler
> optimization level. I will do further investigation, it will take a
> while.

Another comparison that will be worth doing is with talloc() but with
the talloc pool feature removed by #ifdef.

I was shocked to find out how much could be gained by not re-checking
the magic all the time, eg in:

> commit d2a5927bd77a94256f7d8ee7098f66c4c0afd25d
> Author: Andrew Bartlett <abartlet at samba.org>
> Date:   Wed Jun 29 16:41:52 2016 -0700
> 
>     lib: talloc: Rename the internals of _talloc_free_internal() to
> _tc_free_internal().
>     
>     Make it use a struct talloc_chunk *tc parameter. Define
> _talloc_free_internal()
>     in terms of _tc_free_internal().
>     
>     Signed-off-by: Andrew Bartlett <abartlet at samba.org>
>     Reviewed-by: Jeremy Allison <jra at samba.org>

This just removed (a lot of calls to) a single comparison, wrapped in
unlikely().  The talloc benchmark was improved by about 10% from memory
(not written into the git history, drat!).

The pool stuff is complex, and if it gains us the performance we need,
it is worth it (just), but if it is no longer a win, there are
considerable security advantages to malloc() and free(), also likewise
under optimisation by the glibc folks no doubt, because I understand
they work hard to detect memory attacks. 

We have had exploits which were easier because talloc_pool() made the
pointers really predictable: 
https://gist.github.com/worawit/051e881fc94fe4a49295

So why might it not be a win?  It adds a lot of if conditions,
particularly in the talloc_free() path.  I count about 10 if we go down
the pool route, 2 otherwise.  

At a high level, talloc() is just a few more if conditions, yet when
they are all combined it does makes talloc()/talloc_free() surprisingly
expensive compared to malloc()/free().  

Andrew Bartlett
-- 
Andrew Bartlett                       http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba