Re: [Qemu-devel] [PATCH] dataplane: remove EventPoll in favor of AioContext

[Paolo Bonzini]

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 <address@hidden>
>> ---
>>  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 <address@hidden>
>> - *
>> - * 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