Skip to content

alloc: Fix bug in handling first_free member of the block map - #2238

Merged
lgirdwood merged 1 commit into
thesofproject:masterfrom
paulstelian97:alloc_what
Dec 23, 2019
Merged

lgirdwood merged 1 commit into
thesofproject:masterfrom
paulstelian97:alloc_what

Conversation

@paulstelian97

@paulstelian97 paulstelian97 commented Dec 20, 2019 •

Copy link
Copy Markdown
Collaborator

That field is intended to point to the first free block of this
particular block map (part of heap). As long as the heap doesn't fill
up the old code manages it correctly.

However once the block map becomes full (all blocks are occupied)
this member will no longer point to a free block.

Without this fix rfree may fail to update this pointer to point to the
correct block when freeing a block from a full heap. Not updating this
will cause subsequent memory allocations to hand out used blocks as if
they were free. This fix ensures that will not happen in this edge case.

Signed-off-by: Paul Olaru paul.olaru@nxp.com

Fixes #2233

@paulstelian97 paulstelian97 changed the title [HACK] alloc: if first_free is stale recompute it alloc: Fix bug in handling first_free member of the block map Dec 20, 2019
@paulstelian97
paulstelian97 marked this pull request as ready for review December 20, 2019 14:29
@paulstelian97

Copy link
Copy Markdown
Collaborator Author

Copy-pasting my explanation from the issue:

Let's say we have a heap with two blocks. Initially they are both free:

+--------+
|  FREE  |  <-- first_free
+--------+
|  FREE  |
+--------+

Now allocate one of the blocks. It will give out the first free block, from where "first_free" points, and will search for the next free block. Nothing wrong with this. It becomes:

+--------+
|  USED  |
+--------+
|  FREE  |  <-- first_free
+--------+

Let's also allocate the second block now. No free blocks are found, first_free remains stuck in place. Also, free_count is set to 0 now so get_ptr_from_heap will not even check this heap anymore, so first_free doesn't need to be valid anymore.

+--------+
|  USED  |
+--------+
|  USED  |  <-- first_free
+--------+

Let's free the first block:

+--------+
|  FREE  |  <-- first_free
+--------+
|  USED  |
+--------+

Nothing wrong with that, first_free is set correctly in free_block. Now allocate it again:

+--------+
|  USED  |  <-- first_free
+--------+
|  USED  |
+--------+

Fair game, it doesn't need to be valid. Let's free the second block now:

+--------+
|  USED  |  <-- first_free
+--------+
|  FREE  |
+--------+

Oops? Since the block is later than where first_free is pointing, this pointer is not updated and suddenly we are in an invalid situation.

If we try to allocate again the FIRST block will now be returned from this heap, which causes havoc.

This pull request will ensure that first_free will point correctly in the last "picture" here.

That field is intended to point to the first free block of this
particular block map (part of heap). As long as the heap doesn't fill
up the old code manages it correctly.

However once the block map becomes full (all blocks are occupied)
this member will no longer point to a free block.

Without this fix rfree may fail to update this pointer to point to the
correct block when freeing a block from a full heap. Not updating this
will cause subsequent memory allocations to hand out used blocks as if
they were free. This fix ensures that will not happen in this edge case.

Signed-off-by: Paul Olaru <paul.olaru@nxp.com>
@dbaluta

dbaluta commented Dec 20, 2019

Copy link
Copy Markdown
Collaborator

@paulstelian97 thanks a lot for this change! With this I can no longer reproduce #2233

@paulstelian97

Copy link
Copy Markdown
Collaborator Author

SOFCI TEST

@paulstelian97

Copy link
Copy Markdown
Collaborator Author

(Unit tests anyone?)

@paulstelian97

Copy link
Copy Markdown
Collaborator Author

Can't see any failing unit tests but the GH status says "Tests failed". Anyone who can actually see what happened can give me some feedback?

@jajanusz

Copy link
Copy Markdown
Contributor

Can't see any failing unit tests but the GH status says "Tests failed". Anyone who can actually see what happened can give me some feedback?

I guess license server was down and it failed build cos of that... Anyway CI is green now all these errors were false negatives.

@lgirdwood
lgirdwood merged commit 343d111 into thesofproject:master Dec 23, 2019
@paulstelian97
paulstelian97 deleted the alloc_what branch December 23, 2019 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] DSP panic when stopping arecord while aplay is running

4 participants