Revised interfaces for memory allocation / free methods (Comes out of rework done to support shared memory)

[gapisback]

This proposal comes from @rtjohnso , raised as part of his review comments for PR #569.

See this comment.

I think the current memfrag interface is leaky and not general.

I think the interface should look like this:

Proposal is as follows:

1. Memory allocation:

Original Proposal

platform_status               
platform_alloc(memfrag *mf, // OUT 
               int size); 

Revised proposal

static inline void *
platform_aligned_zalloc(platform_memfrag      *memfrag,   // OUT
                        const platform_heap_id heap_id,
                        const size_t           alignment, // IN
                        const size_t           size,      // IN
                         ... other diagnostic parameters ..)

static inline void *
platform_aligned_malloc(platform_memfrag      *memfrag,   // OUT
                        const platform_heap_id heap_id,
                        const size_t           alignment, // IN
                        const size_t           size,      // IN
                         ... other diagnostic parameters ..)

2. Memory Reallocation:

Original Proposal

platform_status
platform_realloc(memfrag *mf, // IN/OUT
                 int newsize);

Revised proposal

void *
platform_do_realloc(platform_memfrag *memfrag, // IN/OUT
                    size_t            newsize, // IN
                    // diagnostic parameters not shown ...
                   )

3. Freeing memory:

Original Proposal

platform_status
platform_free(memfrag *mf); // IN

Revised proposal

No changes from proposed interface.

5. Accessing allocated memory buffer:

void *
memfrag_get_pointer(memfrag *mf);

Update: this may become unnecessary once we rework the remaining interfaces.

As for names, I would advocate renaming memfrag to memory_allocation.

[gapisback]

@rtjohnso - First, let's settle the naming issue, as it will percolate through the rest of the discussion.

I suppose you meant to say to change the name (as existing in the current change-set) of the structure from platform_memfrag to memory_allocation.

We have a convention of prefixing things in platform-land with platform_; e.g., platform_spinlock, platform_status, platform_mutex. Which is why I went with platform_memfrag.

If we go with your suggestion, it will have to become: platform_memory_allocation. I think that's needlessly too long.

Secondly, this handle is not just for allocation, but it's also being used for free() and for get_pointer interface. So, I don't think naming this memory_allocation is appropriate; it's misleading.

After much consideration, I had settled on platform_memfrag as that covers all aspects: it's used for allocation, free and to track memory fragments.

I don't know how strongly you advocate to change the name for the handler structure. If you do insist that we have to reconsider and, if needed, change the name, my choices in decreasing order of preference will be:

1. platform_memfrag - No change; this will reduce code-diffs churn when I have to revise the large change-set.
2. platform_memhandle - That's what this is structure is: It's a handle for managing memory
3. platform_mhandle - Shorter than previous
4. platform_mhdl - Even shorter
5. platform_memory_allocation - My least favourite. Given a choice, I will not pick this one at all.

In the rest of my responses to your proposal I will stick with platform_memfrag, understanding that it may change once we come to a resolution on this name eventually.

[rtjohnso]

I agree with your concerns.

I think platform_memhandle is a good name.

The reason I have concerns about memfrag is because it makes me think: fragment of what? "Fragment" is not a term commonly used when discussing memory allocation, so I just worry it could unnecessarily lead to questions.

So if you're happy with it, let's go with platform_memhandle.

[gapisback]

@rtjohnso : I am going to make my case one more time for the choice of memfrag as the term.

Re: your comment that: '... have concerns about memfrag is because it makes me think: fragment of what? "Fragment" is not a term commonly used when discussing memory allocation, ...'

In my reading, memfrag immediately implies fragment of memory. Isn't that what jumps out at you on 1st reading?

Memory and fragmentation go intimately hand-in-hand, in my experience. For instance see this Wiki article and this other tech article.

This article also uses the term memory fragmentation in ways I am seeing it.

Nested somewhere midway in this RocksDB tech article is the usage of memory fragmentation:

