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/ _______________________________________________ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel