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.

Sign up for GitHub

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

aligned memory allocaion #4277

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

aligned memory allocaion #4277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
84 changes: 79 additions & 5 deletions pjlib/include/pj/pool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,10 @@ struct pj_pool_t
/** The callback to be called when the pool is unable to allocate memory. */
pj_pool_callback *callback;

/** The default alignment of memory block allocated from this pool
* (must be power of 2). */
size_t alignment;

};


Expand All @@ -347,8 +351,9 @@ struct pj_pool_t
#endif

/**
* Create a new pool from the pool factory. This wrapper will call create_pool
* member of the pool factory.
* Create a new pool from the pool factory. This wrapper will call
* pj_pool_aligned_create() with alignment parameter set to 0
* which means use the default alignment (PJ_POOL_ALIGNMENT).
*
* @param factory The pool factory.
* @param name The name to be assigned to the pool. The name should
Expand Down Expand Up @@ -379,6 +384,44 @@ PJ_IDECL(pj_pool_t*) pj_pool_create(pj_pool_factory *factory,
pj_size_t increment_size,
pj_pool_callback *callback);


/**
* Create a new pool from the pool factory. This wrapper will call create_pool
* member of the pool factory.
*
* @param factory The pool factory.
* @param name The name to be assigned to the pool. The name should
* not be longer than PJ_MAX_OBJ_NAME (32 chars), or
* otherwise it will be truncated.
* @param initial_size The size of initial memory blocks taken by the pool.
* Note that the pool will take 68+20 bytes for
* administrative area from this block.
* @param increment_size the size of each additional blocks to be allocated
* when the pool is running out of memory. If user
* requests memory which is larger than this size, then
* an error occurs.
* Note that each time a pool allocates additional block,
* it needs PJ_POOL_SIZE more to store some
* administrative info.
* @param alignment the default alignment of memory block allocated
* from this pool (must be power of 2).
* Value of 0 means use PJ_POOL_ALIGNMENT.
* @param callback Callback to be called when error occurs in the pool.
* If this value is NULL, then the callback from pool
* factory policy will be used.
* Note that when an error occurs during pool creation,
* the callback itself is not called. Instead, NULL
* will be returned.
*
* @return The memory pool, or NULL.
*/
PJ_IDECL(pj_pool_t*) pj_pool_aligned_create(pj_pool_factory *factory,
const char *name,
pj_size_t initial_size,
pj_size_t increment_size,
pj_size_t alignment,
pj_pool_callback *callback);

/**
* Release the pool back to pool factory.
*
Expand Down Expand Up @@ -448,8 +491,10 @@ PJ_IDECL(pj_size_t) pj_pool_get_used_size( pj_pool_t *pool );

/**
* Allocate storage with the specified size from the pool.
* The allocation will be aligned to the default alignment of the pool.
* If there's no storage available in the pool, then the pool can allocate more
* blocks if the increment size is larger than the requested size.
* This function will call pj_pool_aligned_alloc() with alignment set to 0.
*
* @param pool the pool.
* @param size the requested size.
Expand All @@ -460,6 +505,24 @@ PJ_IDECL(pj_size_t) pj_pool_get_used_size( pj_pool_t *pool );
*/
PJ_IDECL(void*) pj_pool_alloc( pj_pool_t *pool, pj_size_t size);


/**
* Allocate storage with the specified size and alignment from the pool.
* If there's no storage available in the pool, then the pool can allocate more
* blocks if the increment size is larger than the requested size.
*
* @param pool the pool.
* @param alignment the requested alignment of the allocation.
* Value of 0 means use the default alignment of this pool.
* @param size the requested size.
*
* @return pointer to the allocated memory.
*
* @see PJ_POOL_ALLOC_T
*/
PJ_IDECL(void *) pj_pool_aligned_alloc(pj_pool_t *pool, pj_size_t alignment,
pj_size_t size);

/**
* Allocate storage from the pool, and initialize it to zero.
* This function behaves like pj_pool_alloc(), except that the storage will
Expand Down Expand Up @@ -523,9 +586,11 @@ PJ_INLINE(void*) pj_pool_zalloc(pj_pool_t *pool, pj_size_t size)
* Internal functions
*/
/** Internal function */
PJ_IDECL(void*) pj_pool_alloc_from_block(pj_pool_block *block, pj_size_t size);
PJ_IDECL(void*) pj_pool_alloc_from_block(pj_pool_block *block, pj_size_t alignment,
pj_size_t size);
/** Internal function */
PJ_DECL(void*) pj_pool_allocate_find(pj_pool_t *pool, pj_size_t size);
PJ_DECL(void*) pj_pool_allocate_find(pj_pool_t *pool, pj_size_t alignment,
pj_size_t size);



