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 <stefa...@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 <stefa...@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
