bluetooth: Behavior change in host causing MESH to fail on sending messages

[m-alperen-sener]

Describe the bug

The changes introduced with this PR and additional PR to avoid deadlocks by overriding K_FOREVER with K_NO_WAIT in buffer allocation requested by syswq. Thus, this changes introduces a big behavior change for the layers that uses host and syswq. For MESH stack it causes to loose mesh messages send by using syswq.

Those changes propagates the failures caused by host and controller interaction mechanism to the upper layers and for the app side it does not make so much sense but confusion. That changes necessitates changes that should handles this particular corner case by every bt_host user.

The possible mitigations:

• Increasing the resources; like increasing the count of the HCI cmd buffers however it is likely to introduce scaling issues for upper layers end application.
• Another solution seems to eliminate the usage of the sysqw in MESH stack, it would eliminate the failure for most part of the mesh however still the end application is susceptible to the same mistake by calling certain MESH APIs from sysqw and failing directly, the same way we got failures from the host. Thus, this option does not seems like a complete solution.

Alongside this bug in MESH, the behavioral change is not visible enough for bt_host users in documentation.

[m-alperen-sener]

@PavelVPV and @jori-nordic can you make a comment on that issue.

[jori-nordic]

I had added this to the documentation. Since it's a net change, not only for the host:

zephyr/include/zephyr/net/buf.h

Lines 1333 to 1335 in ad3e941

	* @note The timeout value will be overridden to K_NO_WAIT if called from the
	* system workqueue.
	*

I agree, it's not super visible. But such are a lot of upstream changes, that's why we have tests.
The host is very synchronous, and as such very prone to deadlocks. Additionally, there are a lot of things that run on the syswq now: application items, various subsystems, and even some GPIO drivers. Blocking this "central" scheduler is not nice for the other users. That's the rationale behind those two PRs.

Since we can't just malloc and spawn worker threads like on a linux machine, we have to change the host API to be non-blocking and allow operations to fail. That's how it is on (multi-user) embedded systems.

Unfortunately, it does push the problems upwards, like you've seen. The two mitigations you highlight are good:

1. Is not scalable, as you point out.
2. Won't work without additional API changes: I would suggest making changes to MESH API to be more asynchronous too, and only allocate and queue controller commands from a separate thread. Sending from syswq should be ok, allocating is more dicey. Application should also handle not being able to send a message at any given time.
3. If you have memory, you can try a simple trick: reschedule your work item when you get a failure. Chances are, when you are scheduled again you will get the resources to send.
4. Since the host manages the buffer pools, we need a new API, that signals other users that an allocation is likely to succeed. We don't have that right now. I made Bluetooth: API to signal host-managed buffers are freed #77249 to track this. Feel free to comment on it too.

An additional note: it has taken 3+ months for us to detect this issue. It would be good to have whatever tests that detected the issue run on the latest upstream. I would even suggest on any PR tagged with "Bluetooth" or at least "Bluetooth host".

[alxelax]

Shouldn't such kind of changes pass Architecture review in Bluetooth group first to consider impact on all existing subsystems within Zephyr? at least related to Bluetooth.
It is quite well that Host improved behavior. However, it made any Mesh communication unreliable, even specification requires opposite behavior. It even cannot pass qualification tests with default configuration anymore.
Seems to consider new Host changes Mesh requires architecture redesign(or at least detailed review) all places those use system work queue for sending messages.
Until above, Mesh, it seems, stopped being reliable in Zephyr. Now, every single configuration needs to be tuned to be sure that Host has sufficient resources to handle all tx requests.

[jori-nordic]

Shouldn't such kind of changes pass Architecture review in Bluetooth group first

It did, I talked a whole bunch about the new arch in the public workgroup meetings, even had diagrams and stuff.
There was some amount of discussion on the PR too. No-one objected in the end.

even specification requires opposite behavior

What do you mean, the spec has a say on the implementation details?

stopped being reliable in Zephyr

We can argue it never was. As a badly-tuned config + alloc at the wrong time/place could end up deadlocking the whole device. This is what we're trying to move away from, this kind of "works most of the time" behavior.