The following patch allows waiters to be notified about the eventfd file*
going away, and give them a change to unregister from the wait queue.
This is turn allows eventfd users to use the eventfd file* w/out
holding a live reference to it.
After the eventfd user callbacks returns, any usage of the eventfd file*
should be dropped. The eventfd user callback can acquire sleepy locks
since it is invoked lockless.
Signed-off-by: Davide Libenzi <[email protected]>
- Davide
---
fs/eventfd.c | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
Index: linux-2.6.mod/fs/eventfd.c
===================================================================
--- linux-2.6.mod.orig/fs/eventfd.c 2009-05-27 12:10:03.000000000 -0700
+++ linux-2.6.mod/fs/eventfd.c 2009-05-27 12:16:57.000000000 -0700
@@ -59,7 +59,15 @@ int eventfd_signal(struct file *file, in
static int eventfd_release(struct inode *inode, struct file *file)
{
- kfree(file->private_data);
+ 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);
return 0;
}
Davide Libenzi wrote:
> The following patch allows waiters to be notified about the eventfd file*
> going away, and give them a change to unregister from the wait queue.
> This is turn allows eventfd users to use the eventfd file* w/out
> holding a live reference to it.
> After the eventfd user callbacks returns, any usage of the eventfd file*
> should be dropped. The eventfd user callback can acquire sleepy locks
> since it is invoked lockless.
>
>
>
> Signed-off-by: Davide Libenzi <[email protected]>
>
Tested-by: Gregory Haskins <[email protected]>
>
> - Davide
>
>
> ---
> fs/eventfd.c | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> Index: linux-2.6.mod/fs/eventfd.c
> ===================================================================
> --- linux-2.6.mod.orig/fs/eventfd.c 2009-05-27 12:10:03.000000000 -0700
> +++ linux-2.6.mod/fs/eventfd.c 2009-05-27 12:16:57.000000000 -0700
> @@ -59,7 +59,15 @@ int eventfd_signal(struct file *file, in
>
> static int eventfd_release(struct inode *inode, struct file *file)
> {
> - kfree(file->private_data);
> + 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);
> return 0;
> }
>
>
On Wed, 03 Jun 2009 15:20:26 -0400
Gregory Haskins <[email protected]> wrote:
> Davide Libenzi wrote:
> > The following patch allows waiters to be notified about the eventfd file*
> > going away, and give them a change to unregister from the wait queue.
> > This is turn allows eventfd users to use the eventfd file* w/out
> > holding a live reference to it.
> > After the eventfd user callbacks returns, any usage of the eventfd file*
> > should be dropped. The eventfd user callback can acquire sleepy locks
> > since it is invoked lockless.
> >
> >
> >
> > Signed-off-by: Davide Libenzi <[email protected]>
> >
>
> Tested-by: Gregory Haskins <[email protected]>
Confused. Did you test this with some new kernel patch, or with some
existing kernel code?
If the latter, what code are we talking about here and what was the test
case and what went wrong when using the current mainline
implementation?
On Wed, 3 Jun 2009, Andrew Morton wrote:
> On Wed, 03 Jun 2009 15:20:26 -0400
> Gregory Haskins <[email protected]> wrote:
>
> > Davide Libenzi wrote:
> > > The following patch allows waiters to be notified about the eventfd file*
> > > going away, and give them a change to unregister from the wait queue.
> > > This is turn allows eventfd users to use the eventfd file* w/out
> > > holding a live reference to it.
> > > After the eventfd user callbacks returns, any usage of the eventfd file*
> > > should be dropped. The eventfd user callback can acquire sleepy locks
> > > since it is invoked lockless.
> > >
> > >
> > >
> > > Signed-off-by: Davide Libenzi <[email protected]>
> > >
> >
> > Tested-by: Gregory Haskins <[email protected]>
>
> Confused. Did you test this with some new kernel patch, or with some
> existing kernel code?
>
> If the latter, what code are we talking about here and what was the test
> case and what went wrong when using the current mainline
> implementation?
Andrew, this is not related to a bug in mainline. It's an helper for
IRQFD, that allows them to not hold reference counts over the underlying
eventfd.
I believe this has been tested using Gregory's IRQFD code.
- Davide
Andrew Morton wrote:
> On Wed, 03 Jun 2009 15:20:26 -0400
> Gregory Haskins <[email protected]> wrote:
>
>
>> Davide Libenzi wrote:
>>
>>> The following patch allows waiters to be notified about the eventfd file*
>>> going away, and give them a change to unregister from the wait queue.
>>> This is turn allows eventfd users to use the eventfd file* w/out
>>> holding a live reference to it.
>>> After the eventfd user callbacks returns, any usage of the eventfd file*
>>> should be dropped. The eventfd user callback can acquire sleepy locks
>>> since it is invoked lockless.
>>>
>>>
>>>
>>> Signed-off-by: Davide Libenzi <[email protected]>
>>>
>>>
>> Tested-by: Gregory Haskins <[email protected]>
>>
>
> Confused. Did you test this with some new kernel patch, or with some
> existing kernel code?
>
>
Hi Andrew,
Davide created this patch in response to a problem we were trying to
solve in kvm.git. I took this patch in question, and combined it with a
proposed patch for KVM and tested it out. You can find the thread here:
http://lkml.org/lkml/2009/6/2/276
I built a test harness and verified that calling close() on an eventfd
does indeed generate a POLLHUP wakeup, and that my code properly cleans
up when it gets the POLLHUP
Acceptance of my kvm optimization is predicated on Davide's patch going
in first, so I asked him to submit it formally. I figured I should
chime in my findings in case it matters for upstream acceptance.
(Apologies if this is not the normal/acceptable "tested-by"
protocol...tested-by tag noob here ;)
> If the latter, what code are we talking about here and what was the test
> case and what went wrong when using the current mainline
> implementation?
>
>
Its not what went wrong per se. Its more of a case of eliminating the
need for an awkward work-around I had to do when eventfd didn't provide
a release notification. You can find the details in my patch 2/2
header, available here for your convenience:
http://lkml.org/lkml/2009/6/2/278
Regards,
-Greg