Skip to content

Commit

Permalink
usb: Allow overriding bulk write size and increase default
Browse files Browse the repository at this point in the history
Since commit 760b3df ("qdl: Rework qdl_write to limit write sizes
to out_maxpktsize") outgoing transfers has been chunked into blocks of
wMaxPacketSize.

As reported by Wiktor Drewniak, Maksim Paimushkin, and Luca Weiss in [1]
there's huge benefits to be found in reverting this change, but out of
caution the limit was untouched.

With the transition to libusb, the throughput dropped another ~15% on my
machine. The numbers for HighSpeed and SuperSpeed are also in the same
ballpark.

With SuperSpeed, benchmarking of different chunk sizes in the megabyte
range shows improvement over these numbers in the range of 15-20x on the
same machine/board combination.

The bug report related to the reduction in size describes a host machine
running out of swiotlb, from the fact that we requested 1MB physically
contiguous memory. It's unclear if attempts was made to correct the
kernel's behavior. libusb does inquiry the capabilities of the kernel
and will split the buffer and submitting multiple URBs at once, as
needed.  While no definitive guidance has been found, multiple sources
seems to recommend passing 1-2MB of buffer to libusb at a time.  So,
let's move the default chunk size back up to 1MB and hope that libusb
resolves the reported problem.

Additionally, introduce a new option --out-chunk-size, which allow the
user to override the chunk size. This would allow any user to reduce the
size if needed, but also allow the value to be increased as needed.

[1] #39

Reported-by: Wiktor Drewniak
Reported-by: Maksim Paimushkin
Reported-by: Luca Weiss
Signed-off-by: Bjorn Andersson <[email protected]>
  • Loading branch information
quic-bjorande authored and andersson committed Jun 10, 2024
1 parent a1cd535 commit cbd4618
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 2 deletions.
12 changes: 11 additions & 1 deletion qdl.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
#include <getopt.h>
#include <stdbool.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <libxml/parser.h>
#include <libxml/tree.h>
Expand Down Expand Up @@ -106,6 +107,10 @@ static void print_usage(void)
__progname);
}

enum {
OPT_OUT_CHUNK_SIZE = 1000,
};

int main(int argc, char **argv)
{
char *prog_mbn, *storage="ufs";
Expand All @@ -115,12 +120,13 @@ int main(int argc, char **argv)
int ret;
int opt;
bool qdl_finalize_provisioning = false;

long out_chunk_size;

static struct option options[] = {
{"debug", no_argument, 0, 'd'},
{"include", required_argument, 0, 'i'},
{"finalize-provisioning", no_argument, 0, 'l'},
{"out-chunk-size", required_argument, 0, OPT_OUT_CHUNK_SIZE },
{"serial", required_argument, 0, 'S'},
{"storage", required_argument, 0, 's'},
{0, 0, 0, 0}
Expand All @@ -137,6 +143,10 @@ int main(int argc, char **argv)
case 'l':
qdl_finalize_provisioning = true;
break;
case OPT_OUT_CHUNK_SIZE:
out_chunk_size = strtol(optarg, NULL, 10);
qdl_set_out_chunk_size(&qdl, out_chunk_size);
break;
case 's':
storage = optarg;
break;
Expand Down
2 changes: 2 additions & 0 deletions qdl.h
Original file line number Diff line number Diff line change
Expand Up @@ -21,13 +21,15 @@ struct qdl_device {

size_t in_maxpktsize;
size_t out_maxpktsize;
size_t out_chunk_size;

char *mappings[MAPPING_SZ]; // array index is the id from the device
};

int qdl_open(struct qdl_device *qdl, const char *serial);
int qdl_read(struct qdl_device *qdl, void *buf, size_t len, unsigned int timeout);
int qdl_write(struct qdl_device *qdl, const void *buf, size_t len);
void qdl_set_out_chunk_size(struct qdl_device *qdl, long size);

int firehose_run(struct qdl_device *qdl, const char *incdir, const char *storage);
int sahara_run(struct qdl_device *qdl, char *img_arr[], bool single_image,
Expand Down
23 changes: 22 additions & 1 deletion usb.c
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@

#include "qdl.h"

#define DEFAULT_OUT_CHUNK_SIZE (1024 * 1024)

/*
* libusb commit f0cce43f882d ("core: Fix definition and use of enum
* libusb_transfer_type") split transfer type and endpoint transfer types.
Expand Down Expand Up @@ -141,6 +143,20 @@ static int qdl_try_open(libusb_device *dev, struct qdl_device *qdl, const char *
qdl->in_maxpktsize = in_size;
qdl->out_maxpktsize = out_size;

if (qdl->out_chunk_size && qdl->out_chunk_size % out_size) {
fprintf(stderr,
"WARNING: requested out-chunk-size must be multiple of the device's wMaxPacketSize %ld, using %ld\n",
out_size, out_size);
qdl->out_chunk_size = out_size;
} else if (!qdl->out_chunk_size) {
qdl->out_chunk_size = DEFAULT_OUT_CHUNK_SIZE;
}

if (qdl_debug) {
fprintf(stderr, "USB: using out-chunk-size of %ld\n",
qdl->out_chunk_size);
}

return 1;
}

Expand Down Expand Up @@ -215,7 +231,7 @@ int qdl_write(struct qdl_device *qdl, const void *buf, size_t len)
int ret;

while (len > 0) {
xfer = (len > qdl->out_maxpktsize) ? qdl->out_maxpktsize : len;
xfer = (len > qdl->out_chunk_size) ? qdl->out_chunk_size : len;

ret = libusb_bulk_transfer(qdl->usb_handle, qdl->out_ep, data,
xfer, &actual, 1000);
Expand All @@ -239,3 +255,8 @@ int qdl_write(struct qdl_device *qdl, const void *buf, size_t len)

return count;
}

void qdl_set_out_chunk_size(struct qdl_device *qdl, long size)
{
qdl->out_chunk_size = size;
}

0 comments on commit cbd4618

Please sign in to comment.