Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic memory block allocator #7651

Merged
merged 2 commits into from Apr 10, 2018

Conversation

@tobhe
Copy link
Contributor

commented Sep 27, 2017

This PR adds a generic block allocator, allowing "dynamic" allocations of structures of the same size in a static memory pool.
It is meant to replace malloc in cases where only fixed size blocks are allocated and freed (see #7615).

Example usage

// Initialize static memory pool "mem" which can hold 3 somestruct_t
MEMARRAY(mem, somestruct_t, 3);

// Initialize mem as a linked list of free elements
memarray_init(&mem);

// Allocate structure
mem = memarray_alloc(&mem);

// Do whatever needs to be done
...

// Free struct
memarray_free(&mem, mem);
@smlng
Copy link
Member

left a comment

some initial comments, and generally: please adapt to RIOT coding style, that is 4 spaces intend (not 2), open braces for function on new line. See wiki.

#endif

/**
* @defgroup sys_memarray memory array allocator

This comment has been minimized.

Copy link
@smlng

smlng Sep 27, 2017

Member

please move this comment block up, directly below the copy right statement - just for consistency with RIOT codebase.

#ifndef MEMARRAY_H
#define MEMARRAY_H

#include "stdint.h"

This comment has been minimized.

Copy link
@smlng

smlng Sep 27, 2017

Member

these standard includes should go into < and >

* @brief pseudo dynamic allocation in static memory arrays
* @author Tobias Heider <heidert@nm.ifi.lmu.de>
*/
typedef struct {

This comment has been minimized.

Copy link
@smlng

smlng Sep 27, 2017

Member

struct need documentation, plus its elements, i.e.

    size_t size;    /**< size of memory array */
    size_t count;   /**< number of ... */
.first_free = _data_##name, \
.data = _data_##name};

void memarray_init(memarray_t *mem);

This comment has been minimized.

Copy link
@smlng

smlng Sep 27, 2017

Member

add full doxygen style documentation for all functions here and below

@smlng smlng requested review from miri64 and kaspar030 Sep 27, 2017

@miri64

This comment has been minimized.

Copy link
Member

commented Sep 28, 2017

How is this different from tlsf? What are it's benefits, what are its drawbacks compared to it?

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2017

While sth like this useful, I think the implementation has several drawbacks, mostly in the way free blocks are handled.
In the current implementation, every free iterates through all free blocks.

How about this:

  • assume the smallest supported blocksize will be 4 bytes (imo it currently does anyways?)
  • on initialization, initialize the memory divided into blocks of the desired size as a clist. Use the first four bytes of each block as list entry, and link all the blocks
  • reduce the memarray struct to contain a clist entry to the first block
  • use simple clist_lpop() / clist_rpush() for alloc/free

That way the actual struct would be down to 4 bytes, and both allocating/deallocating can be done in O(1), also re-using existing code.

}
}

uint8_t memarray_free(memarray_t *mem, void *ptr)

This comment has been minimized.

Copy link
@kaspar030

kaspar030 Sep 28, 2017

Contributor

This looks like O(n) free(). There must be another way.

This comment has been minimized.

Copy link
@tobhe

tobhe Sep 28, 2017

Author Contributor

right, i think to achieve O(1) (in the current implementation) it would be necessary to either not check boundaries at all and just assume ptr is in the memory pool, or to check whether ptr >= mem->data and ptr < mem->data+mem->count*mem->size which doesn't check whether the pointer is aligned in the pool.

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 28, 2017

@kaspar030
I think you are absolutely right, i will look into your solution. The current implementation does indeed assume a minimum blocksize of 4 to store pointers to the next free element.

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

How is this different from tlsf? What are it's benefits, what are its drawbacks compared to it?

@miri64 TLSF is meant to be a full on malloc replacement, allowing allocations of variable size and thus is way bigger and more complex. This module is meant to solve a simpler problem, where for each pool, the size of blocks as well as the number of blocks in a pool are known at compile time.

An example use case would be a network protocol package (like tinydtls), which needs to allocate one structure (holding control information as the peers address etc.) per open connection. Using TLSF would be overkill, as it would require including an external packet which provides a lot of functionality that is not really needed here.
As the size of the structure as well as the maximum required number of connections are usually known at compile time, a static array of structures, which is managed with a linked list (either the way it currently works or the way @kaspar030 proposed), can provide the same performance of O(1) allocations and frees, with less fragmentation, smaller code size and without the need for an external package.