Expand Down Expand Up @@ -687,7 +752,11 @@ struct pj_pool_factory
* Note that each time a pool allocates additional block,
* it needs 20 bytes (equal to sizeof(pj_pool_block)) to
* store some administrative info.
* @param callback Cllback to be called when error occurs in the pool.
* @param alignment the default alignment of memory block allocated
* from this pool (must be power of 2).
* A value of 0 should result in the pool being created
* with alignment equal to PJ_POOL_ALIGNMENT.
* @param callback Callback to be called when error occurs in the pool.
* Note that when an error occurs during pool creation,
* the callback itself is not called. Instead, NULL
* will be returned.
Expand All @@ -698,6 +767,7 @@ struct pj_pool_factory
const char *name,
pj_size_t initial_size,
pj_size_t increment_size,
pj_size_t alignment,
pj_pool_callback *callback);

/**
Expand Down Expand Up @@ -748,25 +818,29 @@ struct pj_pool_factory
* @param name Pool name.
* @param initial_size Initial size.
* @param increment_size Increment size.
* @param alignment Pool alignment.
* @param callback Callback.
* @return The pool object, or NULL.
*/
PJ_DECL(pj_pool_t*) pj_pool_create_int( pj_pool_factory *factory,
const char *name,
pj_size_t initial_size,
pj_size_t increment_size,
pj_size_t alignment,
pj_pool_callback *callback);

/**
* This function is intended to be used by pool factory implementors.
* @param pool The pool.
* @param name Pool name.
* @param increment_size Increment size.
* @param alignment Pool alignment.
* @param callback Callback function.
*/
PJ_DECL(void) pj_pool_init_int( pj_pool_t *pool,
const char *name,
pj_size_t increment_size,
pj_size_t alignment,
pj_pool_callback *callback);

/**
Expand Down
40 changes: 31 additions & 9 deletions pjlib/include/pj/pool_i.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@

#include <pj/string.h>

#define ALIGN_PTR(PTR,ALIGNMENT) (PTR + (-(pj_ssize_t)(PTR) & (ALIGNMENT-1)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to PJ_POOL_ALIGN_PTR, since this potentially be included as public API

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#define IS_ALIGNED(p, a) (!((uintptr_t)(p) & ((a)-1)))
this is from row.h libyuv

Shouldn't this be public too?

Copy link
Member

@bennylp bennylp Feb 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The objective here is to prevent conflict from symbols that we (pjsip) declare with symbols from other software when the users include our header files mixed with header files from other software. That's why we put pj/PJ prefix everywhere.

SInce pool_i.h can potentially be included by user (i.e. when PJ_FUNCTIONS_ARE_INLINED is set to nonzero), all symbols declared here must be properly prefixed with pj/PJ.

As for IS_ALIGNED in libyuv/row.h, since this is not our software (although it is included in our distribution), we can't control what naming they use, and we don't want to modify it, so it is what it is.



PJ_IDEF(pj_size_t) pj_pool_get_capacity( pj_pool_t *pool )
{
Expand All @@ -37,28 +39,38 @@ PJ_IDEF(pj_size_t) pj_pool_get_used_size( pj_pool_t *pool )
return used_size;
}

PJ_IDEF(void*) pj_pool_alloc_from_block( pj_pool_block *block, pj_size_t size )
PJ_IDEF(void*) pj_pool_alloc_from_block( pj_pool_block *block, pj_size_t alignment,
pj_size_t size )
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to check (PJ_ASSERT_RETURN) either here or in pj_pool_aligned_alloc() that the value of alignment is really power of two. Sample implementation:

#define IS_POWER_OF_TWO(val)    (((val)>0) && ((val) & ((val)-1))==0)

Also in pj_pool_aligned_create(), pj_pool_allocate_find(), and possibly others.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB!
/* The operation below is valid for size==0.
* When size==0, the function will return the pointer to the pool
* memory address, but no memory will be allocated.
*/

c++ new() returns uniqiue address for each allocation (even for size==0). This may be better than pjsip's behavior.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO the existing spec is okay, it can be used for example to check the pointer belongs to some buffer.

