Re: [PATCH 5/9] KVM: Adds ability to signal userspace using a file-descriptor

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Davide Libenzi wrote:
> On Thu, 17 May 2007, Avi Kivity wrote:
>
>   
>> Davide Libenzi wrote:
>>     
>>> On Wed, 16 May 2007, Avi Kivity wrote:
>>>
>>>   
>>>       
>>>> IMO doing eventfd_fget() asap is best.  I much prefer refcounted pointers
>>>> to
>>>> handles in the kernel: it's easier to see what things point to, and there
>>>> is
>>>> to context needed for dereferencing.
>>>>     
>>>>         
>>> There are concerns (from Al and Christoph) about file lifetime tracking of
>>> an fd passed down to the kernel.
>>>   
>>>       
>> What concerns are these?
>>
>> I have my own concerns about bare fds, which can change their backing file,
>> and which are dependent on the context.  The kernel appears to be moving away
>> from handles to pointers, as seen by the pid -> task_struck/struct pid
>> conversion.
>>     
>
> The concern is that by keeping a reference to the file* you may end up 
> with a file that has no more userspace links. Although the file_count()==1 
> will tell you that you've the only reference left, this relies on you 
> doing the check. I really don't know how this will be used in KVM, so I 
> can't really say how well these concerns applies.
>
>   

That's perfectly fine for kvm (and as far as I can see, any other 
application: the user losing their eventfd reference is equivalent to 
the user ignoring any events, which should be survivable by the kernel).

>
>
>   
>>> Avi, how about you go the other way around? I expose you something like:
>>>
>>> long eventfd_create(unsigned int count, void (*release)(void *),
>>>                     void *priv);
>>>
>>> This returns you an eventfd (or error), and lets you install a callback to
>>> be used when the file_operations->release is called (basically, userspace
>>> closed the last file instance).
>>> Can KVM pass back an fd to userspace instead of the other way around?
>>>   
>>>       
>> That was my original thought, but when I saw your proposal I preferred that as
>> more flexible.
>>
>> If we go this way, I prefer to have a struct file * and do away with the
>> callback, but that brings us back to the file lifetime concerns.
>>     
>
> Doing the above requires some care too. Access to your copy of the file* 
> must be protected by a lock (you can sleep from inside the callback, so a 
> mutex is fine too). I'll sketch you some code:
>
>
> /* Defined in linux/eventfd.h */
> struct eventfd_relcb {
> 	struct list_head lnk;
> 	void (*proc)(struct eventfd_relcb *);
> };
>
> /* Your context data */
> struct your_ctx {
> 	whatever_lock your_lock;
> 	...
> 	struct eventfd_relcb rcb;
> 	struct file *evfile;
> };
>
> /* Your eventfd release callback */
> void rcb_callback(struct eventfd_relcb *rcb) {
> 	struct your_ctx *c = container_of(rcb, struct your_ctx, rcb);
>
> 	whatever_lock_lock(&c->your_lock);
> 	...
> 	c->evfile = NULL;
> 	whatever_lock_unlock(&c->your_lock);
> }
>
> /* Your notify userspace function */
> void notify_userspace(struct your_ctx *c) {
>
> 	whatever_lock_lock(&c->your_lock);
> 	if (c->evfile != NULL)
> 		eventfd_signal(c->evfile, 1);
> 	whatever_lock_unlock(&c->your_lock);
> }
>
> /* Your eventfd create/setup function (modulo error checks) */
> void setup_eventfd(struct your_ctx *c) {
> 	int fd;
>
> 	c->rcb.proc = rcb_callback;
> 	fd = eventfd_create(0, &c->rcb);
> 	c->evfile = eventfd_fget(fd);
> 	/* Then return fd to userspace in some way */
> 	...
> }
>
>
>
> If, by any chance, you need to detach yourself from the eventfd:
>
> void detach_eventfd(struct your_ctx *c) {
>
> 	whatever_lock_lock(&c->your_lock);
> 	if (c->evfile != NULL) {
> 		eventfd_del_release_cb(c->evfile, &c->rcb);
> 		/* And, optionally ... */
> 		eventfd_set_shutdown(c->evfile);
> 	}
> 	whatever_lock_unlock(&c->your_lock);
> }
>
>
> The eventfd_set_shutdown() function will make a POLLIN signaled to 
> userspace, and a read(2) on the eventfd will return 0 (like peer 
> disconnected sockets). Then userspace will know that no more eventfs will 
> show up in there.
> Tentative patch below (builds, not tested yet).
>
>
>   

Looks like a lot of code for what was supposed to be a code reduction... 
I think I'll look at Gregory's notification-free suggestion instead.


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/