diff --git a/subsys/bluetooth/host/conn.c b/subsys/bluetooth/host/conn.c index 58e0d10c9b49..33d3acd706ea 100644 --- a/subsys/bluetooth/host/conn.c +++ b/subsys/bluetooth/host/conn.c @@ -45,6 +45,11 @@ LOG_MODULE_REGISTER(bt_conn); struct tx_meta { struct bt_conn_tx *tx; + /* This flag indicates if the current buffer has already been partially + * sent to the controller (ie, the next fragments should be sent as + * continuations). + */ + bool is_cont; }; #define tx_data(buf) ((struct tx_meta *)net_buf_user_data(buf)) @@ -429,6 +434,8 @@ int bt_conn_send_cb(struct bt_conn *conn, struct net_buf *buf, tx_data(buf)->tx = NULL; } + tx_data(buf)->is_cont = false; + net_buf_put(&conn->tx_queue, buf); return 0; } @@ -497,24 +504,41 @@ static int send_iso(struct bt_conn *conn, struct net_buf *buf, uint8_t flags) return bt_send(buf); } -static bool send_frag(struct bt_conn *conn, struct net_buf *buf, uint8_t flags, - bool always_consume) +static inline uint16_t conn_mtu(struct bt_conn *conn) +{ +#if defined(CONFIG_BT_BREDR) + if (conn->type == BT_CONN_TYPE_BR || !bt_dev.le.acl_mtu) { + return bt_dev.br.mtu; + } +#endif /* CONFIG_BT_BREDR */ +#if defined(CONFIG_BT_ISO) + if (conn->type == BT_CONN_TYPE_ISO && bt_dev.le.iso_mtu) { + return bt_dev.le.iso_mtu; + } +#endif /* CONFIG_BT_ISO */ +#if defined(CONFIG_BT_CONN) + return bt_dev.le.acl_mtu; +#else + return 0; +#endif /* CONFIG_BT_CONN */ +} + +static int do_send_frag(struct bt_conn *conn, struct net_buf *buf, uint8_t flags) { struct bt_conn_tx *tx = tx_data(buf)->tx; uint32_t *pending_no_cb; unsigned int key; int err = 0; - LOG_DBG("conn %p buf %p len %u flags 0x%02x", conn, buf, buf->len, flags); - - /* Wait until the controller can accept ACL packets */ - k_sem_take(bt_conn_get_pkts(conn), K_FOREVER); - /* Check for disconnection while waiting for pkts_sem */ if (conn->state != BT_CONN_CONNECTED) { + err = -ENOTCONN; goto fail; } + LOG_DBG("conn %p buf %p len %u flags 0x%02x", conn, buf, buf->len, + flags); + /* Add to pending, it must be done before bt_buf_set_type */ key = irq_lock(); if (tx) { @@ -552,12 +576,21 @@ static bool send_frag(struct bt_conn *conn, struct net_buf *buf, uint8_t flags, (*pending_no_cb)--; } irq_unlock(key); + + /* We don't want to end up in a situation where send_acl/iso + * returns the same error code as when we don't get a buffer in + * time. + */ + err = -EIO; goto fail; } - return true; + return 0; fail: + /* If we get here, something has seriously gone wrong: + * We also need to destroy the `parent` buf. + */ k_sem_give(bt_conn_get_pkts(conn)); if (tx) { /* `buf` might not get destroyed, and its `tx` pointer will still be reachable. @@ -567,35 +600,41 @@ static bool send_frag(struct bt_conn *conn, struct net_buf *buf, uint8_t flags, conn_tx_destroy(conn, tx); } - if (always_consume) { - net_buf_unref(buf); - } - return false; + return err; } -static inline uint16_t conn_mtu(struct bt_conn *conn) +static int send_frag(struct bt_conn *conn, + struct net_buf *buf, struct net_buf *frag, + uint8_t flags) { -#if defined(CONFIG_BT_BREDR) - if (conn->type == BT_CONN_TYPE_BR || !bt_dev.le.acl_mtu) { - return bt_dev.br.mtu; + /* Check if the controller can accept ACL packets */ + if (k_sem_take(bt_conn_get_pkts(conn), K_NO_WAIT)) { + LOG_DBG("no controller bufs"); + return -ENOBUFS; } -#endif /* CONFIG_BT_BREDR */ -#if defined(CONFIG_BT_ISO) - if (conn->type == BT_CONN_TYPE_ISO && bt_dev.le.iso_mtu) { - return bt_dev.le.iso_mtu; + + /* Add the data to the buffer */ + if (frag) { + uint16_t frag_len = MIN(conn_mtu(conn), net_buf_tailroom(frag)); + + net_buf_add_mem(frag, buf->data, frag_len); + net_buf_pull(buf, frag_len); + } else { + /* De-queue the buffer now that we know we can send it. + * Only applies if the buffer to be sent is the original buffer, + * and not one of its fragments. + * This buffer was fetched from the FIFO using a peek operation. + */ + buf = net_buf_get(&conn->tx_queue, K_NO_WAIT); + frag = buf; } -#endif /* CONFIG_BT_ISO */ -#if defined(CONFIG_BT_CONN) - return bt_dev.le.acl_mtu; -#else - return 0; -#endif /* CONFIG_BT_CONN */ + + return do_send_frag(conn, frag, flags); } static struct net_buf *create_frag(struct bt_conn *conn, struct net_buf *buf) { struct net_buf *frag; - uint16_t frag_len; switch (conn->type) { #if defined(CONFIG_BT_ISO) @@ -619,52 +658,55 @@ static struct net_buf *create_frag(struct bt_conn *conn, struct net_buf *buf) /* Fragments never have a TX completion callback */ tx_data(frag)->tx = NULL; - - frag_len = MIN(conn_mtu(conn), net_buf_tailroom(frag)); - - net_buf_add_mem(frag, buf->data, frag_len); - net_buf_pull(buf, frag_len); + tx_data(frag)->is_cont = false; return frag; } -static bool send_buf(struct bt_conn *conn, struct net_buf *buf) +static int send_buf(struct bt_conn *conn, struct net_buf *buf) { struct net_buf *frag; + uint8_t flags; + int err; LOG_DBG("conn %p buf %p len %u", conn, buf, buf->len); /* Send directly if the packet fits the ACL MTU */ - if (buf->len <= conn_mtu(conn)) { - return send_frag(conn, buf, FRAG_SINGLE, false); - } - - /* Create & enqueue first fragment */ - frag = create_frag(conn, buf); - if (!frag) { - return false; - } - - if (!send_frag(conn, frag, FRAG_START, true)) { - return false; + if (buf->len <= conn_mtu(conn) && !tx_data(buf)->is_cont) { + LOG_DBG("send single"); + return send_frag(conn, buf, NULL, FRAG_SINGLE); } + LOG_DBG("start fragmenting"); /* * Send the fragments. For the last one simply use the original - * buffer (which works since we've used net_buf_pull on it. + * buffer (which works since we've used net_buf_pull on it). */ + flags = FRAG_START; + if (tx_data(buf)->is_cont) { + flags = FRAG_CONT; + } + while (buf->len > conn_mtu(conn)) { frag = create_frag(conn, buf); if (!frag) { - return false; + return -ENOMEM; } - if (!send_frag(conn, frag, FRAG_CONT, true)) { - return false; + err = send_frag(conn, buf, frag, flags); + if (err) { + LOG_DBG("%p failed, mark as existing frag", buf); + tx_data(buf)->is_cont = flags != FRAG_START; + net_buf_unref(frag); + return err; } + + flags = FRAG_CONT; } - return send_frag(conn, buf, FRAG_END, false); + LOG_DBG("last frag"); + tx_data(buf)->is_cont = true; + return send_frag(conn, buf, NULL, FRAG_END); } static struct k_poll_signal conn_change = @@ -732,10 +774,26 @@ static int conn_prepare_events(struct bt_conn *conn, LOG_DBG("Adding conn %p to poll list", conn); - k_poll_event_init(&events[0], - K_POLL_TYPE_FIFO_DATA_AVAILABLE, - K_POLL_MODE_NOTIFY_ONLY, - &conn->tx_queue); + bool buffers_available = k_sem_count_get(bt_conn_get_pkts(conn)) > 0; + bool packets_waiting = !k_fifo_is_empty(&conn->tx_queue); + + if (packets_waiting && !buffers_available) { + /* Only resume sending when the controller has buffer space + * available for this connection. + */ + LOG_DBG("wait on ctlr buffers"); + k_poll_event_init(&events[0], + K_POLL_TYPE_SEM_AVAILABLE, + K_POLL_MODE_NOTIFY_ONLY, + bt_conn_get_pkts(conn)); + } else { + /* Wait until there is more data to send. */ + LOG_DBG("wait on host fifo"); + k_poll_event_init(&events[0], + K_POLL_TYPE_FIFO_DATA_AVAILABLE, + K_POLL_MODE_NOTIFY_ONLY, + &conn->tx_queue); + } events[0].tag = BT_EVENT_CONN_TX_QUEUE; return 0; @@ -779,6 +837,7 @@ int bt_conn_prepare_events(struct k_poll_event events[]) void bt_conn_process_tx(struct bt_conn *conn) { struct net_buf *buf; + int err; LOG_DBG("conn %p", conn); @@ -789,10 +848,23 @@ void bt_conn_process_tx(struct bt_conn *conn) return; } - /* Get next ACL packet for connection */ - buf = net_buf_get(&conn->tx_queue, K_NO_WAIT); + /* Get next ACL packet for connection. The buffer will only get dequeued + * if there is a free controller buffer to put it in. + * + * Important: no operations should be done on `buf` until it is properly + * dequeued from the FIFO, using the `net_buf_get()` API. + */ + buf = k_fifo_peek_head(&conn->tx_queue); BT_ASSERT(buf); - if (!send_buf(conn, buf)) { + + /* Since we used `peek`, the queue still owns the reference to the + * buffer, so we need to take an explicit additional reference here. + */ + buf = net_buf_ref(buf); + err = send_buf(conn, buf); + net_buf_unref(buf); + + if (err == -EIO) { struct bt_conn_tx *tx = tx_data(buf)->tx; tx_data(buf)->tx = NULL; diff --git a/subsys/bluetooth/host/hci_core.c b/subsys/bluetooth/host/hci_core.c index 1c2fc8dccf09..706029ad2ea1 100644 --- a/subsys/bluetooth/host/hci_core.c +++ b/subsys/bluetooth/host/hci_core.c @@ -2465,6 +2465,13 @@ static void process_events(struct k_poll_event *ev, int count) switch (ev->state) { case K_POLL_STATE_SIGNALED: break; + case K_POLL_STATE_SEM_AVAILABLE: + /* After this fn is exec'd, `bt_conn_prepare_events()` + * will be called once again, and this time buffers will + * be available, so the FIFO will be added to the poll + * list instead of the ctlr buffers semaphore. + */ + break; case K_POLL_STATE_FIFO_DATA_AVAILABLE: if (ev->tag == BT_EVENT_CMD_TX) { send_cmd(); @@ -2524,6 +2531,7 @@ static void hci_tx_thread(void *p1, void *p2, void *p3) events[0].state = K_POLL_STATE_NOT_READY; ev_count = 1; + /* This adds the FIFO per-connection */ if (IS_ENABLED(CONFIG_BT_CONN) || IS_ENABLED(CONFIG_BT_ISO)) { ev_count += bt_conn_prepare_events(&events[1]); } diff --git a/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/prj.conf b/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/prj.conf index 804afdaa12d0..fcb08ee2217c 100644 --- a/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/prj.conf +++ b/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/prj.conf @@ -18,7 +18,14 @@ CONFIG_BT_GAP_AUTO_UPDATE_CONN_PARAMS=n # L2CAP MPS # 23+27+27=77 makes exactly three full packets CONFIG_BT_L2CAP_TX_MTU=77 -CONFIG_BT_BUF_ACL_TX_SIZE=77 + +# Use this to send L2CAP PDUs without any fragmentation. +# In this particular case, we prefer fragmenting to test that code path. +# CONFIG_BT_BUF_ACL_TX_SIZE=81 + +# L2CAP PDUs will be fragmented in 3 ACL packets. +CONFIG_BT_BUF_ACL_TX_SIZE=27 + CONFIG_BT_BUF_ACL_TX_COUNT=4 # The minimum value for this is @@ -31,14 +38,12 @@ CONFIG_BT_BUF_ACL_RX_SIZE=81 # to keep it that way as to stress the stack as much as possible. CONFIG_BT_L2CAP_TX_BUF_COUNT=6 +CONFIG_BT_CTLR_DATA_LENGTH_MAX=27 CONFIG_BT_CTLR_RX_BUFFERS=10 -# The ring buffer now has space for three times as much data -# (default 27, 3*27=81), so that it does not run out of data -# while waiting for new SDUs to be queued. -CONFIG_BT_CTLR_DATA_LENGTH_MAX=81 CONFIG_BT_MAX_CONN=10 CONFIG_LOG=y CONFIG_ASSERT=y CONFIG_BT_DEBUG_LOG=y +CONFIG_NET_BUF_POOL_USAGE=y diff --git a/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/src/common.h b/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/src/common.h index 199033f78f79..d3b01cd1d683 100644 --- a/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/src/common.h +++ b/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/src/common.h @@ -36,7 +36,7 @@ extern enum bst_result_t bst_result; } -#define WAIT_SECONDS 220 /* seconds */ +#define WAIT_SECONDS 270 /* seconds */ #define WAIT_TIME (WAIT_SECONDS * USEC_PER_SEC) /* microseconds*/ #define FAIL(...) \ diff --git a/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/src/main.c b/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/src/main.c index 835a02aef6b1..62871d9697c6 100644 --- a/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/src/main.c +++ b/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/src/main.c @@ -21,20 +21,21 @@ CREATE_FLAG(flag_l2cap_connected); #define INIT_CREDITS 10 #define SDU_NUM 20 #define SDU_LEN 1230 -#define NUM_SEGMENTS 30 +#define NUM_SEGMENTS 10 /* Only one SDU per link will be transmitted at a time */ NET_BUF_POOL_DEFINE(sdu_tx_pool, - CONFIG_BT_MAX_CONN, BT_L2CAP_BUF_SIZE(SDU_LEN), + CONFIG_BT_MAX_CONN, BT_L2CAP_SDU_BUF_SIZE(SDU_LEN), 8, NULL); NET_BUF_POOL_DEFINE(segment_pool, + /* MTU + 4 l2cap hdr + 4 ACL hdr */ NUM_SEGMENTS, BT_L2CAP_BUF_SIZE(CONFIG_BT_L2CAP_TX_MTU), 8, NULL); /* Only one SDU per link will be received at a time */ NET_BUF_POOL_DEFINE(sdu_rx_pool, - CONFIG_BT_MAX_CONN, BT_L2CAP_BUF_SIZE(SDU_LEN), + CONFIG_BT_MAX_CONN, BT_L2CAP_SDU_BUF_SIZE(SDU_LEN), 8, NULL); static struct bt_l2cap_le_chan l2cap_channels[L2CAP_CHANS]; @@ -44,6 +45,7 @@ static uint8_t tx_data[SDU_LEN]; static uint8_t tx_left[L2CAP_CHANS]; static uint16_t rx_cnt; static uint8_t disconnect_counter; +static uint32_t max_seg_allocated; int l2cap_chan_send(struct bt_l2cap_chan *chan, uint8_t *data, size_t len) { @@ -56,7 +58,7 @@ int l2cap_chan_send(struct bt_l2cap_chan *chan, uint8_t *data, size_t len) return -ENOMEM; } - net_buf_reserve(buf, BT_L2CAP_CHAN_SEND_RESERVE); + net_buf_reserve(buf, BT_L2CAP_SDU_CHAN_SEND_RESERVE); net_buf_add_mem(buf, data, len); int ret = bt_l2cap_chan_send(chan, buf); @@ -74,6 +76,10 @@ struct net_buf *alloc_seg_cb(struct bt_l2cap_chan *chan) { struct net_buf *buf = net_buf_alloc(&segment_pool, K_NO_WAIT); + if ((NUM_SEGMENTS - segment_pool.avail_count) > max_seg_allocated) { + max_seg_allocated++; + } + ASSERT(buf, "Ran out of segment buffers"); return buf; @@ -121,6 +127,9 @@ int recv_cb(struct bt_l2cap_chan *chan, struct net_buf *buf) LOG_DBG("len %d", buf->len); rx_cnt++; + /* Verify SDU data matches TX'd data. */ + ASSERT(memcmp(buf->data, tx_data, buf->len) == 0, "RX data doesn't match TX"); + return 0; } @@ -267,6 +276,11 @@ static void test_peripheral_main(void) LOG_DBG("*L2CAP STRESS Peripheral started*"); int err; + /* Prepare tx_data */ + for (size_t i = 0; i < sizeof(tx_data); i++) { + tx_data[i] = (uint8_t)i; + } + err = bt_enable(NULL); if (err) { FAIL("Can't enable Bluetooth (err %d)", err); @@ -372,6 +386,11 @@ static void test_central_main(void) LOG_DBG("*L2CAP STRESS Central started*"); int err; + /* Prepare tx_data */ + for (size_t i = 0; i < sizeof(tx_data); i++) { + tx_data[i] = (uint8_t)i; + } + err = bt_enable(NULL); ASSERT(err == 0, "Can't enable Bluetooth (err %d)\n", err); LOG_DBG("Central Bluetooth initialized."); @@ -409,6 +428,8 @@ static void test_central_main(void) } LOG_DBG("All peripherals disconnected."); + LOG_DBG("Max segment pool usage: %u bufs", max_seg_allocated); + PASS("L2CAP STRESS Central passed\n"); } diff --git a/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/tests_scripts/l2cap.sh b/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/tests_scripts/l2cap.sh index dc58d8dc8f97..bd7bbacbcefd 100755 --- a/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/tests_scripts/l2cap.sh +++ b/tests/bluetooth/bsim_bt/bsim_test_l2cap_stress/tests_scripts/l2cap.sh @@ -35,7 +35,7 @@ Execute "${bsim_exe}" -v=${verbosity_level} -s=${simulation_id} -d=4 -testid=per Execute "${bsim_exe}" -v=${verbosity_level} -s=${simulation_id} -d=5 -testid=peripheral -rs=230 Execute "${bsim_exe}" -v=${verbosity_level} -s=${simulation_id} -d=6 -testid=peripheral -rs=9 -Execute ./bs_2G4_phy_v1 -v=${verbosity_level} -s=${simulation_id} -D=7 -sim_length=240e6 $@ +Execute ./bs_2G4_phy_v1 -v=${verbosity_level} -s=${simulation_id} -D=7 -sim_length=270e6 $@ for process_id in $process_ids; do wait $process_id || let "exit_code=$?"