"Here we see one of the main problems that all allocators try to solve: memory fragmentation. ..."

---

About explaining this and arising questions, this is how I see the conversation going with someone coming upon this new piece of code:

Qs. What is platform_memhandle{}?

Ans. It is a structure to track a piece of memory that was allocated from either the heap or from shared memory. It tracks the heap-ID where the memory piece was allocated from, the size of the allocated memory piece and its start address. platform_memhandle{} is the common Input/Output parameter to all memory management interfaces such as alloc, free, realloc.

Over time, depending on the sequence of allocs and free requests, we may not be able to satisfy a memory request due to memory fragmentation.

---

Now, let's try the same conversation replacing handle and piece with frag and fragment, respectively.

Here is how the conversation will go:

Qs. What is platform_memfrag{}?

Ans. It is a structure to track a fragment of memory that was allocated from either the heap or from shared memory. It tracks the heap-ID where the memory fragment was allocated from, the size of the allocated memory fragment and its start address. platform_memfrag{} is the common Input/Output parameter to all memory management interfaces such as alloc, free, realloc.

Over time, depending on the sequence of allocs and free requests, we may not be able to satisfy a memory request due to memory fragmentation.

---

I find that the 2nd rendering reads better and is more consistent with the name of the control struct, platform_memfrag{}.

Left to my choice, I'd stick with platform_memfrag{}.

(Reiterating the other small benefit from retaining this name: this will reduce code-diffs churn when I have to revise the large change-set.)

What's your vote now?

[rtjohnso]

Interestingly, I've written multiple papers on fragmentation and, although we talked about "fragmentation", we never talked about "fragments". So I am unconvinced that "fragment" is a common term for a piece of allocated memory.

We can get a third opinion if you like.

[gapisback]

@rtjohnso : Re: We can get a third opinion if you like. -- Please do so if you can find a 3rd tie-breaker vote. (Alex replied elsewhere that he's busy, so you will have to do the leg-work to find a tie-breaker vote.)

Re: I am unconvinced that "fragment" is a common term for a piece of allocated memory.: .

Well, it's either a 'piece of memory' or 'fragment of memory'; I don't see many other terms coming into play.

Now, I read these two sentences as being synonymous:

If enough pieces of memory are littered around, you get memory fragmentation.

If enough fragments of memory are littered around, you get memory fragmentation.

That's how I see it.

In any case, I'm not TOTALLY hung-up on this. I gave you my reasoning and line of thinking.

Just let me know which way you want this named, and I'll do it.

[gapisback]

Addendum @rtjohnso : Thinking deeper:

The one reason I keep insisting on platform_memfrag as the name is that I have emotionally internalized it (the term fragment) all through late 2022 and all of 2023 while I have been continually morphing and stabilizing this change-set / feature implementation.

It just has seemed like the right name as I've had to grapple with TONS of memory fragmentation, leaks and instabilities while productizing this implementation.

It just seems like I want to leave a bit of that emotional involvement back in the codeline so it's still around after I'm no longer here at the company.

Account for that ... and let's decide if we want to go with fragment or handle.

(Also, aside: We did use the exact term 'memory fragments' when the kernel team built a memory manager for an In-memory DB arch I had built in my previous job. So, from experience, around kernel engineers, the term fragment for a memory piece is not known to be unnatural.)

Emotions aside, either one will work in the long-run.

I've made my call ; please make yours, and let's finalize.

[gapisback]

Clarifications reg. (1) Memory Allocation:

As we are trying to hash-out exact interfaces, I'd like to do some detailing.

Currently, the signature for allocation interfaces are these two:

 988 static inline void *
 989 platform_aligned_zalloc(platform_heap_id  heap_id,
 990                         size_t            alignment,
 991                         size_t            size,
 992                         platform_memfrag *memfrag, // OUT
 993                         const char       *objname,
 994                         const char       *func,
 995                         const char       *file,
 996                         int               lineno)

and, which calls this one:

447 static inline void *
448 platform_aligned_malloc(const platform_heap_id heap_id,
449                         const size_t           alignment, // IN
450                         const size_t           size,      // IN
451                         platform_memfrag      *memfrag,   // OUT
452                         const char            *objname,
453                         const char            *func,
454                         const char            *file,
455                         const int              line)

All the other caller-macros, such as TYPED_MALLOC() and so on, call one of these two, as doc'ed here:

 239  *  TYPED_MALLOC()
 240  *   -> TYPED_ARRAY_MALLOC()
 241  *        -> TYPED_MANUAL_MALLOC() -> platform_aligned_malloc()
 242  *
 243  *  TYPED_ZALLOC()
 244  *   -> TYPED_ARRAY_ZALLOC()
 245  *        -> TYPED_MANUAL_ZALLOC() -> platform_aligned_zalloc()

Qs. a) I am assuming that you are ok with these TYPED* interfaces -- None of this needs to change.

