Skip to content

[RFC] [proposal] alloc: treat memory caps separately #1394

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 2 commits into from
Closed

[RFC] [proposal] alloc: treat memory caps separately #1394

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4c5e91b
alloc: fix bug in memory caps for buffers
mrajwa May 8, 2019
d360f3f
alloc: remove unnecessary check for single block size
mrajwa May 8, 2019
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
8 changes: 3 additions & 5 deletions src/lib/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -324,13 +324,11 @@ static struct mm_heap *get_heap_from_ptr(void *ptr)
static struct mm_heap *get_heap_from_caps(struct mm_heap *heap, int count,
uint32_t caps)
{
uint32_t mask;
int i;

/* find first heap that support type */
for (i = 0; i < count; i++) {
mask = heap[i].caps & caps;
if (mask == caps)
if (heap[i].caps & caps)

@tlauda tlauda May 9, 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.

Isn't the output the same?

heap[i].caps = SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_EXT | SOF_MEM_CAPS_CACHE
caps = SOF_MEM_CAPS_RAM
mask = SOF_MEM_CAPS_RAM

@libinyang libinyang May 9, 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.

@tlauda
Please image:
heap[i].caps = SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_EXT
caps = SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_CACHE

@mrajwa mrajwa May 9, 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.

Yes, and in the case @libinyang shown the allocation for this heap will be rejected with current version because heap[i].caps != caps . This is wrong we should accept the request because both caps have common SOF_MEM_CAPS_RAM and this is exactly what this PR does :)

@tlauda , do you see it now?

@tlauda tlauda May 9, 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.

@libinyang Your example shows, that this change is even worse, because it actually breaks things. You are requesting memory which should have the RAM and CACHE capabilities. Why do you expect to have successful allocation on the heap, which supports only one of the caps?

@mrajwa mrajwa May 9, 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.

@tlauda, the question is how do we treat CAPS? When we pass 3 different caps along side with request do we want to allocate from first memory region which support either one of these caps OR we want to allocate from memory region which supports all of the caps?
I feel like only the former approach is correct - one of the caps is supported then we can accept request.

@tlauda tlauda May 9, 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.

@mrajwa It's probably very subjective and depends on how to look at it, but in this case I would assume, that the caps are there to enforce something. If I need to allocate memory which requires CACHE and DMA, then I don't want to have it allocated on the heap which is only CACHE. I view them rather as CACHE AND DMA, not CACHE OR DMA. What is your specific usage scenario? Maybe you just need more generic caps created instead of changing the current logic?

@mrajwa mrajwa May 9, 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.

Maybe you just need more generic caps created instead of changing the current logic?

Well I think the reason why we send multiple CAPS in request is to have, as you said "generic caps". Meaning "please try to allocate from memory region which supports SOF_MEM_CAPS_RAM, but if you fail (not enough space?) instead of terminating the allocation please try with SOF_MEM_CAPS_HP". So this is the way I see it being use.

What is your specific usage scenario?

So we have an issue raised by @keyonjie #1382. Basically, with complete, full WoV topology (multiple pipelines, many components, etc) one of the component (selector in this case but could be any other) failed the allocation of its comp_buffer because it requested SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_HP | SOF_MEM_CAPS_CACHE | SOF_MEM_CAPS_DMA. There is only one memory region which supports all of these caps, namely heap hp buffer. So it looks like this memory region was already consumed at the time selector tried to allocate its buffer. Since no other memory region has exact same mem caps whole allocation failed. If we change the logic to pick memory region which supports one of the requested caps, memory allocation continues with next block and and all works fine.

@libinyang libinyang May 9, 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.

Yes. I'm now agree @tlauda . Otherwise, how can we try to allocate the memory meet both CACHE AND DMA?

@mrajwa mrajwa May 9, 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.

@libinyang, with my patch you can not. However do we have such a use case to allocate memory region which supports both CACHE and DMA? Do we in general want to differentiate memory blocks in a way This is for SOF_MEM_CAPS_CACHE AND SOF_MEM_CAPS_DMA but not for SOF_MEM_CAPS_CACHE or SOF_MEM_CAPS_DMA separately?

@mrajwa mrajwa May 9, 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.

So I think it all boils down to the question:
When we send multiple memory CAPS do we want to allocate memory from region which supports either one of the CAPS or only all of them?

return &heap[i];
}

Expand Down Expand Up @@ -647,8 +645,8 @@ static void *alloc_heap_buffer(struct mm_heap *heap, int zone, uint32_t caps,
for (i = heap->blocks - 1; i >= 0; i--) {
map = &heap->map[i];

/* allocate if block size is smaller than request */
if (heap->size >= bytes && map->block_size < bytes) {
/* does whole heap have enough space? */
if (heap->size >= bytes) {

@libinyang libinyang May 9, 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.

The original code have a high performance. I suggest using the original code.

@mrajwa mrajwa May 9, 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.

No, this check was already performed in preceding stage and does not belong here.

ptr = alloc_cont_blocks(heap, i, caps, bytes);
if (ptr)
break;
Expand Down