Skip to content

Conversation

@keyonjie
Copy link
Contributor

When freeing blocks, we need to perform writeback and invalidate to the
dirty cache lines, otherwise, they will be evicted from the cache at
some point in the future, which will break the usage of the same memory
region from another DSP core.

Introduce a free_ptr to make sure the original 'ptr' is not changed, so
we can use it for this wb/inv operation.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

@keyonjie keyonjie requested a review from libinyang as a code owner October 10, 2021 00:44
@keyonjie
Copy link
Contributor Author

SOFCI TEST

1 similar comment
@keyonjie
Copy link
Contributor Author

SOFCI TEST

When freeing blocks, we need to perform writeback and invalidate to the
dirty cache lines, otherwise, they will be evicted from the cache at
some point in the future, which will break the usage of the same memory
region from another DSP core.

Introduce a free_ptr to make sure the original 'ptr' is not changed, so
we can use it for this wb/inv operation.

Signed-off-by: Keyon Jie <yang.jie@linux.intel.com>
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

This an important fix for sure. One comment on implementation, but not a blocking concern.

* 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

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

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

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

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

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

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

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

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.

@lgirdwood lgirdwood merged commit 8107d6a into thesofproject:main Oct 14, 2021
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.

5 participants