Skip to content

bluetooth: host: Split cmd complete & cmd status to separate pool #74645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

bluetooth: host: Split cmd complete & cmd status to separate pool #74645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
81c4841
bluetooth: host: Split cmd complete & cmd status to separate pool
LingaoM Jun 21, 2024
5ceeb2b
bluetooth: host: remove unused ztest
LingaoM Jun 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions subsys/bluetooth/common/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,20 +94,38 @@ config BT_BUF_ACL_RX_COUNT
When Controller to Host flow control is not enabled the Controller
can assume that the Host has infinite amount of buffers.

config BT_BUF_CMD_COMPLETE_EVT_SIZE
int "Maximum supported HCI Command Complete Event buffer length"
# LE Read Supported Commands command complete event.
default 255 if !BT_CTLR
default 68
range 68 255
help
Maximum supported HCI Command Complete event buffer size.
This value does not include the HCI Event header. This value is
used by both the Host and the Controller for buffer sizes that
include HCI Command Complete events. It should be set according
to the expected HCI Command Complete events that can be generated
from the configuration.

If the subset of possible HCI Command Complete events is unknown or
if you use a split build (ie hci on another chip), then you better use 255,

config BT_BUF_EVT_RX_SIZE
int "Maximum supported HCI Event buffer length"
default 255 if (BT_EXT_ADV && BT_OBSERVER) || BT_PER_ADV_SYNC || BT_DF_CONNECTION_CTE_RX || BT_CLASSIC
# LE Read Supported Commands command complete event.
default 68
range 68 255
default BT_BUF_CMD_COMPLETE_EVT_SIZE
range BT_BUF_CMD_COMPLETE_EVT_SIZE 255
Comment on lines +117 to +118
Copy link
Contributor

@Thalley Thalley Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the range and default value here due to the fallback mechanism you have defined?

If we did not fallback to the event buffers in case of error, could this be reduced? Do we have any events larger than ~32 without ext_adv?

Copy link
Contributor Author

@LingaoM LingaoM Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have also thought of a similar approach, but considering that almost all configurations now use extended advertising as a default option, the value of this optimization is not high.

Copy link
Contributor Author

@LingaoM LingaoM Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current design will add about 140 bytes of RAM overhead, but it ensures relative safety, so I think it is worth it and change this way will not be noticeable to users.

help
Maximum supported HCI event buffer size. This value does not include
the HCI Event header.
This value is used by both the Host and the Controller for buffer
sizes that include HCI events. It should be set according to the
expected HCI events that can be generated from the configuration.
If the subset of possible HCI events is unknown, this should be set to
the maximum of 255.
This value is used by both the Host and the Controller for buffer sizes
that include HCI events also include HCI Command Complete event. It should
be set according to the expected HCI events that can be generated from the
configuration.

If the subset of possible HCI events is unknown, this should be set to the
maximum of 255.

config BT_BUF_EVT_RX_COUNT
int "Number of HCI Event buffers"
Expand Down
60 changes: 27 additions & 33 deletions subsys/bluetooth/host/buf.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ NET_BUF_POOL_FIXED_DEFINE(discardable_pool, CONFIG_BT_BUF_EVT_DISCARDABLE_COUNT,
BT_BUF_EVT_SIZE(CONFIG_BT_BUF_EVT_DISCARDABLE_SIZE),
sizeof(struct bt_buf_data), NULL);

NET_BUF_POOL_FIXED_DEFINE(cmd_complete_status_pool, CONFIG_BT_BUF_CMD_TX_COUNT,
BT_BUF_EVT_SIZE(CONFIG_BT_BUF_CMD_COMPLETE_EVT_SIZE),
sizeof(struct bt_buf_data),
NULL);

#if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
NET_BUF_POOL_DEFINE(acl_in_pool, CONFIG_BT_BUF_ACL_RX_COUNT,
BT_BUF_ACL_SIZE(CONFIG_BT_BUF_ACL_RX_SIZE),
Expand Down Expand Up @@ -103,6 +108,28 @@ struct net_buf *bt_buf_get_evt(uint8_t evt, bool discardable,
buf = net_buf_alloc(&sync_evt_pool, timeout);
break;
#endif /* CONFIG_BT_CONN || CONFIG_BT_ISO */
case BT_HCI_EVT_CMD_COMPLETE:
__fallthrough;
case BT_HCI_EVT_CMD_STATUS:
/**
* Use K_NO_WAIT here, to avoiding dead-lock.
* Normally there is always free event buffer corresponding to command.
* When there is no corresponding buf, it usually means that some error
* has occurred.
*
* For example: the Controller spontaneously sends an event.
*/
Comment on lines +120 to +121
Copy link
Contributor

@Thalley Thalley Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that valid behavior of the controller?

Copy link
Contributor

@jori-nordic jori-nordic Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is the command complete for NOP opcode. This is legal to send on boot, and we have a quirk in the host to wait for it, as it is somewhat useful over H4.

Copy link
Contributor Author

@LingaoM LingaoM Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also: #64158

Copy link
Contributor Author

@LingaoM LingaoM Jun 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently normal event pool will also use by extended advertising event report, this is a huge activity, Also other events such as periodic advertising report. So for command complete and command status also use same normal pool not a good way.

buf = net_buf_alloc(&cmd_complete_status_pool, K_NO_WAIT);
if (buf) {
break;
}

LOG_WRN("Insufficient cmd complete status pool for cmd %s event,"
"try to allocate from normal event pool",
evt == BT_HCI_EVT_CMD_COMPLETE ? "complete" : "status");

/* This usually doesn't happen */
__fallthrough;
default:
if (discardable) {
buf = net_buf_alloc(&discardable_pool, timeout);
Expand All @@ -119,39 +146,6 @@ struct net_buf *bt_buf_get_evt(uint8_t evt, bool discardable,
return buf;
}

#ifdef ZTEST_UNITTEST
#if defined(CONFIG_BT_HCI_ACL_FLOW_CONTROL)
struct net_buf_pool *bt_buf_get_evt_pool(void)
{
return &evt_pool;
}

struct net_buf_pool *bt_buf_get_acl_in_pool(void)
{
return &acl_in_pool;
}
#else
struct net_buf_pool *bt_buf_get_hci_rx_pool(void)
{
return &hci_rx_pool;
}
#endif /* CONFIG_BT_HCI_ACL_FLOW_CONTROL */

#if defined(CONFIG_BT_BUF_EVT_DISCARDABLE_COUNT)
struct net_buf_pool *bt_buf_get_discardable_pool(void)
{
return &discardable_pool;
}
#endif /* CONFIG_BT_BUF_EVT_DISCARDABLE_COUNT */

#if defined(CONFIG_BT_CONN) || defined(CONFIG_BT_ISO)
struct net_buf_pool *bt_buf_get_num_complete_pool(void)
{
return &sync_evt_pool;
}
#endif /* CONFIG_BT_CONN || CONFIG_BT_ISO */
#endif /* ZTEST_UNITTEST */

struct net_buf *bt_buf_make_view(struct net_buf *view,
struct net_buf *parent,
size_t len,
Expand Down