On Wed, 19 Apr 2023 14:21:18 +0100
Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx> wrote:
Hi,
> Both ioeventfd and ipc use an epoll thread roughly the same way. In
> order to add a new epoll user, factor the common bits into epoll.c
>
> Slight implementation changes which shouldn't affect behavior:
>
> * At the moment ioeventfd mixes file descriptor (for the stop event) and
> pointers in the epoll_event.data union, which could in theory cause
> aliasing. Use a pointer for the stop event instead. kvm-ipc uses only
> file descriptors. It could be changed but since epoll.c compares the
> stop event pointer first, the risk of aliasing with an fd is much
> lower there.
>
> * kvm-ipc uses EPOLLET, edge-triggered events, but having the stop event
> level-triggered shouldn't make a difference.
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@xxxxxxxxxx>
> ---
> Makefile | 1 +
> include/kvm/epoll.h | 17 ++++++++
> epoll.c | 89 ++++++++++++++++++++++++++++++++++++++
> ioeventfd.c | 94 ++++++----------------------------------
> kvm-ipc.c | 103 +++++++++++++-------------------------------
> 5 files changed, 149 insertions(+), 155 deletions(-)
> create mode 100644 include/kvm/epoll.h
> create mode 100644 epoll.c
>
> diff --git a/Makefile b/Makefile
> index 86e19339..6b742369 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -80,6 +80,7 @@ OBJS += virtio/vhost.o
> OBJS += disk/blk.o
> OBJS += disk/qcow.o
> OBJS += disk/raw.o
> +OBJS += epoll.o
> OBJS += ioeventfd.o
> OBJS += net/uip/core.o
> OBJS += net/uip/arp.o
> diff --git a/include/kvm/epoll.h b/include/kvm/epoll.h
> new file mode 100644
> index 00000000..dbb5a8d9
> --- /dev/null
> +++ b/include/kvm/epoll.h
> @@ -0,0 +1,17 @@
> +#include <sys/epoll.h>
> +#include "kvm/kvm.h"
> +
> +typedef void (*epoll__event_handler_t)(struct kvm *kvm, struct epoll_event *ev);
> +
> +struct kvm__epoll {
> + int fd;
> + int stop_fd;
> + struct kvm *kvm;
> + const char *name;
> + pthread_t thread;
> + epoll__event_handler_t handle_event;
> +};
> +
> +int epoll__init(struct kvm *kvm, struct kvm__epoll *epoll,
> + const char *name, epoll__event_handler_t handle_event);
> +int epoll__exit(struct kvm__epoll *epoll);
> diff --git a/epoll.c b/epoll.c
> new file mode 100644
> index 00000000..e0725a57
> --- /dev/null
> +++ b/epoll.c
> @@ -0,0 +1,89 @@
> +#include <sys/eventfd.h>
> +
> +#include "kvm/epoll.h"
> +
> +#define EPOLLFD_MAX_EVENTS 20
> +
> +static void *epoll__thread(void *param)
> +{
> + u64 stop;
> + int nfds, i;
> + struct kvm__epoll *epoll = param;
> + struct kvm *kvm = epoll->kvm;
> + struct epoll_event events[EPOLLFD_MAX_EVENTS];
> +
> + kvm__set_thread_name(epoll->name);
> +
> + for (;;) {
> + nfds = epoll_wait(epoll->fd, events, EPOLLFD_MAX_EVENTS, -1);
> + for (i = 0; i < nfds; i++) {
> + if (events[i].data.ptr == &epoll->stop_fd)
> + goto done;
> +
> + epoll->handle_event(kvm, &events[i]);
> + }
> + }
> +done:
> + read(epoll->stop_fd, &stop, sizeof(stop));
> + write(epoll->stop_fd, &stop, sizeof(stop));
read(2) and write(2) (sys)calls without checking the return value upsets
Ubuntu's compiler:
epoll.c: In function ‘epoll__thread’:
epoll.c:27:2: error: ignoring return value of ‘read’, declared with attribute warn_unused_result [-Werror=unused-result]
27 | read(epoll->stop_fd, &stop, sizeof(stop));
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
(same for the write in the line after)
Since we use -Werror, this is fatal.
I fixed it for now with:
if (read(epoll->stop_fd, &stop, sizeof(stop)) < 0)
return NULL;
Not sure if there is a more meaningful way to bail out at this point.
Cheers,
Andre
> + return NULL;
> +}
> +
> +int epoll__init(struct kvm *kvm, struct kvm__epoll *epoll,
> + const char *name, epoll__event_handler_t handle_event)
> +{
> + int r;
> + struct epoll_event stop_event = {
> + .events = EPOLLIN,
> + .data.ptr = &epoll->stop_fd,
> + };
> +
> + epoll->kvm = kvm;
> + epoll->name = name;
> + epoll->handle_event = handle_event;
> +
> + epoll->fd = epoll_create(EPOLLFD_MAX_EVENTS);
> + if (epoll->fd < 0)
> + return -errno;
> +
> + epoll->stop_fd = eventfd(0, 0);
> + if (epoll->stop_fd < 0) {
> + r = -errno;
> + goto err_close_fd;
> + }
> +
> + r = epoll_ctl(epoll->fd, EPOLL_CTL_ADD, epoll->stop_fd, &stop_event);
> + if (r < 0)
> + goto err_close_all;
> +
> + r = pthread_create(&epoll->thread, NULL, epoll__thread, epoll);
> + if (r < 0)
> + goto err_close_all;
> +
> + return 0;
> +
> +err_close_all:
> + close(epoll->stop_fd);
> +err_close_fd:
> + close(epoll->fd);
> +
> + return r;
> +}
> +
> +int epoll__exit(struct kvm__epoll *epoll)
> +{
> + int r;
> + u64 stop = 1;
> +
> + r = write(epoll->stop_fd, &stop, sizeof(stop));
> + if (r < 0)
> + return r;
> +
> + r = read(epoll->stop_fd, &stop, sizeof(stop));
> + if (r < 0)
> + return r;
> +
> + close(epoll->stop_fd);
> + close(epoll->fd);
> + return 0;
> +}