/* The operation below is valid for size==0.
* When size==0, the function will return the pointer to the pool
* memory address, but no memory will be allocated.
*/
if (size & (PJ_POOL_ALIGNMENT-1)) {
size = (size + PJ_POOL_ALIGNMENT) & ~(PJ_POOL_ALIGNMENT-1);
if (size & (alignment -1)) {
size = (size + alignment) & ~(alignment -1);
}
if ((pj_size_t)(block->end - block->cur) >= size) {
void *ptr = block->cur;
block->cur += size;
unsigned char *ptr = ALIGN_PTR(block->cur, alignment);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls move ptr declaration to before first statement (re: coding style)

if (ptr < block->end && (pj_size_t)(block->end - ptr) >= size) {
block->cur = ptr + size;
return ptr;
}
return NULL;
}

PJ_IDEF(void*) pj_pool_alloc( pj_pool_t *pool, pj_size_t size)
{
void *ptr = pj_pool_alloc_from_block(pool->block_list.next, size);
return pj_pool_aligned_alloc(pool, 0, size);
}

PJ_IDECL(void *) pj_pool_aligned_alloc(pj_pool_t *pool, pj_size_t alignment,
pj_size_t size)
{
if (alignment < pool->alignment)
alignment = pool->alignment;
void *ptr = pj_pool_alloc_from_block(pool->block_list.next,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls move ptr declaration to before first statement (re: coding style)

alignment, size);
if (!ptr)
ptr = pj_pool_allocate_find(pool, size);
ptr = pj_pool_allocate_find(pool, alignment, size);
return ptr;
}

Expand All @@ -82,7 +94,17 @@ PJ_IDEF(pj_pool_t*) pj_pool_create( pj_pool_factory *f,
pj_size_t increment_size,
pj_pool_callback *callback)
{
return (*f->create_pool)(f, name, initial_size, increment_size, callback);
return pj_pool_aligned_create(f, name, initial_size, increment_size, 0, callback);
}

PJ_IDECL(pj_pool_t *) pj_pool_aligned_create(pj_pool_factory *f,
const char *name,
pj_size_t initial_size,
pj_size_t increment_size,
size_t alignment,
pj_pool_callback *callback)
{
return (*f->create_pool)(f, name, initial_size, increment_size, alignment, callback);
}

PJ_IDEF(void) pj_pool_release( pj_pool_t *pool )
Expand Down
35 changes: 21 additions & 14 deletions pjlib/src/pj/pool.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@
#endif

#define LOG(expr) PJ_LOG(6,expr)
#define ALIGN_PTR(PTR,ALIGNMENT) (PTR + (-(pj_ssize_t)(PTR) & (ALIGNMENT-1)))

PJ_DEF_DATA(int) PJ_NO_MEMORY_EXCEPTION;

Expand Down Expand Up @@ -72,7 +71,7 @@ static pj_pool_block *pj_pool_create_block( pj_pool_t *pool, pj_size_t size)
block->end = ((unsigned char*)block) + size;

/* Set the start pointer, aligning it as needed */
block->cur = ALIGN_PTR(block->buf, PJ_POOL_ALIGNMENT);
block->cur = ALIGN_PTR(block->buf, pool->alignment);

/* Insert in the front of the list. */
pj_list_insert_after(&pool->block_list, block);
Expand All @@ -90,7 +89,8 @@ static pj_pool_block *pj_pool_create_block( pj_pool_t *pool, pj_size_t size)
* a new block might be created (depending on whether the pool is allowed
* to resize).
*/
PJ_DEF(void*) pj_pool_allocate_find(pj_pool_t *pool, pj_size_t size)
PJ_DEF(void*) pj_pool_allocate_find(pj_pool_t *pool, pj_size_t alignment,
pj_size_t size)
{
pj_pool_block *block = pool->block_list.next;
void *p;
Expand All @@ -100,7 +100,7 @@ PJ_DEF(void*) pj_pool_allocate_find(pj_pool_t *pool, pj_size_t size)
PJ_CHECK_STACK();

while (block != &pool->block_list) {
p = pj_pool_alloc_from_block(block, size);
p = pj_pool_alloc_from_block(block, alignment, size);
if (p != NULL)
return p;

Expand Down Expand Up @@ -131,11 +131,11 @@ PJ_DEF(void*) pj_pool_allocate_find(pj_pool_t *pool, pj_size_t size)
* the block.
*/
if (pool->increment_size <
size + sizeof(pj_pool_block) + PJ_POOL_ALIGNMENT)
size + sizeof(pj_pool_block) + alignment)
{
pj_size_t count;
count = (size + pool->increment_size + sizeof(pj_pool_block) +
PJ_POOL_ALIGNMENT) /
alignment) /
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same bug as above

Copy link
Member

@bennylp bennylp Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like GH losts my long written comment... :(
It's supposed to be commenting line 134. I think there is a bug, it's not your bug but it's an existing bug. I think it should be added by 2*alignment instead of just one alignment. The rationale is, in worst case, the alignment is done twice. First to align start of buffer, then to align the size itself.

I can reproduce the bug with this code (it's producing assertion error in pj/pool.c:157):

    int init_size = 50 + sizeof(pj_pool_t) + sizeof(pj_pool_block);
    int pool_alignment = 4;
    int huge_alignment = 256;
    int inc_size = 50;
    void *ptr;
    pj_pool_t *pool = pj_pool_aligned_create(mem, NULL, init_size, inc_size, pool_alignment,
                                             &null_callback);
    ptr = pj_pool_aligned_alloc(pool, huge_alignment, 1);
    pj_pool_release(pool);
    PJ_TEST_NOT_NULL(ptr, NULL, return -220);

If you don't mind you can add that to the pool's unit test

pool->increment_size;
block_size = count * pool->increment_size;

Expand All @@ -153,7 +153,7 @@ PJ_DEF(void*) pj_pool_allocate_find(pj_pool_t *pool, pj_size_t size)
if (!block)
return NULL;

p = pj_pool_alloc_from_block(block, size);
p = pj_pool_alloc_from_block(block, alignment, size);
pj_assert(p != NULL);
#if PJ_DEBUG
if (p == NULL) {
Expand All @@ -166,15 +166,18 @@ PJ_DEF(void*) pj_pool_allocate_find(pj_pool_t *pool, pj_size_t size)
/*
* Internal function to initialize pool.
*/
PJ_DEF(void) pj_pool_init_int( pj_pool_t *pool,
const char *name,
pj_size_t increment_size,
pj_pool_callback *callback)
PJ_DEF(void) pj_pool_init_int(pj_pool_t *pool,
const char *name,
pj_size_t increment_size,
pj_size_t alignment,
pj_pool_callback *callback)
{

PJ_CHECK_STACK();

pool->increment_size = increment_size;
pool->callback = callback;
pool->alignment = (alignment < PJ_POOL_ALIGNMENT) ? PJ_POOL_ALIGNMENT : alignment;

if (name) {
char *p = pj_ansi_strchr(name, '%');
Expand All @@ -196,6 +199,7 @@ PJ_DEF(void) pj_pool_init_int( pj_pool_t *pool,
PJ_DEF(pj_pool_t*) pj_pool_create_int( pj_pool_factory *f, const char *name,
pj_size_t initial_size,
pj_size_t increment_size,
pj_size_t alignment,
pj_pool_callback *callback)
{
pj_pool_t *pool;
Expand All @@ -208,6 +212,9 @@ PJ_DEF(pj_pool_t*) pj_pool_create_int( pj_pool_factory *f, const char *name,
PJ_ASSERT_RETURN(initial_size >= sizeof(pj_pool_t)+sizeof(pj_pool_block),
NULL);

if (alignment < PJ_POOL_ALIGNMENT)
alignment = PJ_POOL_ALIGNMENT;

/* If callback is NULL, set calback from the policy */
if (callback == NULL)
callback = f->policy.callback;
Expand All @@ -230,11 +237,11 @@ PJ_DEF(pj_pool_t*) pj_pool_create_int( pj_pool_factory *f, const char *name,
block->end = buffer + initial_size;

/* Set the start pointer, aligning it as needed */
block->cur = ALIGN_PTR(block->buf, PJ_POOL_ALIGNMENT);
block->cur = ALIGN_PTR(block->buf, alignment);

pj_list_insert_after(&pool->block_list, block);

pj_pool_init_int(pool, name, increment_size, callback);
pj_pool_init_int(pool, name, increment_size, alignment, callback);

/* Pool initial capacity and used size */
pool->capacity = initial_size;
Expand Down Expand Up @@ -275,7 +282,7 @@ static void reset_pool(pj_pool_t *pool)
block = pool->block_list.next;

/* Set the start pointer, aligning it as needed */
block->cur = ALIGN_PTR(block->buf, PJ_POOL_ALIGNMENT);
block->cur = ALIGN_PTR(block->buf, pool->alignment);

pool->capacity = block->end - (unsigned char*)pool;
}
Expand Down
1 change: 1 addition & 0 deletions pjlib/src/pj/pool_buf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ PJ_DEF(pj_pool_t*) pj_pool_create_on_buf(const char *name,
pj_thread_local_set(tls, &param);

return pj_pool_create_int(&stack_based_factory, name, size, 0,
PJ_POOL_ALIGNMENT,
pj_pool_factory_default_policy.callback);
#else
PJ_UNUSED_ARG(buf);
Expand Down
Loading
Loading