---

The main thing to note that is different with your proposal is that all the current interfaces take a platform_heap_id hid as the 1st arg. This is true for platform_free(platform_heap_id hid, void *p), also.

To go to your proposal, we will need platform_heap_id heap_id be a field embedded inside platform_memfrag.

That structure would change (from what it's currently in the change-set) to become:

 777 typedef struct platform_memfrag {
 778    platform_heap_id hid;                         // <<<< Newly added field
 779    size_t           size;
 780    void             *addr;
 781 } platform_memfrag;

All callers to memory allocation would need to supply hid thru this *memfrag IN/OUT parameter.

I would leave the alignment arg as it is in the interface, and not muck it up inside platform_memfrag{} struct.

Qs. b) Is this what you wanted?

---

My thoughts:

Pros of this approach:

• hid inside the struct allows for the possibility in the future that different memory chunks / fragments could come from different heap_id (heaps).
• This allows for better diagnostic to verify (in future) that a specific memory fragment came from which heap-ID.

Cons:

• Adds to more code-churn in the diffs when I revise the implementation.
• In current change-set, if there were any callers that did not need a platform_memfrag * and were supplying NULL, those will need to change. Every client will be forced to have a non-NULL on-stack platform_memfrag struct, simply to pass-down the hid parameter. Currently, that parameter is being supplied through the function's interface.

[rtjohnso]

I had honestly not thought about the hid parameter, but I agree that storing it in the memhandle is a good idea.

But just to make sure we're on the same page, when calling platform_aligned_malloc, the caller should pass a hid parameter, i.e. the memhandle parameter should remain a pure OUT parameter, i.e.

platform_aligned_malloc(hid, alignment, size, &mh, etc); // GOOD

versus

mh.hid = hid;
platform_aligned_malloc(alignment, size, &mh, etc); // NOT GOOD

This keeps the memhandle structure purely opaque to the rest of the code, i.e. my goal is that user code should never ever have to poke around inside a memhandle struct.

Regarding the return type, I think you're right that it's fine to leave the return type of allocations as void *.

[rtjohnso]

Question: how do the TYPED APIs work now? Do you have to pass a memhandle to them?

[gapisback]

Ans. (@rtjohnso ) yes, the TYPED interfaces also require a platform_memfrag{} * in/out parameter supplied to them.

However, the actually code construction is a bit tricky, the way I have implemented it. This sub-thread needs a bit of airing out so I will start a completely new "reply sub-thread" at the bottom of this discussion item so we can have a focussed discussion solely on the TYPED api and its usages.

[gapisback]

Agreed, with your proposed interface, with one minor suggestion for change. Rather than defining the interface as:

platform_aligned_malloc(hid, alignment, size, &mh, etc); // GOOD

How about we consistently have platform_memfrag *mf as the 1st arg to all interfaces?

That will make also interfaces be consistent. And this scheme is also consistent with some approaches where In/Out parameters are up-1st in the sequence of parameters, rather being snuck-in somewhere in the stream of parameters.

