[RFC] [proposal] alloc: treat memory caps separately by mrajwa · Pull Request #1394 · thesofproject/sof

mrajwa · 2019-05-08T19:41:39Z

Current implementation accept allocation for specific
memory region only if all CAPS supported by that region
match requested CAPS. This is wrong we should accept
request if at least one CAP match request.

Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com

Current implementation accept allocation for specific memory region only if all CAPS supported by that region match requested CAPS. This is wrong we should accept request if at least one CAPS match request. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

This check is not needed since at this point we already know there was no sigle block which could fullfill this request. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

tlauda · 2019-05-09T06:20:36Z

 	for (i = 0; i < count; i++) {
-		mask = heap[i].caps & caps;
-		if (mask == caps)
+		if (heap[i].caps & caps)


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

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

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?

@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?

@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.

@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?

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.

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

@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?

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?

libinyang · 2019-05-09T08:31:23Z

-			/* 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) {


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

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

keyonjie · 2019-05-09T09:53:07Z

Hi @mrajwa going back to #1382, the allocation in test_keyword_new() is with SOF_MEM_CAPS_RAM caps, so your fix is not only change for the specific case, it will change the whole policy of the allocation, which actually loosen the criteria of the "Matched" memory selection and possible to support more fully use the the whole memory.

My concern is that as your PR changes the mindset what we defined and might lead to regression under some cases, as all users to the allocation interface assumed that it will get memory with full capabilities it listed.

mrajwa · 2019-05-09T09:59:32Z

@keyonjie, that's true. Let me ask this question, at the time of selector's buffer allocation which failed you sent following request:

SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_HP | SOF_MEM_CAPS_CACHE | SOF_MEM_CAPS_DMA

Now, have you meant "Please allocate memory from region which supports all of these caps or either one of them?"

tlauda · 2019-05-09T10:13:21Z

I see couple of issues with such selector caps:

Why does selector require SOF_MEM_CAPS_DMA? It doesn't seem correct.
For cAVS platforms basically every memory address is HP, besides the ones that are part of LPSRAM (and marked LP). So maybe our problem is in how we set the capabilities for our heaps.

We need some additional comments to come up with the right solution @mmaka1 @lgirdwood?

keyonjie · 2019-05-09T10:15:57Z

@keyonjie, that true. Let me ask this question, at the time of selector's buffer allocation you sent following request for memory region:

SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_HP | SOF_MEM_CAPS_CACHE | SOF_MEM_CAPS_DMA

Now, have you meant "Please allocate memory from region which supports all of these caps or either one of them?"

Really? I think we don't need this strict caps for detector buffer, let me check.

keyonjie · 2019-05-09T10:29:27Z

I see couple of issues with such selector caps:

Why does selector require SOF_MEM_CAPS_DMA? It doesn't seem correct.

No, it is incorrect. I also noticed that and figure out solution to fix #1382 with tplg change, verified works, @mrajwa so you can take it easy now. :)

For cAVS platforms basically every memory address is HP, besides the ones that are part of LPSRAM (and marked LP). So maybe our problem is in how we set the capabilities for our heaps.

We need some additional comments to come up with the right solution @mmaka1 @lgirdwood?

keyonjie · 2019-05-09T10:35:44Z

@mrajwa @tlauda here it is the correction for the detector/selector caps: #1399

mrajwa · 2019-05-09T11:43:07Z

Okay so, we figured out there is a flaw in the topology which was setting incorrect MEM_CAPS for firmware components. However the issue addressed in this PR is still valid and needs to be discussed further. Therefore I opened the discussion here https://github.com/orgs/thesofproject/teams/sof-developers/discussions/14.

I would like to invite you to express your opinion there

mmaka1 · 2019-05-09T12:40:35Z

We need some additional comments to come up with the right solution @mmaka1 @lgirdwood?

In general, modelling mutually exclusive values of the same attribute as bits (flags) might make the readability a bit worse. In this example it applies to RAM vs. ROM vs. EXT vs. DMA for example. Coding them as an "enum" using a couple of bits from the caps dword could help to avoid confusing and incorrect RAM | DMA unions in a caller's code.

Anyway, it seems to me that the following platform abstract mem classes are available:

RAM,
ROM,
EXT,
DMA

Now, we know that we may combine RAM and DMA with HP and LP attributes and advanced caller may either request a specific type depending on his needs or skip it to indicate that he does not care. In the latter case a platform may have just a single RAM heap (specifies RAM|HP|LP as the caps) or separate ones and should provide them in the preferred allocation order to the allocation unit (eg. RAM|LP goes first followed by RAM|HP). Then a simple (req_caps & heap_caps) == req_caps would work. Other attributes like CACHE shold be filtered out before searching for the heap and processed later (really depends on the attribute).

Edit: CACHE does not seem to be good example of cap ignored while searching for the pool. In case of mem caps, all seem important while registering a pool.

lgirdwood · 2019-05-09T21:19:51Z

Current implementation accept allocation for specific
memory region only if all CAPS supported by that region
match requested CAPS. This is wrong we should accept
request if at least one CAP match request.

Signed-off-by: Marcin Rajwa marcin.rajwa@linux.intel.com

NAK all specified caller flags must be respected in alloc request.

lgirdwood

Flags must be respected.

lgirdwood · 2019-05-24T10:27:36Z

@mrajwa this is modelled on how similar API's work on Linux. It must be a bit mask and not an enum so combinations can be explicitly requested. We also cannot remove HP/LP from the request as they have differences in runtime performance.

However, if just RAM was requested with no HP or LP flag then it would be fair to allocate from either HP or LP pool (based on allocator preference).

Btw, you have not mentioned where, why, how exactly you are running out of memory. There are allocator PRs pending that will help here by making the pools more granular.

tlauda · 2019-05-30T09:55:36Z

@lgirdwood @mrajwa Can we leave only the second commit and deal with the potential caps changes in the future after deciding how to proceed? The discussion has been already started here: https://github.com/orgs/thesofproject/teams/sof-developers/discussions/14.

mrajwa · 2019-05-30T10:00:39Z

@lgirdwood the problem was somebody from driver requested allocation of memory poll in SOF_MEM_CAPS_RAM | SOF_MEM_CAPS_HP hoping that FW will allocate it in region which supports either SOF_MEM_CAPS_RAM or SOF_MEM_CAPS_HP. However this is not going to happen since FW will look for and accept only a region which supports both of this flags. This is a bit unintuitive in my opinion.

Although since probably most of our developers are used to it and it works, we can close this PR and in the future we can go forward by defining narrower regions.

PS: I have also added new comment in sof developers discussion https://github.com/orgs/thesofproject/teams/sof-developers/discussions/14

mmaka1 · 2019-05-31T12:28:41Z

@mrajwa The RAM | HP request is valid. It is equal to simpler HP one. HP is RAM (full abbrev. is HPSRAM), so this mem pool should have RAM capability bit set (LP one as well).

In case the client does not care which *P is used it should request just RAM and selection would depend on the order of declaration (I would prefer HP followed by LP to make sure the HP one is selected by default in this case).

lgirdwood · 2019-06-05T19:59:19Z

@mrajwa ok, lets take the second patch and rework the first as discussed https://github.com/orgs/thesofproject/teams/sof-developers/discussions/14
The second patch could come in another PR if its easier.

lbetlej · 2020-01-27T09:20:03Z

@mrajwa What is a status of the RFC ? DO you agree to close it ?

mrajwa · 2020-01-28T21:42:15Z

@lbetlej, yes I am closing now as there is no need for these changes.

mrajwa added 2 commits May 8, 2019 21:33

alloc: remove unnecessary check for single block size

d360f3f

This check is not needed since at this point we already know there was no sigle block which could fullfill this request. Signed-off-by: Marcin Rajwa <marcin.rajwa@linux.intel.com>

mrajwa requested a review from libinyang as a code owner May 8, 2019 19:41

mrajwa mentioned this pull request May 8, 2019

[BUG]selector buffer new fails with KWD integrated topology on GLK #1382

Closed

tlauda suggested changes May 9, 2019

View reviewed changes

libinyang reviewed May 9, 2019

View reviewed changes

mrajwa requested a review from keyonjie May 9, 2019 09:04

mrajwa changed the title ~~alloc: fix bug in memory caps~~ [proposal]alloc: treat memory caps separately May 9, 2019

lgirdwood requested changes May 9, 2019

View reviewed changes

mmaka1 mentioned this pull request May 10, 2019

[RFC] [Do not merge] ipc: support for sending ipc payload using registers added #1339

Closed

jajanusz changed the title ~~[proposal]alloc: treat memory caps separately~~ [RFC] [proposal] alloc: treat memory caps separately May 20, 2019

mrajwa closed this Jan 28, 2020

Conversation

mrajwa commented May 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrajwa May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mrajwa May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keyonjie commented May 9, 2019

Uh oh!

mrajwa commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tlauda commented May 9, 2019

Uh oh!

keyonjie commented May 9, 2019

Uh oh!

keyonjie commented May 9, 2019

Uh oh!

keyonjie commented May 9, 2019

Uh oh!

mrajwa commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmaka1 commented May 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented May 9, 2019

Uh oh!

lgirdwood left a comment

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented May 24, 2019

Uh oh!

tlauda commented May 30, 2019

Uh oh!

mrajwa commented May 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mmaka1 commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lgirdwood commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lbetlej commented Jan 27, 2020

Uh oh!

mrajwa commented Jan 28, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

mrajwa May 9, 2019 •

edited

Loading

mrajwa May 9, 2019 •

edited

Loading

mrajwa commented May 9, 2019 •

edited

Loading

mrajwa commented May 9, 2019 •

edited

Loading

mmaka1 commented May 9, 2019 •

edited

Loading

mrajwa commented May 30, 2019 •

edited

Loading

mmaka1 commented May 31, 2019 •

edited

Loading

lgirdwood commented Jun 5, 2019 •

edited

Loading