Conversation
lyakh
force-pushed
the
fastget
branch
2 times, most recently
from
0ef48e3 to
59b3204
Compare
| k_spinlock_key_t key; | ||
| void *ret; | ||
|
|
||
| key = k_spin_lock(&data->lock); |
There was a problem hiding this comment.
Would it make sense to switch this from a spinlock to a mutex? Right now the spinlock covers a relatively large scope, including array iteration and memory allocation.
There was a problem hiding this comment.
@softwarecki I think so, yes, but should be a separate commit, feel free to make a PR, or I can do it later
softwarecki
left a comment
softwarecki
left a comment
There was a problem hiding this comment.
Memory domain retrieval for the proxy variant needs fixing. The proxy already stores a pointer to the memory domain in its context structure. The struct k_mem_domain *mdom; in the struct processing_module structure is only needed for APPLICATION variant.
|
Looks like these aren't just "minor fixes". This condition is crucial: Without it I observe partition leaked because the corresponding fast_put is called with mdom=NULL. @lyakh could you please resume work on this? I already spoke to @softwarecki. He agreed to resolve potential conflicts in his LLEXT changes. |
lyakh
requested review from
abonislawski,
dbaluta,
lbetlej,
lgirdwood,
mmaka1 and
plbossart
as code owners
@softwarecki not sure which code in this PR you're referring to, could you clarify please? |
There was a problem hiding this comment.
Pull request overview
This PR updates the fast-get userspace handling to fix a heap free call and simplify tracking of userspace vs kernel callers, introducing a small RTOS abstraction helper to detect whether a thread is a userspace thread.
Changes:
- Remove per-entry memory-domain tracking from
fast-getentries and update access-grant logic to key off “current thread is userspace”. - Fix an incorrect heap argument passed to
sof_heap_free()on an error path. - Add
thread_is_userspace()helpers to the Zephyr and POSIX RTOS wrapper headers.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
zephyr/lib/fast-get.c |
Simplifies userspace access-grant logic and fixes heap free argument; updates logging and partition-existence checks. |
zephyr/include/rtos/kernel.h |
Adds thread_is_userspace() for Zephyr builds. |
posix/include/rtos/kernel.h |
Adds a POSIX stub thread_is_userspace() for non-userspace builds. |
| struct k_thread; | ||
| static inline bool thread_is_userspace(struct k_thread *thread) | ||
| { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
thread_is_userspace() introduces the bool type in this header, but the file does not include <stdbool.h>, which will break compilation for any TU that includes this header. Also, the thread parameter is unused and may trigger -Wunused-parameter when the function is referenced; add (void)thread; (or an existing unused-arg macro) to silence warnings.
| if (size > FAST_GET_MAX_COPY_SIZE && current_is_userspace) { | ||
| /* Otherwise we've allocated on thread's heap, so it already has access */ | ||
| int err = fast_get_access_grant(entry->mdom, ret, size); | ||
| int err = fast_get_access_grant(k_current_get()->mem_domain_info.mem_domain, | ||
| ret, size); | ||
|
|
||
| if (err < 0) { | ||
| LOG_ERR("failed to grant access err=%d", err); | ||
| sof_heap_free(NULL, ret); | ||
| sof_heap_free(heap, ret); | ||
| ret = NULL; | ||
| goto out; |
There was a problem hiding this comment.
On the fast_get_access_grant() failure path, entry->size and entry->sram_ptr have already been set, but the function returns with ret = NULL and leaves the entry partially initialized. This can cause subsequent fast_get() calls to hit entry->sram_ptr and then fail the mismatch check (or repeatedly reuse the same slot), effectively breaking fast-get until reboot. Delay assigning entry->{size,sram_ptr} until after all fallible steps succeed, or explicitly clear those fields before goto out on error.
There was a problem hiding this comment.
not introduced by this PR, but added a commit to fix that too
Checking number of memory domain partitions in thread's domain to determine whether it's a userspace thread isn't reliable and is a layering violation. Instead check the K_USER flag which indicates exactly that. Also add a missing check when adding a partition for the newly allocated entry. Suggested-by: Adrian Warecki <adrian.warecki@intel.com> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Use the same heap to free the buffer as the one, used to allocate it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
The mdom member of struct sof_fast_get_entry is redundant, remove it. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
When fast_get() fails it shouldn't leave partially overwritten entries behind. Also fix the call to memcpy_s() to use the proper destination size. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
fix a wrong heap pointer and follow-up on recent changes to remove superfluous code