| Message ID | 1418223122-22481-5-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 10.12.2014 um 15:52 hat Paolo Bonzini geschrieben: > There is no need to do another O(n) pass on the list; the iocb to > splice the list at is already available in the array we passed to > io_submit. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/linux-aio.c | 12 ++++++------ > include/qemu/queue.h | 11 +++++++++++ > 2 files changed, 17 insertions(+), 6 deletions(-) > > diff --git a/block/linux-aio.c b/block/linux-aio.c > index b223d9e..6c98f72 100644 > --- a/block/linux-aio.c > +++ b/block/linux-aio.c > @@ -186,9 +186,10 @@ static void ioq_init(LaioQueue *io_q) > > static int ioq_submit(struct qemu_laio_state *s) > { > - int ret, i, len; > + int ret, len; > struct qemu_laiocb *aiocb; > struct iocb *iocbs[MAX_QUEUED_IO]; > + QSIMPLEQ_HEAD(, qemu_laiocb) completed; > > do { > len = 0; > @@ -201,16 +202,15 @@ static int ioq_submit(struct qemu_laio_state *s) > > ret = io_submit(s->ctx, len, iocbs); > if (ret == -EAGAIN) { > - ret = 0; > + break; > } ioq_submit() returns -EAGAIN now instead of 0. Almost all callers ignore the return value, except laio_io_unplug(), which inherits this change. Fortunately, raw_aio_unplug() completely ignores the return value. So seems that this didn't cause a bug, but we have some cleanup to do and make functions void if their return value isn't used anywhere. > if (ret < 0) { > abort(); > } > > - for (i = 0; i < ret; i++) { > - s->io_q.n--; > - QSIMPLEQ_REMOVE_HEAD(&s->io_q.pending, next); > - } > + s->io_q.n -= ret; > + aiocb = container_of(iocbs[ret - 1], struct qemu_laiocb, iocb); > + QSIMPLEQ_SPLIT_AFTER(&completed, &s->io_q.pending, aiocb, next); > } while (ret == len && !QSIMPLEQ_EMPTY(&s->io_q.pending)); > s->io_q.blocked = (s->io_q.n > 0); > > diff --git a/include/qemu/queue.h b/include/qemu/queue.h > index 0dedd29..2c21d28 100644 > --- a/include/qemu/queue.h > +++ b/include/qemu/queue.h > @@ -279,6 +279,17 @@ struct { \ > (head)->sqh_last = &(head)->sqh_first; \ > } while (/*CONSTCOND*/0) > > +#define QSIMPLEQ_SPLIT_AFTER(head1, head2, elm, field) do { \ > + if (((head1)->sqh_first = (head2)->sqh_first) == NULL) { \ > + (head1)->sqh_last = &(head1)->sqh_first; \ > + } else { \ > + (head1)->sqh_last = &(elm)->field.sqe_next; \ > + if (((head2)->sqh_first = (elm)->field.sqe_next) == NULL) { \ > + (head2)->sqh_last = &(head2)->sqh_first; \ > + } \ > + } \ > +} while (/*CONSTCOND*/0) Wouldn't it be easier to write a macro that doesn't split a queue in two, but simply removes everything up to a given element? Anyway, if you really want to implement a split operation, I think you also need to break the actual chain, i.e. (head1)->sqh_last.sqe_next = NULL. Kevin
On 11/12/2014 14:13, Kevin Wolf wrote: >> > +#define QSIMPLEQ_SPLIT_AFTER(head1, head2, elm, field) do { \ >> > + if (((head1)->sqh_first = (head2)->sqh_first) == NULL) { \ >> > + (head1)->sqh_last = &(head1)->sqh_first; \ >> > + } else { \ >> > + (head1)->sqh_last = &(elm)->field.sqe_next; \ >> > + if (((head2)->sqh_first = (elm)->field.sqe_next) == NULL) { \ >> > + (head2)->sqh_last = &(head2)->sqh_first; \ >> > + } \ >> > + } \ >> > +} while (/*CONSTCOND*/0) > Wouldn't it be easier to write a macro that doesn't split a queue in > two, but simply removes everything up to a given element? Yeah, though I figured that in the common case you'd have to free those elements or otherwise process them. But I left this patch last because I wasn't sure about the API. Feel free to ignore it, also given the above comment about EAGAIN. Paolo > Anyway, if you really want to implement a split operation, I think you > also need to break the actual chain, i.e. (head1)->sqh_last.sqe_next = > NULL.
Am 11.12.2014 um 14:15 hat Paolo Bonzini geschrieben: > > > On 11/12/2014 14:13, Kevin Wolf wrote: > >> > +#define QSIMPLEQ_SPLIT_AFTER(head1, head2, elm, field) do { \ > >> > + if (((head1)->sqh_first = (head2)->sqh_first) == NULL) { \ > >> > + (head1)->sqh_last = &(head1)->sqh_first; \ > >> > + } else { \ > >> > + (head1)->sqh_last = &(elm)->field.sqe_next; \ > >> > + if (((head2)->sqh_first = (elm)->field.sqe_next) == NULL) { \ > >> > + (head2)->sqh_last = &(head2)->sqh_first; \ > >> > + } \ > >> > + } \ > >> > +} while (/*CONSTCOND*/0) > > Wouldn't it be easier to write a macro that doesn't split a queue in > > two, but simply removes everything up to a given element? > > Yeah, though I figured that in the common case you'd have to free those > elements or otherwise process them. But I left this patch last because > I wasn't sure about the API. Feel free to ignore it, also given the > above comment about EAGAIN. I actually like the idea of this patch. > > Anyway, if you really want to implement a split operation, I think you > > also need to break the actual chain, i.e. (head1)->sqh_last.sqe_next = > > NULL. Can you please fix this one and send a v2? The EAGAIN thing doesn't need to be fixed because it's ignored anyway. A cleanup is unrelated and can be done later. As for the abort() in patch 2, I'll leave the decision to you. Overall it looks like a nice series. Kevin
On 11/12/2014 14:22, Kevin Wolf wrote: > Can you please fix this one and send a v2? Done. > The EAGAIN thing doesn't need to be fixed because it's ignored anyway. A > cleanup is unrelated and can be done later. As for the abort() in patch > 2, I'll leave the decision to you. Executive summary: did the cleanup now, and left the abort() in. Paolo
diff --git a/block/linux-aio.c b/block/linux-aio.c index b223d9e..6c98f72 100644 --- a/block/linux-aio.c +++ b/block/linux-aio.c @@ -186,9 +186,10 @@ static void ioq_init(LaioQueue *io_q) static int ioq_submit(struct qemu_laio_state *s) { - int ret, i, len; + int ret, len; struct qemu_laiocb *aiocb; struct iocb *iocbs[MAX_QUEUED_IO]; + QSIMPLEQ_HEAD(, qemu_laiocb) completed; do { len = 0; @@ -201,16 +202,15 @@ static int ioq_submit(struct qemu_laio_state *s) ret = io_submit(s->ctx, len, iocbs); if (ret == -EAGAIN) { - ret = 0; + break; } if (ret < 0) { abort(); } - for (i = 0; i < ret; i++) { - s->io_q.n--; - QSIMPLEQ_REMOVE_HEAD(&s->io_q.pending, next); - } + s->io_q.n -= ret; + aiocb = container_of(iocbs[ret - 1], struct qemu_laiocb, iocb); + QSIMPLEQ_SPLIT_AFTER(&completed, &s->io_q.pending, aiocb, next); } while (ret == len && !QSIMPLEQ_EMPTY(&s->io_q.pending)); s->io_q.blocked = (s->io_q.n > 0); diff --git a/include/qemu/queue.h b/include/qemu/queue.h index 0dedd29..2c21d28 100644 --- a/include/qemu/queue.h +++ b/include/qemu/queue.h @@ -279,6 +279,17 @@ struct { \ (head)->sqh_last = &(head)->sqh_first; \ } while (/*CONSTCOND*/0) +#define QSIMPLEQ_SPLIT_AFTER(head1, head2, elm, field) do { \ + if (((head1)->sqh_first = (head2)->sqh_first) == NULL) { \ + (head1)->sqh_last = &(head1)->sqh_first; \ + } else { \ + (head1)->sqh_last = &(elm)->field.sqe_next; \ + if (((head2)->sqh_first = (elm)->field.sqe_next) == NULL) { \ + (head2)->sqh_last = &(head2)->sqh_first; \ + } \ + } \ +} while (/*CONSTCOND*/0) + #define QSIMPLEQ_REMOVE(head, elm, type, field) do { \ if ((head)->sqh_first == (elm)) { \ QSIMPLEQ_REMOVE_HEAD((head), field); \
There is no need to do another O(n) pass on the list; the iocb to splice the list at is already available in the array we passed to io_submit. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- block/linux-aio.c | 12 ++++++------ include/qemu/queue.h | 11 +++++++++++ 2 files changed, 17 insertions(+), 6 deletions(-)