[PATCH 1/4] kvm tools: Add ioeventfd support

[PATCH 1/4] kvm tools: Add ioeventfd support

[Sasha Levin]

ioeventfd is way provided by KVM to receive notifications about
reads and writes to PIO and MMIO areas within the guest.

Such notifications are usefull if all we need to know is that
a specific area of the memory has been changed, and we don't need
a heavyweight exit to happen.

The implementation uses epoll to scale to large number of ioeventfds.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/Makefile                |    1 +
 tools/kvm/include/kvm/ioeventfd.h |   27 ++++++++
 tools/kvm/ioeventfd.c             |  127 +++++++++++++++++++++++++++++++++++++
 tools/kvm/kvm-run.c               |    4 +
 4 files changed, 159 insertions(+), 0 deletions(-)
 create mode 100644 tools/kvm/include/kvm/ioeventfd.h
 create mode 100644 tools/kvm/ioeventfd.c

diff --git a/tools/kvm/Makefile b/tools/kvm/Makefile
index 2ebc86c..e7ceb5c 100644
--- a/tools/kvm/Makefile
+++ b/tools/kvm/Makefile
@@ -48,6 +48,7 @@ OBJS    += irq.o
 OBJS    += rbtree.o
 OBJS    += util/rbtree-interval.o
 OBJS    += virtio/9p.o
+OBJS    += ioeventfd.o
 
 
 FLAGS_BFD=$(CFLAGS) -lbfd
