Bluetooth host hci_rx_thread can replace by k_work_handler ?

[LingaoM]

In the current zephyr bluetooth host implementation, The meaning of this thread is to solve the potential deadlock problem, so it must exist.

But I think we can use the system worker thread to handle the same work, so as to save the resources of creating a thread.

[carlescufi]

@jhedberg @alwa-nordic

[jhedberg]

Yes, theoretically we could create a new k_fifo (e.g. bt_dev.rx_fifo) and have bt_recv() push buffers to it if BT_RECV_IS_RX_THREAD is not set, then have a system workqueue callback get buffers from this FIFO and process them. The main challenge with doing this change is that it creates potential new deadlocks with code that was assuming that it's safe to perform some short-lived blocking operations in the system workqueue. E.g. the host stack is already using the system workqueue for some things, so we might even have such an internal dependency already now.

[LingaoM]

@jhedberg

Yes, theoretically we could create a new k_fifo (e.g. bt_dev.rx_fifo) and have bt_recv() push buffers to it if BT_RECV_IS_RX_THREAD is not set, then have a system workqueue callback get buffers from this FIFO and process them. The main challenge with doing this change is that it creates potential new deadlocks with code that was assuming that it's safe to perform some short-lived blocking operations in the system workqueue. E.g. the host stack is already using the system workqueue for some things, so we might even have such an internal dependency already now.

I suggest being BT_RECV_IS_RX_THREAD is set to k_fifo, if this macro is not set. We need to replace the existing hci_rx_thread?

[jhedberg]

I actually forgot that we already have a FIFO that can be used for this purpose: bt_dev.rx_queue, so no need to add a new one. What you'd then do is convert hci_rx_thread to a workqueue callback, and instead of waiting with K_FOREVER you'd do something like:

while ((buf = net_buf_get(bt_dev.rx_queue, K_NO_WAIT)) {
        ...
}

[jhedberg]

And then after net_buf_put(buf, bt_dev.rx_queue) in bt_recv() you'd add a call to k_work_submit(&rx_work);

[LingaoM]

My idea is to modify the macro when it is turned on, but your idea is more thorough. When better testing is needed, because the macro is always not turned on.

[LingaoM]

It does work, but it is not normal. It may be necessary to find all the deadlock problems before this feature.

[jhedberg]

@LingaoM feel free to experiment with such a solution if you want, but be sure to test as many use cases as possible (connectionless, multiple connections, mesh, etc) to reduce the risk that there are new hidden deadlocks somewhere.

[jhedberg]

One thing to note, that with the elimination of a dedicated RX thread the BT_RX_STACK_SIZE becomes useless.

[LingaoM]

Sure