-
Couldn't load subscription status.
- Fork 8.1k
Bluetooth: Host: Run tx_processor on its own thread #97913
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
base: main
Are you sure you want to change the base?
Bluetooth: Host: Run tx_processor on its own thread #97913
Conversation
ATT is invoking user callbacks in its net_buf destroy function. It is
common practice that these callbacks can block on bt_hci_cmd_alloc().
This is a deadlock when the net_buf_unref() happens inside the HCI
driver, invoked from tx_processor.
Blocking callbacks like this appear in our own samples. See further down
about how this problem was detected.
tx_processor not protect against blocking callbacks so it is de-facto
forbidden. The Host should not equip net_bufs with dangerous destroy
callbacks.
This commit makes ATT defer its net_buf destruction and user callback
invocation to the system workqueue, so that net_buf_unref is safe to
call from non-blocking threads. In the case of the deadlock, the
net_buf_unref() was below the tx_processor in the call stack, which (at
the time of this commit) is on the system work queue, so defering it to
the system work queue is preserving the existing behavior.
Future improvement may be to allow the user to provide their own
workqueue for ATT callbacks.
This deadlock was detected because the following test was failing while
moving tx_processor to the bt_taskq:
tests/bsim/bluetooth/ll/throughput/tests_scripts/gatt_write.sh
The above test has an ATT callback `write_cmd_cb` invokes
`bt_conn_le_param_update` can block waiting for `tx_processor`.
The reason it was not failing while tx_processor was on the system work
queue is that the GATT API has a special non-blocking behavior when
called from the system work queue.
Signed-off-by: Aleksander Wasaznik <[email protected]>
Reduce BT_MAX_CONN from 62 to 61 to make it build on integration platform qemu_cortex_m3/ti_lm3s6965 when we add bt_taskq in subsequent commit. Signed-off-by: Aleksander Wasaznik <[email protected]>
9e1ec66 to
27904e9
Compare
| default y if !SOC_SERIES_NRF51X | ||
| # This option is automatically selected for all platforms except nRF51 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See #74345 it works in 3.7 with Vinayak's patch (it does not work in-tree as is without changes), it does not work in any later versions as the stack was reworked to a much slower implementation and the nRF51 CPU is not fast enough to handle the interrupts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case there's a bunch of cleanup that needs to be done, since there are still some overlays for the board in Bluetooth sample apps. There's also supported: ble in the board's yaml.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Zephyr Controller's redesigned control procedure max. CPU usage due to handling of tx ack and the processing of the Rx-ed PDU contiguously is higher within the 150 us tIFS. Hence, under controlled conditions nRF51 assertion checks fail detecting the increased CPU usage that is causing violation of hard real-time deadlines. I have private fork with latest upstreamed rebase of this PR #75197 with legacy control procedure implementation that I continue to use with nRF51 with my boards (bbc_microbits that supports extended advertising and extended scanning too, with one day's hope to use it to listen to Auracast LE audio!).
@alwa-nordic what is the reason for default y if !SOC_SERIES_NRF51X ? If this is assertions in the Controller, then you do not need this (and can be documented as Controller's known issue). If this is due to RAM or Flash usage, then on the SoC variant need not be using in samples.yaml that fail build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the reason for
default y if !SOC_SERIES_NRF51X?
# due to limited RAM on nRF51 devices.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhedberg, would you like to move this discussion to an issue or other forum?
| default y if !SOC_SERIES_NRF51X | ||
| # This option is automatically selected for all platforms except nRF51 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Zephyr Controller's redesigned control procedure max. CPU usage due to handling of tx ack and the processing of the Rx-ed PDU contiguously is higher within the 150 us tIFS. Hence, under controlled conditions nRF51 assertion checks fail detecting the increased CPU usage that is causing violation of hard real-time deadlines. I have private fork with latest upstreamed rebase of this PR #75197 with legacy control procedure implementation that I continue to use with nRF51 with my boards (bbc_microbits that supports extended advertising and extended scanning too, with one day's hope to use it to listen to Auracast LE audio!).
@alwa-nordic what is the reason for default y if !SOC_SERIES_NRF51X ? If this is assertions in the Controller, then you do not need this (and can be documented as Controller's known issue). If this is due to RAM or Flash usage, then on the SoC variant need not be using in samples.yaml that fail build.
|
|
||
| config BT_TX_PROCESSOR_THREAD_PRIO | ||
| int | ||
| default -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| default -1 | |
| default -7 |
to be in sync with CONFIG_BT_HCI_TX_PRIO=7
and
BUILD_ASSERT(CONFIG_BT_DRIVER_RX_HIGH_PRIO < CONFIG_BT_HCI_TX_PRIO);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I'll update it to -7. I selected it arbitrarily anyway. But can you help me understand what the point is and formulate a code comment?
zephyr/subsys/bluetooth/common/dummy.c
Lines 38 to 39 in e57676a
| * This is required in order to dispatch Number of Completed Packets event | |
| * before any new data arrives on a connection to the Host threads. |
I don't understand why tx_processor needs to run for the Controller to generate outstanding Number of Completed Packets events.
Is this maybe about sending the Host Number Of Completed Packets command? I guess it helps throughput in situations with high CPU load, when otherwise the Controller could be starved of Host buffers and start NAK-ing on air? If so, would it make sense to future work on prioritizing only the Host Number of Buffers Complete commands instead of all TX?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see the code comment above is from 9a13a0c, which' commit message says:
The Tx thread priority shall be higher than Rx thread priority in order to correctly detect transaction violations in ATT and SMP protocols.
We are not doing this anymore. It did not work correctly with some Controllers, and it was decided it's not a requirement anyway.
Do you still want me to change priority to -7?
All questions in my previous comment still stand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
detect transaction violations in ATT and SMP protocols.
This is to prevent remote initiated starvation of local ACL Tx buffers, i.e. example, a remote peer sending a burst of ATT Read Requests violating transaction rules leading to local enqueuing back ATT Read/Error Responses.
Hence, even if do not have transaction violation detection today, to be consistent with ONFIG_BT_HCI_TX_PRIO we should use -7. Another PR or GH issue can address collective the use of co-operative priority values like 6, 7 and 8 currently in the Bluetooth subsystem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TX processor has been running on sysworkq before which priority is -1. Can we do profiling and address the priority issue in a separate PR? It is hard to change something without having evidence that this change is needed (even if it is needed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to prevent remote initiated starvation of local ACL Tx buffers, i.e. example, a remote peer sending a burst of ATT Read Requests violating transaction rules leading to local enqueuing back ATT Read/Error Responses.
Well, we don't have those checks in att.c now. So changing the priority of tx_processor is pointless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is to prevent remote initiated starvation of local ACL Tx buffers, i.e. example, a remote peer sending a burst of ATT Read Requests violating transaction rules leading to local enqueuing back ATT Read/Error Responses.
It may be sensible to make our implementation impervious against the kind of DoS you describe (future work). But it will have to be something robust, and it must not rely on thread priorities.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TX processor has been running on sysworkq before which priority is -1. Can we do profiling and address the priority issue in a separate PR? It is hard to change something without having evidence that this change is needed (even if it is needed).
Then, inherit the sysworkq priority here instead of hard coding to -1.
It may be sensible to make our implementation impervious against the kind of DoS you describe (future work). But it will have to be something robust, and it must not rely on thread priorities.
Then, make this user configurable (and, making it inherit system work queue prior), this thread does not need to rely on priority values.
| CONFIG_BT_MAX_CONN=61 | ||
| CONFIG_BT_ID_MAX=61 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? Please check with maintainer of qemu_cortex_m3/ti_lm3s6965
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It ran out of RAM. I assume 62 was chosen to fill RAM on the least capable device in CI, not because it is a requirement of the sample. Now the least capable device only allows for 61.
Please tell me why I should check with the maintainer of qemu_cortex_m3/ti_lm3s6965. It's our (Bluetooth subsystem) sample.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4c6174d5112b (Vinayak Kariappa Chettimada 2020-12-15 18:48:33 +0530 1) sample:
ca2bcf8d1566 (Fabio Baltieri 2023-01-23 19:05:52 +0000 2) description: Bluetooth Peripheral Identity
93b63df7627b (Gerard Marull-Paretas 2023-05-08 13:37:42 +0200 3) name: Demonstrates use of multiple identity and the ability to be connected to from
93b63df7627b (Gerard Marull-Paretas 2023-05-08 13:37:42 +0200 4) multiple central devices
4c6174d5112b (Vinayak Kariappa Chettimada 2020-12-15 18:48:33 +0530 5) tests:
4c6174d5112b (Vinayak Kariappa Chettimada 2020-12-15 18:48:33 +0530 6) sample.bluetooth.peripheral_identity:
4c6174d5112b (Vinayak Kariappa Chettimada 2020-12-15 18:48:33 +0530 7) harness: bluetooth
93b63df7627b (Gerard Marull-Paretas 2023-05-08 13:37:42 +0200 8) platform_allow:
93b63df7627b (Gerard Marull-Paretas 2023-05-08 13:37:42 +0200 9) - qemu_cortex_m3
93b63df7627b (Gerard Marull-Paretas 2023-05-08 13:37:42 +0200 10) - qemu_x86
8dc3f856229c (Torsten Rasmussen 2022-09-14 22:23:15 +0200 11) - nrf52840dk/nrf52840
4c6174d5112b (Vinayak Kariappa Chettimada 2020-12-15 18:48:33 +0530 12) tags: bluetooth
ba7d730e9bd9 (Anas Nashif 2022-11-29 00:23:08 +0000 13) integration_platforms:
ba7d730e9bd9 (Anas Nashif 2022-11-29 00:23:08 +0000 14) - qemu_cortex_m3
ti_lm3s6965 comes as part of qemu_cortex_m3 coverage. If this platform cannot be supported we should explicitly note this in release notes and have a plan to deprecate it. Hence, the maintainer should be informed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, missed that we need to disable this code if tx processor thread is enabled:
zephyr/subsys/bluetooth/host/hci_core.c
Lines 473 to 500 in 27904e9
| /* TODO: disallow sending sync commands from syswq altogether */ | |
| /* Since the commands are now processed in the syswq, we cannot suspend | |
| * and wait. We have to send the command from the current context. | |
| */ | |
| if (k_current_get() == &k_sys_work_q.thread) { | |
| /* drain the command queue until we get to send the command of interest. */ | |
| struct net_buf *cmd = NULL; | |
| do { | |
| cmd = k_fifo_peek_head(&bt_dev.cmd_tx_queue); | |
| LOG_DBG("process cmd %p want %p", cmd, buf); | |
| /* Wait for a response from the Bluetooth Controller. | |
| * The Controller may fail to respond if: | |
| * - It was never programmed or connected. | |
| * - There was a fatal error. | |
| * | |
| * See the `BT_HCI_OP_` macros in hci_types.h or | |
| * Core_v5.4, Vol 4, Part E, Section 5.4.1 and Section 7 | |
| * to map the opcode to the HCI command documentation. | |
| * Example: 0x0c03 represents HCI_Reset command. | |
| */ | |
| __maybe_unused bool success = process_pending_cmd(HCI_CMD_TIMEOUT); | |
| BT_ASSERT_MSG(success, "command opcode 0x%04x timeout", opcode); | |
| } while (buf != cmd); | |
| } |
Should we disable it? Or keep it and print a warning like in 93c2d8d? It may be a good idea to keep it until we do a full audit of the code on tx_processor and determine that it cannot possibly happen. |
I think this path should never exist in case of tx processor thread. If it happens that somehow Host calls |
Do you mean this path never does anything useful, even when
I'm not sure I agree a deadlock is better than a warning, for our business. But I am willing to chance that there is no deadlock and remove the workaround. |
What can I as a user do with warning? |
I thought about that: zephyr/subsys/bluetooth/host/hci_core.c Line 495 in 93c2d8d
And it's not about the warning. There is a chance you get a functioning system, as opposed to a deadlock. That's the point of defensive programming. We try to recover also from situations we think are not possible. Because often we are wrong. And in this case we haven't even done an audit yet. And one of the commits in this PR is fixing a case of this problem. There are downsides; there is a chance you get a stack overflow. This is something to weight up. BTW, I think if we keep this defense, we should also add it to |
|
I added a commit disabling the workaround. |
|
@alwa-nordic you might want to set aside some time next week to go through potential breakage resulting from the weekly CI run, if this gets merged this week. I suspect there may very well be some sample & test configurations that don't fit anymore. Even the "push to main" CI run might reveal those, but let's see. Still waiting for an update from @cvinayak since his review is currently blocking this. |
|
Bluetooth WG: let's disable this by default, and then it can go in for 4.3. Some test config should still enable it for coverage. |
subsys/bluetooth/host/hci_core.c
Outdated
| /* TODO: disallow sending sync commands from syswq altogether */ | ||
|
|
||
| /* Since the commands are now processed in the syswq, we cannot suspend | ||
| /* If the commands are now processed in the syswq, we cannot suspend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /* If the commands are now processed in the syswq, we cannot suspend | |
| /* If the commands are processed in the syswq, we cannot suspend |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No concerns from me, but assume you will have to appease the others still :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated
When thread that TX processor is used to send commands and data to Controller is also used for sync commands sending and command buffer allocation, a deadlock happens. This thread is used to avoid such deadlocks by moving TX processor to its own dedicated thread exclusively used by tx processor only. Signed-off-by: Pavel Vasilyev <[email protected]> Signed-off-by: Aleksander Wasaznik <[email protected]>
The workaround in bt_cmd_send_sync should no longer by needed when tx_processor is not on the system work queue. Signed-off-by: Aleksander Wasaznik <[email protected]>
adb8457
936486c to
adb8457
Compare
|
Updated: |
|
|
||
| config BT_TX_PROCESSOR_THREAD_PRIO | ||
| int | ||
| default -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TX processor has been running on sysworkq before which priority is -1. Can we do profiling and address the priority issue in a separate PR? It is hard to change something without having evidence that this change is needed (even if it is needed).
Then, inherit the sysworkq priority here instead of hard coding to -1.
It may be sensible to make our implementation impervious against the kind of DoS you describe (future work). But it will have to be something robust, and it must not rely on thread priorities.
Then, make this user configurable (and, making it inherit system work queue prior), this thread does not need to rely on priority values.
|
|
@alwa-nordic since this missed the feature freeze, it doesn't really make sense to have any references to Zephyr 4.3 anymore. This would have needed to get merged last Friday to make it. |
Ok, then I'll remove the |



Move tx_processor off the system work queue to its own thread. Solves deadlock where
bt_hci_cmd_alloc()blocks the system work queue.