diff --git a/tools/kvm/include/kvm/ioeventfd.h b/tools/kvm/include/kvm/ioeventfd.h
new file mode 100644
index 0000000..fa57b41
--- /dev/null
+++ b/tools/kvm/include/kvm/ioeventfd.h
@@ -0,0 +1,27 @@
+#ifndef KVM__IOEVENTFD_H
+#define KVM__IOEVENTFD_H
+
+#include <linux/types.h>
+#include <linux/list.h>
+#include <sys/eventfd.h>
+
+struct kvm;
+
+struct ioevent {
+	u64			start;
+	u8			len;
+	void			(*ioevent_callback_fn)(struct kvm *kvm, void *ptr);
+	struct kvm		*kvm;
+	void			*ptr;
+	int			event_fd;
+	u64			datamatch;
+
+	struct list_head	list_used;
+};
+
+void ioeventfd__init(void);
+void ioeventfd__start(void);
+void ioeventfd__add_event(struct kvm *kvm, struct ioevent *ioevent);
+void ioeventfd__del_event(struct kvm *kvm, u64 start, u64 datamatch);
+
+#endif
diff --git a/tools/kvm/ioeventfd.c b/tools/kvm/ioeventfd.c
new file mode 100644
index 0000000..5444432
--- /dev/null
+++ b/tools/kvm/ioeventfd.c
@@ -0,0 +1,127 @@
+#include <sys/epoll.h>
+#include <sys/ioctl.h>
+#include <pthread.h>
+#include <unistd.h>
+#include <stdio.h>
+#include <signal.h>
+
+#include <linux/kernel.h>
+#include <linux/kvm.h>
+#include <linux/types.h>
+
+#include "kvm/ioeventfd.h"
+#include "kvm/kvm.h"
+#include "kvm/util.h"
+
+#define IOEVENTFD_MAX_EVENTS	20
+
+static struct	epoll_event events[IOEVENTFD_MAX_EVENTS];
+static int	epoll_fd;
+static LIST_HEAD(used_ioevents);
+
+void ioeventfd__init(void)
+{
+	epoll_fd = epoll_create(IOEVENTFD_MAX_EVENTS);
+	if (epoll_fd < 0)
+		die("Failed creating epoll fd");
+}
+
+void ioeventfd__add_event(struct kvm *kvm, struct ioevent *ioevent)
+{
+	struct kvm_ioeventfd kvm_ioevent;
+	struct epoll_event epoll_event;
+	struct ioevent *new_ioevent;
+	int event;
+
+	new_ioevent = malloc(sizeof(*new_ioevent));
+	if (new_ioevent == NULL)
+		die("Failed allocating memory for new ioevent");
+
+	*new_ioevent = *ioevent;
+	event = new_ioevent->event_fd;
+
+	kvm_ioevent = (struct kvm_ioeventfd) {
+		.addr			= ioevent->start,
+		.len			= ioevent->len,
+		.datamatch		= ioevent->datamatch,
+		.fd			= event,
+		.flags			= KVM_IOEVENTFD_FLAG_PIO | KVM_IOEVENTFD_FLAG_DATAMATCH,
+	};
+
+	if (ioctl(kvm->vm_fd, KVM_IOEVENTFD, &kvm_ioevent) != 0)
+		die("Failed creating new ioeventfd");
+
+	epoll_event = (struct epoll_event) {
+		.events			= EPOLLIN,
+		.data.ptr		= new_ioevent,
+	};
+
+	if (epoll_ctl(epoll_fd, EPOLL_CTL_ADD, event, &epoll_event) != 0)
+		die("Failed assigning new event to the epoll fd");
+
+	list_add_tail(&new_ioevent->list_used, &used_ioevents);
+}
+
+void ioeventfd__del_event(struct kvm *kvm, u64 start, u64 datamatch)
+{
+	struct kvm_ioeventfd kvm_ioevent;
+	struct ioevent *ioevent;
+	u8 found = 0;
+
+	list_for_each_entry(ioevent, &used_ioevents, list_used) {
+		if (ioevent->start == start) {
+			found = 1;
+			break;
+		}
+	}
+
+	if (found == 0 || ioevent == NULL)
+		return;
+
+	kvm_ioevent = (struct kvm_ioeventfd) {
+		.addr			= ioevent->start,
+		.len			= ioevent->len,
+		.datamatch		= ioevent->datamatch,
+		.flags			= KVM_IOEVENTFD_FLAG_PIO
+					| KVM_IOEVENTFD_FLAG_DEASSIGN
+					| KVM_IOEVENTFD_FLAG_DATAMATCH,
+	};
+
+	ioctl(kvm->vm_fd, KVM_IOEVENTFD, &kvm_ioevent);
+
+	epoll_ctl(epoll_fd, EPOLL_CTL_DEL, ioevent->event_fd, NULL);
+
+	list_del(&ioevent->list_used);
+
+	close(ioevent->event_fd);
+	free(ioevent);
+}
+
+static void *ioeventfd__thread(void *param)
+{
+	for (;;) {
+		int nfds, i;
+
+		nfds = epoll_wait(epoll_fd, events, IOEVENTFD_MAX_EVENTS, -1);
+		for (i = 0; i < nfds; i++) {
+			u64 tmp;
+			struct ioevent *ioevent;
+
+			ioevent = events[i].data.ptr;
+
+			if (read(ioevent->event_fd, &tmp, sizeof(tmp)) < 0)
+				die("Failed reading event");
+
+			ioevent->ioevent_callback_fn(ioevent->kvm, ioevent->ptr);
+		}
+	}
+
+	return NULL;
+}
+
+void ioeventfd__start(void)
+{
+	pthread_t thread;
+
+	pthread_create(&thread, NULL, ioeventfd__thread, NULL);
+}
diff --git a/tools/kvm/kvm-run.c b/tools/kvm/kvm-run.c
index f384ddd..48b8e70 100644
--- a/tools/kvm/kvm-run.c
+++ b/tools/kvm/kvm-run.c
@@ -29,6 +29,7 @@
 #include <kvm/symbol.h>
 #include <kvm/virtio-9p.h>
 #include <kvm/vesa.h>
+#include <kvm/ioeventfd.h>
 
 /* header files for gitish interface  */
 #include <kvm/kvm-run.h>
