Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions

From: Gregory Haskins <ghaskins@novell.com>
To: avi@redhat.com
Cc: Davide Libenzi <davidel@xmailserver.org>,
	Rusty Russell <rusty@rustcorp.com.au>,
	mst@redhat.com, kvm@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	paulmck@linux.vnet.ibm.com, Ingo Molnar <mingo@elte.hu>
Subject: Re: [PATCH 3/3] eventfd: add internal reference counting to fix notifier race conditions
Date: Thu, 25 Jun 2009 13:32:22 -0400	[thread overview]
Message-ID: <4A43B4A6.8050001@novell.com> (raw)
In-Reply-To: <alpine.DEB.1.10.0906250922530.9517@makko.or.mcafeemobile.com>

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

Davide Libenzi wrote:
> On Thu, 25 Jun 2009, Rusty Russell wrote:
>
>   
>> On Thu, 25 Jun 2009 08:15:11 am Davide Libenzi wrote:
>>     
>>> Some components would like to know if userspace dropped the fd, and take
>>> proper action accordingly (release resources, drop module instances,
>>> etc...).
>>>       
>> Like to know?  Possibly.  Need to know?  Not anything I've seen so far.
>>
>> If userspace creates the fd, component grab a ref and if userspace wants that 
>> fd completely freed must close the fd *and* tell component.  Simple, race free 
>> and explicit.  All wins.
>>
>> As this discussion shows, doing some kind of implies non-reference is hard, 
>> complex and racy.
>>     
>
> Easier, we agree. Not doing anything is always easier, provided the 
> userspace interface allows for it.
> Cleaner, I'm not sure. Again, it depends from the userspace interface, but 
> usually when you close(2) something, you expect the kernel to react 
> accordingly, and not on relying on userspace issuing extra calls in order 
> to proper cleanup the kernel context.
> This is even more true when the eventfd is the sole handle to the visisble 
> userspace interface.
> In such cases, not taking proper action on close(2) and requiring extra 
> calls, would lead to designing interface with the close-no-really-i-mean-it
> patterns.
>   

Avi,
  Davide's argument is compelling in favor of considering the latest
irqfd fixes I pushed.  You could run into weirdness if we tried to
revert to the old requirement on explicit DEASSIGN ioctl once we start
doing things like handing the fd to other apps/components.  Its cleaner
IMO to retain the implicit-deassign-on-close behavior.

Since v5 also restores optional explicit DEASSIGN support, the only
downside to accepting my patches (vs reverting POLLHUP completely) is
the complexity required to deal with POLLHUP.  But this has been
substantially reduced and clarified since yesterdays review, and I am
reasonably confident the protocol is sound.

Please take a close look at it and consider for merging, if you would.

Thanks,
-Greg

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