Bluetooth: Host: ACL/ISO buffers and bt_conn_tx objects mismatch #89258
Conversation
PavelVPV
force-pushed
the
acl_iso_pkts_and_bt_conn_tx_misalignment
branch
3 times, most recently
from
6285e8e to
3024c4b
Compare
PavelVPV
changed the title
PavelVPV
changed the title
github-actions
bot
added
Release Notes
github-actions
bot
requested review from
alwa-nordic,
cvinayak,
danieldegrasse,
dkalowsk,
gzh-terry,
hermabe,
jhedberg,
kartben,
lylezhu2012,
makeshi,
MarkWangChinese,
rugeGerritsen,
sjanc,
Thalley and
theob-pro
| @@ -140,8 +140,8 @@ config BT_RFCOMM_L2CAP_MTU | |||
|
|
|||
| config BT_RFCOMM_TX_MAX | |||
| int "Maximum number of pending TX buffers for RFCOMM" | |||
| default BT_CONN_TX_MAX | |||
| range BT_CONN_TX_MAX $(UINT8_MAX) | |||
| default BT_BUF_ACL_TX_COUNT | |||
There was a problem hiding this comment.
@lylezhu2012 , I see that the usage of CONFIG_BT_RFCOMM_TX_MAX has been removed in 17e2564 but it is used as a default value still for BT_HFP_AG_TX_BUF_COUNT. Not sure if you are aware. Just wanted to point out this.
There was a problem hiding this comment.
Looks fine, although in the long run I think it'd be better to reduce (and ideally completely remove) dependencies from the controller upward in the stack. The bt_buf_*() APIs and CONFIG_BT_BUF_ACL_TX_COUNT are kind of such a dependency - I know they can be considered a "shared" or "common" for the host and controller, but the controller should IMO have its own configuration options which the host-side can synchronize its options to when the host is using a local controller.
@jhedberg This was originally the case until 6483e12, but now the Zephyr Controller does inherit Do you think we still need Host and Controller seperate Kconfig, and they inherit values (hide one or the other in case of combined build) ? |
| @@ -329,7 +329,7 @@ config BT_CONN_FRAG_COUNT | |||
| if BT_CONN | |||
|
|
|||
| config BT_CONN_TX_MAX | |||
| int "Maximum number of pending TX buffers with a callback" | |||
| int "Maximum number of pending TX buffers with a callback [DEPRECATED]" | |||
There was a problem hiding this comment.
Shouldn't deprecated KConfig select DEPRECATED option?
Like here: https://github.com/zephyrproject-rtos/zephyr/blob/main/subsys/bluetooth/mesh/Kconfig#L1492-L1494
There was a problem hiding this comment.
This doesn't work for numeric Kconfigs.
What I'd like to move towards is to make the HCI driver API clean in the sense that its implementors shouldn't know or care who its users are (the user could be the Zephyr host, HCI raw, or something completely different). Right now we have |
PavelVPV
force-pushed
the
acl_iso_pkts_and_bt_conn_tx_misalignment
branch
from
3024c4b to
c2d155f
Compare
After zephyrproject-rtos#72090, each packet to be sent (wether ACL or ISO data) has a corresponding `bt_conn_tx` object, regardless of whether a callback is used. This means that number of packets Host can send to Controller is limited by the smaller of two values: ACL/ISO packets Controller can receive, and the number of `bt_conn_tx` objects allocated by Host. A mismatch between these numbers may lead to inefficient resource usage on either Host or Controller side. If Host allocates fewer `bt_conn_tx` objects than the number of buffers available on Controller for a given data type, some Controller buffers may go unused. Conversely, if Host allocates more `bt_conn_tx` objects than Controller can consume, the excess objects remain unused. This commit adds a check and issues a warning if the number of `bt_conn_tx` objects is not aligned with the number of ACL/ISO buffers reported by Controller via the LE Read Buffer Size v1 or v2 command. Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no>
After zephyrproject-rtos#72090, `conn_tx_alloc` no longer blocks, and each buffer always has a corresponding `bt_conn_tx` object. This eliminates the need to configure the number of `bt_conn_tx` objects via `CONFIG_BT_CONN_TX_MAX`, since every buffer now carries its own context even when no callback is used. This commit deprecates `CONFIG_BT_CONN_TX_MAX` as it is no longer necessary. Instead, `CONFIG_BT_BUF_ACL_TX_COUNT` is used to allocate `bt_conn_tx` objects for outgoing ACL data. ZLL already uses `CONFIG_BT_BUF_ACL_TX_COUNT` to configure the number of outgoing ACL packets. With this change, modifying the packet count will automatically adjust the number of corresponding contexts, preventing both context starvatoin and underutilization. This approach also aligns with ISO, where the number of `bt_conn_tx` objects for outgoing ISOdata matches `CONFIG_BT_ISO_TX_BUF_COUNT`. Signed-off-by: Pavel Vasilyev <pavel.vasilyev@nordicsemi.no>
PavelVPV
force-pushed
the
acl_iso_pkts_and_bt_conn_tx_misalignment
branch
from
c2d155f to
b53d7c8
Compare
|
|
@jhedberg, @alwa-nordic, please take a look at this PR one more time. |
This PR addresses an issue of mismatch between ACL/ISO buffers reported by Controller and
bt_conn_txobjects allocated by Host.After #72090, each packet to be sent (whether ACL or ISO data) has a corresponding
bt_conn_txobject, regardless of whether a callback is used.This means that:
bt_conn_txobjects allocated by Host.CONFIG_BT_CONN_TX_MAXis superseded byCONFIG_BT_BUF_ACL_TX_COUNTbecause Host starts using a freebt_conn_txobject only after checking that Controller can receive more ACL or ISO data over a connection.The issue reported in #72017 is valid and results in inefficient resource usage. The points mentioned above mean that the solution suggested in #72017 is outdated after #72090. Using separate FIFOs for ACL and ISO data will only hide the problem of resource utilization.
This PR does 2 things:
CONFIG_BT_CONN_TX_MAXand replaces it withCONFIG_BT_BUF_ACL_TX_COUNTforbt_conn_txobjects allocation for ACL tx buffers.bt_conn_txobjects mismatches with ACL/ISO buffers reported by Controller and thus need to be changed for better resources utilization.Fixes #72017.