So I would like to change this one to:

447 static inline void *
448 platform_aligned_malloc(platform_memfrag      *memfrag,   // OUT
449                         const platform_heap_id heap_id,
450                         const size_t           alignment, // IN
451                         const size_t           size,      // IN
452                         const char            *objname,
453                         const char            *func,
454                         const char            *file,
455                         const int              line)

i.e., the call will be: platform_aligned_malloc(&mf, hid, alignment, size, diagnostics-parameters ...); // GOOD

[rtjohnso]

Moving the memhandle to be the first arg everywhere seems like a good idea. Go for it.

[gapisback]

Clarifications reg (2). realloc() interface:

The current interface is (I agree) klunky like:

501 #define platform_realloc(hid, oldsize, ptr, newsize)                           \
502    platform_do_realloc(                                                        \
503       (hid), (oldsize), (ptr), (newsize), __func__, __FILE__, __LINE__)

which calls:

505 static inline void *
506 platform_do_realloc(const platform_heap_id heap_id,
507                     const size_t           oldsize,
508                     void                  *ptr,     // IN
509                     size_t                *newsize, // IN/OUT
510                     const char            *func,
511                     const char            *file,
512                     const int              line)

If we do want to push-down hid into platform_memfrag then, yes, I agree, what you proposed is fine. And doable.

realloc() is called very few times in code, so there isn't much by way of code churn issues even if we change this interface.

[gapisback]

Upon re-examination of your proposed interface, I don't understand why it and platform_alloc() are returning:
platform_status platform_realloc().

Why don't we have both the allocator interfaces simply return void *, like they do currently?

Failure to alloc / realloc can be signaled by returning NULL ptr. Why isn't that sufficient?

If we did that, then I think void *memfrag_get_pointer() will mostly become unnecessary.

[rtjohnso]

Just to make sure we're on the same page: the new api would be

void *
platform_do_realloc(platform_memhandle *mh, size_t newsize, debugging parameters...);

(I don't know why newsize is an IN/OUT size_t * in the current code.)

[gapisback]

Agreed. I will revise the interface of the platform_realloc() caller-macro and platform_do_realloc() appropriately in my final summary mail.

Qs. newsize being IN/OUT in current implementation is a wart in the current change-set. The idea was to "inform" the caller the size of the reallocated memory fragment, so that the caller can do stuff with it to enable an eventual free.

All these shenanigans will be corrected with this suggested rework.

[gapisback]

Clarifications reg. (3) platform_free() interface:

Currently the interface is like: #define platform_free(hid, p).

The issue is the same as for alloc; i.e. we will need to push-down hid into platform_memfrag{} to go to the one you proposed.

Qs. I am assuming that's what you had in mind.

[rtjohnso]

It is now. :)

[gapisback]

Qs. reg. (4) memfrag_get_pointer(memfrag *mf);:

Are you sure you want to call it this, while the rest are named platform_*()?

I think platform_get_addr(platform_memfrag *mf) is a better interface.

I can do what you want, but instead, I think memfrag_get_addr() is better.

My vote is for platform_get_addr().

What's your vote?

---

Also, and more importantly, if platform_alloc() and platform_realloc() were to return void *, then we don't even need this interface to return the addr of the allocated memory.

If we change the interface of those two to return platform_status, one has to now change all the callers to also check for an error. Currently, in many places we check for NULL returned ptr to signal an error in memory allocation.

I think that scheme could be continued as it is consistent with traditional malloc() behaviour. Don't you think so?

[rtjohnso]

Your name is better.

If we don't need it, feel free to leave it out.

[gapisback]

@rtjohnso : Based on above exchange, here's a revised nearly-final set of interfaces.

I will reply to this post, listing each interface separately, so we can rebut each and resolve if there are still discrepancies to work through.

[gapisback]

1. Memory Allocation:

