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

Bluetooth: Host: Allow application to leverage the LONG WQ #85060

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Yagoor
Copy link

@Yagoor Yagoor commented Feb 3, 2025

  • Applications can also benefit from the WQ for long-running tasks without having to create a new WQ

* Applications can also benefit from the WQ for long-running
tasks without having to create a new WQ

Signed-off-by: Yago Fontoura do Rosario <[email protected]>
@Yagoor Yagoor force-pushed the contrib/configurable_long_workqueue branch from 989f1e9 to 27245d0 Compare February 3, 2025 13:28
@jhedberg jhedberg requested a review from PavelVPV February 3, 2025 13:43
@jhedberg
Copy link
Member

jhedberg commented Feb 3, 2025

The long wq is an internal API (no public header file). As long as that's the case, it doesn't make sense to talk about theoretical application access to it. If there ever was to be such a public API, I don't think it should be in a Bluetooth context, since there's nothing Bluetooth specific about it.

@Yagoor
Copy link
Author

Yagoor commented Feb 3, 2025

The long wq is an internal API (no public header file). As long as that's the case, it doesn't make sense to talk about theoretical application access to it. If there ever was to be such a public API, I don't think it should be in a Bluetooth context, since there's nothing Bluetooth specific about it.

So you suggest that I move the long work queue up to the kernel? So that is co-exists along side with the k_sys_work_q?

@jhedberg
Copy link
Member

jhedberg commented Feb 3, 2025

So you suggest that I move the long work queue up to the kernel? So that is co-exists along side with the k_sys_work_q?

That's one option, however one argument I can imagine against it is that the system wq can be configured to have a preemptible priority, giving you the same behavior.

Do you have any specific use case in mind here? E.g. some application that you specifically have that needs this?

@PavelVPV
Copy link
Collaborator

PavelVPV commented Feb 3, 2025

The long wq is an internal API (no public header file). As long as that's the case, it doesn't make sense to talk about theoretical application access to it. If there ever was to be such a public API, I don't think it should be in a Bluetooth context, since there's nothing Bluetooth specific about it.

But any implementation that uses different ITS implementation or crypto backend can run into stack overflow. We met this problem in nRF Connect SDK:

I'm not saying PR is the right approach to fix this and in our case we have found a workaround. But just showing that this can brake.

@alwa-nordic
Copy link
Collaborator

The long wq is an internal API (no public header file). As long as that's the case, it doesn't make sense to talk about theoretical application access to it. If there ever was to be such a public API, I don't think it should be in a Bluetooth context, since there's nothing Bluetooth specific about it.

So you suggest that I move the long work queue up to the kernel? So that is co-exists along side with the k_sys_work_q?

I have some alternatives to consider:

  • Create asynchronous crypto APIs, use them in the Host, and remove the long work queue thread. The application now has memory to create its own thread.
  • Make the application provide the work queue for long work, instead of the other way around. A default snippet creates a dedicated long work queue thread and gives it to the Host, so old applications continue to run.

@Yagoor
Copy link
Author

Yagoor commented Feb 3, 2025

So you suggest that I move the long work queue up to the kernel? So that is co-exists along side with the k_sys_work_q?

That's one option, however one argument I can imagine against it is that the system wq can be configured to have a preemptible priority, giving you the same behavior.

This is partially correct, because bluetooth application require the system workqueue to execute at a cooperative priority

Do you have any specific use case in mind here? E.g. some application that you specifically have that needs this?

Yes, we have an application that is doing some long encryptions. But we don't want to make a new work queue, for RAM reasons and since our use case is very similar to the bt stack one, we just want to be able to defer some long encryptions that it is okay to take time.

@jhedberg
Copy link
Member

jhedberg commented Feb 3, 2025

But any implementation that uses different ITS implementation or crypto backend can run into stack overflow. We met this problem in nRF Connect SDK

That's a fair point, although I wouldn't call that something arising from the app layer. Simply based on this I think it's justifiable to add a prompt for the stack size. That said, you could also use a Kconfig overlay in your downstream tree to tweak the stack.

  • Create asynchronous crypto APIs, use them in the Host, and remove the long work queue thread. The application now has memory to create its own thread.
  • Make the application provide the work queue for long work, instead of the other way around. A default snippet creates a dedicated long work queue thread and gives it to the Host, so old applications continue to run.

