Skip to content
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.

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

[DNM] llext_manager: Fix for multipipeline issue with llext #8939

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

softwarecki
Copy link
Collaborator

The module_data structure is unique for each module instance. We will not be able to tell whether this is the first loading of the module based on the value of any of its fields. I moved the llext structure to the module context and added an instance counter to protect against loading the module code into memory for subsequent instances. When releasing a module, we check whether it was the last instance of it.

Now llext_manager_link funtions gets struct llext **llext instead of
struct module_data *md.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
Moved the llext handle to the module context. Each module instance has its
own module_data structure, which resulted in the module being loaded every
time.

Signed-off-by: Adrian Warecki <adrian.warecki@intel.com>
@lyakh
Copy link
Collaborator

lyakh commented Mar 14, 2024

@softwarecki one question: have you tested these patches with llext and in particular with the scenario that you are trying to fix - with multiple pipelines? If not, let's put some "Do Not Merge" indication in the PRs title. It's currently a draft, but would be safer to keep a reminder there too. Unfortunately we don't yet have LLEXT tests in the CI (yes, I'm working towards that) and without testing these changes are too intrusive to verify just by reading.

@@ -267,6 +270,7 @@ uintptr_t llext_manager_allocate_module(struct processing_module *proc,
return 0;
}

ctx->instance_count++;
Copy link
Collaborator

Choose a reason for hiding this comment

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

no need. LLEXT in Zephyr already does refcounting.

@softwarecki softwarecki changed the title llext_manager: Fix for multipipeline issue with llext [DNM] llext_manager: Fix for multipipeline issue with llext Mar 14, 2024
@softwarecki
Copy link
Collaborator Author

@lyakh Don't treat it like a real PR. I created it just to show my vision of where I thought the zephyr llext handle should be stored. The module_data structure is created per instance and doesn't seem to be the right place.

@lyakh
Copy link
Collaborator

lyakh commented Mar 15, 2024

@lyakh Don't treat it like a real PR. I created it just to show my vision of where I thought the zephyr llext handle should be stored. The module_data structure is created per instance and doesn't seem to be the right place.

Thanks, it wasn't quite clear, that this was just an "idea demonstration," not a real PR. With a "DNM" prefix now it's clearer.
Yes, you're right, the llext context should be per driver, not per module. The library manager context might indeed be a good place for it, thanks for the idea.

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.

2 participants