From 91ed10f1ace48c0d1b02684ab99650e5932ea4fc Mon Sep 17 00:00:00 2001 From: Pavel Vasilyev Date: Fri, 22 Nov 2024 08:57:33 +0100 Subject: [PATCH 1/3] bluetooth: buf: Convert bt_buf_type enum to bitmask This allows to combine several types in a single value. Signed-off-by: Pavel Vasilyev --- include/zephyr/bluetooth/buf.h | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/include/zephyr/bluetooth/buf.h b/include/zephyr/bluetooth/buf.h index c5ad4ddf9a8f..8109950d70eb 100644 --- a/include/zephyr/bluetooth/buf.h +++ b/include/zephyr/bluetooth/buf.h @@ -28,22 +28,22 @@ extern "C" { #endif -/** Possible types of buffers passed around the Bluetooth stack */ +/** Possible types of buffers passed around the Bluetooth stack in a form of bitmask. */ enum bt_buf_type { /** HCI command */ - BT_BUF_CMD, + BT_BUF_CMD = BIT(0), /** HCI event */ - BT_BUF_EVT, + BT_BUF_EVT = BIT(1), /** Outgoing ACL data */ - BT_BUF_ACL_OUT, + BT_BUF_ACL_OUT = BIT(2), /** Incoming ACL data */ - BT_BUF_ACL_IN, + BT_BUF_ACL_IN = BIT(3), /** Outgoing ISO data */ - BT_BUF_ISO_OUT, + BT_BUF_ISO_OUT = BIT(4), /** Incoming ISO data */ - BT_BUF_ISO_IN, + BT_BUF_ISO_IN = BIT(5), /** H:4 data */ - BT_BUF_H4, + BT_BUF_H4 = BIT(6), }; /** @brief This is a base type for bt_buf user data. */ From b7ed6d30c700f26fa230c677aecb54cd1d927ce8 Mon Sep 17 00:00:00 2001 From: Pavel Vasilyev Date: Thu, 7 Nov 2024 09:50:37 +0100 Subject: [PATCH 2/3] bluetooth: buf: Add a callback for freed buffer in rx pool 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. Signed-off-by: Pavel Vasilyev --- include/zephyr/bluetooth/buf.h | 21 ++++++++ subsys/bluetooth/host/buf.c | 71 ++++++++++++++++++++++++---- subsys/bluetooth/host/hci_raw.c | 21 +++++++- subsys/bluetooth/host/iso.c | 19 +++++++- subsys/bluetooth/host/iso_internal.h | 10 ++++ 5 files changed, 130 insertions(+), 12 deletions(-) diff --git a/include/zephyr/bluetooth/buf.h b/include/zephyr/bluetooth/buf.h index 8109950d70eb..dd452342ef72 100644 --- a/include/zephyr/bluetooth/buf.h +++ b/include/zephyr/bluetooth/buf.h @@ -138,6 +138,27 @@ BUILD_ASSERT(BT_BUF_ACL_RX_COUNT <= BT_BUF_ACL_RX_COUNT_MAX, */ struct net_buf *bt_buf_get_rx(enum bt_buf_type type, k_timeout_t timeout); +/** A callback to notify about freed buffer in the incoming data pool. + * + * This callback is called when a buffer of a given type is freed and can be requested through the + * @ref bt_buf_get_rx function. However, this callback is called from the context of the buffer + * freeing operation and must not attempt to allocate a new buffer from the same pool. + * + * @warning When this callback is called, the scheduler is locked and the callee must not perform + * any action that makes the current thread unready. This callback must only be used for very + * short non-blocking operation (e.g. submitting a work item). + * + * @param type_mask A bit mask of buffer types that have been freed. + */ +typedef void (*bt_buf_rx_freed_cb_t)(enum bt_buf_type type_mask); + +/** Set the callback to notify about freed buffer in the incoming data pool. + * + * @param cb Callback to notify about freed buffer in the incoming data pool. If NULL, the callback + * is disabled. + */ +void bt_buf_rx_freed_cb_set(bt_buf_rx_freed_cb_t cb); + /** Allocate a buffer for outgoing data * * This will set the buffer type so bt_buf_set_type() does not need to diff --git a/subsys/bluetooth/host/buf.c b/subsys/bluetooth/host/buf.c index b7ed19303be2..75bd15834ae9 100644 --- a/subsys/bluetooth/host/buf.c +++ b/subsys/bluetooth/host/buf.c @@ -25,6 +25,26 @@ LOG_MODULE_REGISTER(bt_buf, CONFIG_BT_LOG_LEVEL); */ #define SYNC_EVT_SIZE (BT_BUF_RESERVE + BT_HCI_EVT_HDR_SIZE + 255) +static bt_buf_rx_freed_cb_t buf_rx_freed_cb; + +static void buf_rx_freed_notify(enum bt_buf_type mask) +{ + k_sched_lock(); + + if (buf_rx_freed_cb) { + buf_rx_freed_cb(mask); + } + + k_sched_unlock(); +} + +#if defined(CONFIG_BT_ISO_RX) +static void iso_rx_freed_cb(void) +{ + buf_rx_freed_notify(BT_BUF_ISO_IN); +} +#endif + /* Pool for RX HCI buffers that are always freed by `bt_recv` * before it returns. * @@ -40,17 +60,37 @@ NET_BUF_POOL_FIXED_DEFINE(discardable_pool, CONFIG_BT_BUF_EVT_DISCARDABLE_COUNT, sizeof(struct bt_buf_data), NULL); #if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL) -NET_BUF_POOL_DEFINE(acl_in_pool, BT_BUF_ACL_RX_COUNT, - BT_BUF_ACL_SIZE(CONFIG_BT_BUF_ACL_RX_SIZE), - sizeof(struct acl_data), bt_hci_host_num_completed_packets); +static void acl_in_pool_destroy(struct net_buf *buf) +{ + bt_hci_host_num_completed_packets(buf); + buf_rx_freed_notify(BT_BUF_ACL_IN); +} -NET_BUF_POOL_FIXED_DEFINE(evt_pool, CONFIG_BT_BUF_EVT_RX_COUNT, - BT_BUF_EVT_RX_SIZE, sizeof(struct bt_buf_data), - NULL); +static void evt_pool_destroy(struct net_buf *buf) +{ + net_buf_destroy(buf); + buf_rx_freed_notify(BT_BUF_EVT); +} + +NET_BUF_POOL_DEFINE(acl_in_pool, BT_BUF_ACL_RX_COUNT, BT_BUF_ACL_SIZE(CONFIG_BT_BUF_ACL_RX_SIZE), + sizeof(struct acl_data), acl_in_pool_destroy); + +NET_BUF_POOL_FIXED_DEFINE(evt_pool, CONFIG_BT_BUF_EVT_RX_COUNT, BT_BUF_EVT_RX_SIZE, + sizeof(struct bt_buf_data), evt_pool_destroy); #else -NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT, - BT_BUF_RX_SIZE, sizeof(struct acl_data), - NULL); +static void hci_rx_pool_destroy(struct net_buf *buf) +{ + net_buf_destroy(buf); + + /* When ACL Flow Control is disabled, a single pool is used for events and acl data. + * Therefore the callback will always notify about both types of buffers, BT_BUF_EVT and + * BT_BUF_ACL_IN. + */ + buf_rx_freed_notify(BT_BUF_EVT | BT_BUF_ACL_IN); +} + +NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT, BT_BUF_RX_SIZE, sizeof(struct acl_data), + hci_rx_pool_destroy); #endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */ struct net_buf *bt_buf_get_rx(enum bt_buf_type type, k_timeout_t timeout) @@ -82,6 +122,19 @@ struct net_buf *bt_buf_get_rx(enum bt_buf_type type, k_timeout_t timeout) return buf; } +void bt_buf_rx_freed_cb_set(bt_buf_rx_freed_cb_t cb) +{ + k_sched_lock(); + + buf_rx_freed_cb = cb; + +#if defined(CONFIG_BT_ISO_RX) + bt_iso_buf_rx_freed_cb_set(cb != NULL ? iso_rx_freed_cb : NULL); +#endif + + k_sched_unlock(); +} + struct net_buf *bt_buf_get_evt(uint8_t evt, bool discardable, k_timeout_t timeout) { diff --git a/subsys/bluetooth/host/hci_raw.c b/subsys/bluetooth/host/hci_raw.c index 037a301458f8..1580fea5ba55 100644 --- a/subsys/bluetooth/host/hci_raw.c +++ b/subsys/bluetooth/host/hci_raw.c @@ -35,8 +35,20 @@ static uint8_t raw_mode = BT_HCI_RAW_MODE_H4; static uint8_t raw_mode; #endif -NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT, - BT_BUF_RX_SIZE, sizeof(struct bt_buf_data), NULL); +static bt_buf_rx_freed_cb_t buf_rx_freed_cb; + +static void hci_rx_buf_destroy(struct net_buf *buf) +{ + net_buf_destroy(buf); + + if (buf_rx_freed_cb) { + /* bt_buf_get_rx is used for all types of RX buffers */ + buf_rx_freed_cb(BT_BUF_EVT | BT_BUF_ACL_IN | BT_BUF_ISO_IN); + } +} + +NET_BUF_POOL_FIXED_DEFINE(hci_rx_pool, BT_BUF_RX_COUNT, BT_BUF_RX_SIZE, sizeof(struct bt_buf_data), + hci_rx_buf_destroy); NET_BUF_POOL_FIXED_DEFINE(hci_cmd_pool, CONFIG_BT_BUF_CMD_TX_COUNT, BT_BUF_CMD_SIZE(CONFIG_BT_BUF_CMD_TX_SIZE), sizeof(struct bt_buf_data), NULL); @@ -93,6 +105,11 @@ struct net_buf *bt_buf_get_rx(enum bt_buf_type type, k_timeout_t timeout) return buf; } +void bt_buf_rx_freed_cb_set(bt_buf_rx_freed_cb_t cb) +{ + buf_rx_freed_cb = cb; +} + struct net_buf *bt_buf_get_tx(enum bt_buf_type type, k_timeout_t timeout, const void *data, size_t size) { diff --git a/subsys/bluetooth/host/iso.c b/subsys/bluetooth/host/iso.c index f44acbcc522d..28674f7e5b9b 100644 --- a/subsys/bluetooth/host/iso.c +++ b/subsys/bluetooth/host/iso.c @@ -51,8 +51,20 @@ LOG_MODULE_REGISTER(bt_iso, CONFIG_BT_ISO_LOG_LEVEL); #define iso_chan(_iso) ((_iso)->iso.chan); #if defined(CONFIG_BT_ISO_RX) +static bt_iso_buf_rx_freed_cb_t buf_rx_freed_cb; + +static void iso_rx_buf_destroy(struct net_buf *buf) +{ + net_buf_destroy(buf); + + if (buf_rx_freed_cb) { + buf_rx_freed_cb(); + } +} + NET_BUF_POOL_FIXED_DEFINE(iso_rx_pool, CONFIG_BT_ISO_RX_BUF_COUNT, - BT_ISO_SDU_BUF_SIZE(CONFIG_BT_ISO_RX_MTU), sizeof(struct iso_data), NULL); + BT_ISO_SDU_BUF_SIZE(CONFIG_BT_ISO_RX_MTU), sizeof(struct iso_data), + iso_rx_buf_destroy); static struct bt_iso_recv_info iso_info_data[CONFIG_BT_ISO_RX_BUF_COUNT]; #define iso_info(buf) (&iso_info_data[net_buf_id(buf)]) @@ -553,6 +565,11 @@ struct net_buf *bt_iso_get_rx(k_timeout_t timeout) return buf; } +void bt_iso_buf_rx_freed_cb_set(bt_iso_buf_rx_freed_cb_t cb) +{ + buf_rx_freed_cb = cb; +} + void bt_iso_recv(struct bt_conn *iso, struct net_buf *buf, uint8_t flags) { struct bt_hci_iso_sdu_hdr *hdr; diff --git a/subsys/bluetooth/host/iso_internal.h b/subsys/bluetooth/host/iso_internal.h index 404a09a92624..c01881bc3023 100644 --- a/subsys/bluetooth/host/iso_internal.h +++ b/subsys/bluetooth/host/iso_internal.h @@ -87,6 +87,16 @@ void hci_iso(struct net_buf *buf); /* Allocates RX buffer */ struct net_buf *bt_iso_get_rx(k_timeout_t timeout); +/** A callback used to notify about freed buffer in the iso rx pool. */ +typedef void (*bt_iso_buf_rx_freed_cb_t)(void); + +/** Set a callback to notify about freed buffer in the iso rx pool. + * + * @param cb Callback to notify about freed buffer in the iso rx pool. If NULL, the callback is + * disabled. + */ +void bt_iso_buf_rx_freed_cb_set(bt_iso_buf_rx_freed_cb_t cb); + /* Process CIS Established event */ void hci_le_cis_established(struct net_buf *buf); From 2e323420c356329c7d02b6145654a6b33cde9f43 Mon Sep 17 00:00:00 2001 From: Pavel Vasilyev Date: Tue, 19 Nov 2024 10:11:26 +0100 Subject: [PATCH 3/3] tests: bluetooth: buf: Test the freed buf callback This commit adds a unit test that checks the freed buffer callback of the bluetooth data buffer API. Signed-off-by: Pavel Vasilyev --- tests/bluetooth/buf/CMakeLists.txt | 8 ++++ tests/bluetooth/buf/prj.conf | 10 +++++ tests/bluetooth/buf/src/main.c | 72 ++++++++++++++++++++++++++++++ tests/bluetooth/buf/testcase.yaml | 30 +++++++++++++ 4 files changed, 120 insertions(+) create mode 100644 tests/bluetooth/buf/CMakeLists.txt create mode 100644 tests/bluetooth/buf/prj.conf create mode 100644 tests/bluetooth/buf/src/main.c create mode 100644 tests/bluetooth/buf/testcase.yaml diff --git a/tests/bluetooth/buf/CMakeLists.txt b/tests/bluetooth/buf/CMakeLists.txt new file mode 100644 index 000000000000..3da24b0b5512 --- /dev/null +++ b/tests/bluetooth/buf/CMakeLists.txt @@ -0,0 +1,8 @@ +# SPDX-License-Identifier: Apache-2.0 + +cmake_minimum_required(VERSION 3.20.0) + +find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE}) +project(buf) + +target_sources(app PRIVATE src/main.c) diff --git a/tests/bluetooth/buf/prj.conf b/tests/bluetooth/buf/prj.conf new file mode 100644 index 000000000000..697529ffb9d7 --- /dev/null +++ b/tests/bluetooth/buf/prj.conf @@ -0,0 +1,10 @@ +CONFIG_TEST=y +CONFIG_ZTEST=y + +CONFIG_BT=y +CONFIG_BT_CTLR=n +CONFIG_BT_H4=n + +# Needed to enable and test the iso rx pool +CONFIG_BT_OBSERVER=y +CONFIG_BT_ISO_SYNC_RECEIVER=y diff --git a/tests/bluetooth/buf/src/main.c b/tests/bluetooth/buf/src/main.c new file mode 100644 index 000000000000..36b57a0e3f91 --- /dev/null +++ b/tests/bluetooth/buf/src/main.c @@ -0,0 +1,72 @@ +/* + * Copyright (c) 2024 Nordic Semiconductor ASA + * + * SPDX-License-Identifier: Apache-2.0 + */ + +#include + +#include +#include +#include + +#include +#include +#include +#include +#include + +static enum bt_buf_type freed_buf_type; +static K_SEM_DEFINE(rx_sem, 0, 1); + +void bt_buf_rx_freed_cb(enum bt_buf_type type) +{ + freed_buf_type = type; + k_sem_give(&rx_sem); +} + +ZTEST_SUITE(test_buf_data_api, NULL, NULL, NULL, NULL, NULL); + +ZTEST(test_buf_data_api, test_buf_freed_cb) +{ + struct net_buf *buf; + int err; + + bt_buf_rx_freed_cb_set(bt_buf_rx_freed_cb); + + /* Test that the callback is called for the BT_BUF_EVT type */ + buf = bt_buf_get_rx(BT_BUF_EVT, K_NO_WAIT); + zassert_not_null(buf, "Failed to get event buffer"); + + net_buf_unref(buf); + /* The freed buf cb is called from net_buf_unref, therefore the semaphore should + * already by given. + */ + err = k_sem_take(&rx_sem, K_NO_WAIT); + zassert_equal(err, 0, "Timeout while waiting for event buffer to be freed"); + zassert_equal(BT_BUF_EVT, BT_BUF_EVT & freed_buf_type, "Event buffer wasn't freed"); + + /* Test that the callback is called for the BT_BUF_ACL_IN type */ + buf = bt_buf_get_rx(BT_BUF_ACL_IN, K_NO_WAIT); + zassert_not_null(buf, "Failed to get ACL buffer"); + + net_buf_unref(buf); + /* The freed buf cb is called from net_buf_unref, therefore the semaphore should + * already by given. + */ + err = k_sem_take(&rx_sem, K_NO_WAIT); + zassert_equal(err, 0, "Timeout while waiting for ACL buffer to be freed"); + zassert_equal(BT_BUF_ACL_IN, BT_BUF_ACL_IN & freed_buf_type, "ACL buffer wasn't freed"); + + /* Test that the callback is called for the BT_BUF_ISO_IN type */ + buf = bt_buf_get_rx(BT_BUF_ISO_IN, K_NO_WAIT); + zassert_not_null(buf, "Failed to get ISO buffer"); + + net_buf_unref(buf); + /* The freed buf cb is called from net_buf_unref, therefore the semaphore should + * already by given. + */ + err = k_sem_take(&rx_sem, K_NO_WAIT); + zassert_equal(err, 0, "Timeout while waiting for ISO buffer to be freed"); + zassert_equal(BT_BUF_ISO_IN, BT_BUF_ISO_IN & freed_buf_type, "ISO buffer wasn't freed"); +} diff --git a/tests/bluetooth/buf/testcase.yaml b/tests/bluetooth/buf/testcase.yaml new file mode 100644 index 000000000000..418d357d4fbd --- /dev/null +++ b/tests/bluetooth/buf/testcase.yaml @@ -0,0 +1,30 @@ +common: + tags: + - bluetooth + - host + +tests: + bluetooth.buf: + platform_allow: + - native_sim + - native_sim/native/64 + integration_platforms: + - native_sim + extra_configs: + - CONFIG_BT_HCI_ACL_FLOW_CONTROL=y + bluetooth.buf.no_acl_flow_control: + platform_allow: + - native_sim + - native_sim/native/64 + integration_platforms: + - native_sim + extra_configs: + - CONFIG_BT_HCI_ACL_FLOW_CONTROL=n + bluetooth.buf.hci_raw: + platform_allow: + - native_sim + - native_sim/native/64 + integration_platforms: + - native_sim + extra_configs: + - CONFIG_BT_HCI_RAW=y