static inline void *
platform_aligned_zalloc(platform_memfrag *memfrag, // IN/OUT
                        platform_heap_id  heap_id,
                        size_t            alignment,
                        size_t            size,
                        const char       *objname,
                        const char       *func,
                        const char       *file,
                        int               lineno)

Which will call:

447 static inline void *
448 platform_aligned_malloc(platform_memfrag      *memfrag,   // IN/OUT
449                         const platform_heap_id heap_id,
450                         const size_t           alignment, // IN
451                         const size_t           size,      // IN
                               [... diagnostic parameters not shown hereafter]

[gapisback]

2. Memory Reallocation:

Caller-macro: external interface platform_realloc():

#define platform_realloc(mf, newsize)                           \
   platform_do_realloc((mf), (newsize), __func__, __FILE__, __LINE__)

Definition of do_realloc():

static inline void *
platform_do_realloc(platform_memfrag *memfrag, // IN/OUT
                    size_t            newsize, // IN
                    // diagnostic parameters not shown ...
                   )

[gapisback]

3. Free Memory interface:

The proposed (tentative) interface for free is: platform_status platform_free(platform_memfrag *mf); // IN

This proposed interface requires that all clients that allocate memory will necessarily have to deal with a platform_memfrag{} structure, which is usually allocated on-stack or in some common structure.

This has implications and affects clients that allocate large-memory from the heap even when shared-memory is configured. The code-level usage issues are best illustrated with an example:

Currently (in this change-set), the new code chunks in routing_filter.c look like so:

 403    uint32 *temp = TYPED_ARRAY_ZALLOC_MF(
 404       PROCESS_PRIVATE_HEAP_ID, temp, temp_buffer_count, NULL);

What's happening is that in routing_filter_add(), even if shared-memory is configured we want to allocate memory for temp buffer from the heap, indicated by the use of PROCESS_PRIVATE_HEAP_ID.

Later, before exiting, we do: 641 platform_free(PROCESS_PRIVATE_HEAP_ID, temp);

Currently, in the change set, the free interface is: #define platform_free(hid, p), where p is a ptr to the "thing" being freed.

The body of the caller-macro platform_free() has been updated to allow for two cases:

1. Input hid is a valid heap-ID, in which case p is expected to be a ptr toplatform_memfrag *.
2. Input hid is "the heap"; i.e. not shared memory heap-ID. It is PROCESS_PRIVATE_HEAP_ID, then p is a ptr to the allocated memory itself, and can be freed directly using native free(p) system call.

All this was done to retain the "convenience" for callers such as routing_filter_add() to not have to do extra work to allocate an on-stack platform_memfrag{} and to directly deal with allocated memory addr ptrs.

Going to the proposed simplified platform_free(platform_memfrag *mf) interface will mean reworking a handful of consumers that currently deal with PROCESS_PRIVATE_HEAP_ID handling to always work with an on-stack platform_memfrag{} struct.

Yes, it seems brain-dead obvious that we should do this make-consistent rework, but just thought I'd air out the specific usage-issues around PROCESS_PRIVATE_HEAP_ID, ... in case you end up thinking differently about your proposed new interface.

---

My vote: Yes, we can rework the change set now to require that free() will always require platform_memfrag{}, and also recognize that it somewhat belabors a few clients.

Your call?

[rtjohnso]

Yes let's make it consistent.

[gapisback]

Qs. How do the TYPED APIs work now? Do you have to pass a memhandle to them?

@rtjohnso -- This discussion sub-thread is w.r.t this question you raised in one of your earlier comments.

The short-answer is: Yes, all the TYPED_* interfaces do require a platform_memfrag{} arg.

Long-answer is: However, they do it in sort-of a weird (or cute, depending on your point of view) way.

[gapisback]

Let's take an example for this:

#define TYPED_MALLOC(hid, v) TYPED_ARRAY_MALLOC_MF(hid, v, 1, &memfrag_##v)

The hierarchy of caller-macro definitions is like so:

#define TYPED_MALLOC_MF(hid, v, mf) TYPED_ARRAY_MALLOC_MF(hid, v, 1, mf)

#define TYPED_ARRAY_MALLOC_MF(hid, v, n, mf)                                   \
   TYPED_MANUAL_MALLOC(hid, (v), (n) * sizeof(*(v)), mf)

.. and so on, till we eventually call platform_aligned_malloc() with the required platform_memfrag *mf argument.

What's happening in the outer-most call to TYPED_MALLOC is that the macro synthesizes the name of the required platform_memfrag * arg off of the name of structure for which memory is being allocated.

---

Here is an example usage from shard_log.c:

So, in the caller's context, if you want to allocate memory for a thing named slog, you need to have declared platform_memfrag memfrag_slog; somewhere in scope of this call.

329    platform_memfrag  memfrag_slog;

331    shard_log        *slog = TYPED_MALLOC(hid, slog);

Reason why I did it like so is: We could have gone the way this new revised proposal is headed; i.e., explicitly show platform_memfrag * as an argument to this macro. Then the above call would become:

331    shard_log        *slog = TYPED_MALLOC(&memfrag_slog, hid, slog);

This would require updating 10s or 100s of calls to TYPED_* allocation interface, all of them changing to just supply the named &memfrag_something variable to the call.

I found it convenient to:
(a) impose a coding discipline that one has to declare in-scope a platform_memfrag{} struct with a particular naming convention
(b) Otherwise the compilation will fail.
(c) And then not have to churn through a lot of instances of TYPED* calls.

[gapisback]

The same pattern of synthesized name for platform_memfrag{} arg appears all over this family of TYPED* macros.

Here is another example:

#define TYPED_ARRAY_MALLOC(hid, v, n)                                          \
   TYPED_MANUAL_MALLOC(hid, (v), (n) * sizeof(*(v)), &memfrag_##v)

Usage in functional/splinter_test.h: If you want to allocate memory for an array of structs tracked by temp_cfg, you need to declare a platform_memfrag{} named appropriately.

Otherwise the build will fail.

 87    platform_memfrag memfrag_temp_cfg;
 88    temp_config     *temp_cfg =
 89       TYPED_ARRAY_MALLOC(platform_get_heap_id(), temp_cfg, num_config);

---

NOTE: If you do not like this approach, let's decide immediately.

I will have to plumb through the entire change-set and revise the implementation, solely to externalize platform_memfrag *mf arg as the 1st arg to these TYPED_* caller-macro interfaces.

I don't think there is a great benefit in that code-churn revision as the current scheme does have its simplicity and inherent code-discipline advantges.

[rtjohnso]

I'm afraid it has to be done. Otherwise, how will they free the memory later?

[gapisback]

Update: @rtjohnso -- I have revised the implementation in PR #569 based on the discussions we've had so far.

Following are the new interfaces at a high-level:

- TYPED_MANUAL_MALLOC(mf, hid, v, n)
- TYPED_MANUAL_ZALLOC(mf, hid, v, n)
- ... and so on ...

- platform_aligned_zalloc(platform_memfrag *memfrag, platform_heap_id  heap_id, size_t alignment, size_t size, ...)
- platform_aligned_malloc(platform_memfrag *memfrag, platform_heap_id  heap_id, size_t alignment, size_t size, ...)

- platform_realloc(mf, newsize)

- platform_free(platform_memfrag *mf) 

I have made the required changes to the consumer code. Basic tests are passing on my Docker build.

Right now draft PR #616 to shake out CI-jobs is going on. I expect it will succeed.

Let me know if there are any other interface changes you'd like to bring up. I can implement them in this round before re-starting the review.

Otherwise, once CI-jobs all pass, I will update the delta-diffs arising from this revision to the change-set in PR #569 and will request you to re-start the review.

[gapisback]

Most changes arising from discussions so far have been applied to the change-set under PR #569 under this one single commit.

Review can resume now.