You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mynewt.apache.org by GitBox <gi...@apache.org> on 2020/06/04 14:55:24 UTC
[GitHub] [mynewt-nimble] haukepetersen opened a new pull request #827: l2cap/coc: make ble_l2cap_coc_continue_tx() thread safe
haukepetersen opened a new pull request #827:
URL: https://github.com/apache/mynewt-nimble/pull/827
When debugging my favorite L2CAP throughput issues (#818, #373), I sometimes noticed some packets being dropped. In these (mostly rare) cases `l2cap_send()` returned `BLE_HS_ENOMEM` even though there was sufficient space in the used `msys` mbuf pool.
Looking into this, I found that in `ble_l2cap_coc_continue_tx()` the `chan->coc_tx->data_offset` value as well as the value for `OS_MBUF_PKTLEN(tx->sdu)` were corrupted, leading `os_mbuf_appendfrom()` fail.
See the commit description for my explanation for this:
```
ble_l2cap_coc_continue_tx() is executed in different tasks: it is
(i) called from ble_l2cap_coc_send() in one or more different task
contexts from user application(s) and (ii) called from
ble_l2cap_coc_le_credits_update() from the NimBLE host task.
Typically, the host task has a higher priority then the user tasks,
so that ble_l2cap_coc_continue_tx() can be interrupted by itself.
This leads to potentially inconsistent state for
`chan->coc_tx->credits` and `chan->coc_tx->data_offset`, consequently
leading to dropped packets.
This commit fixes this by putting all the code code working on these
variables into a critical section.
```
This solution feels a little bit brute force, but I could not think of anything else to make `ble_l2cap_coc_continue_tx()` thread safe. My initial idea was to put an event into the hosts event queue from `ble_l2cap_coc_send()`, instead of calling `ble_l2cap_coc_continue_tx()` directly. But then we would need to allocate an event for each l2cap channel. But more severe we would not be able to synchronously pass the return code of `ble_l2cap_coc_continue_tx()` to `ble_l2cap_coc_send()`. So it seemed to me locking the host for the duration of `ble_l2cap_coc_continue_tx()` is the easier path?!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #827: l2cap/coc: make ble_l2cap_coc_continue_tx() thread safe
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #827:
URL: https://github.com/apache/mynewt-nimble/pull/827#issuecomment-638926454
fixed two more style issues pointed out by the `apache-mynewt-bot`
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen merged pull request #827: l2cap/coc: make ble_l2cap_coc_continue_tx() thread safe
Posted by GitBox <gi...@apache.org>.
haukepetersen merged pull request #827:
URL: https://github.com/apache/mynewt-nimble/pull/827
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] haukepetersen commented on pull request #827: l2cap/coc: make ble_l2cap_coc_continue_tx() thread safe
Posted by GitBox <gi...@apache.org>.
haukepetersen commented on pull request #827:
URL: https://github.com/apache/mynewt-nimble/pull/827#issuecomment-642447517
I am still trying to come up with a more elegant solution, but so far I was not able to make anything work. So lets go for now.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot removed a comment on pull request #827: l2cap/coc: make ble_l2cap_coc_continue_tx() thread safe
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot removed a comment on pull request #827:
URL: https://github.com/apache/mynewt-nimble/pull/827#issuecomment-638920208
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### nimble/host/src/ble_l2cap_coc.c
<details>
```diff
@@ -469,12 +469,12 @@
txom = NULL;
goto failed;
} else {
- tx->credits --;
+ tx->credits--;
tx->data_offset += len - sdu_size_offset;
}
BLE_HS_LOG(DEBUG, "Sent %d bytes, credits=%d, to send %d bytes \n",
- len, tx->credits, OS_MBUF_PKTLEN(tx->sdu)- tx->data_offset );
+ len, tx->credits, OS_MBUF_PKTLEN(tx->sdu)- tx->data_offset);
if (tx->data_offset == OS_MBUF_PKTLEN(tx->sdu)) {
BLE_HS_LOG(DEBUG, "Complete package sent\n");
```
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #827: l2cap/coc: make ble_l2cap_coc_continue_tx() thread safe
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #827:
URL: https://github.com/apache/mynewt-nimble/pull/827#issuecomment-638931210
<!-- style-bot -->
## Style check summary
#### No suggestions at this time!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mynewt-nimble] apache-mynewt-bot commented on pull request #827: l2cap/coc: make ble_l2cap_coc_continue_tx() thread safe
Posted by GitBox <gi...@apache.org>.
apache-mynewt-bot commented on pull request #827:
URL: https://github.com/apache/mynewt-nimble/pull/827#issuecomment-638920208
<!-- style-bot -->
## Style check summary
### Our coding style is [here!](https://github.com/apache/mynewt-core/blob/master/CODING_STANDARDS.md)
#### nimble/host/src/ble_l2cap_coc.c
<details>
```diff
@@ -469,12 +469,12 @@
txom = NULL;
goto failed;
} else {
- tx->credits --;
+ tx->credits--;
tx->data_offset += len - sdu_size_offset;
}
BLE_HS_LOG(DEBUG, "Sent %d bytes, credits=%d, to send %d bytes \n",
- len, tx->credits, OS_MBUF_PKTLEN(tx->sdu)- tx->data_offset );
+ len, tx->credits, OS_MBUF_PKTLEN(tx->sdu)- tx->data_offset);
if (tx->data_offset == OS_MBUF_PKTLEN(tx->sdu)) {
BLE_HS_LOG(DEBUG, "Complete package sent\n");
```
</details>
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org