Bluetooth: Host: Fixed where bt_send returns an error but is actually…

[LingaoM]

Enhancement

1. clear var sync make more safe.
2. add rsp to avoiding deep-copy. (related: Bluetooth: Host: Remove bt_buf_get_cmd_complete #68008)

[jhedberg]

@LingaoM while the change looks reasonable, you need to explain (in the commit message) in more detail the sequence of events which will trigger it. Is this purely theoretical, or you actually saw it in practice? If the latter, what kind of build configuration & HW did you have?

Btw, there's a merge conflict, so you need to rebase.

[LingaoM]

@LingaoM while the change looks reasonable, you need to explain (in the commit message) in more detail the sequence of events which will trigger it. Is this purely theoretical, or you actually saw it in practice?

In fact, it is not a problem with Zephyr itself. The problem occurred when we ran the Zephyr protocol stack on Linux and tested it. However, we believe that the zephyr protocol stack uses a local variable and should be cleared, which is safer :).

[LingaoM]

@alwa-nordic CC :).

[jori-nordic]

@alwa-nordic isn't that what you were trying to avoid with 1cb83a8 ?
Specifically allowing the application to ref() event buffers.

[LingaoM]

@alwa-nordic isn't that what you were trying to avoid with 1cb83a8 ? Specifically allowing the application to ref() event buffers.

#68008 (comment)

I think the deadlocking problems the host has can really only be solved a principled way: We must stop blocking the event stream from the controller. All these priority levels and extra pools and stuff are just band-aids without technical soundness.

I mean all events must be treated as ISR. And I mean the event buffer must be made available for the next event immediately without any dependencies. If an event handler must keep data around, it must have it's own buffers (it can copy data into or give to the driver for DMA), or it can choose to drop the data. The handler cannot delay the event stream.

I don't think my approach delays the event flow. If you look at the implementation of bt_recv, for the events of evt_flags & BT_HCI_EVT_FLAG_RECV, buf will still be occupied by bt_dev.rx_queue until the BT RX is processed, there is no difference between this method and the method I am using for this PR, and more I think this method is safe than BT RX process, because in most cases only the protocol stack will use the bt_hci_cmd_send_sync API, So we can completely ensure that there is no delayed code in all existing places that use this API and rsp is not empty, on the contrary, BT RX processing buffer events will have many callbacks involving the user layer, which will increase uncertainty.

[Thalley]

So what this PR really does (besides the renames), is that it replaced a copy with another pointer.

I'm not convinced that this is a better approach as it doesn't seem to solve any issues, but does increase our RAM usage for a small performance gain.

The RAM usage is easily measurable, but do we save anything meaningful when not doing the copy?

[Thalley]

Is cmd(buf)->rsp always non-NULL here?

[LingaoM]

Changed :)

[LingaoM]

I'm not convinced that this is a better approach as it doesn't seem to solve any issues, but does increase our RAM usage for a small performance gain.

The RAM usage is easily measurable, but do we save anything meaningful when not doing the copy?

@Thalley I don't think this change will increase ram too lot, if you see https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/bluetooth/host/hci_core.c#L121 , that the default CONFIG_BT_BUF_CMD_TX_COUNT only 2, so only 8-bytes ram increase on 32bits machine, after this change, can completely avoid double-copy design, this is valuable i think.

BTW: From coding style, use double-copy is not a good-idea, although not cause performance decrease, but it's odd.

[Thalley]

I'm not convinced that this is a better approach as it doesn't seem to solve any issues, but does increase our RAM usage for a small performance gain.
The RAM usage is easily measurable, but do we save anything meaningful when not doing the copy?

@Thalley I don't think this change will increase ram too lot, if you see https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/bluetooth/host/hci_core.c#L121 , that the default CONFIG_BT_BUF_CMD_TX_COUNT only 2, so only 8-bytes ram increase on 32bits machine, after this change, can completely avoid double-copy design, this is valuable i think.

BTW: From coding style, use double-copy is not a good-idea, although not cause performance decrease, but it's odd.

I tend to agree, but also consider that the size of events and responses are usually so small that it doesn't really matter either :)

Not opposed to the change, but still unsure whether it's overall better.

[LingaoM]

Not opposed to the change, but still unsure whether it's overall better.

That this true this PR not improve performance a lot , but for coding style become more concise indeed, at least i think. :)

[jori-nordic]

most cases only the protocol stack will use the bt_hci_cmd_send_sync API, So we can completely ensure that there is no delayed code in all existing places that use this API

Most cases did indeed involve the host making the deadlock by using send_sync() in the wrong place at the wrong time. With the current design that is very synchronous, it's hard to avoid deadlocks in general.

Anyway, I was left scratching my head at the previous logic (what does if (evt_buf != buf) even mean?), so your PR would be an improvement in readability for me.
I'll trigger our internal HW testing pipeline to double-check there are no issues.

[LingaoM]

@jori-nordic BabbleSim Tests PASSED.

[jhedberg]

what does if (evt_buf != buf) even mean?)

I think it's a sanity-check that the HCI driver used the appropriate buffer allocation method for the command complete event which should result in getting hold of the original command buffer, and if it didn't do that (used some generic allocator or even its own pool) then the code was trying to work around it.

[alwa-nordic]

@alwa-nordic isn't that what you were trying to avoid with 1cb83a8 ? Specifically allowing the application to ref() event buffers.

#68008 (comment)

I think the deadlocking problems the host has can really only be solved a principled way: We must stop blocking the event stream from the controller. All these priority levels and extra pools and stuff are just band-aids without technical soundness.

