fast-get: fix a recent regression by lyakh · Pull Request #10639 · thesofproject/sof

lyakh · 2026-03-20T15:55:47Z

Recent commits 7d0621e ("fast-get: fix partition leak for multi-thread usage") and 3c757dd ("fast-get: fix crash by freeing buffer before removing partition") introduced a regression when using SRC in DP mode in userspace on PTL and running two aplay -r 44100
back to back. The reason is that SRC isn't calling mod_fast_put() explicitly and is instead relying on automatic clean up, which then happens in a different thread.

In fact those two commits weren't needed AFAICS, since fast_put() shouldn't be called directly from userspace, instead only mod_fast_put() should be called, which is a syscall, and therefore fast_put() then will have access to all the memory.

Copilot

Pull request overview

Fixes a userspace regression in Zephyr’s fast-get cleanup path where memory-domain partition removal can occur from a different thread than the one that originally fast-get’d the buffer (observed with SRC in DP mode and back-to-back aplay -r 44100).

Changes:

Change fast_put() userspace partition removal to use the entry->thread memory domain instead of k_current_get().
Update debug logging to reference entry->thread instead of the current thread.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

zephyr/lib/fast-get.c

lgirdwood

LGTM ,can you respond to copilot. Thanks !

wjablon1 · 2026-03-23T14:04:28Z

zephyr/lib/fast-get.c

 #if CONFIG_USERSPACE
+		struct k_mem_domain *mdom = k_current_get()->mem_domain_info.mem_domain;
+
 		/* We only get there for large buffers */


The relation between num_partitions and large buffers is not obvious. Could you make the comment clearer?

wjablon1 · 2026-03-23T14:20:23Z

zephyr/lib/fast-get.c

 			/* A userspace thread makes the request */
