[RFC] [CTDB] Optimized memory handling.

[Amitay Isaacs]

On Fri, Dec 15, 2017 at 12:46 AM, Swen Schillig <swen at vnet.ibm.com> wrote:
> On Thu, 2017-12-14 at 08:47 +0100, Swen Schillig via samba-technical
> wrote:
>> On Thu, 2017-12-14 at 10:42 +1100, Amitay Isaacs via samba-technical
>> wrote:
>> > On Wed, Dec 13, 2017 at 10:52 AM, Jeremy Allison via samba-
>> > technical
>> > <samba-technical at lists.samba.org> wrote:
>> > > On Tue, Dec 12, 2017 at 12:49:08PM +0100, Swen Schillig via
>> > > samba-
>> > > technical wrote:
>> > > > ...
>> > > > The currently used magic numbers for the pool sizes are still
>> > > > under
>> > > > investigation...hints and recommendations are welcome.
>> > > >
>> > > > However, initial tests showed big improvements.
>> > >
>> > > Can you give more details on what you mean by "big improvements"
>> > > ?
>> > > Some numbers would be good :-).
>> > >
>> > > Thanks,
>> > >
>> > > Jeremy.
>> > >
>> >
>> > Even without seeing the proof I can imagine the improvements in
>> > packet
>> > handling using the talloc pool.  That code is in the hot path and
>> > avoiding extra malloc() calls will definitely result in reducing
>> > the
>> > load on ctdb daemon.  However, as Jeremy mentioned, it would be
>> > good
>> > to verify with real numbers.
>> > ...
>> >
>> > Amitay.
>> >
>>
>> Thank you guys for having a look, it's really appreciated.
>>
>> As for the numbers, I do not have any "real" numbers yet.
>> But an isolated test of the code in question did show improvements of
>> approx. 40% of pool-alloc-code vs. non-pool-alloc-code.
>> This means just for the areas in question and not running a
>> real-life load.
>> Anyhow, as Amitay stated, this is the hot path and if we're "only"
>> getting 5% in the end that would be "a big improvement" :-)
>>
>> I'm in the process of getting a cluster ready to perform
>> real-life-tests to get an idea of some good initial pool-size values,
>> (especially for the data pool).
>>
>> @Amitay: I will try and transfer the changes to sock_io today so you
>> guys can have a look again.... making sure the direction is right.
>>
>> Regarding the "static" data pool.
>> I'm not a fan of those either, but I couldn't find a decent context
>> to
>> attach the pool to. Therefore, I decided to setup "one" pool once
>> for all data-buffer requirements instead of being dependent on some
>> upper structure which is not related in any way to the data mem.
>>
>> Cheers Swen
> Now that's embarrassing.
>
> The changes I proposed are almost all there in
> Amitay's code (socket_io.c) already.
> I should have read more before posting, sorry.
>
> From the patches/code I posted so far, I guess I have to re-order the
> changes for them to make sense again.
> First memory-opt (now in socket_io.c) and then ctdb_io.c adoptions.
>
> A few comments on the code.
> 1) why do we have to free the memory for queue->buf everytime ?
>    I don't think this is necessary (talloc_realloc if needed) and
>    would save time again. Once done with the queue it'll be
>    free'd anyway.

To avoid the buffer sizes growing.  Instead of adding some heuristic
to shrink the buffer size, this was simpler.

> 2) I still believe we don't need a destructor.
>    There's nothing done which is either required (queue-> = -1) or
>    not done automatically (TALLOC_FREE(queue->fde)).

Sure.

> 3) @Amitay: I really like the removal (in comparison to ctdb_io) of
>    the really expensive "data" memory / memcpy thing, that on it's own
>    should save time already. I just have to make sure to change all
>    the callbacks for ctdb_io to NOT free that memory anymore.

That may not be possible.  The model used here is that the callback is
supposed to free the memory once the callback has processed the
packet.  Usually the packets are processed quickly and freed, so we
can still use talloc pools.

>
> Attached you can find two patches, the memory-opt patch is pretty small
> and straight forward. I could prepare a committable patch by tomorrow,
> if it's wanted. Please let me know if you'd be ok with the 2 addtl.
> changes I suggested above( no destructor / no mem-free).
>
> Regarding the other patch (ctdb_sock_io replacement) this is WIP and
> won't come out of this state until next year.
> It does compile but has most likely a good few bugs in it.

Can you send the patches created with "git format-patch" command?  I
cannot "git am" the attached patches.

Amitay.