Re: [Qemu-devel] [PATCH v5 06/11] dataplane: add Linux AIO request queue

[Kevin Wolf <kwolf@redhat.com>]

Am 05.12.2012 21:47, schrieb Stefan Hajnoczi:
> The IOQueue has a pool of iocb structs and a function to add new
> read/write requests.  Multiple requests can be added before calling the
> submit function to actually tell the host kernel to begin I/O.  This
> allows callers to batch requests and submit them in one go.
> 
> The actual I/O is performed using Linux AIO.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  hw/dataplane/Makefile.objs |   2 +-
>  hw/dataplane/ioq.c         | 118 +++++++++++++++++++++++++++++++++++++++++++++
>  hw/dataplane/ioq.h         |  57 ++++++++++++++++++++++
>  3 files changed, 176 insertions(+), 1 deletion(-)
>  create mode 100644 hw/dataplane/ioq.c
>  create mode 100644 hw/dataplane/ioq.h
> 
> diff --git a/hw/dataplane/Makefile.objs b/hw/dataplane/Makefile.objs
> index e26bd7d..abd408f 100644
> --- a/hw/dataplane/Makefile.objs
> +++ b/hw/dataplane/Makefile.objs
> @@ -1,3 +1,3 @@
>  ifeq ($(CONFIG_VIRTIO), y)
> -common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += hostmem.o vring.o event-poll.o
> +common-obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += hostmem.o vring.o event-poll.o ioq.o
>  endif
> diff --git a/hw/dataplane/ioq.c b/hw/dataplane/ioq.c
> new file mode 100644
> index 0000000..7adeb5d
> --- /dev/null
> +++ b/hw/dataplane/ioq.c
> @@ -0,0 +1,118 @@
> +/*
> + * Linux AIO request queue
> + *
> + * Copyright 2012 IBM, Corp.
> + * Copyright 2012 Red Hat, Inc. and/or its affiliates
> + *
> + * Authors:
> + *   Stefan Hajnoczi <stefanha@redhat.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "hw/dataplane/ioq.h"
> +
> +void ioq_init(IOQueue *ioq, int fd, unsigned int max_reqs)
> +{
> +    int rc;
> +
> +    ioq->fd = fd;
> +    ioq->max_reqs = max_reqs;
> +
> +    memset(&ioq->io_ctx, 0, sizeof ioq->io_ctx);
> +    rc = io_setup(max_reqs, &ioq->io_ctx);
> +    if (rc != 0) {
> +        fprintf(stderr, "ioq io_setup failed %d\n", rc);
> +        exit(1);
> +    }
> +
> +    rc = event_notifier_init(&ioq->io_notifier, 0);
> +    if (rc != 0) {
> +        fprintf(stderr, "ioq io event notifier creation failed %d\n", rc);
> +        exit(1);
> +    }
> +
> +    ioq->freelist = g_malloc0(sizeof ioq->freelist[0] * max_reqs);
> +    ioq->freelist_idx = 0;
> +
> +    ioq->queue = g_malloc0(sizeof ioq->queue[0] * max_reqs);
> +    ioq->queue_idx = 0;
> +}
> +
> +void ioq_cleanup(IOQueue *ioq)
> +{
> +    g_free(ioq->freelist);
> +    g_free(ioq->queue);
> +
> +    event_notifier_cleanup(&ioq->io_notifier);
> +    io_destroy(ioq->io_ctx);
> +}
> +
> +EventNotifier *ioq_get_notifier(IOQueue *ioq)
> +{
> +    return &ioq->io_notifier;
> +}
> +
> +struct iocb *ioq_get_iocb(IOQueue *ioq)
> +{
> +    if (unlikely(ioq->freelist_idx == 0)) {
> +        fprintf(stderr, "ioq underflow\n");
> +        exit(1);
> +    }

Can this happen? If no, it should be an assertion. If yes, the error
handling code is wrong, we can't just exit qemu. It's already not nice
to do it in setup functions, but during runtime I think it's not acceptable.

> +    struct iocb *iocb = ioq->freelist[--ioq->freelist_idx];
> +    ioq->queue[ioq->queue_idx++] = iocb;
> +    return iocb;
> +}
> +
> +void ioq_put_iocb(IOQueue *ioq, struct iocb *iocb)
> +{
> +    if (unlikely(ioq->freelist_idx == ioq->max_reqs)) {
> +        fprintf(stderr, "ioq overflow\n");
> +        exit(1);
> +    }

Same here.

Kevin