-			if (k_current_get() != entry->thread &&
+			if (mdom != entry->mdom &&
 			    !fast_get_domain_exists(k_current_get(), ret,


I see that the function fast_get_domain_exists iterates through partitions. Why not calling it fast_get_partition_exists?

Recent commits 7d0621e ("fast-get: fix partition leak for multi-thread usage") and 3c757dd ("fast-get: fix crash by freeing buffer before removing partition") introduced a regression when using SRC in DP mode in userspace on PTL and running two aplay -r 44100 back to back. The reason is that SRC isn't calling mod_fast_put() explicitly and is instead relying on automatic clean up, which then happens in a different thread. In fact those two commits weren't needed AFAICS, since fast_put() shouldn't be called directly from userspace, instead only mod_fast_put() should be called, which is a syscall, and therefore fast_put() then will have access to all the memory. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

In fast_put() sram_ptr == entry->sram_ptr, use the former since it's shorter. Also clarify a comment and rename a function to better match its role. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

Before dropping the DP thread memory domain, check if any partitions are left still attached to it. If so, warn and remove them. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

wjablon1 · 2026-03-23T16:06:41Z

zephyr/lib/fast-get.c

-		/* We only get there for large buffers */
+		/*
+		 * We only get there for large buffers, since small buffers with
+		 * enabled userspace don't create fast-get entries


Just to clarify, I think this part is well understood (you can deduce this info just by examining fast_get function itself). No need to further explain it.
The mystery part that I was referring to in my previous comment is this:

mdom->num_partitions > 1

I am not sure if this comment is even related. I am guessing the related comment is below:

/* A userspace thread makes the request */

Am I right? If yes, maybe you could just change it to:

/* if num_partitions > 0, the request comes form a userspace thread */

Or you can combine both comments like this:

/* We only get there for large buffers.
/* Check if the request comes from a userspace thread by examining num_partitions
*/

@wjablon1 yes, that test is related to the "userspace thread" comment. @softwarecki wanted to replace that test with a K_USER check, then it will be clearer

Ok, if we already have a plan for this, forget about my comment and remove unnecessary clarification

lyakh · 2026-03-24T08:23:59Z

@lrudyX I see a single WCL "module creation" test failing to create a copier module with this PR. Doesn't look related?

lyakh · 2026-03-24T08:24:38Z

no PTL nocodec device was available. Re-run jenkins tests

lyakh · 2026-03-24T08:24:43Z

SOFCI TEST

lrudyX · 2026-03-24T08:37:50Z

@lrudyX I see a single WCL "module creation" test failing to create a copier module with this PR. Doesn't look related?

Test failed at deleting pipeline with SRC in DP, which was fixed by "Recent commits 7d0621e ("fast-get: fix partition leak for multi-thread usage") and 3c757dd ("fast-get: fix crash by freeing buffer before removing partition")". It is highly related.

tmleman

LGTM, thanks for fixing!

singalsu

OK for commit "audio: src: free copied coefficients". I don't know enough to review the other commits.

lyakh · 2026-03-24T13:50:38Z

"Internal Intel CI" https://sof-ci.01.org/sof-pr-viewer/#/build/PR10639/build14876904 passes now. Let me just remove the added debugging, after which it should be ready for merging

Until now SRC was relying on the final module clean up to free copied with fast_get() coefficients. This however isn't clean. Fix it and release partitions explicitly. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh · 2026-03-24T15:11:47Z

The "Internal Intel CI" failure this time is a known intermittent "random HDA DMA" failure

softwarecki

The fast_put flow is still incorrect for multiple module instances. The fast_get adds partitions to the memory domain of every instance that allocates the buffer, but the partition is removed only for the instance that releases the memory last.

Storing the memory domain pointer inside the entry also looks pointless in the current design. Should we instead maintain a separate entry per module instance?

Additionally, memory domain retrieval for the proxy variant needs fixing. The proxy already stores a pointer to the memory domain in its context structure.

softwarecki · 2026-03-24T15:43:06Z

src/audio/module_adapter/module/generic.c

 	case MOD_RES_FAST_GET:
-		fast_put(res->heap, container->sram_ptr);
+#if CONFIG_USERSPACE
+		mdom = mod->mdom;


The userspace proxy already stores a pointer to the memory domain in a different place. Need to distinguishing the behavior for PROXY/APPLICATION.

@softwarecki is this patch breaking anything for the "thread" model? If not, I'd propose to merge it to fix the "application" model which is currently broken because of this fast-get regression. Once it's all fixed we can carefully check what we can improve

softwarecki · 2026-03-24T15:45:02Z

src/include/module/module/base.h

 #if CONFIG_USERSPACE
 	struct userspace_context *user_ctx;
-#endif /* CONFIG_USERSPACE */
-#if CONFIG_SOF_USERSPACE_APPLICATION


I think this change is unnecessary, because the proxy already stores the memory domain in its context.

softwarecki · 2026-03-24T15:51:26Z

zephyr/lib/fast-get.c

-				LOG_DBG("grant access to thread %p first was %p", k_current_get(),
-					entry->thread);
-				int err = fast_get_access_grant(k_current_get(), ret, size);
+			if (mdom != entry->mdom &&


I don't really understand the purpose of this check. The entry allocation together with the mdom assignment happens in a different part of the code.

If the same module instance calls fast_get again for the same address (unlikely), then this condition will indeed trigger. But what about other instances of the same module? Do they always rely on fast_get_partition_exists on every call?

maybe just checking for "partition exists" would be enough, but this would be an optimisation, we can optimise it later.

lyakh · 2026-03-24T16:58:08Z

The fast_get adds partitions to the memory domain of every instance that allocates the buffer, but the partition is removed only for the instance that releases the memory last.

@softwarecki I don't think this is correct. That's exactly why fast_put() checks refcount before and after removing the partition, but the partition is removed every time

Copilot AI review requested due to automatic review settings March 20, 2026 15:55

lyakh requested review from abonislawski, dbaluta, iuliana-prodan and kv2019i as code owners March 20, 2026 15:55

Copilot started reviewing on behalf of lyakh March 20, 2026 15:59 View session

Copilot AI reviewed Mar 20, 2026

View reviewed changes

zephyr/lib/fast-get.c Outdated Show resolved Hide resolved

lyakh force-pushed the fastget branch from 98482e3 to 44bcba0 Compare March 23, 2026 12:21

lyakh requested review from lbetlej, lgirdwood, mmaka1 and plbossart as code owners March 23, 2026 12:21

lgirdwood approved these changes Mar 23, 2026

View reviewed changes

lyakh force-pushed the fastget branch from 44bcba0 to b4377aa Compare March 23, 2026 13:12

lyakh added the DNM Do Not Merge tag label Mar 23, 2026

wjablon1 reviewed Mar 23, 2026

View reviewed changes

lyakh force-pushed the fastget branch from b4377aa to 4e94efe Compare March 23, 2026 15:16

lyakh requested review from LaurentiuM1234, marcinszkudlinski, pblaszko and singalsu as code owners March 23, 2026 15:16

lyakh force-pushed the fastget branch from 4e94efe to c77f594 Compare March 23, 2026 15:25

lyakh added 2 commits March 23, 2026 16:31

fast-get: (cosmetic) use a local variable

64b8940

In fast_put() sram_ptr == entry->sram_ptr, use the former since it's shorter. Also clarify a comment and rename a function to better match its role. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

dp: application: check left-over partitions

38a4b9e

Before dropping the DP thread memory domain, check if any partitions are left still attached to it. If so, warn and remove them. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the fastget branch from c77f594 to 59db2cc Compare March 23, 2026 15:33

wjablon1 reviewed Mar 23, 2026

View reviewed changes

lyakh removed the DNM Do Not Merge tag label Mar 23, 2026

lyakh mentioned this pull request Mar 24, 2026

userspace: proxy: Add support for llext modules #10643

Open

tmleman approved these changes Mar 24, 2026

View reviewed changes

singalsu approved these changes Mar 24, 2026

View reviewed changes

abonislawski approved these changes Mar 24, 2026

View reviewed changes

lyakh force-pushed the fastget branch from 59db2cc to 76b764a Compare March 24, 2026 11:16

audio: src: free copied coefficients

930eeee

Until now SRC was relying on the final module clean up to free copied with fast_get() coefficients. This however isn't clean. Fix it and release partitions explicitly. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>

lyakh force-pushed the fastget branch from 76b764a to 930eeee Compare March 24, 2026 13:52

softwarecki requested changes Mar 24, 2026

View reviewed changes

Conversation

lyakh commented Mar 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

lgirdwood left a comment

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!

wjablon1 Mar 23, 2026 • 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!

lyakh commented Mar 24, 2026

Uh oh!

lyakh commented Mar 24, 2026

Uh oh!

lyakh commented Mar 24, 2026

Uh oh!

lrudyX commented Mar 24, 2026

Uh oh!

tmleman left a comment

Choose a reason for hiding this comment

Uh oh!

singalsu left a comment

Choose a reason for hiding this comment

Uh oh!

lyakh commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lyakh commented Mar 24, 2026

Uh oh!

softwarecki left a comment

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!

lyakh commented Mar 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

wjablon1 Mar 23, 2026 •

edited

Loading

lyakh commented Mar 24, 2026 •

edited

Loading