2
\$\begingroup\$

Mainly for practicing purposes, I'm trying to implement a simple, yet efficient memory pool. It's basically a linked list that is able to grow, with fixed sized members. Each node has a flag if it's free or not. I'm not sure if I'm doing it the right way, and especially confused about how to deal with alignment, and thread safety. I know the basic concept, just don't know how to implement in this case. Could you please help me how to improve my code? This is what I have now:

#include <stdbool.h>
#include "mem_pool.h"


typedef struct block Block;

typedef struct block
{
    Block *next;
    void *ptr;
    bool free;
} Block;

struct mem_pool
{
    Block *head;
    void *memory;
    size_t memory_size;
    size_t memb_size;
};


#define get_ptr_and_increase(pool, size) pool->memory; pool->memory += size

#define add_block_size(pool) pool->memory_size += sizeof(Block) + pool->memb_size


static Block *new_block(MemoryPool *pool, bool is_free)
{
    Block *block = get_ptr_and_increase(pool, sizeof(Block));
    block->ptr = get_ptr_and_increase(pool, pool->memb_size);
    block->next = NULL;
    block->free = is_free;

    return block;
}

MemoryPool *pool_init(size_t memb_size)
{
    MemoryPool *pool = malloc(sizeof(MemoryPool));
    pool->memb_size = memb_size;
    pool->memory_size = 0;
    add_block_size(pool);
    pool->memory = malloc(pool->memory_size);
    pool->head = new_block(pool, true);

    return pool;
}

static Block *find_free(MemoryPool *pool)
{
    Block *current = pool->head;

    while (NULL != current && false == current->free) {
        current = current->next;
    }

    return current;
}

static void add_new_head(MemoryPool *pool)
{
    Block *tmp = pool->head;
    pool->head = new_block(pool, false);
    pool->head->next = tmp;
    add_block_size(pool);
    pool->memory = realloc(pool->memory, pool->memory_size);
}

void *pool_alloc(MemoryPool *pool)
{
    Block *free = find_free(pool);

    if (NULL != free) {
        return free->ptr;
    }
    add_new_head(pool);

    return pool->head->ptr;
}

void pool_free(MemoryPool *pool, void *block)
{
    Block *current = pool->head;

    while (current->ptr != block) {
        current = current->next;
    }

    current->free = true;
}

void pool_destroy(MemoryPool *pool)
{
    free(pool->memory - pool->memory_size);
    free(pool);
}
\$\endgroup\$
  • \$\begingroup\$ Does this code compile? \$\endgroup\$ – Siobhan Oct 23 '16 at 20:50
  • \$\begingroup\$ It did last time. \$\endgroup\$ – Isty001 Oct 23 '16 at 21:53
4
\$\begingroup\$

Serious Bug

The moment you do this:

pool->memory = realloc(pool->memory, pool->memory_size);

all the existing head, next, ptr, and memory pointers become invalid, since they were pointing into the memory that was just freed. So essentially, your memory pool becomes useless after you expand it.

Unsafe macros

This macro:

#define get_ptr_and_increase(pool, size) pool->memory; pool->memory += size

is unsafe for multiple reasons:

  1. Arguments are not used inside parentheses, e.g. (pool)
  2. The two statements could get split unexpectedly, such as in:

    if (condition)
        p = get_ptr_and_increase(pool, size);
    

I suggest creating a static inline function instead:

static inline void *get_ptr_and_increase(MemoryPool *pool, size_t size)
{
    void *ret = pool->memory;
    pool->memory += size;
    ret += size;
}
\$\endgroup\$
  • \$\begingroup\$ Thanks. What kind of techniques are there to implement a dynamically growing pool? \$\endgroup\$ – Isty001 Oct 24 '16 at 16:28
  • 2
    \$\begingroup\$ @Isty001 First, I would allocate large chunks instead of one element at a time. Second, I would never free or realloc anything. I would instead allocate a new large chunk when the first chunk got completely used up. \$\endgroup\$ – JS1 Oct 24 '16 at 17:21

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.