| Message ID | 1361464195-4119-1-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Feb 21, 2013 at 05:29:55PM +0100, Paolo Bonzini wrote: > During the review of the dataplane code, the EventPoll API morphed itself > (not concidentially) into something very very similar to an AioContext. > Thus, it is trivial to convert virtio-blk-dataplane to use AioContext, > and a first baby step towards letting dataplane talk directly to the > QEMU block layer. > > The only interesting note is the value-copy of EventNotifiers. At least > in my opinion this is part of the EventNotifier API and is even portable > to Windows. Of course, in this case you should not close the notifier's > underlying file descriptors or handle with event_notifier_cleanup. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/dataplane/Makefile.objs | 2 +- > hw/dataplane/event-poll.c | 100 --------------------------------------------- > hw/dataplane/event-poll.h | 40 ------------------ > hw/dataplane/virtio-blk.c | 41 ++++++++++--------- > 4 files changed, 22 insertions(+), 161 deletions(-) > delete mode 100644 hw/dataplane/event-poll.c > delete mode 100644 hw/dataplane/event-poll.h > > diff --git a/hw/dataplane/Makefile.objs b/hw/dataplane/Makefile.objs > index 3e47d05..701111c 100644 > --- a/hw/dataplane/Makefile.objs > +++ b/hw/dataplane/Makefile.objs > @@ -1 +1 @@ > -obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += hostmem.o vring.o event-poll.o ioq.o virtio-blk.o > +obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += hostmem.o vring.o ioq.o virtio-blk.o > diff --git a/hw/dataplane/event-poll.c b/hw/dataplane/event-poll.c > deleted file mode 100644 > index 2b55c6e..0000000 > --- a/hw/dataplane/event-poll.c > +++ /dev/null > @@ -1,100 +0,0 @@ > -/* > - * Event loop with file descriptor polling > - * > - * 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 <sys/epoll.h> > -#include "hw/dataplane/event-poll.h" > - > -/* Add an event notifier and its callback for polling */ > -void event_poll_add(EventPoll *poll, EventHandler *handler, > - EventNotifier *notifier, EventCallback *callback) > -{ > - struct epoll_event event = { > - .events = EPOLLIN, > - .data.ptr = handler, > - }; > - handler->notifier = notifier; > - handler->callback = callback; > - if (epoll_ctl(poll->epoll_fd, EPOLL_CTL_ADD, > - event_notifier_get_fd(notifier), &event) != 0) { > - fprintf(stderr, "failed to add event handler to epoll: %m\n"); > - exit(1); > - } > -} > - > -/* Event callback for stopping event_poll() */ > -static void handle_stop(EventHandler *handler) > -{ > - /* Do nothing */ > -} > - > -void event_poll_init(EventPoll *poll) > -{ > - /* Create epoll file descriptor */ > - poll->epoll_fd = epoll_create1(EPOLL_CLOEXEC); > - if (poll->epoll_fd < 0) { > - fprintf(stderr, "epoll_create1 failed: %m\n"); > - exit(1); > - } > - > - /* Set up stop notifier */ > - if (event_notifier_init(&poll->stop_notifier, 0) < 0) { > - fprintf(stderr, "failed to init stop notifier\n"); > - exit(1); > - } > - event_poll_add(poll, &poll->stop_handler, > - &poll->stop_notifier, handle_stop); > -} > - > -void event_poll_cleanup(EventPoll *poll) > -{ > - event_notifier_cleanup(&poll->stop_notifier); > - close(poll->epoll_fd); > - poll->epoll_fd = -1; > -} > - > -/* Block until the next event and invoke its callback */ > -void event_poll(EventPoll *poll) > -{ > - EventHandler *handler; > - struct epoll_event event; > - int nevents; > - > - /* Wait for the next event. Only do one event per call to keep the > - * function simple, this could be changed later. */ > - do { > - nevents = epoll_wait(poll->epoll_fd, &event, 1, -1); > - } while (nevents < 0 && errno == EINTR); > - if (unlikely(nevents != 1)) { > - fprintf(stderr, "epoll_wait failed: %m\n"); > - exit(1); /* should never happen */ > - } > - > - /* Find out which event handler has become active */ > - handler = event.data.ptr; > - > - /* Clear the eventfd */ > - event_notifier_test_and_clear(handler->notifier); Wouldn't we need to move this into the handle_io/handle_notify to maintain the old behavior?
Il 21/02/2013 22:32, mdroth ha scritto: > On Thu, Feb 21, 2013 at 05:29:55PM +0100, Paolo Bonzini wrote: >> During the review of the dataplane code, the EventPoll API morphed itself >> (not concidentially) into something very very similar to an AioContext. >> Thus, it is trivial to convert virtio-blk-dataplane to use AioContext, >> and a first baby step towards letting dataplane talk directly to the >> QEMU block layer. >> >> The only interesting note is the value-copy of EventNotifiers. At least >> in my opinion this is part of the EventNotifier API and is even portable >> to Windows. Of course, in this case you should not close the notifier's >> underlying file descriptors or handle with event_notifier_cleanup. >> >> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> >> --- >> hw/dataplane/Makefile.objs | 2 +- >> hw/dataplane/event-poll.c | 100 --------------------------------------------- >> hw/dataplane/event-poll.h | 40 ------------------ >> hw/dataplane/virtio-blk.c | 41 ++++++++++--------- >> 4 files changed, 22 insertions(+), 161 deletions(-) >> delete mode 100644 hw/dataplane/event-poll.c >> delete mode 100644 hw/dataplane/event-poll.h >> >> diff --git a/hw/dataplane/Makefile.objs b/hw/dataplane/Makefile.objs >> index 3e47d05..701111c 100644 >> --- a/hw/dataplane/Makefile.objs >> +++ b/hw/dataplane/Makefile.objs >> @@ -1 +1 @@ >> -obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += hostmem.o vring.o event-poll.o ioq.o virtio-blk.o >> +obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += hostmem.o vring.o ioq.o virtio-blk.o >> diff --git a/hw/dataplane/event-poll.c b/hw/dataplane/event-poll.c >> deleted file mode 100644 >> index 2b55c6e..0000000 >> --- a/hw/dataplane/event-poll.c >> +++ /dev/null >> @@ -1,100 +0,0 @@ >> -/* >> - * Event loop with file descriptor polling >> - * >> - * 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 <sys/epoll.h> >> -#include "hw/dataplane/event-poll.h" >> - >> -/* Add an event notifier and its callback for polling */ >> -void event_poll_add(EventPoll *poll, EventHandler *handler, >> - EventNotifier *notifier, EventCallback *callback) >> -{ >> - struct epoll_event event = { >> - .events = EPOLLIN, >> - .data.ptr = handler, >> - }; >> - handler->notifier = notifier; >> - handler->callback = callback; >> - if (epoll_ctl(poll->epoll_fd, EPOLL_CTL_ADD, >> - event_notifier_get_fd(notifier), &event) != 0) { >> - fprintf(stderr, "failed to add event handler to epoll: %m\n"); >> - exit(1); >> - } >> -} >> - >> -/* Event callback for stopping event_poll() */ >> -static void handle_stop(EventHandler *handler) >> -{ >> - /* Do nothing */ >> -} >> - >> -void event_poll_init(EventPoll *poll) >> -{ >> - /* Create epoll file descriptor */ >> - poll->epoll_fd = epoll_create1(EPOLL_CLOEXEC); >> - if (poll->epoll_fd < 0) { >> - fprintf(stderr, "epoll_create1 failed: %m\n"); >> - exit(1); >> - } >> - >> - /* Set up stop notifier */ >> - if (event_notifier_init(&poll->stop_notifier, 0) < 0) { >> - fprintf(stderr, "failed to init stop notifier\n"); >> - exit(1); >> - } >> - event_poll_add(poll, &poll->stop_handler, >> - &poll->stop_notifier, handle_stop); >> -} >> - >> -void event_poll_cleanup(EventPoll *poll) >> -{ >> - event_notifier_cleanup(&poll->stop_notifier); >> - close(poll->epoll_fd); >> - poll->epoll_fd = -1; >> -} >> - >> -/* Block until the next event and invoke its callback */ >> -void event_poll(EventPoll *poll) >> -{ >> - EventHandler *handler; >> - struct epoll_event event; >> - int nevents; >> - >> - /* Wait for the next event. Only do one event per call to keep the >> - * function simple, this could be changed later. */ >> - do { >> - nevents = epoll_wait(poll->epoll_fd, &event, 1, -1); >> - } while (nevents < 0 && errno == EINTR); >> - if (unlikely(nevents != 1)) { >> - fprintf(stderr, "epoll_wait failed: %m\n"); >> - exit(1); /* should never happen */ >> - } >> - >> - /* Find out which event handler has become active */ >> - handler = event.data.ptr; >> - >> - /* Clear the eventfd */ >> - event_notifier_test_and_clear(handler->notifier); > > Wouldn't we need to move this into the handle_io/handle_notify to maintain the > old behavior? Yes, thanks. Paolo
On Thu, Feb 21, 2013 at 05:29:55PM +0100, Paolo Bonzini wrote: > The only interesting note is the value-copy of EventNotifiers. At least > in my opinion this is part of the EventNotifier API and is even portable > to Windows. Of course, in this case you should not close the notifier's > underlying file descriptors or handle with event_notifier_cleanup. This is worth a comment in the code. > > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > hw/dataplane/Makefile.objs | 2 +- > hw/dataplane/event-poll.c | 100 --------------------------------------------- > hw/dataplane/event-poll.h | 40 ------------------ > hw/dataplane/virtio-blk.c | 41 ++++++++++--------- > 4 files changed, 22 insertions(+), 161 deletions(-) > delete mode 100644 hw/dataplane/event-poll.c > delete mode 100644 hw/dataplane/event-poll.h Looks good aside from what mdroth pointed out. Stefan
diff --git a/hw/dataplane/Makefile.objs b/hw/dataplane/Makefile.objs index 3e47d05..701111c 100644 --- a/hw/dataplane/Makefile.objs +++ b/hw/dataplane/Makefile.objs @@ -1 +1 @@ -obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += hostmem.o vring.o event-poll.o ioq.o virtio-blk.o +obj-$(CONFIG_VIRTIO_BLK_DATA_PLANE) += hostmem.o vring.o ioq.o virtio-blk.o diff --git a/hw/dataplane/event-poll.c b/hw/dataplane/event-poll.c deleted file mode 100644 index 2b55c6e..0000000 --- a/hw/dataplane/event-poll.c +++ /dev/null @@ -1,100 +0,0 @@ -/* - * Event loop with file descriptor polling - * - * 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 <sys/epoll.h> -#include "hw/dataplane/event-poll.h" - -/* Add an event notifier and its callback for polling */ -void event_poll_add(EventPoll *poll, EventHandler *handler, - EventNotifier *notifier, EventCallback *callback) -{ - struct epoll_event event = { - .events = EPOLLIN, - .data.ptr = handler, - }; - handler->notifier = notifier; - handler->callback = callback; - if (epoll_ctl(poll->epoll_fd, EPOLL_CTL_ADD, - event_notifier_get_fd(notifier), &event) != 0) { - fprintf(stderr, "failed to add event handler to epoll: %m\n"); - exit(1); - } -} - -/* Event callback for stopping event_poll() */ -static void handle_stop(EventHandler *handler) -{ - /* Do nothing */ -} - -void event_poll_init(EventPoll *poll) -{ - /* Create epoll file descriptor */ - poll->epoll_fd = epoll_create1(EPOLL_CLOEXEC); - if (poll->epoll_fd < 0) { - fprintf(stderr, "epoll_create1 failed: %m\n"); - exit(1); - } - - /* Set up stop notifier */ - if (event_notifier_init(&poll->stop_notifier, 0) < 0) { - fprintf(stderr, "failed to init stop notifier\n"); - exit(1); - } - event_poll_add(poll, &poll->stop_handler, - &poll->stop_notifier, handle_stop); -} - -void event_poll_cleanup(EventPoll *poll) -{ - event_notifier_cleanup(&poll->stop_notifier); - close(poll->epoll_fd); - poll->epoll_fd = -1; -} - -/* Block until the next event and invoke its callback */ -void event_poll(EventPoll *poll) -{ - EventHandler *handler; - struct epoll_event event; - int nevents; - - /* Wait for the next event. Only do one event per call to keep the - * function simple, this could be changed later. */ - do { - nevents = epoll_wait(poll->epoll_fd, &event, 1, -1); - } while (nevents < 0 && errno == EINTR); - if (unlikely(nevents != 1)) { - fprintf(stderr, "epoll_wait failed: %m\n"); - exit(1); /* should never happen */ - } - - /* Find out which event handler has become active */ - handler = event.data.ptr; - - /* Clear the eventfd */ - event_notifier_test_and_clear(handler->notifier); - - /* Handle the event */ - handler->callback(handler); -} - -/* Stop event_poll() - * - * This function can be used from another thread. - */ -void event_poll_notify(EventPoll *poll) -{ - event_notifier_set(&poll->stop_notifier); -} diff --git a/hw/dataplane/event-poll.h b/hw/dataplane/event-poll.h deleted file mode 100644 index 3e8d3ec..0000000 --- a/hw/dataplane/event-poll.h +++ /dev/null @@ -1,40 +0,0 @@ -/* - * Event loop with file descriptor polling - * - * 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. - * - */ - -#ifndef EVENT_POLL_H -#define EVENT_POLL_H - -#include "qemu/event_notifier.h" - -typedef struct EventHandler EventHandler; -typedef void EventCallback(EventHandler *handler); -struct EventHandler { - EventNotifier *notifier; /* eventfd */ - EventCallback *callback; /* callback function */ -}; - -typedef struct { - int epoll_fd; /* epoll(2) file descriptor */ - EventNotifier stop_notifier; /* stop poll notifier */ - EventHandler stop_handler; /* stop poll handler */ -} EventPoll; - -void event_poll_add(EventPoll *poll, EventHandler *handler, - EventNotifier *notifier, EventCallback *callback); -void event_poll_init(EventPoll *poll); -void event_poll_cleanup(EventPoll *poll); -void event_poll(EventPoll *poll); -void event_poll_notify(EventPoll *poll); - -#endif /* EVENT_POLL_H */ diff --git a/hw/dataplane/virtio-blk.c b/hw/dataplane/virtio-blk.c index 3f2da22..8819a04 100644 --- a/hw/dataplane/virtio-blk.c +++ b/hw/dataplane/virtio-blk.c @@ -14,13 +14,13 @@ #include "trace.h" #include "qemu/iov.h" -#include "event-poll.h" #include "qemu/thread.h" #include "vring.h" #include "ioq.h" #include "migration/migration.h" #include "hw/virtio-blk.h" #include "hw/dataplane/virtio-blk.h" +#include "block/aio.h" enum { SEG_MAX = 126, /* maximum number of I/O segments */ @@ -51,9 +51,9 @@ struct VirtIOBlockDataPlane { Vring vring; /* virtqueue vring */ EventNotifier *guest_notifier; /* irq */ - EventPoll event_poll; /* event poller */ - EventHandler io_handler; /* Linux AIO completion handler */ - EventHandler notify_handler; /* virtqueue notify handler */ + AioContext *ctx; + EventNotifier io_notifier; /* Linux AIO completion */ + EventNotifier host_notifier; /* doorbell */ IOQueue ioqueue; /* Linux AIO queue (should really be per dataplane thread) */ @@ -256,10 +256,10 @@ static int process_request(IOQueue *ioq, struct iovec iov[], } } -static void handle_notify(EventHandler *handler) +static void handle_notify(EventNotifier *e) { - VirtIOBlockDataPlane *s = container_of(handler, VirtIOBlockDataPlane, - notify_handler); + VirtIOBlockDataPlane *s = container_of(e, VirtIOBlockDataPlane, + host_notifier); /* There is one array of iovecs into which all new requests are extracted * from the vring. Requests are read from the vring and the translated @@ -334,10 +334,10 @@ static void handle_notify(EventHandler *handler) } } -static void handle_io(EventHandler *handler) +static void handle_io(EventNotifier *e) { - VirtIOBlockDataPlane *s = container_of(handler, VirtIOBlockDataPlane, - io_handler); + VirtIOBlockDataPlane *s = container_of(e, VirtIOBlockDataPlane, + io_notifier); if (ioq_run_completion(&s->ioqueue, complete_request, s) > 0) { notify_guest(s); @@ -348,7 +348,7 @@ static void handle_io(EventHandler *handler) * requests. */ if (unlikely(vring_more_avail(&s->vring))) { - handle_notify(&s->notify_handler); + handle_notify(&s->host_notifier); } } @@ -357,7 +357,7 @@ static void *data_plane_thread(void *opaque) VirtIOBlockDataPlane *s = opaque; do { - event_poll(&s->event_poll); + aio_poll(s->ctx, true); } while (!s->stopping || s->num_reqs > 0); return NULL; } @@ -445,7 +445,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) return; } - event_poll_init(&s->event_poll); + s->ctx = aio_context_new(); /* Set up guest notifier (irq) */ if (s->vdev->binding->set_guest_notifiers(s->vdev->binding_opaque, 1, @@ -462,17 +462,16 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s) fprintf(stderr, "virtio-blk failed to set host notifier\n"); exit(1); } - event_poll_add(&s->event_poll, &s->notify_handler, - virtio_queue_get_host_notifier(vq), - handle_notify); + s->host_notifier = *virtio_queue_get_host_notifier(vq); + aio_set_event_notifier(s->ctx, &s->host_notifier, handle_notify, NULL); /* Set up ioqueue */ ioq_init(&s->ioqueue, s->fd, REQ_MAX); for (i = 0; i < ARRAY_SIZE(s->requests); i++) { ioq_put_iocb(&s->ioqueue, &s->requests[i].iocb); } - event_poll_add(&s->event_poll, &s->io_handler, - ioq_get_notifier(&s->ioqueue), handle_io); + s->io_notifier = *ioq_get_notifier(&s->ioqueue); + aio_set_event_notifier(s->ctx, &s->io_notifier, handle_io, NULL); s->started = true; trace_virtio_blk_data_plane_start(s); @@ -498,15 +497,17 @@ void virtio_blk_data_plane_stop(VirtIOBlockDataPlane *s) qemu_bh_delete(s->start_bh); s->start_bh = NULL; } else { - event_poll_notify(&s->event_poll); + aio_notify(s->ctx); qemu_thread_join(&s->thread); } + aio_set_event_notifier(s->ctx, &s->io_notifier, NULL, NULL); ioq_cleanup(&s->ioqueue); + aio_set_event_notifier(s->ctx, &s->host_notifier, NULL, NULL); s->vdev->binding->set_host_notifier(s->vdev->binding_opaque, 0, false); - event_poll_cleanup(&s->event_poll); + aio_context_unref(s->ctx); /* Clean up guest notifier (irq) */ s->vdev->binding->set_guest_notifiers(s->vdev->binding_opaque, 1, false);
During the review of the dataplane code, the EventPoll API morphed itself (not concidentially) into something very very similar to an AioContext. Thus, it is trivial to convert virtio-blk-dataplane to use AioContext, and a first baby step towards letting dataplane talk directly to the QEMU block layer. The only interesting note is the value-copy of EventNotifiers. At least in my opinion this is part of the EventNotifier API and is even portable to Windows. Of course, in this case you should not close the notifier's underlying file descriptors or handle with event_notifier_cleanup. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- hw/dataplane/Makefile.objs | 2 +- hw/dataplane/event-poll.c | 100 --------------------------------------------- hw/dataplane/event-poll.h | 40 ------------------ hw/dataplane/virtio-blk.c | 41 ++++++++++--------- 4 files changed, 22 insertions(+), 161 deletions(-) delete mode 100644 hw/dataplane/event-poll.c delete mode 100644 hw/dataplane/event-poll.h