Both of these options sound reasonable to me. Not sure how likely the first one is, however, considering that you'd need to get such a change into the PSA APIs.

@PavelVPV
Copy link
Collaborator

PavelVPV commented Feb 3, 2025

Create asynchronous crypto APIs, use them in the Host, and remove the long work queue thread. The application now has memory to create its own thread.\nMake the application provide the work queue for long work, instead of the other way around. A default snippet creates a dedicated long work queue thread and gives it to the Host, so old applications continue to run.

I'm not sure this is good approach. If an application uses the host directly and for example runs mesh, and uses (like in the case of nRR Connect SDK) some sdk on top of the host, should each subsystem create its own thread? Or how it should be managed? And how this should be explained to users?

This basically moves this problem to an application making it harder to start implementing a simple bluetooth application.

@Yagoor
Copy link
Author

Yagoor commented Feb 3, 2025

Create asynchronous crypto APIs, use them in the Host, and remove the long work queue thread. The application now has memory to create its own thread.\nMake the application provide the work queue for long work, instead of the other way around. A default snippet creates a dedicated long work queue thread and gives it to the Host, so old applications continue to run.

I'm not sure this is good approach. If an application uses the host directly and for example runs mesh, and uses (like in the case of nRR Connect SDK) some sdk on top of the host, should each subsystem create its own thread? Or how it should be managed? And how this should be explained to users?

This basically moves this problem to an application making it harder to start implementing a simple bluetooth application.

What do you think of moving the work queue ownership to the kernel, along side the "normal" work queue.
We can make it configurable, so that it is disabled by default and enabled in all bluetooth applications (and it can also be leveraged by other applications that can need it).
We can call it preemptive work queue or a better name, since we cannot make the system work queue preemptive in bluetooth applications.

@alwa-nordic
Copy link
Collaborator

alwa-nordic commented Feb 3, 2025

  • Create asynchronous crypto APIs, use them in the Host, and remove the long work queue thread. The application now has memory to create its own thread.

[...] sound reasonable to me. Not sure how likely [it] is, however, considering that you'd need to get such a change into the PSA APIs.

Ideally yes, but it only needs to look like an asynchronous API from the outside. It can be implemented by creating a system crypto thread, or a system long work thread, in lieu of a Bluetooth long work thread. Such a thread can of course be used by OS for any approved use cases that don't interfere with each other. But this must be hidden as an implementation detail. I caution against creating another system work queue with open access to it. The system work queue has no rules, so it also has no guarantees.

Of course, crypto vendors can later implement the asynchronous API on asynchronous primitives, saving the memory a thread would take.

@alwa-nordic
Copy link
Collaborator

It appears to me what everyone really wants is a thread to do crypto on. So let's have kernel init spawn a crypto thread, and YAGNI suggests we should not generalize this for other uses until we need it.

I think we must stress that the crypto thread is only for "leaf work" that has no dependencies so it will never deadlock.

@Thalley
Copy link
Collaborator

Thalley commented Feb 3, 2025

It appears to me what everyone really wants is a thread to do crypto on. So let's have kernel init spawn a crypto thread, and YAGNI suggests we should not generalize this for other uses until we need it.

I think we must stress that the crypto thread is only for "leaf work" that has no dependencies so it will never deadlock.

Would the BT GATT databash hashing belong in such a thread too, which we today still use the long WQ for?

@Yagoor
Copy link
Author

Yagoor commented Feb 3, 2025

It appears to me what everyone really wants is a thread to do crypto on. So let's have kernel init spawn a crypto thread, and YAGNI suggests we should not generalize this for other uses until we need it.
I think we must stress that the crypto thread is only for "leaf work" that has no dependencies so it will never deadlock.

Would the BT GATT databash hashing belong in such a thread too, which we today still use the long WQ for?

If we create a crypto wq, we can replace the long wq. It should be "plug & play"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Bluetooth Host Bluetooth Host (excluding BR/EDR) area: Bluetooth
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants