Skip to content

[proposal]alloc: don't log error if failure was allowed #2058

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

Closed
mrajwa wants to merge 1 commit into from
Closed

[proposal]alloc: don't log error if failure was allowed #2058

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
2 changes: 1 addition & 1 deletion src/arch/xtensa/schedule/task.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ int task_context_init(void *task_ctx, void *entry, void *arg0, void *arg1,
ctx->stack_size = stack_size;
} else {
ctx->stack_base = rballoc(RZONE_BUFFER, SOF_MEM_CAPS_RAM,
SOF_TASK_DEFAULT_STACK_SIZE);
SOF_TASK_DEFAULT_STACK_SIZE, false);
if (!ctx->stack_base)
return -ENOMEM;
ctx->stack_size = SOF_TASK_DEFAULT_STACK_SIZE;
Expand Down
5 changes: 3 additions & 2 deletions src/audio/buffer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ struct comp_buffer *buffer_alloc(uint32_t size, uint32_t caps, uint32_t align)
return NULL;
}

buffer->addr = rballoc_align(RZONE_BUFFER, caps, size, align);
buffer->addr = rballoc_align(RZONE_BUFFER, caps, size, align, false);
if (!buffer->addr) {
rfree(buffer);
trace_buffer_error("buffer_alloc() error: "

@dbaluta dbaluta Nov 5, 2019

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@mrajwa But we will still see errors logged here right?

@mrajwa mrajwa Nov 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@dbaluta, no you will see only regular log message which shall be treated as a warning. Previously it was error message and people were confused, linking almost every WoV issue with this error message like "Ohh of course it happens because there was not enough memory".

Expand Down Expand Up @@ -86,7 +86,8 @@ int buffer_set_size(struct comp_buffer *buffer, uint32_t size)
if (size == buffer->size)
return 0;

new_ptr = rbrealloc(buffer->addr, RZONE_BUFFER, buffer->caps, size);
new_ptr = rbrealloc(buffer->addr, RZONE_BUFFER, buffer->caps, size,
false);

/* we couldn't allocate bigger chunk */
if (!new_ptr && size > buffer->size) {
Expand Down
2 changes: 1 addition & 1 deletion src/audio/detect_test.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,7 @@ static int alloc_mem_load(struct comp_data *cd, uint32_t size)

free_mem_load(cd);

cd->model.data = rballoc(RZONE_BUFFER, SOF_MEM_CAPS_RAM, size);
cd->model.data = rballoc(RZONE_BUFFER, SOF_MEM_CAPS_RAM, size, false);

if (!cd->model.data) {
trace_keyword_error("alloc_mem_load() alloc failed");
Expand Down
9 changes: 6 additions & 3 deletions src/audio/eq_fir/eq_fir.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,7 +345,8 @@ static int eq_fir_setup(struct comp_data *cd, int nch)
return 0;

/* Allocate all FIR channels data in a big chunk and clear it */
cd->fir_delay = rballoc(RZONE_RUNTIME, SOF_MEM_CAPS_RAM, size_sum);
cd->fir_delay = rballoc(RZONE_RUNTIME, SOF_MEM_CAPS_RAM, size_sum,
false);
if (!cd->fir_delay) {
trace_eq_error("eq_fir_setup() error: alloc failed, size = %u",
size_sum);
Expand Down Expand Up @@ -435,7 +436,8 @@ static struct comp_dev *eq_fir_new(struct sof_ipc_comp *comp)
* the EQ is configured later in run-time the size is zero.
*/
if (bs) {
cd->config = rballoc(RZONE_RUNTIME, SOF_MEM_CAPS_RAM, bs);
cd->config = rballoc(RZONE_RUNTIME, SOF_MEM_CAPS_RAM, bs,
false);
if (!cd->config) {
rfree(dev);
rfree(cd);
Expand Down Expand Up @@ -609,7 +611,8 @@ static int fir_cmd_set_data(struct comp_dev *dev,
/* Allocate buffer for copy of the blob. */
cd->config = rballoc(RZONE_RUNTIME, SOF_MEM_CAPS_RAM,
cdata->num_elems +
cdata->elems_remaining);
cdata->elems_remaining,
false);

if (!cd->config) {
trace_eq_error_with_ids(dev, "fir_cmd_set_data() "
Expand Down
3 changes: 2 additions & 1 deletion src/audio/kpb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,8 @@ static size_t kpb_allocate_history_buffer(struct comp_data *kpb)
/* Try to allocate ca_size (current allocation size). At first
* attempt it will be equal to hb_size (history buffer size).
*/
new_mem_block = rballoc(RZONE_RUNTIME, hb_mcp[i], ca_size);
new_mem_block = rballoc(RZONE_RUNTIME, hb_mcp[i], ca_size,
true);

if (new_mem_block) {
/* We managed to allocate a block of ca_size.
Expand Down
2 changes: 1 addition & 1 deletion src/audio/src/src.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -637,7 +637,7 @@ static int src_params(struct comp_dev *dev)
rfree(cd->delay_lines);

cd->delay_lines = rballoc(RZONE_BUFFER, SOF_MEM_CAPS_RAM,
delay_lines_size);
delay_lines_size, false);
if (!cd->delay_lines) {
trace_src_error_with_ids(dev, "src_params() error: "
"failed to alloc cd->delay_lines, "
Expand Down
39 changes: 21 additions & 18 deletions src/include/sof/lib/alloc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,10 +106,11 @@ struct mm {
/* heap allocation and free */
void *_malloc(int zone, uint32_t caps, size_t bytes);
void *_zalloc(int zone, uint32_t caps, size_t bytes);
void *_balloc(int zone, uint32_t caps, size_t bytes, uint32_t alignment);
void *_balloc(int zone, uint32_t caps, size_t bytes, uint32_t alignment,
bool can_fail);
void *_realloc(void *ptr, int zone, uint32_t caps, size_t bytes);

@dbaluta dbaluta Nov 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Instead of adding a new parameter I think we can extend the caps field to also include a flag named SOF_MEM_NOWARN (similar with GFP_NOWARN from Linux kernel).

When this flag is set it has the same meaning as your can_fail parameter.

Also, it might worth renaming caps parameter to flags.

@mrajwa mrajwa Nov 5, 2019

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I was thinking about that too. However I am afraid it may confuse people more than extra single argument. Anyway it is worth discussing - if more people like this idea too I am more than happy to go with this direction.

@jajanusz jajanusz Nov 5, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

caps is for capabilities, not error reporting so it would be confusing, I'd go for another function

@dbaluta dbaluta Nov 5, 2019 •
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@jajanusz will rename parameter from caps to flags and so that we can avoid confusion. Have a look at GFP_ macros in Linux kernel (GFP_NOWARN).

@jajanusz jajanusz Nov 6, 2019

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@dbaluta If you change it to flags then it's fine for me

void *_brealloc(void *ptr, int zone, uint32_t caps, size_t bytes,
uint32_t alignment);
uint32_t alignment, bool can_fail);
void rfree(void *ptr);

#if CONFIG_DEBUG_HEAP
Expand Down Expand Up @@ -142,10 +143,11 @@ void rfree(void *ptr);
} while (0); \
_ptr; })

#define rballoc(zone, caps, bytes) \
#define rballoc(zone, caps, bytes, can_fail) \
({void *_ptr; \
do { \
_ptr = _balloc(zone, caps, bytes, PLATFORM_DCACHE_ALIGN);\
_ptr = _balloc(zone, caps, bytes, PLATFORM_DCACHE_ALIGN, \
can_fail);\
if (!_ptr) { \
trace_error(TRACE_CLASS_MEM, \
"failed to alloc 0x%x bytes caps 0x%x", \
Expand All @@ -168,11 +170,11 @@ void rfree(void *ptr);
} while (0); \
_ptr; })

#define rbrealloc(ptr, zone, caps, bytes) \
#define rbrealloc(ptr, zone, caps, bytes, can_fail) \
({void *_ptr; \
do { \
_ptr = _brealloc(ptr, zone, caps, bytes,\
PLATFORM_DCACHE_ALIGN);\
PLATFORM_DCACHE_ALIGN, can_fail);\
if (!_ptr) { \
trace_error(TRACE_CLASS_MEM, \
"failed to alloc 0x%x bytes caps 0x%x", \
Expand All @@ -182,10 +184,11 @@ void rfree(void *ptr);
} while (0); \
_ptr; })

#define rbrealloc_align(ptr, zone, caps, bytes, alignment) \
#define rbrealloc_align(ptr, zone, caps, bytes, alignment, can_fail) \
({void *_ptr; \
do { \
_ptr = _brealloc(ptr, zone, caps, bytes, alignment); \
_ptr = _brealloc(ptr, zone, caps, bytes, alignment, \
can_fail); \
if (!_ptr) { \
trace_error(TRACE_CLASS_MEM, \
"failed to alloc 0x%x bytes caps 0x%x", \
Expand All @@ -195,10 +198,10 @@ void rfree(void *ptr);
} while (0); \
_ptr; })

#define rballoc_align(zone, caps, bytes, alignment) \
#define rballoc_align(zone, caps, bytes, alignment, can_fail) \
({void *_ptr; \
do { \
_ptr = _balloc(zone, caps, bytes, alignment); \
_ptr = _balloc(zone, caps, bytes, alignment, can_fail); \
if (!_ptr) { \
trace_error(TRACE_CLASS_MEM, \
"failed to alloc 0x%x bytes caps 0x%x", \
Expand All @@ -215,16 +218,16 @@ void alloc_trace_buffer_heap(int zone, uint32_t caps, size_t bytes);

#define rmalloc(zone, caps, bytes) _malloc(zone, caps, bytes)
#define rzalloc(zone, caps, bytes) _zalloc(zone, caps, bytes)
#define rballoc(zone, caps, bytes) \
_balloc(zone, caps, bytes, PLATFORM_DCACHE_ALIGN)
#define rballoc_align(zone, caps, bytes, alignment) \
_balloc(zone, caps, bytes, alignment)
#define rballoc(zone, caps, bytes, can_fail) \
_balloc(zone, caps, bytes, PLATFORM_DCACHE_ALIGN, can_fail)
#define rballoc_align(zone, caps, bytes, alignment, can_fail) \
_balloc(zone, caps, bytes, alignment, can_fail)
#define rrealloc(ptr, zone, caps, bytes) \
_realloc(ptr, zone, caps, bytes)
#define rbrealloc(ptr, zone, caps, bytes) \
_brealloc(ptr, zone, caps, bytes, PLATFORM_DCACHE_ALIGN)
#define rbrealloc_align(ptr, zone, caps, bytes, alignment) \
_brealloc(ptr, zone, caps, bytes, alignment)
#define rbrealloc(ptr, zone, caps, bytes, can_fail) \
_brealloc(ptr, zone, caps, bytes, PLATFORM_DCACHE_ALIGN, can_fail)
#define rbrealloc_align(ptr, zone, caps, bytes, alignment, can_fail) \
_brealloc(ptr, zone, caps, bytes, alignment, can_fail)

#endif

Expand Down
34 changes: 22 additions & 12 deletions src/lib/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ static void *alloc_block(struct mm_heap *heap, int level,

/* allocates continuous blocks */
static void *alloc_cont_blocks(struct mm_heap *heap, int level,
uint32_t caps, size_t bytes, uint32_t alignment)
uint32_t caps, size_t bytes, uint32_t alignment, bool can_fail)
{
struct block_map *map = &heap->map[level];
struct block_hdr *hdr;
Expand Down Expand Up @@ -292,9 +292,16 @@ static void *alloc_cont_blocks(struct mm_heap *heap, int level,
}

if (count > map->count || remaining < count) {
trace_mem_error("error: %d blocks needed for allocation "
"but only %d blocks are remaining",
count, remaining);
if (can_fail)
trace_mem_error("Error: %d blocks needed for "
"allocation of %d bytes but only %d "
"blocks are remaining", count, bytes,
remaining);
else
trace_mem_init("Warning! Not enough free blocks to "
"satisfy request of %d bytes. "
"Available blocks %d, needed %d",
bytes, count, remaining);
return NULL;
}

Expand Down Expand Up @@ -717,7 +724,7 @@ void *rzalloc_core_sys(int core, size_t bytes)

/* allocates continuous buffers - not for direct use, clients use rballoc() */
static void *alloc_heap_buffer(struct mm_heap *heap, int zone, uint32_t caps,
size_t bytes, uint32_t alignment)
size_t bytes, uint32_t alignment, bool can_fail)
{
struct block_map *map;
int i, temp_bytes = bytes;
Expand Down Expand Up @@ -770,7 +777,8 @@ static void *alloc_heap_buffer(struct mm_heap *heap, int zone, uint32_t caps,
/* allocate if block size is smaller than request */
if (heap->size >= bytes && map->block_size < bytes) {
ptr = alloc_cont_blocks(heap, i, caps,
bytes, alignment);
bytes, alignment,
can_fail);
if (ptr)
break;
}
Expand All @@ -789,7 +797,7 @@ static void *alloc_heap_buffer(struct mm_heap *heap, int zone, uint32_t caps,
}

static void *_balloc_unlocked(int zone, uint32_t caps, size_t bytes,
uint32_t alignment)
uint32_t alignment, bool can_fail)
{
struct mm_heap *heap;
unsigned int i, n;
Expand All @@ -803,7 +811,8 @@ static void *_balloc_unlocked(int zone, uint32_t caps, size_t bytes,
if (!heap)
break;

ptr = alloc_heap_buffer(heap, zone, caps, bytes, alignment);
ptr = alloc_heap_buffer(heap, zone, caps, bytes, alignment,
can_fail);
if (ptr)
break;

Expand All @@ -814,14 +823,15 @@ static void *_balloc_unlocked(int zone, uint32_t caps, size_t bytes,
}

/* allocates continuous buffers - not for direct use, clients use rballoc() */
void *_balloc(int zone, uint32_t caps, size_t bytes, uint32_t alignment)
void *_balloc(int zone, uint32_t caps, size_t bytes, uint32_t alignment,
bool can_fail)
{
void *ptr = NULL;
uint32_t flags;

spin_lock_irq(memmap.lock, flags);

ptr = _balloc_unlocked(zone, caps, bytes, alignment);
ptr = _balloc_unlocked(zone, caps, bytes, alignment, can_fail);

spin_unlock_irq(memmap.lock, flags);

Expand Down Expand Up @@ -891,7 +901,7 @@ void *_realloc(void *ptr, int zone, uint32_t caps, size_t bytes)
}

void *_brealloc(void *ptr, int zone, uint32_t caps, size_t bytes,
uint32_t alignment)
uint32_t alignment, bool can_fail)
{
void *new_ptr = NULL;
uint32_t flags;
Expand All @@ -901,7 +911,7 @@ void *_brealloc(void *ptr, int zone, uint32_t caps, size_t bytes,

spin_lock_irq(memmap.lock, flags);

new_ptr = _balloc_unlocked(zone, caps, bytes, alignment);
new_ptr = _balloc_unlocked(zone, caps, bytes, alignment, can_fail);

if (new_ptr && ptr)
memcpy_s(new_ptr, bytes, ptr, bytes);
Expand Down
2 changes: 1 addition & 1 deletion src/trace/dma-trace.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ static int dma_trace_buffer_init(struct dma_trace_data *d)
/* allocate new buffer */
buf = rballoc(RZONE_BUFFER,
SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_DMA,
DMA_TRACE_LOCAL_SIZE);
DMA_TRACE_LOCAL_SIZE, false);
if (!buf) {
trace_buffer_error("dma_trace_buffer_init() error: "
"alloc failed");
Expand Down