bluetooth: buf: Add a callback for freed buffer in rx pool

[PavelVPV]

The Bluetooth data buffer API currently lacks a mechanism to notify when
a buffer is freed in the RX pool. This limitation forces HCI drivers to
adopt inefficient workarounds to manage buffer allocation.

HCI drivers face two suboptimal options:

• Blocking calls: Use bt_buf_get_rx with K_FOREVER, which blocks the
  execution context until a buffer becomes available.
• Polling: Repeatedly call bt_buf_get_rx with K_NO_WAIT, which increases
  CPU load and reduces efficiency.

This commit introduces a callback mechanism that is triggered each time
a buffer is freed in the RX pool. With this feature, HCI drivers can:

• Call bt_buf_get_rx with K_NO_WAIT.
• Wait for the callback notification if a NULL buffer is returned,
  avoiding unnecessary polling.

The new callback improves efficiency by enabling event-driven behavior
for buffer management, reducing CPU overhead while maintaining
responsiveness.

[alwa-nordic]

We have gotten feedback about lack of thread safety in the Host. I think we should make sure all code we add is thread safe, or have the API documentation say exactly what to synchronize on to safely call the API, since we have committed to this.

I have sketched some (compile time tested) changes at alwa-nordic@03b0d39.

In the sketch, I chose to make it thread safe. I also considered that an application may collide with the driver and both will call the cb_set function. To prevent "silent corruption" in that case, I changed it from a single cb_set to a cb_register and cb_unregister.

Apart from this, I removed the dependency injection from buf.c to iso.c and instead make iso.c call buf.c directly. I think this simplifies the code (at the cost of adding a circular dependency).

[alwa-nordic]

I think we can assign single-bit values to enum bt_buf_type, and then we wouldn't need enum bt_buf_type_bit.

[rugeGerritsen]

We have gotten feedback about lack of thread safety in the Host. I think we should make sure all code we add is thread safe, or have the API documentation say exactly what to synchronize on to safely call the API, since we have committed to this.

I have sketched some (compile time tested) changes at alwa-nordic@03b0d39.

In the sketch, I chose to make it thread safe. I also considered that an application may collide with the driver and both will call the cb_set function. To prevent "silent corruption" in that case, I changed it from a single cb_set to a cb_register and cb_unregister.

Apart from this, I removed the dependency injection from buf.c to iso.c and instead make iso.c call buf.c directly. I think this simplifies the code (at the cost of adding a circular dependency).

Can you elaborate why we want bt_buf_rx_freed_cb_set() to be thread safe? AFAIK, the RX buffers are only used when passed from controller to host, so the application should not call this API.

[alwa-nordic]

Can you elaborate why we want bt_buf_rx_freed_cb_set() to be thread safe? AFAIK, the RX buffers are only used when passed from controller to host, so the application should not call this API.

Thread safe is the default, as stated in documentation. The alternative is to document the API's thread safety aspects. It may be right that this API does not have to be thread safe, I'm not arguing for or against this right now, but then the documentation of this API has to do some heavy lifting.

[alwa-nordic]

Food for thought: buf_rx_freed_notify as implemented is invoked from net_buf destroy callbacks. Those come from potentially arbitrary threads.

bt_buf_rx_freed_cb_t may have to be designated thread safe in case multiple threads call destroy concurrently.

Without synchronization with buf_rx_freed_notify, bt_buf_rx_freed_cb_set is safe to call only when the caller can guarantee a concurrent buf_rx_freed_notify is impossible. At what times can the caller infer this guarantee?

[PavelVPV]

We have gotten feedback about lack of thread safety in the Host. I think we should make sure all code we add is thread safe, or have the API documentation say exactly what to synchronize on to safely call the API, since we have committed to this.

Thread safe is the default, as stated in documentation. The alternative is to document the API's thread safety aspects. It may be right that this API does not have to be thread safe, I'm not arguing for or against this right now, but then the documentation of this API has to do some heavy lifting.

If you can come up with the scenario where this function is called from multiple contexts then I'll make it thread safe. But from my understanding there is only a single user of this API. More other, only 1 callback can be set, so calling this function from different contexts means that there is misunderstanding of the purpose of this API, which in my understanding should be called once at the initialization phase. Therefore, I'd rather update the API description.

[PavelVPV]

Food for thought: buf_rx_freed_notify as implemented is invoked from net_buf destroy callbacks. Those come from potentially arbitrary threads.

bt_buf_rx_freed_cb_t may have to be designated thread safe in case multiple threads call destroy concurrently.

Without synchronization with buf_rx_freed_notify, bt_buf_rx_freed_cb_set is safe to call only when > the caller can guarantee a concurrent buf_rx_freed_notify is impossible. At what times can the caller infer this guarantee?

With this I fully agree. I'll make the callback thread safe.

[PavelVPV]

Apart from this, I removed the dependency injection from buf.c to iso.c and instead make iso.c call buf.c directly. I think this simplifies the code (at the cost of adding a circular dependency).

I think I disagree with this suggestion precisely because it creates dependency of iso.c on buf.c. The dependency injection from buf.c to iso.c already exists.

[alwa-nordic]

If you can come up with the scenario where this function is called from multiple contexts then I'll make it thread safe. But from my understanding there is only a single user of this API. More other, only 1 callback can be set, so calling this function from different contexts means that there is misunderstanding of the purpose of this API, which in my understanding should be called once at the initialization phase. Therefore, I'd rather update the API description.

I don't have any scenario in mind. I am in agreement with you if we update the API documentation so the user is informed about when it's safe to call the API, and when it's safe to free the callback.

I would like any assumptions we make about our own code to be documented and treated like internal API. Let's see what assumptions we need to make, if any. We could put it in code comments or create an architecture design document.

I'll make the callback thread safe.

It's a good idea to document this requirement on bt_buf_rx_freed_cb_t.

[alwa-nordic]

This should not test for equality, but just the expected bit. Extra bits should not fail this test, right? Like for hci_raw that uses the same buffer pool for all pool types.

[alwa-nordic]

Assuming the above is done, exp_type_mask in the test vector is over-fitting to the implementation. We should get rid of exp_type_mask and instead just check that type bit is set in the callback-provided value.

[PavelVPV]

I'd rather add a dedicated test for hci_raw that checks that it passes all 3 types. Then we know exactly what each variant returns.

[alwa-nordic]

It seems we want to test different things.

I want a test which accepts any implementation that follows the API specification. You want a test that fully specifies the behavior of the implementation with no wiggle room.

Both types of tests are useful. Your type is useful for enforcing strict refactoring without behavior change. My type is useful for testing multiple implementations or a re-implementation that takes advantage of the wiggle room in the specification.

Maybe we should have both tests? One test for the API, and one for each fully-specified implementation.

[PavelVPV]

Then I suggest a different test where this wiggle room is not needed: 5f1f5d9

[alwa-nordic]

Should we add extra_configs here as well to make sure the config is as we expect, no matter what the default is?

[PavelVPV]

Right, though if we check the exact returned value, this may not be needed. I added CONFIG_BT_HCI_ACL_FLOW_CONTROL=y any way.

[jhedberg]

@PavelVPV there's a conflict that needs resolving - please rebase.

[PavelVPV]

@PavelVPV there's a conflict that needs resolving - please rebase.

Done

[LuoZhongYao]

Because enum bt_buf_type is now a bitmap, and struct bt_buf_data.type is uint8_t, and in the foreseeable future, bt_buf_type will increase, should we consider changing struct bt_buf_data.type to uint32_t?