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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Fri, Dec 07, 2012 at 03:21:22PM +0100, Kevin Wolf wrote:
> 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.

The ioq is sized so that guest cannot submit more than max_reqs due to
vring size.  Therefore this cannot happen and I have changed them to
asserts.

Stefan