@gebart

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

Since the memarray is a coherent array of blocks, would it be possible to use a bit map of the free blocks, instead of a linked list? I believe this would make free O(1) and malloc O(N) with lower memory overhead than the linked list implementation.

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

Since the memarray is a coherent array of blocks, would it be possible to use a bit map of the free blocks, instead of a linked list? I believe this would make free O(1) and malloc O(N) with lower memory overhead than the linked list implementation.

I've thought about using a bitmap index, i think it is even possible to malloc in O(1) using bitmagic. However I think the allocator list is the least complex solution and does not have notably more overhead than the bitmap in small pools and scales better.

That way the actual struct would be down to 4 bytes, and both allocating/deallocating can be done in O(1), also re-using existing code.

I've thought about what you said and I actually don't see any real advantage of using a clist. The non-clist approach works just as well as the one you described. The only reason memarray_free is O(N) is the check whether the pointer is actually a element of the list (which I think is actually not necessary as no other free implementation does this). Other than that, size and number of elements are only needed in the memarray_init function, regardless whether a clist or a normal list is initialized, so the memarray_t could be reduced to one 4 byte "head of the list" pointer in both cases.

@tobhe tobhe force-pushed the tobhe:memarray branch from d6ece58 to afad7b3 Oct 2, 2017

*/

#include "memarray.h"
#include <string.h>

This comment has been minimized.

Copy link
@kaspar030

kaspar030 Nov 2, 2017

Contributor

please reverse header sequence

This comment has been minimized.

Copy link
@tobhe

tobhe Nov 30, 2017

Author Contributor

done

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

I actually don't see any real advantage of using a clist.

I agree, it looks much better now and using clist would complicate things.

I'm wondering about the use of memcpy and your assumptions on alignment. It looks like there's no alignment being done, and that's why you actually use memcpy for storing/retrieving the pointers, right?

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

I'm wondering about the use of memcpy and your assumptions on alignment. It looks like there's no alignment being done, and that's why you actually use memcpy for storing/retrieving the pointers, right?

Indeed, because memcpy allows me to not care about the alignment.

@miri64 miri64 added State: WIP and removed Type: enhancement labels Nov 15, 2017

@miri64 miri64 removed their request for review Nov 15, 2017

rfuentess added a commit to rfuentess/TinyDTLS that referenced this pull request Nov 22, 2017

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Nov 30, 2017

Small update:
@rfuentess contributed a very nice testing application,
also @smlng: I guess the coding style should be fine now.

At the beginning of the tests is printed the memory being used by the threads.
At the end of the tests, the threads memory usage is printed once more time.

This test is passed if the memory used by the threads are correct and if the legend

This comment has been minimized.

Copy link
@rfuentess

rfuentess Nov 30, 2017

Contributor

Sorry, it's seems that I undid the final sentence by accident. It should be:

This test is passed if the memory used by the main thread remains static.

This comment has been minimized.

Copy link
@tobhe

tobhe Nov 30, 2017

Author Contributor

thanks, fixed it

@tobhe tobhe changed the title [WIP] Generic memory block allocator Generic memory block allocator Dec 19, 2017

# Comment this out to disable code in RIOT that does safety checking
# which is not needed in a production environment but helps in the
# development process:
CFLAGS += -DDEVELHELP

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

please remove, not needed since #8080

Expected result
===============

This application should runs a number of tests equal to NUMBER_OF_TESTS (Default 12).

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

typo /runs/run/ or remove should


This application should runs a number of tests equal to NUMBER_OF_TESTS (Default 12).

At the beginning of the tests is printed the memory being used by the threads.

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

wording, consider At the beginning of the tests memory usage of each thread is printed.

Background
==========

The module `memarray` is the static memory allocator for RIOT O.S.

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

typo: /RIOT O.S./RIOT-OS/


#include "memarray.h"

#define MAX_NUMBER_BLOCKS 10

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

align indention of defines, add parentheses and consider making the unsigned, i.e. like

