Skip to content

alloc: free_block: perform wb/inv for the blocks #4861

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

Merged
lgirdwood merged 1 commit into from
Oct 14, 2021
Merged

alloc: free_block: perform wb/inv for the blocks #4861

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
31 changes: 20 additions & 11 deletions src/lib/alloc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,7 @@ static void free_block(void *ptr)
struct block_hdr *hdr;
void *cached_ptr = uncache_to_cache(ptr);
void *uncached_ptr = cache_to_uncache(ptr);
void *free_ptr;
int i;
int block;
int used_blocks;
Expand All @@ -465,21 +466,21 @@ static void free_block(void *ptr)
if (!heap) {
heap = get_heap_from_ptr(uncached_ptr);
if (!heap) {
tr_err(&mem_tr, "free_block(): invalid heap = %p, cpu = %d",
tr_err(&mem_tr, "free_block(): invalid heap, ptr = %p, cpu = %d",
ptr, cpu_get_id());
return;
}
ptr = uncached_ptr;
free_ptr = uncached_ptr;
} else {
ptr = cached_ptr;
free_ptr = cached_ptr;
}

/* find block that ptr belongs to */
for (i = 0; i < heap->blocks; i++) {
block_map = &heap->map[i];

/* is ptr in this block */
if ((uint32_t)ptr < (block_map->base +
if ((uint32_t)free_ptr < (block_map->base +
(block_map->block_size * block_map->count)))
break;

Expand All @@ -488,13 +489,13 @@ static void free_block(void *ptr)
if (i == heap->blocks) {

/* not found */
tr_err(&mem_tr, "free_block(): invalid ptr = %p cpu = %d",
ptr, cpu_get_id());
tr_err(&mem_tr, "free_block(): invalid free_ptr = %p cpu = %d",
free_ptr, cpu_get_id());
return;
}

/* calculate block header */
block = ((uint32_t)ptr - block_map->base) / block_map->block_size;
block = ((uint32_t)free_ptr - block_map->base) / block_map->block_size;

hdr = &block_map->block[block];

Expand All @@ -503,17 +504,25 @@ static void free_block(void *ptr)
* be from different block since we got user pointer here
* or null if header was not set)
*/
if (hdr->unaligned_ptr != ptr && hdr->unaligned_ptr) {
ptr = hdr->unaligned_ptr;
block = ((uint32_t)ptr - block_map->base)
if (hdr->unaligned_ptr != free_ptr && hdr->unaligned_ptr) {
free_ptr = hdr->unaligned_ptr;
block = ((uint32_t)free_ptr - block_map->base)
/ block_map->block_size;
hdr = &block_map->block[block];
}

/* report an error if ptr is not aligned to block */
if (block_map->base + block_map->block_size * block != (uint32_t)ptr)
if (block_map->base + block_map->block_size * block != (uint32_t)free_ptr)
panic(SOF_IPC_PANIC_MEM);

/* There may still be live dirty cache lines in the region
* on the current core. Those must be invalidated, otherwise
* they will be evicted from the cache at some point in the
* future, on top of the memory region now being used for
* different purposes on another core.
*/
dcache_writeback_invalidate_region(ptr, block_map->block_size * hdr->size);
Copy link
Collaborator

@kv2019i kv2019i Oct 11, 2021

Choose a reason for hiding this comment

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

@keyonjie I wonder why you use "ptr" here. I guess this is harmless, but it seems this may be called also on noncached pointers.

Copy link
Contributor Author

@keyonjie keyonjie Oct 12, 2021

Choose a reason for hiding this comment

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

@keyonjie I wonder why you use "ptr" here. I guess this is harmless, but it seems this may be called also on noncached pointers.

The dcache_writeback_invalidate_region() internal will do nothing for noncached "ptr", and do flush and invalidate for cached "ptr".

The "ptr" is what the user/caller used, while it could be different with the one used for memory blocks management. Imagine that we are using uncached address for BUFFER zone management, while user can use its corresponding cached alias to read/write from the cache lines. That's why we might need to free blocks with uncached_ptr, and do wb/inv with cached one.

On the other hand, if the user/caller uses uncached ptr directly, we don't want/need to do extra/superfluous wb/inv with its alias cached ptr (cached_ptr), so use "ptr" -- the one the user used and pass it into dcache_writeback_invalidate_region() to decide if wb/inv is actually needed.

Copy link
Collaborator

@ranj063 ranj063 Oct 12, 2021

Choose a reason for hiding this comment

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

@keyonjie what about the size of the memory you're invalidatign and writing back here? Shouldnt we be using the original size that was allocated?

Copy link
Contributor Author

@keyonjie keyonjie Oct 13, 2021

Choose a reason for hiding this comment

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

@keyonjie what about the size of the memory you're invalidatign and writing back here? Shouldnt we be using the original size that was allocated?

We don't have the original size from the rfree() invoking, and since the whole block is occupied by the caller exclusively, so wb/invalidate the whole block size is safe, although the caller may actually not touching some portion of it at all.

OTOH, if parts of the specified address is not in the cache (as the caller not touching it at all), the wb/inv instruction will actually has no effect on those address, no actual writeback or invalidate happens with those address.

Copy link
Collaborator

@ranj063 ranj063 Oct 13, 2021

Choose a reason for hiding this comment

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

@keyonjie thats good but then shouldnt we be using free_ptr instead of ptr?

Copy link
Contributor Author

@keyonjie keyonjie Oct 14, 2021

Choose a reason for hiding this comment

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

@keyonjie thats good but then shouldnt we be using free_ptr instead of ptr?

No, please see my comment above: #4861 (comment)
The free_ptr could be different with the user used ptr.

Copy link
Collaborator

@paulstelian97 paulstelian97 Oct 14, 2021

Choose a reason for hiding this comment

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

@keyonjie Isn't it possible that you then wbinv part of the next block? To be fair you're not losing data so this is reasonable but was this accounted for when you wrote the patch?

Copy link
Contributor Author

@keyonjie keyonjie Oct 14, 2021

Choose a reason for hiding this comment

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

@paulstelian97 Yes, that was considered: one fact is that all the return allocated pointers are PLATFORM_DCACHE_ALIGN aligned today, so there is no 2 allocated buffers share a same dcache line, wbinv of one allocated cached buffer will not take the cache line from another allocated buffer.


heap_is_full = !block_map->free_count;

/* free block header and continuous blocks */
Expand Down