usb: device_next: class: cdc_acm: throughput improvements (low hanging fruit) #99838
Conversation
JordanYates
force-pushed
the
251122_cdc_acm_throughput
branch
from
881dc65 to
e88357a
Compare
|
|
||
| static struct net_buf *cdc_acm_buf_alloc(struct usbd_class_data *const c_data, | ||
| const uint8_t ep) | ||
| const uint8_t ep, size_t size_hint) |
There was a problem hiding this comment.
Please add const and name it just size.
static struct net_buf *cdc_acm_buf_alloc(struct usbd_class_data *const c_data,
const uint8_t ep, const size_t size)There was a problem hiding this comment.
Added const, left the name as size_hint as that is what it is. A hint of how much data is pending to be sent, allowing this function to allocate an appropriately sized buffer (through the additional modifications in the function).
| /* The USB stack handles packetizing to the endpoint size, therefore | ||
| * allocating buffers largers than the endpoint size reduces the number | ||
| * of round trips through this class driver when the TX FIFO size is | ||
| * larger than the endpoint, improving throughput. | ||
| * | ||
| * Limit the maximum allocation size to 1/4 of the total pool size | ||
| * to prevent starving other USB users. Ensure the maximum allocation | ||
| * never drops below USBD_MAX_BULK_MPS. | ||
| */ |
There was a problem hiding this comment.
/* The USB stack handles packetizing to the endpoint size, therefore
"USB stack" does nothing like that. That is the UDC driver (and sometimes controller) who handles a transfer, transaction by transaction. Please change this comment to be just:
/*
* Limit the maximum allocation size to 1/4 of the UDC pool size
* to prevent starving other pool users. Ensure the maximum allocation
* never drops below USBD_MAX_BULK_MPS.
*/Please fix your commit message as well.
There was a problem hiding this comment.
Removed "USB stack" references. Left the higher reasoning, since it was not obvious to me as someone looking at the driver for the first time that the UDC request sizes don't need to match the MPS.
| default 512 if USBD_MAX_SPEED_HIGH | ||
| default 64 if USBD_MAX_SPEED_FULL |
There was a problem hiding this comment.
For the full speed, it defaults to the a size that is worse than using UDC pool. Perhaps the default should always be 512. And there should be a range 512 $(UINT16_MAX) below default 512 and
BUILD_ASSERT((CONFIG_USBD_CDC_ACM_BUF_POOL_SIZE % USBD_MAX_BULK_MPS) == 0, \
"USBD_CDC_ACM_BUF_POOL_SIZE is not multiple of bulk endpoint MPS");in usbd_cdc_acm.c.
There was a problem hiding this comment.
My intent here was not to change any behaviors, just make the values configurable from Kconfig. The pool was previously created with a size of USBD_MAX_BULK_MPS, defined as:
/* 1 if USB device stack is compiled with High-Speed support */
#define USBD_SUPPORTS_HIGH_SPEED IS_EQ(CONFIG_USBD_MAX_SPEED, 1)
/* Maximum bulk max packet size the stack supports */
#define USBD_MAX_BULK_MPS COND_CODE_1(USBD_SUPPORTS_HIGH_SPEED, (512), (64))
Unless I am misunderstanding something.
If the current values are not optimal, that can be a future PR IMO.
There was a problem hiding this comment.
I missed this originally, but why is there any requirement for the pool to be a multiple of the MPS size? The driver makes no attempt to ensure that only buffers with a multiple of MPS bytes are submitted.
| default 512 if USBD_MAX_SPEED_HIGH | ||
| default 64 if USBD_MAX_SPEED_FULL | ||
| help | ||
| Increasing the size of buffers past the endpoint size can reduce the |
There was a problem hiding this comment.
Please use endpoint MPS (maximum packet size) and change it to
Increasing the size of buffers to a multiple of endpoint MPS can reduce
the number of round trips through the CDC ACM class when the TX FIFO is
larger than the endpoint MPS, increasing throughput.
e88357a to
66fa5ca
Compare
Do not artificially limit the size of the USB request to the endpoint size, as this introduces multiple round trips through the CDC ACM class implementation. The lower USB layers handle packetizing to the maximum endpoint size. The maximum allocation size is limited to prevent starving other USB users. Throughput testing on a nRF52840DK with `CONFIG_UDC_BUF_POOL_SIZE=1024` shows a throughput increase from ~1.7 Mbps to ~2.7 Mbps. Signed-off-by: Jordan Yates <[email protected]>
When a custom pool is used for CDC ACM, allow the buffer sizes to be configured in Kconfig. Signed-off-by: Jordan Yates <[email protected]>
JordanYates
force-pushed
the
251122_cdc_acm_throughput
branch
from
66fa5ca to
af4ebe9
Compare
|
3 participants
Do not artificially limit the size of the USB request to the endpoint MPS, as this forces multiple round trips through the CDC ACM class implementation when the TX FIFO is larger than the endpoint MPS. The lower USB layers handles packetizing to the maximum endpoint size. The maximum allocation size is limited to prevent starving other USB users.
When a custom pool is used for CDC ACM, allow the buffer sizes to be configured in Kconfig.
Throughput testing on a nRF52840DK with
CONFIG_UDC_BUF_POOL_SIZE=1024shows a throughput increase from ~1.7 Mbps to ~2.7 Mbps.Steps towards improving the issue of #99779, but this is only the absolute lowest hanging fruit).
Throughput tested on the reproducer steps from the linked issue.
The limit of
CONFIG_UDC_BUF_POOL_SIZE / 4was picked out of thin air as a reasonable default, happy to hear arguments against.