On Thu, Jan 30 2014, Robert Baldyga wrote:
> diff --git a/tools/usb/aio_multibuff/device_app/aio_multibuff.c b/tools/usb/aio_multibuff/device_app/aio_multibuff.c
> +static void display_event(struct usb_functionfs_event *event)
> +{
> + static const char *const names[] = {
> + [FUNCTIONFS_BIND] = "BIND",
> + [FUNCTIONFS_UNBIND] = "UNBIND",
> + [FUNCTIONFS_ENABLE] = "ENABLE",
> + [FUNCTIONFS_DISABLE] = "DISABLE",
> + [FUNCTIONFS_SETUP] = "SETUP",
> + [FUNCTIONFS_SUSPEND] = "SUSPEND",
> + [FUNCTIONFS_RESUME] = "RESUME",
> + };
> + switch (event->type) {
> + case FUNCTIONFS_BIND:
> + case FUNCTIONFS_UNBIND:
> + case FUNCTIONFS_ENABLE:
> + case FUNCTIONFS_DISABLE:
> + case FUNCTIONFS_SETUP:
> + case FUNCTIONFS_SUSPEND:
> + case FUNCTIONFS_RESUME:
> + printf("Event %s\n", names[event->type]);
> + default:
> + break;
The default case serves no purpose here.
> + }
> +}
> +struct io_buffer {
> + struct iocb **iocb;
I'm wondering whether it would be easier to just declare it as:
+ struct iocb **iocb;
and then allocate the structures as a single flat array rather than
array of pointers to the structures. I can see why you might prefer not
to do that with “buf”, but struct iocb should not be that big, and
having one indirection less should simplify the code.
> + unsigned char **buf;
> + int cnt;
> + int len;
Make those unsigned. Add:
+ unsigned requested;
> +};
> +
> +void init_bufs(struct io_buffer *iobuf, int n, int len)
Make those unsigned.
> +{
> + int i;
unsigned
> + iobuf->buf = malloc(n*sizeof(*iobuf->buf));
> + iobuf->iocb = malloc(n*sizeof(*iobuf->iocb));
> + iobuf->cnt = n;
> + iobuf->len = len;
> + for (i = 0; i < n; ++i) {
> + iobuf->buf[i] = malloc(len*sizeof(**iobuf->buf));
> + iobuf->iocb[i] = malloc(sizeof(**iobuf->iocb));
> + }
+ iobuf->requested = 0;
> +}
> +
> +void delete_bufs(struct io_buffer *iobuf)
> +{
> + int i;
> + for (i = 0; i < iobuf->cnt; ++i) {
> + free(iobuf->buf[i]);
> + free(iobuf->iocb[i]);
> + }
> + free(iobuf->buf);
> + free(iobuf->iocb);
> +}
> +
> +#define BUF_LEN 8192
> +#define BUFS_MAX 128
> +#define AIO_MAX (BUFS_MAX*2)
> +
> +int main(int argc, char *argv[])
> +{
> + int i, ret;
> + char *ep_path;
> +
> + int ep0, ep1;
> +
> + io_context_t ctx;
> +
> + struct io_buffer iobuf1, iobuf2;
+ struct io_buffer iobuf[2];
But to be honest, why are there two of those anyway? Each have a bunch
of buffers so why multiply number of buffers even more?
> + int requested1 = 0, requested2 = 0;
> + int actual;
> + bool ready;
> +
> + if (argc != 2) {
> + printf("ffs directory not specified!\n");
> + return 1;
> + }
> +
> + ep_path = malloc(strlen(argv[1]) + 4 /* "/ep#" */ + 1 /* '\0' */);
> + if (!ep_path) {
> + perror("malloc");
> + return 1;
> + }
> +
> + /* open endpoint files */
> + sprintf(ep_path, "%s/ep0", argv[1]);
> + ep0 = open(ep_path, O_RDWR);
> + if (ep0 < 0) {
> + perror("unable to open ep0");
> + return 1;
> + }
> + if (write(ep0, &descriptors, sizeof(descriptors)) < 0) {
> + perror("unable do write descriptors");
> + return 1;
> + }
> + if (write(ep0, &strings, sizeof(strings)) < 0) {
> + perror("unable to write strings");
> + return 1;
> + }
> + sprintf(ep_path, "%s/ep1", argv[1]);
> + ep1 = open(ep_path, O_RDWR);
> + if (ep1 < 0) {
> + perror("unable to open ep1");
> + return 1;
> + }
> +
> + free(ep_path);
> +
> + memset(&ctx, 0, sizeof(ctx));
> + /* setup aio context to handle up to AIO_MAX requests */
> + if (io_setup(AIO_MAX, &ctx) < 0) {
> + perror("unable to setup aio");
> + return 1;
> + }
> +
> + init_bufs(&iobuf1, BUFS_MAX, BUF_LEN);
> + init_bufs(&iobuf2, BUFS_MAX, BUF_LEN);
+ for (i = 0; i < sizeof iobuf / sizeof *iobuf; ++i)
+ init_bufs(iobuf + i, BUFS_MAX, BUF_LEN);
> +
> + while (1) {
> + handle_ep0(ep0, &ready);
> + /* we are waiting for function ENABLE */
> + if (!ready)
> + continue;
> + /*
> + * when we're preparing new data to submit,
> + * second buffer being transmitted
> + */
> + if (!requested1) { /* if all req's from iocb1 completed */
> + actual = 2;
> + for (i = 0; i < iobuf1.cnt; ++i) /* prepare requests */
> + io_prep_pwrite(iobuf1.iocb[i], ep1,
> + iobuf1.buf[i], iobuf1.len, 0);
> + /* submit table of requests */
> + ret = io_submit(ctx, iobuf1.cnt, iobuf1.iocb);
> + if (ret >= 0) {
> + requested1 = ret;
> + printf("submit: %d requests from buf 1\n", ret);
> + } else
> + perror("unable to submit reqests for buf2");
> + }
> + if (!requested2) { /* if all req's from iocb2 completed */
> + actual = 1;
> + for (i = 0; i < iobuf2.cnt; ++i) /* prepare requests */
> + io_prep_pwrite(iobuf2.iocb[i], ep1,
> + iobuf2.buf[i], iobuf2.len, 0);
> + /* submit table of requests */
> + ret = io_submit(ctx, iobuf2.cnt, iobuf2.iocb);
> + if (ret >= 0) {
> + requested2 = ret;
> + printf("submit: %d requests from buf 2\n", ret);
> + } else
> + perror("unable to submit reqests for buf2");
> + }
+ for (i = 0; i < sizeof iobuf / sizeof *iobuf; ++i) {
+ if (iobuf[i].requested)
+ continue;
+ …
+ }
This will avoid duplication of code.
> + /* if something was submitted we wait for event */
> + if (requested1 || requested2) {
> + struct io_event e;
> + struct timespec timeout = {0, 1000};
> + /* we wait for one event */
> + ret = io_getevents(ctx, 1, 1, &e, &timeout);
This is not a good way to do this. You are looping around with
1 µseconds delays in each loop. This is active polling and is not a way
to implement anything. To properly use AIO with a single thread while
you also handling ep0, you need to use signals and ppoll. In pseudo
code:
1. Set up ep0, open up ep1.
1. Set up SIGUSR1 handler to do nothing.
2. Set up signal mask such that SIGUSR1 is blocked.
3. Schedule transfers to ep1 if needed.
4. Use ppoll with sigmask where SIGUSR1 is not blocked.
5a. If ppoll returned SIGINTR io_getevents with a zero timeout.
5b. If ppoll returned > 0, process ep0.
5c. If ppoll returned < 0, exit with an error.
6. Go to step 3.
Useful reading:
* man ppoll
* man sigevent
* man sigprocmask
You can find a bit of code that uses this technique at line 95 of:
https://github.com/mina86/p2p-chat/blob/master/src/application.cpp
> + if (ret > 0) { /* if we got event */
> + if (actual == 1)
> + requested1--;
> + else
> + requested2--;
> + }
> + }
> + }
> +
> + /* free resources */
> +
> + delete_bufs(&iobuf1);
> + delete_bufs(&iobuf2);
> + io_destroy(ctx);
> +
> + close(ep1);
> + close(ep0);
> +
> + return 0;
> +}
--
Best regards, _ _
.o. | Liege of Serenely Enlightened Majesty of o' \,=./ `o
..o | Computer Science, Michał “mina86” Nazarewicz (o o)
ooo +--<mpn@xxxxxxxxxx>--<xmpp:mina86@xxxxxxxxxx>--ooO--(_)--Ooo--
Attachment:
signature.asc
Description: PGP signature