I mean all events must be treated as ISR. And I mean the event buffer must be made available for the next event immediately without any dependencies. If an event handler must keep data around, it must have it's own buffers (it can copy data into or give to the driver for DMA), or it can choose to drop the data. The handler cannot delay the event stream.

I don't think my approach delays the event flow. If you look at the implementation of bt_recv, for the events of evt_flags & BT_HCI_EVT_FLAG_RECV, buf will still be occupied by bt_dev.rx_queue until the BT RX is processed, there is no difference between this method and the method I am using for this PR, and more I think this method is safe than BT RX process, because in most cases only the protocol stack will use the bt_hci_cmd_send_sync API, So we can completely ensure that there is no delayed code in all existing places that use this API and rsp is not empty, on the contrary, BT RX processing buffer events will have many callbacks involving the user layer, which will increase uncertainty.

The significant change in this PR is that applications get a reference to the buffer the Command Complete event was received into.

This is orthogonal to the primary reason for 1cb83a8, to remove the assumption that a Command Complete event is a response to the previously sent command.

More importantly, there exists a separate issue that blocking the HCI event stream makes it impossibly complicated to guarantee no deadlocks form. Giving the application a reference to a event buffer is a hazard in this respect. The hazard is equivalent to invoking an application callback from bt_recv_prio.

To remain safe, the application must give the buffer back before it can expect any synchronizing with the Bluetooth Host to complete, since the Host may potentially be blocked by the application. In terms if a callback, we would just say that the callback should be ISR-safe.

Stalling due to a held reference is very non-intuitive for our users. It's even less intuitive than stalling due to control held in a callback, which is already a confusing topic. (Aside: The obvious version would be a event loop, and the application not getting any events when the application is not polling for events because it's handling the previous event.)

Due to the hazard outlined above, I am against allowing the application to get a reference to a stack-internal buffer in the common case. I would ok adding a second 'expert API' for those who got to go fast.

Then there is also the question of benchmarking this. Do you have any numbers from experiments that show a gain in speed or a real reduction in power use? I fear we are doing premature optimization.

[LingaoM]

I don't think so, generally this API bt_hci_cmd_send_sync only call by host stack, not by application user. Even this api is public, but only in some situation where the user maybe call this function with vendor command. But most of this API only call by host stack, which code belongs ours maintained, we can ensure that.

[LingaoM]

@alwa-nordic Here we actually borrow the buffer of cmd to carry the data of rsp, but there is a premise here that the length of cmd must be greater than the length of rsp. Therefore, the current code implementation is actually the maximum value of the two areas. https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/bluetooth/host/hci_core.c#L162 .After my PR, this constraint can actually be avoided.

[LingaoM]

I don't think my approach delays the event flow. If you look at the implementation of bt_recv, for the events of evt_flags & BT_HCI_EVT_FLAG_RECV, buf will still be occupied by bt_dev.rx_queue until the BT RX is processed, there is no difference between this method and the method I am using for this PR, and more I think this method is safe than BT RX process, because in most cases only the protocol stack will use the bt_hci_cmd_send_sync API, So we can completely ensure that there is no delayed code in all existing places that use this API and rsp is not empty, on the contrary, BT RX processing buffer events will have many callbacks involving the user layer, which will increase uncertainty.

More importantly, there exists a separate issue that blocking the HCI event stream makes it impossibly complicated to guarantee no deadlocks form. Giving the application a reference to a event buffer is a hazard in this respect. The hazard is equivalent to invoking an application callback from bt_recv_prio.

To remain safe, the application must give the buffer back before it can expect any synchronizing with the Bluetooth Host to complete, since the Host may potentially be blocked by the application. In terms if a callback, we would just say that the callback should be ISR-safe.

Like all command completed events, the completion event for
BT_HCI_OP_HOST_NUM_COMPLETED_PACKETS is now placed in normal event
buffers.

Summarize:

1. After 1cb83a8, there are no different between bt_recv_prio vs bt_recv, both of them use same normal net_buf pool.
2. If you look at the implementation of bt_recv, for the events of evt_flags & BT_HCI_EVT_FLAG_RECV, buf will still be occupied by bt_dev.rx_queue until the BT RX is processed, which use same net_buf pool.
3. In most cases only the protocol stack will use the bt_hci_cmd_send_sync API, so we can completely ensure that there is no delayed code in all existing places that use this bt_hci_cmd_send_sync API with rsp is not empty.

BTW:

Stalling due to a held reference is very non-intuitive for our users. It's even less intuitive than stalling due to control held in a callback, which is already a confusing topic. (Aside: The obvious version would be a event loop, and the application not getting any events when the application is not polling for events because it's handling the previous event.)

Due to the hazard outlined above, I am against allowing the application to get a reference to a stack-internal buffer in the common case. I would ok adding a second 'expert API' for those who got to go fast.

I checked all the places where rsp is used in the host stack. There are 38 places in total, and there is no block in any place.

[alwa-nordic]

1. After 1cb83a8, there are no different between bt_recv_prio vs bt_recv, both of them use same normal net_buf pool.

2. If you look at the implementation of bt_recv, for the events of evt_flags & BT_HCI_EVT_FLAG_RECV, buf will still be occupied by bt_dev.rx_queue until the BT RX is processed, which use same net_buf pool.

From my perspective, you have identified a defect here. The Command Complete events can and should go in sync_evt_pool.

[LingaoM]

From my perspective, you have identified a defect here. The Command Complete events can and should go in sync_evt_pool.

#74645