[patch] epoll - send POLLHUP on ->release

[patch] epoll - send POLLHUP on ->release

[Davide Libenzi]

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 <davidel@xmailserver.org>


- 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;
 }
 

Re: [patch] epoll - send POLLHUP on ->release

[Gregory Haskins]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

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 <davidel@xmailserver.org>
>   

Tested-by: Gregory Haskins <ghaskins@novell.com>
>
> - 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;
>  }
>  
>   



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]

Re: [patch] epoll - send POLLHUP on ->release

[Andrew Morton]

On Wed, 03 Jun 2009 15:20:26 -0400
Gregory Haskins <ghaskins@novell.com> 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 <davidel@xmailserver.org>
> >   
> 
> Tested-by: Gregory Haskins <ghaskins@novell.com>

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?

Re: [patch] epoll - send POLLHUP on ->release

[Davide Libenzi]

On Wed, 3 Jun 2009, Andrew Morton wrote:

> On Wed, 03 Jun 2009 15:20:26 -0400
> Gregory Haskins <ghaskins@novell.com> 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 <davidel@xmailserver.org>
> > >   
> > 
> > Tested-by: Gregory Haskins <ghaskins@novell.com>
> 
> 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

Re: [patch] epoll - send POLLHUP on ->release

[Gregory Haskins]

[-- Attachment #1: Type: text/plain, Size: 2115 bytes --]

Andrew Morton wrote:
> On Wed, 03 Jun 2009 15:20:26 -0400
> Gregory Haskins <ghaskins@novell.com> 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 <davidel@xmailserver.org>
>>>   
>>>       
>> Tested-by: Gregory Haskins <ghaskins@novell.com>
>>     
>
> 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 266 bytes --]