#define MAX_NUMBER_BLOCKS   (10U)
#define MESSAGE_SIZE        (8U)
#define NUMBER_OF_TESTS     (12U)

int main(void)
{

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

remove empty line


void memarray_init(memarray_t *mem, size_t size, size_t num)
{
for (size_t i = 0; i < num - 1; i++) {

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

put parentheses: (num - 1)

} memarray_t;

/**
* @brief Define static memory pool

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

add empty line below

static memarray_t name = { .first_free = _data_ ## name };

/**
* @brief Initialize memarray pool with free list

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

dito, add empty line between @brief and @params, here and below

/**
* @brief Allocate memory chunk in memarray pool
* @param[in,out] mem memarray pool to allocate block in
* @return pointer to newly allocated memarray chunk

This comment has been minimized.

Copy link
@smlng

smlng Dec 22, 2017

Member

mhm, I'm unsure if that's correct usage of @return, bc the function is void but uses a param as return.

@tobhe tobhe force-pushed the tobhe:memarray branch from 378042d to 4a9ffdc Dec 22, 2017

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Dec 22, 2017

@smlng I think i fixed it all

@cladmi
Copy link
Contributor

left a comment

I want to converge on the API as its needed for Tinydtls.

The implementation could evolve later in a separate PR as long as the API is stable.

* @param[in] num number of chunks in pool
*/
#define MEMARRAY(name, size, num) \
static uint8_t _data_ ## name[num * size]; \

This comment has been minimized.

Copy link
@cladmi

cladmi Jan 8, 2018

Contributor

Why change from structure to size here ?

As a memarray it makes sense to allocate aligned memory element, and here, I am not even sure the array will always be aligned. malloc returns universally aligned data all the time.

You silently expect size >= sizeof(void *), I would just allocate big enough cells all the time.
By using structure you can create a type with an union to ensure the cell has the right size.

By principle, I think its ok to allocate '8' length items even if requester asks for '5'.

This comment has been minimized.

Copy link
@tobhe

tobhe Feb 16, 2018

Author Contributor

Why change from structure to size here ?

I'm not sure why i changed this in the first place. I think i will change it back.

You silently expect size >= sizeof(void *), I would just allocate big enough cells all the time.
By principle, I think its ok to allocate '8' length items even if requester asks for '5'.

agree, thanks for pointing this out

* @param[in] size size of single memory chunk
* @param[in] num number of chunks in pool
*/
void memarray_init(memarray_t *mem, size_t num, size_t size);

This comment has been minimized.

Copy link
@cladmi

cladmi Jan 8, 2018

Contributor

By removing num/size from memarray_t, the api now requires repeating them for init, which requires making them public if different modules allocate and init it.

Also, it makes free unsafe for invalid pointers, it may always be the case on free but few asserts could help debug mistakes.

This comment has been minimized.

Copy link
@tobhe

tobhe Feb 16, 2018

Author Contributor

By removing num/size from memarray_t, the api now requires repeating them for init, which requires making them public if different modules allocate and init it.

@ALL any other opinions on this? otherwise i will just put them back into the memarray_t

Also, it makes free unsafe for invalid pointers, it may always be the case on free but few asserts could help debug mistakes.

i don't know about any free where this is not the case, but we can assert the pointer to be between _data and _data + (num*size), maybe also the alignment

@kaspar030

This comment has been minimized.

Copy link
Contributor

commented Mar 6, 2018

Because of memory efficiency i use the pointer to the head of the free list as a initial pointer to the memory location, which seems quite dirty, but should not make problems as this is the same what implicitly happend before. If you think this is too hacky, let me know and i will add seperate data and first_free fields to the struct.

Hm, how about changing the memarray_init() signature back to include the necessary info (memarray_t*, ptr, element size, num of elements)?

That way the initialization would look more standard, like:

my_type_t _array[NUM_ELEMENTS];
memarray_t memarray;
memarray_init(&memarray, _array, sizeof(_array[0]), NUM_ELEMENTS);

Or maybe memarray_init(..., ptr, total_size, element_size) is more intuitive?

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 6, 2018

Hm, how about changing the memarray_init() signature back to include the necessary info (memarray_t*, ptr, element size, num of elements)?

I think you're right, this seems to be the better solution. As far as I've seen other modules, e.G. crypto/ciphers do their initialization in a similar way, so it would also add to the consistency of the API as a whole.

@tobhe tobhe force-pushed the tobhe:memarray branch from 7dbbe95 to bac67d6 Mar 6, 2018

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Mar 13, 2018

@cladmi @kaspar030
I have changed the api as requested, also i think got the documentation right this time.
any more comments on the current version?

@cladmi
Copy link
Contributor

left a comment

I am ok with the API and the current code. Sorry to have made you change to removing size and num before.

I have just minor remarks below.

I would like an added assert(size >= sizeof(void *)); in init as its an hidden requirement.

I tested on iotlab-m3 and wsn430-v1_3b the test works.
There are just some issues when printing on stm32 and wsn430 nodes as %zu is not implemented. We usually cast it to (unsigned) and use %u.
Also for pointers, it prints 0x0x12345. I will do a PR on your branch for these. Feel free to squash my commits later.

@bergzand

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

@tobhe ping

I'd like to use this in combination with the cn-cbor package :)

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

@bergzand As I am back from vaction now and just merged @cladmi's format string fixes I think this might be ready to get merged soon™. I was not planning to make any bigger API changes, and overall everyone seems happy with the current state (or at least no one has raised any concerns). So, feel free to use this, I'm glad there is so much interest in this

@bergzand

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

As I am back from vaction now

Didn't want to rush you here, thanks for continuing this. :)

@kaspar030, @cladmi: Are all your comments addressed? Is this ready to squash?

@miri64
Copy link
Member

left a comment

Some asserts missing (and documentation on them missing)


void *memarray_alloc(memarray_t *mem)
{
if (mem->free_data == NULL) {

This comment has been minimized.

Copy link
@miri64

miri64 Apr 6, 2018

Member

Please add an assert(mem != NULL); before this line (and document this with the @pre command in this function's documentation).


void memarray_free(memarray_t *mem, void *ptr)
{
memcpy(ptr, &mem->free_data, sizeof(void *));

This comment has been minimized.

Copy link
@miri64

miri64 Apr 6, 2018

Member

Please add an assert((mem != NULL) && (ptr != NULL)); before this line (and document this with the @pre command in this function's documentation).


void memarray_init(memarray_t *mem, void *data, size_t size, size_t num)
{
assert(size >= sizeof(void *));

This comment has been minimized.

Copy link
@miri64

miri64 Apr 6, 2018

Member

mem and data should also be checked, if they are not NULL pointers here ;-). Also please document your assertions with the @pre command.

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 6, 2018

@miri64 thanks, i totally forgot the @pre. I think i got them all ;)

@@ -0,0 +1,117 @@
/*
* Copyright (C) 2017 Inria

This comment has been minimized.

Copy link
@miri64

miri64 Apr 6, 2018

Member

2018 Tobias Heider heidert@nm.ifi.lmu.de ;-)

This comment has been minimized.

Copy link
@tobhe

tobhe Apr 6, 2018

Author Contributor

That one was initially done by @rfuentess so I don't want to change it without his consent.
I can update the date though.

This comment has been minimized.

Copy link
@rfuentess

rfuentess Apr 6, 2018

Contributor

For me is OK to share authorship or given it to @tobhe, after all he had to upgrade the code.

This comment has been minimized.

Copy link
@tobhe

tobhe Apr 6, 2018

Author Contributor

I'd say we leave it as is, after all you included me in the @author tag, so I don't see a point in changing the copyright notice. As said before I updated the date to 2018

@@ -0,0 +1,82 @@
/*
* Copyright (C) 2017 Tobias Heider <heidert@nm.ifi.lmu.de>

This comment has been minimized.

Copy link
@miri64

miri64 Apr 6, 2018

Member

2017-18 ;-)

@@ -0,0 +1,52 @@
/*
* Copyright (C) 2017 Tobias Heider <heidert@nm.ifi.lmu.de>

This comment has been minimized.

Copy link
@miri64

miri64 Apr 6, 2018

Member

2017-18 ;-)

@miri64

This comment has been minimized.

Copy link
Member

commented Apr 6, 2018

I'm fine, after the copyright issue is solved.

@miri64

miri64 approved these changes Apr 6, 2018

Copy link
Member

left a comment

ACK

Background
==========

The module `memarray` is the static memory allocator for RIOT-OS

This comment has been minimized.

Copy link
@kaspar030

kaspar030 Apr 6, 2018

Contributor

IMO this needs re-phrasing. What is a "static memory allocator"?

This comment has been minimized.

Copy link
@cladmi

cladmi Apr 9, 2018

Contributor

What do you think of "fixed-size blocks allocator"?

Taken from https://en.wikipedia.org/wiki/Memory_management#FIXED-SIZE and I find it clearer than "memory pool allocator"

This comment has been minimized.

Copy link
@tobhe

tobhe Apr 10, 2018

Author Contributor

I think "fixed-size block allocator" and "memory pool allocator" both are true, but highlight different properties of the allocator. Also, the fact that it just allocates fixed-size blocks may be the more distinct feature of this implementation, so I'm on your side with this.

mem->size = size;
mem->num = num;

for (size_t i = 0; i < (mem->num - 1); i++) {

This comment has been minimized.

Copy link
@kaspar030

kaspar030 Apr 6, 2018

Contributor

This makes mem->num - 1 re-evaluate on every loop iteration.

This comment has been minimized.

Copy link
@cladmi

cladmi Apr 9, 2018

Contributor

Does it ? has mem->num is not volatile there is no reason for the compiler to re-read it I think.
When replacing with an intermediate variable I get the same size for samr21-xpro, wsn430-v1_3b and native.
If you have another solution that reduces code size it would be good.

When using (num -1) in the loop it gets bigger for samr21-xpro, no idea why.

This comment has been minimized.

Copy link
@kaspar030

kaspar030 Apr 9, 2018

Contributor

Any function call in between two dereferences usually forces re-read from memory. The called function might have changed what's behind that pointer, after all. But maybe the compiler has outsmarted me (again).

This comment has been minimized.

Copy link
@cladmi

cladmi Apr 9, 2018

Contributor

Yeah memcpy could be doing some stuff to mem->num by overwriting it if mem and data overlap and in that case the compiler should indeed re-read it. Maybe it is re-read but does not cost more.

I found one that reduces code size and also solves the num == 0 case.
But it would print many debug free message at init so maybe not that good.

    for (char *cur = data; cur < (char *)data + (num * size); cur += size) {
        memarray_free(mem, cur);
    }

We can keep the optimizations to another PR this way it is not tied to tobhe.

@kaspar030
Copy link
Contributor

left a comment

ACK. Please squash. (feel free to address my last minor comments).


void memarray_init(memarray_t *mem, void *data, size_t size, size_t num)
{
assert((mem != NULL) && (data != NULL) && (size >= sizeof(void *)));

This comment has been minimized.

Copy link
@cladmi

cladmi Apr 9, 2018

Contributor

In the same part of having assert for the API I also saw that there could be an assert for num != 0 and its @pre counterpart as its a case that is not handled right now.

This comment has been minimized.

Copy link
@tobhe

tobhe Apr 10, 2018

Author Contributor

True, thanks for the hint

@tobhe tobhe force-pushed the tobhe:memarray branch from f27dcd8 to 62ca3d3 Apr 10, 2018

@cladmi

cladmi approved these changes Apr 10, 2018

@cladmi cladmi merged commit 747b11d into RIOT-OS:master Apr 10, 2018

2 checks passed

Murdock The build succeeded. runtime: 5m:47s
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cladmi

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

@tobhe Thank you for following this PR all this time.

@bergzand

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

🎉

@tobhe

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

@cladmi You're welcome, thanks for all the help

@aabadie

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2018

Nice, this will certainly interest @rfuentess for tinydtls

@miri64

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

🎉 @tobhe congratz!

nmeum pushed a commit to ruby-dtls/tinydtls that referenced this pull request Jul 14, 2018

RIOT: fix memarray use
This change fixes the invocation of the MEMARRAY macro and
memarray_init() to conform with the proposed API in [1].

[1] RIOT-OS/RIOT#7651

Change-Id: Iaede8ac17dfef758e54cd4072d58212c64ca4b08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.