Skip to content

usb: device_next: class: cdc_acm: throughput improvements (low hanging fruit) #99838

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+31 −6
Open

usb: device_next: class: cdc_acm: throughput improvements (low hanging fruit) #99838

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
4d0934c
usb: device_next: class: cdc_acm: batch writes
JordanYates Nov 22, 2025
af4ebe9
usb: device_next: class: cdc_acm: configurable buffer size
JordanYates Nov 22, 2025
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
10 changes: 10 additions & 0 deletions subsys/usb/device_next/class/Kconfig.cdc_acm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,16 @@ config USBD_CDC_ACM_BUF_POOL
ACM instances and the size of the bulk endpoints. When disabled, the
implementation uses the UDC driver's pool.

config USBD_CDC_ACM_BUF_POOL_SIZE
int "Size of the buffers in the dedicated buffer pool"
depends on USBD_CDC_ACM_BUF_POOL
default 512 if USBD_MAX_SPEED_HIGH
default 64 if USBD_MAX_SPEED_FULL
Comment on lines +43 to +44
Copy link
Contributor

@jfischer-no jfischer-no Nov 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@JordanYates JordanYates Nov 25, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@JordanYates JordanYates Nov 25, 2025

Choose a reason for hiding this comment

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

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.

help
Increasing the size of buffers past the 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.

module = USBD_CDC_ACM
module-str = usbd cdc_acm
default-count = 1
Expand Down
27 changes: 21 additions & 6 deletions subsys/usb/device_next/class/usbd_cdc_acm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,12 +132,14 @@ static void cdc_acm_irq_rx_enable(const struct device *dev);
#if CONFIG_USBD_CDC_ACM_BUF_POOL
UDC_BUF_POOL_DEFINE(cdc_acm_ep_pool,
DT_NUM_INST_STATUS_OKAY(DT_DRV_COMPAT) * 2,
USBD_MAX_BULK_MPS, sizeof(struct udc_buf_info), NULL);
CONFIG_USBD_CDC_ACM_BUF_POOL_SIZE, sizeof(struct udc_buf_info),
NULL);

static struct net_buf *cdc_acm_buf_alloc(struct usbd_class_data *const c_data,
const uint8_t ep)
const uint8_t ep, const size_t size_hint)
{
ARG_UNUSED(c_data);
ARG_UNUSED(size_hint);
struct net_buf *buf = NULL;
struct udc_buf_info *bi;

Expand All @@ -157,9 +159,21 @@ static struct net_buf *cdc_acm_buf_alloc(struct usbd_class_data *const c_data,
* common (UDC) buffer, as this results in a smaller footprint.
*/
static struct net_buf *cdc_acm_buf_alloc(struct usbd_class_data *const c_data,
const uint8_t ep)
const uint8_t ep, const size_t size_hint)
{
return usbd_ep_buf_alloc(c_data, ep, USBD_MAX_BULK_MPS);
/* The lower layers handle packetizing to the endpoint MPS, therefore
* allocating buffers largers than the endpoint MPS reduces the number
* of round trips through this class driver when the TX FIFO size is
* larger than the endpoint MPS, 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.
*/
size_t max_alloc = max(USBD_MAX_BULK_MPS, CONFIG_UDC_BUF_POOL_SIZE / 4);
size_t alloc_size = min(max_alloc, size_hint);

return usbd_ep_buf_alloc(c_data, ep, alloc_size);
}
#endif /* CONFIG_USBD_CDC_ACM_BUF_POOL */

Expand Down Expand Up @@ -651,7 +665,8 @@ static void cdc_acm_tx_fifo_handler(struct k_work *work)
return;
}

buf = cdc_acm_buf_alloc(c_data, cdc_acm_get_bulk_in(c_data));
buf = cdc_acm_buf_alloc(c_data, cdc_acm_get_bulk_in(c_data),
ring_buf_size_get(data->tx_fifo.rb));
if (buf == NULL) {
atomic_clear_bit(&data->state, CDC_ACM_TX_FIFO_BUSY);
cdc_acm_work_schedule(&data->tx_fifo_work, K_MSEC(1));
Expand Down Expand Up @@ -707,7 +722,7 @@ static void cdc_acm_rx_fifo_handler(struct k_work *work)
return;
}

buf = cdc_acm_buf_alloc(c_data, cdc_acm_get_bulk_out(c_data));
buf = cdc_acm_buf_alloc(c_data, cdc_acm_get_bulk_out(c_data), USBD_MAX_BULK_MPS);
if (buf == NULL) {
return;
}
Expand Down