On 01/30/2014 03:09 PM, Michal Nazarewicz wrote:
> 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.
>
It looks like AIO API needs this. Last parameter of io_submit is struct
iocb **, and it expects it will be pointer to array of pointers to
struct iocb.
>> + 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?
>
It can be useful for multibuffering, when you need to use huge buffers
(for example when you want to send video data). Then you need to split
your buffer into many small buffers of size up to endpoint maxpacket
value (it does not apply to bulk transfers, but it's necessary when you
use isochronous mode). So this example shows how to do it.
>> + 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
>
This code is an example. Don't you think it would be better to keep it
simple?
>> + 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;
>> +}
>
Thanks for help :)
Best regards
Robert Baldyga
Samsung R&D Institute Poland
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
