Skip to content

Bluetooth: Fix deadlock issues when running low on buffers #17137

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

Merged
merged 2 commits into from
Jul 1, 2019
Merged

Bluetooth: Fix deadlock issues when running low on buffers #17137

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
61f8037
Bluetooth: Add dedicated pool for HCI_Num_Completed_Packets HCI event
Jun 17, 2019
3684c82
Bluetooth: Introduce separate pool for discardable events
Jun 28, 2019
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
11 changes: 3 additions & 8 deletions drivers/bluetooth/hci/h4.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -161,16 +161,11 @@ static struct net_buf *get_rx(int timeout)
{
BT_DBG("type 0x%02x, evt 0x%02x", rx.type, rx.evt.evt);

if (rx.type == H4_EVT && (rx.evt.evt == BT_HCI_EVT_CMD_COMPLETE ||
rx.evt.evt == BT_HCI_EVT_CMD_STATUS)) {
return bt_buf_get_cmd_complete(timeout);
if (rx.type == H4_EVT) {
return bt_buf_get_evt(rx.evt.evt, rx.discardable, timeout);
}

if (rx.type == H4_ACL) {
return bt_buf_get_rx(BT_BUF_ACL_IN, timeout);
} else {
return bt_buf_get_rx(BT_BUF_EVT, timeout);
}
return bt_buf_get_rx(BT_BUF_ACL_IN, timeout);
}

static void rx_thread(void *p1, void *p2, void *p3)
Expand Down
11 changes: 1 addition & 10 deletions drivers/bluetooth/hci/h5.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,16 +408,7 @@ static inline struct net_buf *get_evt_buf(u8_t evt)
{
struct net_buf *buf;

switch (evt) {
case BT_HCI_EVT_CMD_COMPLETE:
case BT_HCI_EVT_CMD_STATUS:
buf = bt_buf_get_cmd_complete(K_NO_WAIT);
break;
default:
buf = bt_buf_get_rx(BT_BUF_EVT, K_NO_WAIT);
break;
}

buf = bt_buf_get_evt(evt, false, K_NO_WAIT);
if (buf) {
net_buf_add_u8(h5.rx_buf, evt);
}
Expand Down
7 changes: 2 additions & 5 deletions drivers/bluetooth/hci/ipm_stm32wb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,9 @@ void TM_EvtReceivedCb(TL_EvtPacket_t *hcievt)
BT_ERR("Unknown evtcode type 0x%02x",
hcievt->evtserial.evt.evtcode);
goto out;
case BT_HCI_EVT_CMD_COMPLETE:
case BT_HCI_EVT_CMD_STATUS:
buf = bt_buf_get_cmd_complete(K_FOREVER);
break;
default:
buf = bt_buf_get_rx(BT_BUF_EVT, K_FOREVER);
buf = bt_buf_get_evt(evtserial.evt.evtcode, false,
K_FOREVER);
break;
}
net_buf_add_mem(buf, &hcievt->evtserial.evt,
Expand Down
7 changes: 2 additions & 5 deletions drivers/bluetooth/hci/spi.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,12 +353,9 @@ static void bt_spi_rx_thread(void)
/* Vendor events are currently unsupported */
bt_spi_handle_vendor_evt(rxmsg);
continue;
case BT_HCI_EVT_CMD_COMPLETE:
case BT_HCI_EVT_CMD_STATUS:
buf = bt_buf_get_cmd_complete(K_FOREVER);
break;
default:
buf = bt_buf_get_rx(BT_BUF_EVT, K_FOREVER);
buf = bt_buf_get_evt(rxmsg[EVT_HEADER_EVENT],
false, K_FOREVER);
break;
}

Expand Down
11 changes: 3 additions & 8 deletions drivers/bluetooth/hci/userchan.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,16 +57,11 @@ static int bt_dev_index = -1;

static struct net_buf *get_rx(const u8_t *buf)
{
if (buf[0] == H4_EVT && (buf[1] == BT_HCI_EVT_CMD_COMPLETE ||
buf[1] == BT_HCI_EVT_CMD_STATUS)) {
return bt_buf_get_cmd_complete(K_FOREVER);
if (buf[0] == H4_EVT) {
return bt_buf_get_evt(buf[1], false, K_FOREVER);
}

if (buf[0] == H4_ACL) {
return bt_buf_get_rx(BT_BUF_ACL_IN, K_FOREVER);
} else {
return bt_buf_get_rx(BT_BUF_EVT, K_FOREVER);
}
return bt_buf_get_rx(BT_BUF_ACL_IN, K_FOREVER);
}

static bool uc_ready(void)
Expand Down
13 changes: 13 additions & 0 deletions include/bluetooth/buf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,19 @@ struct net_buf *bt_buf_get_rx(enum bt_buf_type type, s32_t timeout);
*/
struct net_buf *bt_buf_get_cmd_complete(s32_t timeout);

/** Allocate a buffer for an HCI Event
*
* This will set the buffer type so bt_buf_set_type() does not need to
* be explicitly called before bt_recv_prio() or bt_recv().
*
* @param evt HCI event code
* @param discardable Whether the driver considers the event discardable.
* @param timeout Timeout in milliseconds, or one of the special values
* K_NO_WAIT and K_FOREVER.
* @return A new buffer.
*/
struct net_buf *bt_buf_get_evt(u8_t evt, bool discardable, s32_t timeout);
Copy link
Contributor

@Vudentz Vudentz Jun 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I assume the K_NO_WAIT would discard anyway so Im not sure why we need another variable besides that is completely ignored when CONFIG_BT_DISCARDABLE_POOL is not set.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Vudentz those are not quite the same. A HCI driver might be calling this from an ISR for a non-discardable event, in which case K_NO_WAIT is the right timeout to pass. E.g. the h4.c driver does this, and then if that fails it defers the further allocation attempts to the RX thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair enough, I though would use the new pool as well but it seems they are exclusive to each type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm all for eliminating redundancy in parameters, but in this case they're a bit orthogonal and we can't make too many assumptions about getting a K_NO_WAIT.


/** Set the buffer type
*
* @param buf Bluetooth buffer
Expand Down
1 change: 1 addition & 0 deletions include/bluetooth/hci.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1362,6 +1362,7 @@ struct bt_hci_cp_le_set_privacy_mode {

/* Event definitions */

#define BT_HCI_EVT_UNKNOWN 0x00
#define BT_HCI_EVT_VENDOR 0xff

#define BT_HCI_EVT_INQUIRY_COMPLETE 0x01
Expand Down
1 change: 0 additions & 1 deletion samples/bluetooth/mesh/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ CONFIG_BT_PERIPHERAL=y

CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69
CONFIG_BT_L2CAP_TX_BUF_COUNT=5
Expand Down
2 changes: 2 additions & 0 deletions samples/bluetooth/mesh/prj_bbc_microbit.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ CONFIG_SETTINGS_FCB=y
CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_STACK_SIZE=1100
CONFIG_BT_RX_BUF_COUNT=3
CONFIG_BT_DISCARDABLE_BUF_COUNT=3

CONFIG_BT_CTLR_DUP_FILTER_LEN=0
CONFIG_BT_OBSERVER=y
Expand Down
1 change: 0 additions & 1 deletion samples/bluetooth/mesh_demo/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ CONFIG_MAIN_STACK_SIZE=512
CONFIG_SYSTEM_WORKQUEUE_STACK_SIZE=2048

CONFIG_BT=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_TINYCRYPT_ECC=y
#CONFIG_BT_DEBUG_LOG=y
CONFIG_BT_OBSERVER=y
Expand Down
1 change: 0 additions & 1 deletion samples/boards/nrf52/mesh/onoff-app/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@ CONFIG_BT_PERIPHERAL=y

CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69
CONFIG_BT_L2CAP_TX_BUF_COUNT=8
Expand Down
1 change: 0 additions & 1 deletion samples/boards/nrf52/mesh/onoff_level_lighting_vnd_app/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ CONFIG_BT_CTLR_TX_PWR_PLUS_8=y
CONFIG_BT_PERIPHERAL=y
CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_RX_STACK_SIZE=4096
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69
Expand Down
1 change: 0 additions & 1 deletion samples/boards/reel_board/mesh_badge/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ CONFIG_BT_MESH_DEBUG=y

CONFIG_BT_OBSERVER=y
CONFIG_BT_BROADCASTER=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69
CONFIG_BT_L2CAP_TX_BUF_COUNT=8
Expand Down
4 changes: 2 additions & 2 deletions subsys/bluetooth/controller/hci/hci.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,7 @@ void *hci_cmd_complete(struct net_buf **buf, u8_t plen)
{
struct bt_hci_evt_cmd_complete *cc;

*buf = bt_buf_get_cmd_complete(K_FOREVER);
*buf = bt_buf_get_evt(BT_HCI_EVT_CMD_COMPLETE, false, K_FOREVER);

hci_evt_create(*buf, BT_HCI_EVT_CMD_COMPLETE, sizeof(*cc) + plen);

Expand All @@ -137,7 +137,7 @@ static struct net_buf *cmd_status(u8_t status)
struct bt_hci_evt_cmd_status *cs;
struct net_buf *buf;

buf = bt_buf_get_cmd_complete(K_FOREVER);
buf = bt_buf_get_evt(BT_HCI_EVT_CMD_STATUS, false, K_FOREVER);
hci_evt_create(buf, BT_HCI_EVT_CMD_STATUS, sizeof(*cs));

cs = net_buf_add(buf, sizeof(*cs));
Expand Down
6 changes: 4 additions & 2 deletions subsys/bluetooth/controller/hci/hci_driver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ static void prio_recv_thread(void *p1, void *p2, void *p3)
#if defined(CONFIG_BT_CONN)
struct net_buf *buf;

buf = bt_buf_get_rx(BT_BUF_EVT, K_FOREVER);
buf = bt_buf_get_evt(BT_HCI_EVT_NUM_COMPLETED_PACKETS,
false, K_FOREVER);
hci_num_cmplt_encode(buf, handle, num_cmplt);
BT_DBG("Num Complete: 0x%04x:%u", handle, num_cmplt);
bt_recv_prio(buf);
Expand Down Expand Up @@ -145,7 +146,8 @@ static inline struct net_buf *encode_node(struct node_rx_pdu *node_rx,
case HCI_CLASS_EVT_REQUIRED:
case HCI_CLASS_EVT_CONNECTION:
if (class == HCI_CLASS_EVT_DISCARDABLE) {
buf = bt_buf_get_rx(BT_BUF_EVT, K_NO_WAIT);
buf = bt_buf_get_evt(BT_HCI_EVT_UNKNOWN, true,
K_NO_WAIT);
} else {
buf = bt_buf_get_rx(BT_BUF_EVT, K_FOREVER);
}
Expand Down
15 changes: 15 additions & 0 deletions subsys/bluetooth/host/Kconfig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ config BT_HCI_CMD_COUNT
config BT_RX_BUF_COUNT
int "Number of HCI RX buffers"
default 3 if BT_RECV_IS_RX_THREAD
default 20 if (BT_MESH && !BT_DISCARDABLE_BUF_COUNT)
default 10
range 2 255
help
Expand All @@ -47,6 +48,20 @@ config BT_RX_BUF_LEN
an L2CAP MTU of 65 bytes. On top of this there's the L2CAP header
(4 bytes) and the ACL header (also 4 bytes) which yields 73 bytes.

config BT_DISCARDABLE_BUF_COUNT
int "Number of discardable event buffers"
range 1 255
Copy link
Member

Choose a reason for hiding this comment

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

Change the range here to 0 255

default 20 if BT_MESH
default 3
depends on BT_H4 || BT_CTLR
help
Number of buffers in a separate buffer pool for events which
the HCI driver considers discardable. Examples of such events
could be e.g. Advertising Reports. The benefit of having such
a pool means that the if there is a heavy inflow of such events
it will not cause the allocation for other critical events to
block and may even eliminate deadlocks in some cases.

config BT_HCI_TX_STACK_SIZE
# NOTE: This value is derived from other symbols and should not be
# user-configurable. Do not give it a prompt.
Expand Down
54 changes: 54 additions & 0 deletions subsys/bluetooth/host/hci_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,21 @@ NET_BUF_POOL_DEFINE(hci_cmd_pool, CONFIG_BT_HCI_CMD_COUNT,
NET_BUF_POOL_DEFINE(hci_rx_pool, CONFIG_BT_RX_BUF_COUNT,
BT_BUF_RX_SIZE, BT_BUF_USER_DATA_MIN, NULL);

#if defined(CONFIG_BT_CONN)
/* Dedicated pool for HCI_Number_of_Completed_Packets. This event is always
* consumed synchronously by bt_recv_prio() so a single buffer is enough.
* Having a dedicated pool for it ensures that exhaustion of the RX pool
* cannot block the delivery of this priority event.
*/
NET_BUF_POOL_DEFINE(num_complete_pool, 1, BT_BUF_RX_SIZE,
BT_BUF_USER_DATA_MIN, NULL);
#endif /* CONFIG_BT_CONN */

#if defined(CONFIG_BT_DISCARDABLE_BUF_COUNT)
NET_BUF_POOL_DEFINE(discardable_pool, CONFIG_BT_DISCARDABLE_BUF_COUNT,
BT_BUF_RX_SIZE, BT_BUF_USER_DATA_MIN, NULL);
#endif /* CONFIG_BT_DISCARDABLE_BUF_COUNT */

struct event_handler {
u8_t event;
u8_t min_len;
Expand Down Expand Up @@ -5662,6 +5677,45 @@ struct net_buf *bt_buf_get_cmd_complete(s32_t timeout)
return bt_buf_get_rx(BT_BUF_EVT, timeout);
}

struct net_buf *bt_buf_get_evt(u8_t evt, bool discardable, s32_t timeout)
{
switch (evt) {
#if defined(CONFIG_BT_CONN)
case BT_HCI_EVT_NUM_COMPLETED_PACKETS:
{
struct net_buf *buf;

buf = net_buf_alloc(&num_complete_pool, timeout);
if (buf) {
net_buf_reserve(buf, CONFIG_BT_HCI_RESERVE);
bt_buf_set_type(buf, BT_BUF_EVT);
}

return buf;
}
#endif /* CONFIG_BT_CONN */
case BT_HCI_EVT_CMD_COMPLETE:
case BT_HCI_EVT_CMD_STATUS:
return bt_buf_get_cmd_complete(timeout);
default:
#if defined(CONFIG_BT_DISCARDABLE_BUF_COUNT)
if (discardable) {
struct net_buf *buf;

buf = net_buf_alloc(&discardable_pool, timeout);
if (buf) {
net_buf_reserve(buf, CONFIG_BT_HCI_RESERVE);
bt_buf_set_type(buf, BT_BUF_EVT);
}

return buf;
}
#endif /* CONFIG_BT_DISCARDABLE_BUF_COUNT */

return bt_buf_get_rx(BT_BUF_EVT, timeout);
}
}

#if defined(CONFIG_BT_BREDR)
static int br_start_inquiry(const struct bt_br_discovery_param *param)
{
Expand Down
2 changes: 1 addition & 1 deletion subsys/bluetooth/host/hci_ecc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ static void send_cmd_status(u16_t opcode, u8_t status)

BT_DBG("opcode %x status %x", opcode, status);

buf = bt_buf_get_cmd_complete(K_FOREVER);
buf = bt_buf_get_evt(BT_HCI_EVT_CMD_STATUS, false, K_FOREVER);
bt_buf_set_type(buf, BT_BUF_EVT);

hdr = net_buf_add(buf, sizeof(*hdr));
Expand Down
12 changes: 12 additions & 0 deletions subsys/bluetooth/host/hci_raw.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,18 @@ struct net_buf *bt_buf_get_cmd_complete(s32_t timeout)
return buf;
}

struct net_buf *bt_buf_get_evt(u8_t evt, bool discardable, s32_t timeout)
{
struct net_buf *buf;

buf = net_buf_alloc(&hci_rx_pool, timeout);
if (buf) {
bt_buf_set_type(buf, BT_BUF_EVT);
}

return buf;
}

int bt_recv(struct net_buf *buf)
{
BT_DBG("buf %p len %u", buf, buf->len);
Expand Down
2 changes: 1 addition & 1 deletion tests/bluetooth/hci_prop_evt/src/main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ static void *cmd_complete(struct net_buf **buf, u8_t plen, u16_t opcode)
{
struct bt_hci_evt_cmd_complete *cc;

*buf = bt_buf_get_cmd_complete(K_FOREVER);
*buf = bt_buf_get_evt(BT_HCI_EVT_CMD_COMPLETE, false, K_FOREVER);
evt_create(*buf, BT_HCI_EVT_CMD_COMPLETE, sizeof(*cc) + plen);
cc = net_buf_add(*buf, sizeof(*cc));
cc->ncmd = 1U;
Expand Down
1 change: 0 additions & 1 deletion tests/bluetooth/mesh/dbg.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ CONFIG_BT_PERIPHERAL=y

CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69

Expand Down
1 change: 0 additions & 1 deletion tests/bluetooth/mesh/friend.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ CONFIG_BT_CTLR_PRIVACY=n

CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30

CONFIG_BT_MESH=y
CONFIG_BT_MESH_RELAY=y
Expand Down
1 change: 0 additions & 1 deletion tests/bluetooth/mesh/gatt.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ CONFIG_BT_PERIPHERAL=y

CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69

Expand Down
1 change: 0 additions & 1 deletion tests/bluetooth/mesh/lpn.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ CONFIG_BT_PERIPHERAL=y

CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69

Expand Down
1 change: 0 additions & 1 deletion tests/bluetooth/mesh/pb_gatt.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ CONFIG_BT_PERIPHERAL=y

CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69

Expand Down
1 change: 0 additions & 1 deletion tests/bluetooth/mesh/prj.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ CONFIG_BT_PERIPHERAL=y

CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69

Expand Down
1 change: 0 additions & 1 deletion tests/bluetooth/mesh/proxy.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ CONFIG_BT_PERIPHERAL=y

CONFIG_BT=y
CONFIG_BT_TINYCRYPT_ECC=y
CONFIG_BT_RX_BUF_COUNT=30
CONFIG_BT_L2CAP_RX_MTU=69
CONFIG_BT_L2CAP_TX_MTU=69

Expand Down
Loading