@@ -505,6 +506,8 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 
 	kvm = kvm__init(kvm_dev, ram_size);
 
+	ioeventfd__init();
+
 	max_cpus = kvm__max_cpus(kvm);
 
 	if (nrcpus > max_cpus) {
@@ -612,6 +615,7 @@ int kvm_cmd_run(int argc, const char **argv, const char *prefix)
 		vesa__init(kvm);
 
 	thread_pool__init(nr_online_cpus);
+	ioeventfd__start();
 
 	for (i = 0; i < nrcpus; i++) {
 		if (pthread_create(&kvm_cpus[i]->thread, NULL, kvm_cpu_thread, kvm_cpus[i]) != 0)
-- 
1.7.5.rc3

[PATCH 2/4] kvm tools: Use ioeventfd in virtio-blk

[Sasha Levin]

Use ioeventfds to receive notifications of IO events in virtio-blk.
Doing so prevents an exit every time we read/write from/to the
virtio disk.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/virtio/blk.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/tools/kvm/virtio/blk.c b/tools/kvm/virtio/blk.c
index a8f9d8c..eb38038 100644
--- a/tools/kvm/virtio/blk.c
+++ b/tools/kvm/virtio/blk.c
@@ -10,6 +10,7 @@
 #include "kvm/kvm.h"
 #include "kvm/pci.h"
 #include "kvm/threadpool.h"
+#include "kvm/ioeventfd.h"
 
 #include <linux/virtio_ring.h>
 #include <linux/virtio_blk.h>
@@ -243,11 +244,19 @@ static struct ioport_operations virtio_blk_io_ops = {
 	.io_out	= virtio_blk_pci_io_out,
 };
 
+static void ioevent_callback(struct kvm *kvm, void *param)
+{
+	struct blk_dev_job *job = param;
+
+	thread_pool__do_job(job->job_id);
+}
+
 void virtio_blk__init(struct kvm *kvm, struct disk_image *disk)
 {
 	u16 blk_dev_base_addr;
-	u8 dev, pin, line;
+	u8 dev, pin, line, i;
 	struct blk_dev *bdev;
+	struct ioevent ioevent;
 
 	if (!disk)
 		return;
@@ -293,6 +302,20 @@ void virtio_blk__init(struct kvm *kvm, struct disk_image *disk)
 	bdev->pci_hdr.irq_line	= line;
 
 	pci__register(&bdev->pci_hdr, dev);
+
+	for (i = 0; i < NUM_VIRT_QUEUES; i++) {
+		ioevent = (struct ioevent) {
+			.start			= blk_dev_base_addr + VIRTIO_PCI_QUEUE_NOTIFY,
+			.len			= sizeof(u16),
+			.ioevent_callback_fn	= ioevent_callback,
+			.datamatch		= i,
+			.ptr			= &bdev->jobs[i],
+			.kvm			= kvm,
+			.event_fd		= eventfd(0, 0),
+		};
+
+		ioeventfd__add_event(kvm, &ioevent);
+	}
 }
 
 void virtio_blk__init_all(struct kvm *kvm)
@@ -309,6 +332,7 @@ void virtio_blk__delete_all(struct kvm *kvm)
 		struct blk_dev *bdev;
 
 		bdev = list_first_entry(&bdevs, struct blk_dev, list);
+		ioeventfd__del_event(kvm, bdev->base_addr + VIRTIO_PCI_QUEUE_NOTIFY, 0);
 		list_del(&bdev->list);
 		free(bdev);
 	}
-- 
1.7.5.rc3

[PATCH 3/4] kvm tools: Use ioeventfd in virtio-net

[Sasha Levin]

Use ioeventfds to receive notifications of IO events in virtio-net.
Doing so prevents an exit every time we receive/send a packet.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/virtio/net.c |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/tools/kvm/virtio/net.c b/tools/kvm/virtio/net.c
index 3064da6..0e47be5 100644
--- a/tools/kvm/virtio/net.c
+++ b/tools/kvm/virtio/net.c
@@ -8,6 +8,7 @@
 #include "kvm/kvm.h"
 #include "kvm/pci.h"
 #include "kvm/irq.h"
+#include "kvm/ioeventfd.h"
 
 #include <linux/virtio_net.h>
 #include <linux/if_tun.h>
@@ -280,6 +281,11 @@ static bool virtio_net_pci_io_out(struct ioport *ioport, struct kvm *kvm, u16 po
 	return ret;
 }
 
+static void ioevent_callback(struct kvm *kvm, void *param)
+{
+	virtio_net_handle_callback(kvm, (u64)param);
+}
+
 static struct ioport_operations virtio_net_io_ops = {
 	.io_in	= virtio_net_pci_io_in,
 	.io_out	= virtio_net_pci_io_out,
@@ -388,6 +394,8 @@ void virtio_net__init(const struct virtio_net_parameters *params)
 	if (virtio_net__tap_init(params)) {
 		u8 dev, line, pin;
 		u16 net_base_addr;
+		u64 i;
+		struct ioevent ioevent;
 
 		if (irq__register_device(VIRTIO_ID_NET, &dev, &pin, &line) < 0)
 			return;
@@ -401,5 +409,19 @@ void virtio_net__init(const struct virtio_net_parameters *params)
 		pci__register(&pci_header, dev);
 
 		virtio_net__io_thread_init(params->kvm);
+
+		for (i = 0; i < VIRTIO_NET_NUM_QUEUES; i++) {
+			ioevent = (struct ioevent) {
+				.start			= net_base_addr + VIRTIO_PCI_QUEUE_NOTIFY,
+				.len			= sizeof(u16),
+				.ioevent_callback_fn	= ioevent_callback,
+				.datamatch		= i,
+				.ptr			= (void *)i,
+				.kvm			= params->kvm,
+				.event_fd		= eventfd(0, 0),
+			};
+
+			ioeventfd__add_event(params->kvm, &ioevent);
+		}
 	}
 }
-- 
1.7.5.rc3

[PATCH 4/4] kvm tools: Use ioeventfd in virtio-rng

[Sasha Levin]

Use ioeventfds to receive notifications of IO events in virtio-rng.
Doing so prevents an exit every time we need to supply randomness
to the guest.

Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
---
 tools/kvm/virtio/rng.c |   26 +++++++++++++++++++++++++-
 1 files changed, 25 insertions(+), 1 deletions(-)

diff --git a/tools/kvm/virtio/rng.c b/tools/kvm/virtio/rng.c
index 1a3bda3..207f2d6 100644
--- a/tools/kvm/virtio/rng.c
+++ b/tools/kvm/virtio/rng.c
@@ -10,6 +10,7 @@
 #include "kvm/pci.h"
 #include "kvm/threadpool.h"
 #include "kvm/irq.h"
+#include "kvm/ioeventfd.h"
 
 #include <linux/virtio_ring.h>
 #include <linux/virtio_rng.h>
@@ -154,6 +155,7 @@ static bool virtio_rng_pci_io_out(struct ioport *ioport, struct kvm *kvm, u16 po
 		break;
 	case VIRTIO_PCI_QUEUE_NOTIFY: {
 		u16 queue_index;
+		printf("regular exit");
 		queue_index		= ioport__read16(data);
 		thread_pool__do_job(rdev->jobs[queue_index].job_id);
 		break;
@@ -177,11 +179,19 @@ static struct ioport_operations virtio_rng_io_ops = {
 	.io_out				= virtio_rng_pci_io_out,
 };
 
+static void ioevent_callback(struct kvm *kvm, void *param)
+{
+	struct rng_dev_job *job = param;
+
+	thread_pool__do_job(job->job_id);
+}
+
 void virtio_rng__init(struct kvm *kvm)
 {
-	u8 pin, line, dev;
+	u8 pin, line, dev, i;
 	u16 rdev_base_addr;
 	struct rng_dev *rdev;
+	struct ioevent ioevent;
 
 	rdev = malloc(sizeof(*rdev));
 	if (rdev == NULL)
@@ -213,6 +223,20 @@ void virtio_rng__init(struct kvm *kvm)
 	pci__register(&rdev->pci_hdr, dev);
 
 	list_add_tail(&rdev->list, &rdevs);
+
+	for (i = 0; i < NUM_VIRT_QUEUES; i++) {
+		ioevent = (struct ioevent) {
+			.start			= rdev_base_addr + VIRTIO_PCI_QUEUE_NOTIFY,
+			.len			= sizeof(u16),
+			.ioevent_callback_fn	= ioevent_callback,
+			.ptr			= &rdev->jobs[i],
+			.datamatch		= i,
+			.kvm			= kvm,
+			.event_fd		= eventfd(0, 0),
+		};
+
+		ioeventfd__add_event(kvm, &ioevent);
+	}
 }
 
 void virtio_rng__delete_all(struct kvm *kvm)
-- 
1.7.5.rc3

Re: [PATCH 1/4] kvm tools: Add ioeventfd support

[Stefan Hajnoczi]

On Fri, May 27, 2011 at 11:36 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> ioeventfd is way provided by KVM to receive notifications about
> reads and writes to PIO and MMIO areas within the guest.
>
> Such notifications are usefull if all we need to know is that
> a specific area of the memory has been changed, and we don't need
> a heavyweight exit to happen.
>
> The implementation uses epoll to scale to large number of ioeventfds.
>
> Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> ---
>  tools/kvm/Makefile                |    1 +
>  tools/kvm/include/kvm/ioeventfd.h |   27 ++++++++
>  tools/kvm/ioeventfd.c             |  127 +++++++++++++++++++++++++++++++++++++
>  tools/kvm/kvm-run.c               |    4 +
>  4 files changed, 159 insertions(+), 0 deletions(-)
>  create mode 100644 tools/kvm/include/kvm/ioeventfd.h
>  create mode 100644 tools/kvm/ioeventfd.c

Did you run any benchmarks?

Stefan

Re: [PATCH 1/4] kvm tools: Add ioeventfd support

[Sasha Levin]

On Fri, 2011-05-27 at 11:47 +0100, Stefan Hajnoczi wrote:
> On Fri, May 27, 2011 at 11:36 AM, Sasha Levin <levinsasha928@gmail.com> wrote:
> > ioeventfd is way provided by KVM to receive notifications about
> > reads and writes to PIO and MMIO areas within the guest.
> >
> > Such notifications are usefull if all we need to know is that
> > a specific area of the memory has been changed, and we don't need
> > a heavyweight exit to happen.
> >
> > The implementation uses epoll to scale to large number of ioeventfds.
> >
> > Signed-off-by: Sasha Levin <levinsasha928@gmail.com>
> > ---
> >  tools/kvm/Makefile                |    1 +
> >  tools/kvm/include/kvm/ioeventfd.h |   27 ++++++++
> >  tools/kvm/ioeventfd.c             |  127 +++++++++++++++++++++++++++++++++++++
> >  tools/kvm/kvm-run.c               |    4 +
> >  4 files changed, 159 insertions(+), 0 deletions(-)
> >  create mode 100644 tools/kvm/include/kvm/ioeventfd.h
> >  create mode 100644 tools/kvm/ioeventfd.c
> 
> Did you run any benchmarks?
> 
> Stefan

Yes, they showed a nice improvements - I'll post them with a V2 of the
patch.

-- 

Sasha.

Re: [PATCH 1/4] kvm tools: Add ioeventfd support

[Pekka Enberg]

On Fri, May 27, 2011 at 1:36 PM, Sasha Levin <levinsasha928@gmail.com> wrote:
> +void ioeventfd__start(void)
> +{
> +       pthread_t thread;
> +
> +       pthread_create(&thread, NULL, ioeventfd__thread, NULL);

Please be more careful with error handling. If an API call can fail,
there's almost never any reason to silently ignore it.

                        Pekka

Re: [PATCH 1/4] kvm tools: Add ioeventfd support

[Ingo Molnar]

* Sasha Levin <levinsasha928@gmail.com> wrote:

> ioeventfd is way provided by KVM to receive notifications about
> reads and writes to PIO and MMIO areas within the guest.
> 
> Such notifications are usefull if all we need to know is that
> a specific area of the memory has been changed, and we don't need
> a heavyweight exit to happen.
> 
> The implementation uses epoll to scale to large number of ioeventfds.

Nice! :-)

> +struct ioevent {
> +	u64			start;
> +	u8			len;

If that's an mmio address then it might be worth naming it 
ioevent->mmio_addr, ioevent->mmio_end.

> +	void			(*ioevent_callback_fn)(struct kvm *kvm, void *ptr);

Please only 'fn', we already know this is an ioevent.

> +	struct kvm		*kvm;
> +	void			*ptr;

what is the purpose of the pointer?

AFAICS it the private data of the callback function. In such cases 
please name them in a harmonizing fashion, such as:

	void			(*fn)(struct kvm *kvm, void *data);
	struct kvm		*fn_kvm;
	void			*fn_data;

Also, will tools/kvm/ ever run with multiple 'struct kvm' instances 
present?

A sidenote: i think 'struct kvm *kvm' was a naming mistake - it's way 
too aspecific, it tells us nothing. What is a 'kvm'?

A much better name would be 'struct machine *machine', hm? Even if 
everyone agrees this would be a separate patch, obviously.

Also, can ioevent->kvm *ever* be different from the kvm that the 
mmio-event receiving vcpu thread is associated with? If not then the 
fn_kvm field is really superfluous - we get the machine from the mmio 
handler and can pass it down to the callback function.

> +	int			event_fd;

'fd'

> +	u64			datamatch;

what's a datamatch? 'cookie'? 'key'?

> +
> +	struct list_head	list_used;

just 'list' is enough i think - it's obvious that ioevent->list is a 
list of ioevents, right?

> +	kvm_ioevent = (struct kvm_ioeventfd) {
> +		.addr			= ioevent->start,
> +		.len			= ioevent->len,

Do you see how confusing the start/len naming is? Here you are 
assigning a 'start' field to an 'addr' and the reviewer is kept 
wondering whether that's right. If it was ->mmio_addr then it would 
be a lot more obvious what is going on.
> +static void *ioeventfd__thread(void *param)
> +{
> +	for (;;) {
> +		int nfds, i;
> +
> +		nfds = epoll_wait(epoll_fd, events, IOEVENTFD_MAX_EVENTS, -1);
> +		for (i = 0; i < nfds; i++) {
> +			u64 tmp;
> +			struct ioevent *ioevent;
> +
> +			ioevent = events[i].data.ptr;
> +
> +			if (read(ioevent->event_fd, &tmp, sizeof(tmp)) < 0)
> +				die("Failed reading event");
> +
> +			ioevent->ioevent_callback_fn(ioevent->kvm, ioevent->ptr);
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +void ioeventfd__start(void)
> +{
> +	pthread_t thread;
> +
> +	pthread_create(&thread, NULL, ioeventfd__thread, NULL);
> +}

Shouldnt this use the thread pool, so that we know about each and 
every worker thread we have started, in one central place?

(This might have relevance, see the big-reader-lock mail i sent 
earlier today.)

Thanks,

	Ingo

Re: [PATCH 1/4] kvm tools: Add ioeventfd support

[Sasha Levin]

On Fri, 2011-05-27 at 12:54 +0200, Ingo Molnar wrote:
> * Sasha Levin <levinsasha928@gmail.com> wrote:
> 
> > ioeventfd is way provided by KVM to receive notifications about
> > reads and writes to PIO and MMIO areas within the guest.
> > 
> > Such notifications are usefull if all we need to know is that
> > a specific area of the memory has been changed, and we don't need
> > a heavyweight exit to happen.
> > 
> > The implementation uses epoll to scale to large number of ioeventfds.
> 
> Nice! :-)
> 
> > +struct ioevent {
> > +	u64			start;
> > +	u8			len;
> 
> If that's an mmio address then it might be worth naming it 
> ioevent->mmio_addr, ioevent->mmio_end.
> 
> > +	void			(*ioevent_callback_fn)(struct kvm *kvm, void *ptr);
> 
> Please only 'fn', we already know this is an ioevent.
> 
> > +	struct kvm		*kvm;
> > +	void			*ptr;
> 
> what is the purpose of the pointer?
> 
> AFAICS it the private data of the callback function. In such cases 
> please name them in a harmonizing fashion, such as:
> 
> 	void			(*fn)(struct kvm *kvm, void *data);
> 	struct kvm		*fn_kvm;
> 	void			*fn_data;
> 
> Also, will tools/kvm/ ever run with multiple 'struct kvm' instances 
> present?

I'm not sure. We pass it around to all our functions instead of using a
global, so I assumed we might have several guests under one process.

> A sidenote: i think 'struct kvm *kvm' was a naming mistake - it's way 
> too aspecific, it tells us nothing. What is a 'kvm'?
> 
> A much better name would be 'struct machine *machine', hm? Even if 
> everyone agrees this would be a separate patch, obviously.
> 
> Also, can ioevent->kvm *ever* be different from the kvm that the 
> mmio-event receiving vcpu thread is associated with? If not then the 
> fn_kvm field is really superfluous - we get the machine from the mmio 
> handler and can pass it down to the callback function.
> 
> > +	int			event_fd;
> 
> 'fd'
> 
> > +	u64			datamatch;
> 
> what's a datamatch? 'cookie'? 'key'?

The kernel-side ioeventfd matches the value written to the PIO port and
signals the event only if both values match.
It's named this way in the kernel code so I wanted to be consistent.

> 
> > +
> > +	struct list_head	list_used;
> 
> just 'list' is enough i think - it's obvious that ioevent->list is a 
> list of ioevents, right?
> 

We might have a list of free ioevents if we ever decide to scale it
beyond the max 20 event limit we currently have, so I would rather be
specific with the list names.

> > +	kvm_ioevent = (struct kvm_ioeventfd) {
> > +		.addr			= ioevent->start,
> > +		.len			= ioevent->len,
> 
> Do you see how confusing the start/len naming is? Here you are 
> assigning a 'start' field to an 'addr' and the reviewer is kept 
> wondering whether that's right. If it was ->mmio_addr then it would 
> be a lot more obvious what is going on.

Yes, I'll rename them to addr/len to match with KVM naming.

> > +static void *ioeventfd__thread(void *param)
> > +{
> > +	for (;;) {
> > +		int nfds, i;
> > +
> > +		nfds = epoll_wait(epoll_fd, events, IOEVENTFD_MAX_EVENTS, -1);
> > +		for (i = 0; i < nfds; i++) {
> > +			u64 tmp;
> > +			struct ioevent *ioevent;
> > +
> > +			ioevent = events[i].data.ptr;
> > +
> > +			if (read(ioevent->event_fd, &tmp, sizeof(tmp)) < 0)
> > +				die("Failed reading event");
> > +
> > +			ioevent->ioevent_callback_fn(ioevent->kvm, ioevent->ptr);
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +void ioeventfd__start(void)
> > +{
> > +	pthread_t thread;
> > +
> > +	pthread_create(&thread, NULL, ioeventfd__thread, NULL);
> > +}
> 
> Shouldnt this use the thread pool, so that we know about each and 
> every worker thread we have started, in one central place?
> 
Our thread pool currently responds to events - it runs a callback if it
has received a notification to do so. It doesn't manage threads which
have to run all the time like in this case.

Though once we return from epoll_wait() here we do minimal work before
sending the IO event into the thread pool.


> (This might have relevance, see the big-reader-lock mail i sent 
> earlier today.)
> 
> Thanks,
> 
> 	Ingo

-- 

Sasha.

Re: [PATCH 1/4] kvm tools: Add ioeventfd support

[Ingo Molnar]

* Sasha Levin <levinsasha928@gmail.com> wrote:

> > > +	pthread_create(&thread, NULL, ioeventfd__thread, NULL);
> > > +}
> > 
> > Shouldnt this use the thread pool, so that we know about each and 
> > every worker thread we have started, in one central place?
> 
> Our thread pool currently responds to events - it runs a callback 
> if it has received a notification to do so. It doesn't manage 
> threads which have to run all the time like in this case.

then it should perhaps be extended to handle that, because it's 
always good to have explicit enumeration of all worker threads, and 
because:

> > (This might have relevance, see the big-reader-lock mail i sent 
> > earlier today.)

Thanks,

	Ingo

Re: [PATCH 1/4] kvm tools: Add ioeventfd support

[Pekka Enberg]

Hi Ingo,

On Fri, May 27, 2011 at 1:54 PM, Ingo Molnar <mingo@elte.hu> wrote:
> A sidenote: i think 'struct kvm *kvm' was a naming mistake - it's way
> too aspecific, it tells us nothing. What is a 'kvm'?

Why, an instance of a kernel virtual machine, of course! It was the
very first thing I wrote for this project!

On Fri, May 27, 2011 at 1:54 PM, Ingo Molnar <mingo@elte.hu> wrote:
> A much better name would be 'struct machine *machine', hm? Even if
> everyone agrees this would be a separate patch, obviously.

Well, I don't really think 'struct machine' is that much better. The
obvious benefits of 'struct kvm' is that it's the same name that's
used by Qemu and libkvm and maps nicely to '/dev/kvm'.

If you really, really wanna change it, I could use some more
convincing or bribes of some sort.

                        Pekka

Re: [PATCH 1/4] kvm tools: Add ioeventfd support

[Ingo Molnar]

* Pekka Enberg <penberg@kernel.org> wrote:

> Hi Ingo,
> 
> On Fri, May 27, 2011 at 1:54 PM, Ingo Molnar <mingo@elte.hu> wrote:
> > A sidenote: i think 'struct kvm *kvm' was a naming mistake - it's way
> > too aspecific, it tells us nothing. What is a 'kvm'?
> 
> Why, an instance of a kernel virtual machine, of course! It was the
> very first thing I wrote for this project!

Oh, that is way too funny: i never thought of 'KVM' as a 'kernel 
virtual machine' :-/ It's the name of a cool kernel subsystem - not 
the name of one instance of a virtual machine.

> On Fri, May 27, 2011 at 1:54 PM, Ingo Molnar <mingo@elte.hu> wrote:
> > A much better name would be 'struct machine *machine', hm? Even if
> > everyone agrees this would be a separate patch, obviously.
> 
> Well, I don't really think 'struct machine' is that much better. The
> obvious benefits of 'struct kvm' is that it's the same name that's
> used by Qemu and libkvm and maps nicely to '/dev/kvm'.

To me /dev/kvm is what interfaces to 'KVM' - where 'KVM' is the 
magic, axiomatic name for the aforementioned cool kernel subsystem! :-)

> If you really, really wanna change it, I could use some more
> convincing or bribes of some sort.

No, i guess the naming is just fine, i just need to rewire 5 years 
worth of neural pathways ;-)

I'll save the bribes for a worthier goal! :-)

Thanks,

	Ingo