* [KVM PATCH v7 1/5] kvm: prepare irqfd for having interrupts disabled during eventfd->release
2009-06-29 18:28 [KVM PATCH v7 0/5] irqfd fixes and enhancements Gregory Haskins
@ 2009-06-29 18:29 ` Gregory Haskins
2009-06-29 18:29 ` [KVM PATCH v7 2/5] eventfd - revised interface and cleanups (4th rev) Gregory Haskins
` (4 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Gregory Haskins @ 2009-06-29 18:29 UTC (permalink / raw)
To: kvm; +Cc: linux-kernel, mst, avi, davidel
We need to plug some race conditions on eventfd shutdown. In order to
do this, we need to change the context in which the release notification
is delivered so that the wqh lock is now held. However, there is currently
code in the release callback that assumes it can sleep.
We have a slight chicken and egg problem where we cant fix the race
without adding the lock, and we can't add the lock without breaking
the sleepy code. Normally we could deal with this by making both
changes in an atomic changeset. However, we want to keep the eventfd
and kvm specific changes isolated to ease the reviewer burden on upstream
eventfd (at the specific request of upstream). Therefore, we have this
intermediate patch.
This intermediate patch allows the release() method to work in an atomic
context, at the expense of correctness w.r.t. memory-leaks. Today we have
a race condition. With this patch applied we leak. Both issues will be
resolved later in the series. It is the author's opinion that a leak is
better for bisectability than the hang would be should we leave the sleepy
code in place after the locking changeover.
Signed-off-by: Gregory Haskins <ghaskins@novell.com>
---
virt/kvm/eventfd.c | 89 ++++++++++++++++------------------------------------
1 files changed, 27 insertions(+), 62 deletions(-)
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index a9e7de7..9656027 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -28,7 +28,6 @@
#include <linux/file.h>
#include <linux/list.h>
#include <linux/eventfd.h>
-#include <linux/srcu.h>
/*
* --------------------------------------------------------------------
@@ -38,8 +37,6 @@
* --------------------------------------------------------------------
*/
struct _irqfd {
- struct mutex lock;
- struct srcu_struct srcu;
struct kvm *kvm;
int gsi;
struct list_head list;
@@ -53,48 +50,12 @@ static void
irqfd_inject(struct work_struct *work)
{
struct _irqfd *irqfd = container_of(work, struct _irqfd, inject);
- struct kvm *kvm;
- int idx;
+ struct kvm *kvm = irqfd->kvm;
- idx = srcu_read_lock(&irqfd->srcu);
-
- kvm = rcu_dereference(irqfd->kvm);
- if (kvm) {
- mutex_lock(&kvm->irq_lock);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
- kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
- mutex_unlock(&kvm->irq_lock);
- }
-
- srcu_read_unlock(&irqfd->srcu, idx);
-}
-
-static void
-irqfd_disconnect(struct _irqfd *irqfd)
-{
- struct kvm *kvm;
-
- mutex_lock(&irqfd->lock);
-
- kvm = rcu_dereference(irqfd->kvm);
- rcu_assign_pointer(irqfd->kvm, NULL);
-
- mutex_unlock(&irqfd->lock);
-
- if (!kvm)
- return;
-
- mutex_lock(&kvm->lock);
- list_del(&irqfd->list);
- mutex_unlock(&kvm->lock);
-
- /*
- * It is important to not drop the kvm reference until the next grace
- * period because there might be lockless references in flight up
- * until then
- */
- synchronize_srcu(&irqfd->srcu);
- kvm_put_kvm(kvm);
+ mutex_lock(&kvm->irq_lock);
+ kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 1);
+ kvm_set_irq(kvm, KVM_USERSPACE_IRQ_SOURCE_ID, irqfd->gsi, 0);
+ mutex_unlock(&kvm->irq_lock);
}
static int
@@ -103,26 +64,24 @@ irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
unsigned long flags = (unsigned long)key;
+ /*
+ * Assume we will be called with interrupts disabled
+ */
if (flags & POLLIN)
/*
- * The POLLIN wake_up is called with interrupts disabled.
- * Therefore we need to defer the IRQ injection until later
- * since we need to acquire the kvm->lock to do so.
+ * Defer the IRQ injection until later since we need to
+ * acquire the kvm->lock to do so.
*/
schedule_work(&irqfd->inject);
if (flags & POLLHUP) {
/*
- * The POLLHUP is called unlocked, so it theoretically should
- * be safe to remove ourselves from the wqh using the locked
- * variant of remove_wait_queue()
+ * for now, just remove ourselves from the list and let
+ * the rest dangle. We will fix this up later once
+ * the races in eventfd are fixed
*/
- remove_wait_queue(irqfd->wqh, &irqfd->wait);
- flush_work(&irqfd->inject);
- irqfd_disconnect(irqfd);
-
- cleanup_srcu_struct(&irqfd->srcu);
- kfree(irqfd);
+ __remove_wait_queue(irqfd->wqh, &irqfd->wait);
+ irqfd->wqh = NULL;
}
return 0;
@@ -150,8 +109,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
if (!irqfd)
return -ENOMEM;
- mutex_init(&irqfd->lock);
- init_srcu_struct(&irqfd->srcu);
irqfd->kvm = kvm;
irqfd->gsi = gsi;
INIT_LIST_HEAD(&irqfd->list);
@@ -172,8 +129,6 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
events = file->f_op->poll(file, &irqfd->pt);
- kvm_get_kvm(kvm);
-
mutex_lock(&kvm->lock);
list_add_tail(&irqfd->list, &kvm->irqfds);
mutex_unlock(&kvm->lock);
@@ -211,6 +166,16 @@ kvm_irqfd_release(struct kvm *kvm)
{
struct _irqfd *irqfd, *tmp;
- list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list)
- irqfd_disconnect(irqfd);
+ list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
+ if (irqfd->wqh)
+ remove_wait_queue(irqfd->wqh, &irqfd->wait);
+
+ flush_work(&irqfd->inject);
+
+ mutex_lock(&kvm->lock);
+ list_del(&irqfd->list);
+ mutex_unlock(&kvm->lock);
+
+ kfree(irqfd);
+ }
}
^ permalink raw reply related [flat|nested] 8+ messages in thread* [KVM PATCH v7 2/5] eventfd - revised interface and cleanups (4th rev)
2009-06-29 18:28 [KVM PATCH v7 0/5] irqfd fixes and enhancements Gregory Haskins
2009-06-29 18:29 ` [KVM PATCH v7 1/5] kvm: prepare irqfd for having interrupts disabled during eventfd->release Gregory Haskins
@ 2009-06-29 18:29 ` Gregory Haskins
2009-06-29 18:29 ` [KVM PATCH v7 3/5] KVM: Fix races in irqfd using new eventfd_kref_get interface Gregory Haskins
` (3 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Gregory Haskins @ 2009-06-29 18:29 UTC (permalink / raw)
To: kvm; +Cc: linux-kernel, mst, avi, davidel
From: Davide Libenzi <davidel@xmailserver.org>
The following patch changes the eventfd interface to de-couple the eventfd
memory context, from the file pointer instance.
Without such change, there is no clean way to racely free handle the
POLLHUP event sent when the last instance of the file* goes away.
Also, now the internal eventfd APIs are using the eventfd context instead
of the file*.
Gregory, none of the APIs changed, so your patches do not need to be
rebased over this one. The last three revisions just updated comments.
Andrew, I'm not posting the "AIO select EVENTFD" patch at this time. Will
eventually try another push in your skull once I come back from vacation ;)
[gmh: resolved merge conflict against prior POLLHUP patch ]
Signed-off-by: Davide Libenzi <davidel@xmailserver.org>
Signed-off-by: Gregory Haskins <ghaskins@novell.com>
---
drivers/lguest/lg.h | 2 -
drivers/lguest/lguest_user.c | 4 +
fs/aio.c | 24 ++------
fs/eventfd.c | 126 ++++++++++++++++++++++++++++++++++++------
include/linux/aio.h | 4 +
include/linux/eventfd.h | 35 +++++++++---
6 files changed, 147 insertions(+), 48 deletions(-)
diff --git a/drivers/lguest/lg.h b/drivers/lguest/lg.h
index d4e8979..9c31382 100644
--- a/drivers/lguest/lg.h
+++ b/drivers/lguest/lg.h
@@ -82,7 +82,7 @@ struct lg_cpu {
struct lg_eventfd {
unsigned long addr;
- struct file *event;
+ struct eventfd_ctx *event;
};
struct lg_eventfd_map {
diff --git a/drivers/lguest/lguest_user.c b/drivers/lguest/lguest_user.c
index 32e2971..9f9a295 100644
--- a/drivers/lguest/lguest_user.c
+++ b/drivers/lguest/lguest_user.c
@@ -50,7 +50,7 @@ static int add_eventfd(struct lguest *lg, unsigned long addr, int fd)
/* Now append new entry. */
new->map[new->num].addr = addr;
- new->map[new->num].event = eventfd_fget(fd);
+ new->map[new->num].event = eventfd_ctx_fdget(fd);
if (IS_ERR(new->map[new->num].event)) {
kfree(new);
return PTR_ERR(new->map[new->num].event);
@@ -357,7 +357,7 @@ static int close(struct inode *inode, struct file *file)
/* Release any eventfds they registered. */
for (i = 0; i < lg->eventfds->num; i++)
- fput(lg->eventfds->map[i].event);
+ eventfd_ctx_put(lg->eventfds->map[i].event);
kfree(lg->eventfds);
/* If lg->dead doesn't contain an error code it will be NULL or a
diff --git a/fs/aio.c b/fs/aio.c
index 76da125..d065b2c 100644
--- a/fs/aio.c
+++ b/fs/aio.c
@@ -485,6 +485,8 @@ static inline void really_put_req(struct kioctx *ctx, struct kiocb *req)
{
assert_spin_locked(&ctx->ctx_lock);
+ if (req->ki_eventfd != NULL)
+ eventfd_ctx_put(req->ki_eventfd);
if (req->ki_dtor)
req->ki_dtor(req);
if (req->ki_iovec != &req->ki_inline_vec)
@@ -509,8 +511,6 @@ static void aio_fput_routine(struct work_struct *data)
/* Complete the fput(s) */
if (req->ki_filp != NULL)
__fput(req->ki_filp);
- if (req->ki_eventfd != NULL)
- __fput(req->ki_eventfd);
/* Link the iocb into the context's free list */
spin_lock_irq(&ctx->ctx_lock);
@@ -528,8 +528,6 @@ static void aio_fput_routine(struct work_struct *data)
*/
static int __aio_put_req(struct kioctx *ctx, struct kiocb *req)
{
- int schedule_putreq = 0;
-
dprintk(KERN_DEBUG "aio_put(%p): f_count=%ld\n",
req, atomic_long_read(&req->ki_filp->f_count));
@@ -549,24 +547,16 @@ static int __aio_put_req(struct kioctx *ctx, struct kiocb *req)
* we would not be holding the last reference to the file*, so
* this function will be executed w/out any aio kthread wakeup.
*/
- if (unlikely(atomic_long_dec_and_test(&req->ki_filp->f_count)))
- schedule_putreq++;
- else
- req->ki_filp = NULL;
- if (req->ki_eventfd != NULL) {
- if (unlikely(atomic_long_dec_and_test(&req->ki_eventfd->f_count)))
- schedule_putreq++;
- else
- req->ki_eventfd = NULL;
- }
- if (unlikely(schedule_putreq)) {
+ if (unlikely(atomic_long_dec_and_test(&req->ki_filp->f_count))) {
get_ioctx(ctx);
spin_lock(&fput_lock);
list_add(&req->ki_list, &fput_head);
spin_unlock(&fput_lock);
queue_work(aio_wq, &fput_work);
- } else
+ } else {
+ req->ki_filp = NULL;
really_put_req(ctx, req);
+ }
return 1;
}
@@ -1622,7 +1612,7 @@ static int io_submit_one(struct kioctx *ctx, struct iocb __user *user_iocb,
* an eventfd() fd, and will be signaled for each completed
* event using the eventfd_signal() function.
*/
- req->ki_eventfd = eventfd_fget((int) iocb->aio_resfd);
+ req->ki_eventfd = eventfd_ctx_fdget((int) iocb->aio_resfd);
if (IS_ERR(req->ki_eventfd)) {
ret = PTR_ERR(req->ki_eventfd);
req->ki_eventfd = NULL;
diff --git a/fs/eventfd.c b/fs/eventfd.c
index 72f5f8d..31d12de 100644
--- a/fs/eventfd.c
+++ b/fs/eventfd.c
@@ -14,35 +14,44 @@
#include <linux/list.h>
#include <linux/spinlock.h>
#include <linux/anon_inodes.h>
-#include <linux/eventfd.h>
#include <linux/syscalls.h>
#include <linux/module.h>
+#include <linux/kref.h>
+#include <linux/eventfd.h>
struct eventfd_ctx {
+ struct kref kref;
wait_queue_head_t wqh;
/*
* Every time that a write(2) is performed on an eventfd, the
* value of the __u64 being written is added to "count" and a
* wakeup is performed on "wqh". A read(2) will return the "count"
* value to userspace, and will reset "count" to zero. The kernel
- * size eventfd_signal() also, adds to the "count" counter and
+ * side eventfd_signal() also, adds to the "count" counter and
* issue a wakeup.
*/
__u64 count;
unsigned int flags;
};
-/*
- * Adds "n" to the eventfd counter "count". Returns "n" in case of
- * success, or a value lower then "n" in case of coutner overflow.
- * This function is supposed to be called by the kernel in paths
- * that do not allow sleeping. In this function we allow the counter
- * to reach the ULLONG_MAX value, and we signal this as overflow
- * condition by returining a POLLERR to poll(2).
+/**
+ * eventfd_signal - Adds @n to the eventfd counter.
+ * @ctx: [in] Pointer to the eventfd context.
+ * @n: [in] Value of the counter to be added to the eventfd internal counter.
+ * The value cannot be negative.
+ *
+ * This function is supposed to be called by the kernel in paths that do not
+ * allow sleeping. In this function we allow the counter to reach the ULLONG_MAX
+ * value, and we signal this as overflow condition by returining a POLLERR
+ * to poll(2).
+ *
+ * Returns @n in case of success, a non-negative number lower than @n in case
+ * of overflow, or the following error codes:
+ *
+ * -EINVAL : The value of @n is negative.
*/
-int eventfd_signal(struct file *file, int n)
+int eventfd_signal(struct eventfd_ctx *ctx, int n)
{
- struct eventfd_ctx *ctx = file->private_data;
unsigned long flags;
if (n < 0)
@@ -59,17 +68,45 @@ int eventfd_signal(struct file *file, int n)
}
EXPORT_SYMBOL_GPL(eventfd_signal);
+static void eventfd_free(struct kref *kref)
+{
+ struct eventfd_ctx *ctx = container_of(kref, struct eventfd_ctx, kref);
+
+ kfree(ctx);
+}
+
+/**
+ * eventfd_ctx_get - Acquires a reference to the internal eventfd context.
+ * @ctx: [in] Pointer to the eventfd context.
+ *
+ * Returns: In case of success, returns a pointer to the eventfd context.
+ */
+struct eventfd_ctx *eventfd_ctx_get(struct eventfd_ctx *ctx)
+{
+ kref_get(&ctx->kref);
+ return ctx;
+}
+EXPORT_SYMBOL_GPL(eventfd_ctx_get);
+
+/**
+ * eventfd_ctx_put - Releases a reference to the internal eventfd context.
+ * @ctx: [in] Pointer to eventfd context.
+ *
+ * The eventfd context reference must have been previously acquired either
+ * with eventfd_ctx_get() or eventfd_ctx_fdget()).
+ */
+void eventfd_ctx_put(struct eventfd_ctx *ctx)
+{
+ kref_put(&ctx->kref, eventfd_free);
+}
+EXPORT_SYMBOL_GPL(eventfd_ctx_put);
+
static int eventfd_release(struct inode *inode, struct file *file)
{
struct eventfd_ctx *ctx = file->private_data;
- /*
- * No need to hold the lock here, since we are on the file cleanup
- * path and the ones still attached to the wait queue will be
- * serialized by wake_up_locked_poll().
- */
- wake_up_locked_poll(&ctx->wqh, POLLHUP);
- kfree(ctx);
+ wake_up_poll(&ctx->wqh, POLLHUP);
+ eventfd_ctx_put(ctx);
return 0;
}
@@ -193,6 +230,16 @@ static const struct file_operations eventfd_fops = {
.write = eventfd_write,
};
+/**
+ * eventfd_fget - Acquire a reference of an eventfd file descriptor.
+ * @fd: [in] Eventfd file descriptor.
+ *
+ * Returns a pointer to the eventfd file structure in case of success, or the
+ * following error pointer:
+ *
+ * -EBADF : Invalid @fd file descriptor.
+ * -EINVAL : The @fd file descriptor is not an eventfd file.
+ */
struct file *eventfd_fget(int fd)
{
struct file *file;
@@ -209,6 +256,48 @@ struct file *eventfd_fget(int fd)
}
EXPORT_SYMBOL_GPL(eventfd_fget);
+/**
+ * eventfd_ctx_fdget - Acquires a reference to the internal eventfd context.
+ * @fd: [in] Eventfd file descriptor.
+ *
+ * Returns a pointer to the internal eventfd context, otherwise the error
+ * pointers returned by the following functions:
+ *
+ * eventfd_fget
+ */
+struct eventfd_ctx *eventfd_ctx_fdget(int fd)
+{
+ struct file *file;
+ struct eventfd_ctx *ctx;
+
+ file = eventfd_fget(fd);
+ if (IS_ERR(file))
+ return (struct eventfd_ctx *) file;
+ ctx = eventfd_ctx_get(file->private_data);
+ fput(file);
+
+ return ctx;
+}
+EXPORT_SYMBOL_GPL(eventfd_ctx_fdget);
+
+/**
+ * eventfd_ctx_fileget - Acquires a reference to the internal eventfd context.
+ * @file: [in] Eventfd file pointer.
+ *
+ * Returns a pointer to the internal eventfd context, otherwise the error
+ * pointer:
+ *
+ * -EINVAL : The @fd file descriptor is not an eventfd file.
+ */
+struct eventfd_ctx *eventfd_ctx_fileget(struct file *file)
+{
+ if (file->f_op != &eventfd_fops)
+ return ERR_PTR(-EINVAL);
+
+ return eventfd_ctx_get(file->private_data);
+}
+EXPORT_SYMBOL_GPL(eventfd_ctx_fileget);
+
SYSCALL_DEFINE2(eventfd2, unsigned int, count, int, flags)
{
int fd;
@@ -225,6 +314,7 @@ SYSCALL_DEFINE2(eventfd2, unsigned int, count, int, flags)
if (!ctx)
return -ENOMEM;
+ kref_init(&ctx->kref);
init_waitqueue_head(&ctx->wqh);
ctx->count = count;
ctx->flags = flags;
diff --git a/include/linux/aio.h b/include/linux/aio.h
index b16a957..47f7d93 100644
--- a/include/linux/aio.h
+++ b/include/linux/aio.h
@@ -121,9 +121,9 @@ struct kiocb {
/*
* If the aio_resfd field of the userspace iocb is not zero,
- * this is the underlying file* to deliver event to.
+ * this is the underlying eventfd context to deliver events to.
*/
- struct file *ki_eventfd;
+ struct eventfd_ctx *ki_eventfd;
};
#define is_sync_kiocb(iocb) ((iocb)->ki_key == KIOCB_SYNC_KEY)
diff --git a/include/linux/eventfd.h b/include/linux/eventfd.h
index f45a8ae..3b85ba6 100644
--- a/include/linux/eventfd.h
+++ b/include/linux/eventfd.h
@@ -8,10 +8,8 @@
#ifndef _LINUX_EVENTFD_H
#define _LINUX_EVENTFD_H
-#ifdef CONFIG_EVENTFD
-
-/* For O_CLOEXEC and O_NONBLOCK */
#include <linux/fcntl.h>
+#include <linux/file.h>
/*
* CAREFUL: Check include/asm-generic/fcntl.h when defining
@@ -27,16 +25,37 @@
#define EFD_SHARED_FCNTL_FLAGS (O_CLOEXEC | O_NONBLOCK)
#define EFD_FLAGS_SET (EFD_SHARED_FCNTL_FLAGS | EFD_SEMAPHORE)
+#ifdef CONFIG_EVENTFD
+
+struct eventfd_ctx *eventfd_ctx_get(struct eventfd_ctx *ctx);
+void eventfd_ctx_put(struct eventfd_ctx *ctx);
struct file *eventfd_fget(int fd);
-int eventfd_signal(struct file *file, int n);
+struct eventfd_ctx *eventfd_ctx_fdget(int fd);
+struct eventfd_ctx *eventfd_ctx_fileget(struct file *file);
+int eventfd_signal(struct eventfd_ctx *ctx, int n);
#else /* CONFIG_EVENTFD */
-#define eventfd_fget(fd) ERR_PTR(-ENOSYS)
-static inline int eventfd_signal(struct file *file, int n)
-{ return 0; }
+/*
+ * Ugly ugly ugly error layer to support modules that uses eventfd but
+ * pretend to work in !CONFIG_EVENTFD configurations. Namely, AIO.
+ */
+static inline struct eventfd_ctx *eventfd_ctx_fdget(int fd)
+{
+ return ERR_PTR(-ENOSYS);
+}
+
+static inline int eventfd_signal(struct eventfd_ctx *ctx, int n)
+{
+ return -ENOSYS;
+}
+
+static inline void eventfd_ctx_put(struct eventfd_ctx *ctx)
+{
+
+}
-#endif /* CONFIG_EVENTFD */
+#endif
#endif /* _LINUX_EVENTFD_H */
^ permalink raw reply related [flat|nested] 8+ messages in thread* [KVM PATCH v7 3/5] KVM: Fix races in irqfd using new eventfd_kref_get interface
2009-06-29 18:28 [KVM PATCH v7 0/5] irqfd fixes and enhancements Gregory Haskins
2009-06-29 18:29 ` [KVM PATCH v7 1/5] kvm: prepare irqfd for having interrupts disabled during eventfd->release Gregory Haskins
2009-06-29 18:29 ` [KVM PATCH v7 2/5] eventfd - revised interface and cleanups (4th rev) Gregory Haskins
@ 2009-06-29 18:29 ` Gregory Haskins
2009-06-29 18:29 ` [KVM PATCH v7 4/5] KVM: add irqfd DEASSIGN feature Gregory Haskins
` (2 subsequent siblings)
5 siblings, 0 replies; 8+ messages in thread
From: Gregory Haskins @ 2009-06-29 18:29 UTC (permalink / raw)
To: kvm; +Cc: linux-kernel, mst, avi, davidel
eventfd currently emits a POLLHUP wakeup on f_ops->release() to generate a
"release" callback. This lets eventfd clients know if the eventfd is about
to go away and is very useful particularly for in-kernel clients. However,
until recently it is not possible to use this feature of eventfd in a
race-free way. This patch utilizes a new eventfd interface to rectify
the problem.
Signed-off-by: Gregory Haskins <ghaskins@novell.com>
---
include/linux/kvm_host.h | 5 +-
virt/kvm/eventfd.c | 153 ++++++++++++++++++++++++++++++++++++++--------
2 files changed, 129 insertions(+), 29 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 2451f48..5a90c6a 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -141,7 +141,10 @@ struct kvm {
struct kvm_io_bus mmio_bus;
struct kvm_io_bus pio_bus;
#ifdef CONFIG_HAVE_KVM_EVENTFD
- struct list_head irqfds;
+ struct {
+ spinlock_t lock;
+ struct list_head items;
+ } irqfds;
#endif
struct kvm_vm_stat stat;
struct kvm_arch arch;
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index 9656027..cb29a85 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -36,16 +36,22 @@
* Credit goes to Avi Kivity for the original idea.
* --------------------------------------------------------------------
*/
+
struct _irqfd {
struct kvm *kvm;
+ struct eventfd_ctx *eventfd;
int gsi;
struct list_head list;
poll_table pt;
wait_queue_head_t *wqh;
wait_queue_t wait;
struct work_struct inject;
+ struct work_struct shutdown;
+ int active:1; /* guarded by kvm->irqfds.lock */
};
+static struct workqueue_struct *irqfd_cleanup_wq;
+
static void
irqfd_inject(struct work_struct *work)
{
@@ -58,30 +64,78 @@ irqfd_inject(struct work_struct *work)
mutex_unlock(&kvm->irq_lock);
}
+/*
+ * Race-free decouple logic (ordering is critical)
+ */
+static void
+irqfd_shutdown(struct work_struct *work)
+{
+ struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
+
+ /*
+ * Synchronize with the wait-queue and unhook ourselves to prevent
+ * further events.
+ */
+ remove_wait_queue(irqfd->wqh, &irqfd->wait);
+
+ /*
+ * We know no new events will be scheduled at this point, so block
+ * until all previously outstanding events have completed
+ */
+ flush_work(&irqfd->inject);
+
+ /*
+ * It is now safe to release the object's resources
+ */
+ eventfd_ctx_put(irqfd->eventfd);
+ kfree(irqfd);
+}
+
+/*
+ * Mark the irqfd as inactive and schedule it for removal
+ *
+ * assumes kvm->irqfds.lock is held
+ */
+static void
+irqfd_deactivate(struct _irqfd *irqfd)
+{
+ BUG_ON(!irqfd->active);
+
+ irqfd->active = false;
+ list_del(&irqfd->list);
+
+ queue_work(irqfd_cleanup_wq, &irqfd->shutdown);
+}
+
+/*
+ * Called with wqh->lock held and interrupts disabled
+ */
static int
irqfd_wakeup(wait_queue_t *wait, unsigned mode, int sync, void *key)
{
struct _irqfd *irqfd = container_of(wait, struct _irqfd, wait);
unsigned long flags = (unsigned long)key;
- /*
- * Assume we will be called with interrupts disabled
- */
if (flags & POLLIN)
- /*
- * Defer the IRQ injection until later since we need to
- * acquire the kvm->lock to do so.
- */
+ /* An event has been signaled, inject an interrupt */
schedule_work(&irqfd->inject);
if (flags & POLLHUP) {
- /*
- * for now, just remove ourselves from the list and let
- * the rest dangle. We will fix this up later once
- * the races in eventfd are fixed
- */
- __remove_wait_queue(irqfd->wqh, &irqfd->wait);
- irqfd->wqh = NULL;
+ /* The eventfd is closing, detach from KVM */
+ struct kvm *kvm = irqfd->kvm;
+ unsigned long flags;
+
+ spin_lock_irqsave(&kvm->irqfds.lock, flags);
+
+ if (irqfd->active)
+ /*
+ * If we get here, we can be sure no-one else is
+ * trying to shutdown this object at the same time
+ * since we hold the list lock.
+ */
+ irqfd_deactivate(irqfd);
+
+ spin_unlock_irqrestore(&kvm->irqfds.lock, flags);
}
return 0;
@@ -102,6 +156,7 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
{
struct _irqfd *irqfd;
struct file *file = NULL;
+ struct eventfd_ctx *eventfd = NULL;
int ret;
unsigned int events;
@@ -113,6 +168,8 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
irqfd->gsi = gsi;
INIT_LIST_HEAD(&irqfd->list);
INIT_WORK(&irqfd->inject, irqfd_inject);
+ INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
+ irqfd->active = true;
file = eventfd_fget(fd);
if (IS_ERR(file)) {
@@ -120,6 +177,14 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
goto fail;
}
+ eventfd = eventfd_ctx_fileget(file);
+ if (IS_ERR(file)) {
+ ret = PTR_ERR(file);
+ goto fail;
+ }
+
+ irqfd->eventfd = eventfd;
+
/*
* Install our own custom wake-up handling so we are notified via
* a callback whenever someone signals the underlying eventfd
@@ -129,12 +194,13 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
events = file->f_op->poll(file, &irqfd->pt);
- mutex_lock(&kvm->lock);
- list_add_tail(&irqfd->list, &kvm->irqfds);
- mutex_unlock(&kvm->lock);
+ spin_lock_irq(&kvm->irqfds.lock);
+ list_add_tail(&irqfd->list, &kvm->irqfds.items);
+ spin_unlock_irq(&kvm->irqfds.lock);
/*
- * Check if there was an event already queued
+ * Check if there was an event already pending on the eventfd
+ * before we registered, and trigger it as if we didn't miss it.
*/
if (events & POLLIN)
schedule_work(&irqfd->inject);
@@ -148,6 +214,9 @@ kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
return 0;
fail:
+ if (eventfd && !IS_ERR(eventfd))
+ eventfd_ctx_put(eventfd);
+
if (file && !IS_ERR(file))
fput(file);
@@ -158,24 +227,52 @@ fail:
void
kvm_irqfd_init(struct kvm *kvm)
{
- INIT_LIST_HEAD(&kvm->irqfds);
+ spin_lock_init(&kvm->irqfds.lock);
+ INIT_LIST_HEAD(&kvm->irqfds.items);
}
+/*
+ * This function is called as the kvm VM fd is being released. Shutdown all
+ * irqfds that still remain open
+ */
void
kvm_irqfd_release(struct kvm *kvm)
{
struct _irqfd *irqfd, *tmp;
- list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds, list) {
- if (irqfd->wqh)
- remove_wait_queue(irqfd->wqh, &irqfd->wait);
+ spin_lock_irq(&kvm->irqfds.lock);
- flush_work(&irqfd->inject);
+ list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds.items, list)
+ irqfd_deactivate(irqfd);
- mutex_lock(&kvm->lock);
- list_del(&irqfd->list);
- mutex_unlock(&kvm->lock);
+ spin_unlock_irq(&kvm->irqfds.lock);
- kfree(irqfd);
- }
+ /*
+ * Block until we know all outstanding shutdown jobs have completed
+ * since we do not take a kvm* reference.
+ */
+ flush_workqueue(irqfd_cleanup_wq);
+
+}
+
+/*
+ * create a host-wide workqueue for issuing deferred shutdown requests
+ * aggregated from all vm* instances. We need our own isolated single-thread
+ * queue to prevent deadlock against flushing the normal work-queue.
+ */
+static int __init irqfd_module_init(void)
+{
+ irqfd_cleanup_wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
+ if (!irqfd_cleanup_wq)
+ return -ENOMEM;
+
+ return 0;
}
+
+static void __exit irqfd_module_exit(void)
+{
+ destroy_workqueue(irqfd_cleanup_wq);
+}
+
+module_init(irqfd_module_init);
+module_exit(irqfd_module_exit);
^ permalink raw reply related [flat|nested] 8+ messages in thread* [KVM PATCH v7 4/5] KVM: add irqfd DEASSIGN feature
2009-06-29 18:28 [KVM PATCH v7 0/5] irqfd fixes and enhancements Gregory Haskins
` (2 preceding siblings ...)
2009-06-29 18:29 ` [KVM PATCH v7 3/5] KVM: Fix races in irqfd using new eventfd_kref_get interface Gregory Haskins
@ 2009-06-29 18:29 ` Gregory Haskins
2009-06-29 18:29 ` [KVM PATCH v7 5/5] KVM: Make irqfd use slow-work for shutdown Gregory Haskins
2009-07-01 8:53 ` [KVM PATCH v7 0/5] irqfd fixes and enhancements Avi Kivity
5 siblings, 0 replies; 8+ messages in thread
From: Gregory Haskins @ 2009-06-29 18:29 UTC (permalink / raw)
To: kvm; +Cc: linux-kernel, mst, avi, davidel
DEASSIGN allows us to optionally disassociate an IRQFD from its underlying
eventfd without destroying the eventfd in the process. This is useful
for conditions like live-migration which may have an eventfd associated
with a device and an IRQFD. We need to be able to decouple the guest
from the event source while not perturbing the event source itself.
Signed-off-by: Gregory Haskins <ghaskins@novell.com>
CC: Michael S. Tsirkin <mst@redhat.com>
---
include/linux/kvm.h | 2 ++
virt/kvm/eventfd.c | 46 ++++++++++++++++++++++++++++++++++++++++++++--
2 files changed, 46 insertions(+), 2 deletions(-)
diff --git a/include/linux/kvm.h b/include/linux/kvm.h
index 38ff31e..6710518 100644
--- a/include/linux/kvm.h
+++ b/include/linux/kvm.h
@@ -490,6 +490,8 @@ struct kvm_x86_mce {
};
#endif
+#define KVM_IRQFD_FLAG_DEASSIGN (1 << 0)
+
struct kvm_irqfd {
__u32 fd;
__u32 gsi;
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index cb29a85..c1ade4f 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -151,8 +151,8 @@ irqfd_ptable_queue_proc(struct file *file, wait_queue_head_t *wqh,
add_wait_queue(wqh, &irqfd->wait);
}
-int
-kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
+static int
+kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
{
struct _irqfd *irqfd;
struct file *file = NULL;
@@ -232,6 +232,48 @@ kvm_irqfd_init(struct kvm *kvm)
}
/*
+ * shutdown any irqfd's that match fd+gsi
+ */
+static int
+kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
+{
+ struct _irqfd *irqfd, *tmp;
+ struct eventfd_ctx *eventfd;
+
+ eventfd = eventfd_ctx_fdget(fd);
+ if (IS_ERR(eventfd))
+ return PTR_ERR(eventfd);
+
+ spin_lock_irq(&kvm->irqfds.lock);
+
+ list_for_each_entry_safe(irqfd, tmp, &kvm->irqfds.items, list) {
+ if (irqfd->eventfd == eventfd && irqfd->gsi == gsi)
+ irqfd_deactivate(irqfd);
+ }
+
+ spin_unlock_irq(&kvm->irqfds.lock);
+ eventfd_ctx_put(eventfd);
+
+ /*
+ * Block until we know all outstanding shutdown jobs have completed
+ * so that we guarantee there will not be any more interrupts on this
+ * gsi once this deassign function returns.
+ */
+ flush_workqueue(irqfd_cleanup_wq);
+
+ return 0;
+}
+
+int
+kvm_irqfd(struct kvm *kvm, int fd, int gsi, int flags)
+{
+ if (flags & KVM_IRQFD_FLAG_DEASSIGN)
+ return kvm_irqfd_deassign(kvm, fd, gsi);
+
+ return kvm_irqfd_assign(kvm, fd, gsi);
+}
+
+/*
* This function is called as the kvm VM fd is being released. Shutdown all
* irqfds that still remain open
*/
^ permalink raw reply related [flat|nested] 8+ messages in thread* [KVM PATCH v7 5/5] KVM: Make irqfd use slow-work for shutdown
2009-06-29 18:28 [KVM PATCH v7 0/5] irqfd fixes and enhancements Gregory Haskins
` (3 preceding siblings ...)
2009-06-29 18:29 ` [KVM PATCH v7 4/5] KVM: add irqfd DEASSIGN feature Gregory Haskins
@ 2009-06-29 18:29 ` Gregory Haskins
2009-07-01 8:53 ` [KVM PATCH v7 0/5] irqfd fixes and enhancements Avi Kivity
5 siblings, 0 replies; 8+ messages in thread
From: Gregory Haskins @ 2009-06-29 18:29 UTC (permalink / raw)
To: kvm; +Cc: linux-kernel, mst, avi, davidel
This patch eliminates the mostly idle, dedicated work-queue in favor of
utilizing the slow-work thread pool. The downside to this approach is
that we add ~30 lines of complexity to irqfd to get rid of the thread,
but this may be a worthwhile tradeoff.
Note that a race is known to exist: the slow-work thread may race with
module removal. We are currently working with slow-work upstream to fix
this issue as well.
Signed-off-by: Gregory Haskins <ghaskins@novell.com>
---
include/linux/kvm_host.h | 2 +
virt/kvm/Kconfig | 1
virt/kvm/eventfd.c | 100 +++++++++++++++++++++++++++++-----------------
3 files changed, 65 insertions(+), 38 deletions(-)
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 5a90c6a..d94ee72 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -144,6 +144,8 @@ struct kvm {
struct {
spinlock_t lock;
struct list_head items;
+ atomic_t outstanding;
+ wait_queue_head_t wqh;
} irqfds;
#endif
struct kvm_vm_stat stat;
diff --git a/virt/kvm/Kconfig b/virt/kvm/Kconfig
index daece36..ab7848a 100644
--- a/virt/kvm/Kconfig
+++ b/virt/kvm/Kconfig
@@ -9,6 +9,7 @@ config HAVE_KVM_IRQCHIP
config HAVE_KVM_EVENTFD
bool
select EVENTFD
+ select SLOW_WORK
config KVM_APIC_ARCHITECTURE
bool
diff --git a/virt/kvm/eventfd.c b/virt/kvm/eventfd.c
index c1ade4f..5fbd97b 100644
--- a/virt/kvm/eventfd.c
+++ b/virt/kvm/eventfd.c
@@ -28,6 +28,7 @@
#include <linux/file.h>
#include <linux/list.h>
#include <linux/eventfd.h>
+#include <linux/slow-work.h>
/*
* --------------------------------------------------------------------
@@ -46,12 +47,10 @@ struct _irqfd {
wait_queue_head_t *wqh;
wait_queue_t wait;
struct work_struct inject;
- struct work_struct shutdown;
+ struct slow_work shutdown;
int active:1; /* guarded by kvm->irqfds.lock */
};
-static struct workqueue_struct *irqfd_cleanup_wq;
-
static void
irqfd_inject(struct work_struct *work)
{
@@ -64,13 +63,53 @@ irqfd_inject(struct work_struct *work)
mutex_unlock(&kvm->irq_lock);
}
+static struct _irqfd *
+work_to_irqfd(struct slow_work *work)
+{
+ return container_of(work, struct _irqfd, shutdown);
+}
+
+static int
+irqfd_shutdown_get_ref(struct slow_work *work)
+{
+ struct _irqfd *irqfd = work_to_irqfd(work);
+ struct kvm *kvm = irqfd->kvm;
+
+ atomic_inc(&kvm->irqfds.outstanding);
+
+ return 0;
+}
+
+static void
+irqfd_shutdown_put_ref(struct slow_work *work)
+{
+ struct _irqfd *irqfd = work_to_irqfd(work);
+ struct kvm *kvm = irqfd->kvm;
+
+ /*
+ * It is now safe to release the object's resources
+ */
+ eventfd_ctx_put(irqfd->eventfd);
+ kfree(irqfd);
+
+ if (atomic_dec_and_test(&kvm->irqfds.outstanding))
+ wake_up(&kvm->irqfds.wqh);
+}
+
+
+static void
+irqfd_shutdown_flush(struct kvm *kvm)
+{
+ wait_event(kvm->irqfds.wqh, (!atomic_read(&kvm->irqfds.outstanding)));
+}
+
/*
* Race-free decouple logic (ordering is critical)
*/
static void
-irqfd_shutdown(struct work_struct *work)
+irqfd_shutdown_execute(struct slow_work *work)
{
- struct _irqfd *irqfd = container_of(work, struct _irqfd, shutdown);
+ struct _irqfd *irqfd = work_to_irqfd(work);
/*
* Synchronize with the wait-queue and unhook ourselves to prevent
@@ -80,17 +119,19 @@ irqfd_shutdown(struct work_struct *work)
/*
* We know no new events will be scheduled at this point, so block
- * until all previously outstanding events have completed
+ * until all previously outstanding events have completed. Once this
+ * job completes, the slow_work infrastructure will drop the irqfd
+ * object completely via put_ref
*/
flush_work(&irqfd->inject);
-
- /*
- * It is now safe to release the object's resources
- */
- eventfd_ctx_put(irqfd->eventfd);
- kfree(irqfd);
}
+const static struct slow_work_ops irqfd_shutdown_work_ops = {
+ .get_ref = irqfd_shutdown_get_ref,
+ .put_ref = irqfd_shutdown_put_ref,
+ .execute = irqfd_shutdown_execute,
+};
+
/*
* Mark the irqfd as inactive and schedule it for removal
*
@@ -104,7 +145,7 @@ irqfd_deactivate(struct _irqfd *irqfd)
irqfd->active = false;
list_del(&irqfd->list);
- queue_work(irqfd_cleanup_wq, &irqfd->shutdown);
+ slow_work_enqueue(&irqfd->shutdown);
}
/*
@@ -168,7 +209,7 @@ kvm_irqfd_assign(struct kvm *kvm, int fd, int gsi)
irqfd->gsi = gsi;
INIT_LIST_HEAD(&irqfd->list);
INIT_WORK(&irqfd->inject, irqfd_inject);
- INIT_WORK(&irqfd->shutdown, irqfd_shutdown);
+ slow_work_init(&irqfd->shutdown, &irqfd_shutdown_work_ops);
irqfd->active = true;
file = eventfd_fget(fd);
@@ -227,8 +268,12 @@ fail:
void
kvm_irqfd_init(struct kvm *kvm)
{
+ slow_work_register_user();
+
spin_lock_init(&kvm->irqfds.lock);
INIT_LIST_HEAD(&kvm->irqfds.items);
+ atomic_set(&kvm->irqfds.outstanding, 0);
+ init_waitqueue_head(&kvm->irqfds.wqh);
}
/*
@@ -259,7 +304,7 @@ kvm_irqfd_deassign(struct kvm *kvm, int fd, int gsi)
* so that we guarantee there will not be any more interrupts on this
* gsi once this deassign function returns.
*/
- flush_workqueue(irqfd_cleanup_wq);
+ irqfd_shutdown_flush(kvm);
return 0;
}
@@ -293,28 +338,7 @@ kvm_irqfd_release(struct kvm *kvm)
* Block until we know all outstanding shutdown jobs have completed
* since we do not take a kvm* reference.
*/
- flush_workqueue(irqfd_cleanup_wq);
+ irqfd_shutdown_flush(kvm);
+ slow_work_unregister_user();
}
-
-/*
- * create a host-wide workqueue for issuing deferred shutdown requests
- * aggregated from all vm* instances. We need our own isolated single-thread
- * queue to prevent deadlock against flushing the normal work-queue.
- */
-static int __init irqfd_module_init(void)
-{
- irqfd_cleanup_wq = create_singlethread_workqueue("kvm-irqfd-cleanup");
- if (!irqfd_cleanup_wq)
- return -ENOMEM;
-
- return 0;
-}
-
-static void __exit irqfd_module_exit(void)
-{
- destroy_workqueue(irqfd_cleanup_wq);
-}
-
-module_init(irqfd_module_init);
-module_exit(irqfd_module_exit);
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [KVM PATCH v7 0/5] irqfd fixes and enhancements
2009-06-29 18:28 [KVM PATCH v7 0/5] irqfd fixes and enhancements Gregory Haskins
` (4 preceding siblings ...)
2009-06-29 18:29 ` [KVM PATCH v7 5/5] KVM: Make irqfd use slow-work for shutdown Gregory Haskins
@ 2009-07-01 8:53 ` Avi Kivity
2009-07-01 9:03 ` Michael S. Tsirkin
5 siblings, 1 reply; 8+ messages in thread
From: Avi Kivity @ 2009-07-01 8:53 UTC (permalink / raw)
To: Gregory Haskins; +Cc: kvm, linux-kernel, mst, davidel
On 06/29/2009 09:28 PM, Gregory Haskins wrote:
> (Applies to kvm.git/master:4631e094)
>
> The following is the latest attempt to fix the races in irqfd/eventfd, as
> well as restore DEASSIGN support. For more details, please read the patch
> headers. I've restored the slow-work variant of the logic as a separate
> patch (5/5) so we can get a better idea of the true difference between
> the workqueue approach and the slow-work style. I am personally in favor
> of the slow-work approach since it doesnt require a mostly-idle+dedicated
> thread to hang around. But that is just me. 5/5 can be ignored, folded
> in to 3/5+4/5 as appropriate, or merged as is per the whim of Avi et. al.
>
I think a good compromise would be to create the workqueue when the
first VM is launched.
> As always, this series has been tested against the kvm-eventfd unit test
> with both 5/5 applied and unapplied, and everything appears to be
> functioning properly. You can download this test here:
>
> ftp://ftp.novell.com/dev/ghaskins/kvm-eventfd.tar.bz2
>
> I've included version 4 of Davide's eventfd patch (ported to kvm.git) so
> that its a complete reviewable series. Note, however, that there may be
> later versions of his patch to consider for merging, so we should
> coordinate with him.
>
Davide's patch was merged upstream. Please take a look to make sure
this patchset is compatible with what was merged.
Patchset looks good to me, but I'd appreciate an ack from Michael (or
anyone else who's interested).
--
error compiling committee.c: too many arguments to function
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [KVM PATCH v7 0/5] irqfd fixes and enhancements
2009-07-01 8:53 ` [KVM PATCH v7 0/5] irqfd fixes and enhancements Avi Kivity
@ 2009-07-01 9:03 ` Michael S. Tsirkin
0 siblings, 0 replies; 8+ messages in thread
From: Michael S. Tsirkin @ 2009-07-01 9:03 UTC (permalink / raw)
To: Avi Kivity; +Cc: Gregory Haskins, kvm, linux-kernel, davidel
On Wed, Jul 01, 2009 at 11:53:26AM +0300, Avi Kivity wrote:
> On 06/29/2009 09:28 PM, Gregory Haskins wrote:
>> (Applies to kvm.git/master:4631e094)
>>
>> The following is the latest attempt to fix the races in irqfd/eventfd, as
>> well as restore DEASSIGN support. For more details, please read the patch
>> headers. I've restored the slow-work variant of the logic as a separate
>> patch (5/5) so we can get a better idea of the true difference between
>> the workqueue approach and the slow-work style. I am personally in favor
>> of the slow-work approach since it doesnt require a mostly-idle+dedicated
>> thread to hang around. But that is just me. 5/5 can be ignored, folded
>> in to 3/5+4/5 as appropriate, or merged as is per the whim of Avi et. al.
>>
>
> I think a good compromise would be to create the workqueue when the
> first VM is launched.
>
>> As always, this series has been tested against the kvm-eventfd unit test
>> with both 5/5 applied and unapplied, and everything appears to be
>> functioning properly. You can download this test here:
>>
>> ftp://ftp.novell.com/dev/ghaskins/kvm-eventfd.tar.bz2
>>
>> I've included version 4 of Davide's eventfd patch (ported to kvm.git) so
>> that its a complete reviewable series. Note, however, that there may be
>> later versions of his patch to consider for merging, so we should
>> coordinate with him.
>>
>
> Davide's patch was merged upstream. Please take a look to make sure
> this patchset is compatible with what was merged.
>
> Patchset looks good to me, but I'd appreciate an ack from Michael (or
> anyone else who's interested).
For patches 1,3,4:
Acked-by: Michael S. Tsirkin <mst@redhat.com>
As for 5: IMHO, let's keep it simple meanwhile.
--
MST
^ permalink raw reply [flat|nested] 8+ messages in thread