Re: [kvm-devel] [PATCH 05/10] KVM: Adds ability to signal userspace using a file-descriptor
On Mon, 14 May 2007, Avi Kivity wrote: > Gregory Haskins wrote: > > > > > > Is having a read() (or a write()) actually necessary? > > > > > > > Based on what I know: yes. It could be a case of ignorance, however ;) > > > > Heres why I think its necessary: You need poll to simply tell you when > > something is pending. You can't clear the pending status in poll because > > you cannot predict the internal access pattern (e.g. I assume it could be > > polled multiple times by the kernel without returning immediately to > > userspace). Therefore, you need a second method to actually clear the > > pending "signal", which I use the read() method for. I can be convinced > > otherwise, but that was my original thinking. > > > > I think you are right, but am cc'ing an expert. Davide, we're using an fd to > signal something to userspace, but have nothing to actually read() or write(). > Is a read() or write() avoidable? I don't know exactly what you want to do, but signalfd signal de-queueing competes with the standard Linux signal delivery, if signals are not blocked. So if you don't want to read() the signal, you can just leave the signal unblocked, and it'll be delivered to the signal handler. You can even leave the signal blocked and avoid read(), but poll() on the signalfd will always return POLLIN if the sigmask includes the pending signal. - Davide - 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
Re: [kvm-devel] [PATCH 05/10] KVM: Adds ability to signal userspace using a file-descriptor
On Mon, 14 May 2007, Avi Kivity wrote: > Davide Libenzi wrote: > > On Mon, 14 May 2007, Avi Kivity wrote: > > > > > > > Gregory Haskins wrote: > > > > > > > > Is having a read() (or a write()) actually necessary? > > > > > > > > > Based on what I know: yes. It could be a case of ignorance, however ;) > > > > > > > > Heres why I think its necessary: You need poll to simply tell you when > > > > something is pending. You can't clear the pending status in poll > > > > because > > > > you cannot predict the internal access pattern (e.g. I assume it could > > > > be > > > > polled multiple times by the kernel without returning immediately to > > > > userspace). Therefore, you need a second method to actually clear the > > > > pending "signal", which I use the read() method for. I can be convinced > > > > otherwise, but that was my original thinking. > > > > > > > I think you are right, but am cc'ing an expert. Davide, we're using an fd > > > to > > > signal something to userspace, but have nothing to actually read() or > > > write(). > > > Is a read() or write() avoidable? > > > > > > > I don't know exactly what you want to do, but signalfd signal de-queueing > > competes with the standard Linux signal delivery, if signals are not > > blocked. > > So if you don't want to read() the signal, you can just leave the signal > > unblocked, and it'll be delivered to the signal handler. > > You can even leave the signal blocked and avoid read(), but poll() on the > > signalfd will always return POLLIN if the sigmask includes the pending > > signal. > > > > This is not about a real signal. We have an fd (for a pseudo filesystem) > which wants to indicate its readiness to select(), but which doesn't have any > real data to produce. Is it possible to implement this without a read() or a > write()? > > We're also looking at using an eventfd for this, so this may be moot. Do you close the signaled fd after receiving the signal/event? If you don't close it, eventfd will always return ready (POLLIN). With the signalfd, you can leave the signal un-blocked and the signal will be delivered through the standard signal delivery, and the ready condition will be cleared. - Davide - 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
Re: [kvm-devel] [PATCH 05/10] KVM: Adds ability to signal userspace using a file-descriptor
On Mon, 14 May 2007, Gregory Haskins wrote:
> >>> On Mon, May 14, 2007 at 1:23 PM, in message <[EMAIL PROTECTED]>,
> Avi Kivity <[EMAIL PROTECTED]> wrote:
> >
> >> Do you close the signaled fd after receiving the signal/event? If you
> >> don't close it, eventfd will always return ready (POLLIN).
> >>
> >
> > We don't. Anyway, that's what we thought. Thanks for the confirmation.
> >
>
> And plus I just finished converting to Davide's eventfd, so its moot ;)
>
> On that topic, I could use some advice:
>
> I was originally planning on adding a new ioctl like KVM_VCPU_CREATE_EVENTFD
> which would allocate a new eventfd and return it. However, I soon realized
> that the only method to create an eventfd is sys_eventfd(), which is not
> exported by the eventfd.h headerfile (presumably this must be a new system
> call).
>
> So based on that, I figured I would change the model so that the usermode app
> should call the eventfd open() call on its own, and then they could register
> the fd with me. So KVM_VCPU_CREATE_EVENTFD becomes KVM_VCPU_SET_EVENTFD
> (where -1 "unregisters" it).
>
> Does this sound like a reasonable approach? If so, how does the usermode app
> actually open the eventfd today? Is there a new glibc that I need to get the
> new system call? Or can the app use open() somehow? If open(), what is the
> path that should be specified?
>
> Conversely, if my first approach was the right one how do I invoke the
> sys_eventfd()? Is there a way to invoke system calls in kernel mode? A
> better way?
>
> Any advice appreciated.
The eventfd syscall is defined in include/linux/syscalls.h
>From userspace, till glibc aligns:
#include
#ifndef __NR_eventfd
#if defined(__x86_64__)
#define __NR_eventfd 283
#elif defined(__i386__)
#define __NR_eventfd 323
#else
#error Cannot detect your architecture!
#endif
#endif
static int eventfd(int count) {
return syscall(__NR_eventfd, count);
}
If the kernel side receives an fd from userspace, it must use:
file = eventfd_fget(fd);
if (IS_ERR(file))
eventfd_signal(file, 1);
fput(file);
- Davide
-
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
Re: [kvm-devel] [PATCH 05/10] KVM: Adds ability to signal userspace using a file-descriptor
On Mon, 14 May 2007, Gregory Haskins wrote: > Thanks Davide, > That is very helpful. Is there any reason why we can't export > eventfd_signal() and eventfd_fget() to modules? I'll push a patch for that. - Davide - 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
Re: [kvm-devel] [PATCH 5/9] KVM: Adds ability to signal userspace using a file-descriptor
On Tue, 15 May 2007, Gregory Haskins wrote:
> >>> On Tue, May 15, 2007 at 3:45 AM, in message <[EMAIL PROTECTED]>,
> Avi Kivity <[EMAIL PROTECTED]> wrote:
> > Gregory Haskins wrote:
> >> Signed- off- by: Gregory Haskins <[EMAIL PROTECTED]>
> >> ---
> >>
> >> drivers/kvm/kvm.h |1 +
> >> drivers/kvm/kvm_main.c | 52
> >> ++--
> >> include/linux/kvm.h|1 +
> >> 3 files changed, 48 insertions(+), 6 deletions(- )
> >>
> >> diff -- git a/drivers/kvm/kvm.h b/drivers/kvm/kvm.h
> >> index 7b5d5e6..f5731c4 100644
> >> --- a/drivers/kvm/kvm.h
> >> +++ b/drivers/kvm/kvm.h
> >> @@ - 333,6 +333,7 @@ struct kvm_vcpu_irq {
> >>int deferred;
> >>struct task_struct *task;
> >>int guest_mode;
> >> + int eventfd;
> >>
> >
> > Best to convert the fd to a filp when you install it. This avoids the
> > conversion during runtime and allows you to do error checking earlier.
>
> That was my initial impression also, but then I realized there was a
> problem with that: Eventfd doesnt appear to have any way to notify
> other entities when the fd is closed. Therefore the filp could be left
> dangling in this case. By using the fd instead, I can validate the
> pointer each time I need it. Perhaps Davide will have a suggestion
> here.
I don't know how critical is the path where you will be doing check. The
eventfd_fget() is pretty fast, so if you're not looking at a performance
critical path, I'd suggest that. Otherwise you can do an early
eventfd_get, and keep the file*. If you have no the ways to know if the
userspace disconnected, an atomic_read(&file->f_count)==1 will tell you
that you're the only owner of the file* (that is, userspace closed the
eventfd descriptor). I'd give preference to the former option though.
Avi, as you may have read from lkml, Andrew prefers the eventfd symbols
export to come through the kvm tree, since they do not want to export
symbols that so far has no module users.
- Davide
-
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
Re: [kvm-devel] [PATCH 5/9] KVM: Adds ability to signal userspace using a file-descriptor
On Tue, 15 May 2007, Christoph Hellwig wrote: > On Tue, May 15, 2007 at 12:18:17PM -0400, Gregory Haskins wrote: > > >>> On Tue, May 15, 2007 at 11:40 AM, in message > > <[EMAIL PROTECTED]>, Davide Libenzi > > <[EMAIL PROTECTED]> wrote: > > > > > > I don't know how critical is the path where you will be doing check. The > > > eventfd_fget() is pretty fast, so if you're not looking at a performance > > > critical path, I'd suggest that. Otherwise you can do an early > > > eventfd_get, and keep the file*. If you have no the ways to know if the > > > userspace disconnected, an atomic_read(&file- >f_count)==1 will tell you > > > that you're the only owner of the file* (that is, userspace closed the > > > eventfd descriptor). I'd give preference to the former option though. > > > > Thanks for the insight, Davide. It sounds like we can probably stay with > > the way I have it for now, and keep the file->f_count idea in our back > > pocket should a performance problem arise. > > accessing file->f_count is not allowed for drivers. It took us quite a > bit of effort to clean up all users a while ago, and it turned out most > of them were rather buggy. Right, you could use the file_count() macro :) Seriuosly, if this (doing an eventfd_fget+fput) becomes a problem, I can have the check done in eventfd_signal() and return a proper error code. - Davide - 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
Re: [kvm-devel] [PATCH 5/9] KVM: Adds ability to signal userspace using a file-descriptor
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. 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? - Davide - 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
Re: [kvm-devel] [PATCH 5/9] KVM: Adds ability to signal userspace using a file-descriptor
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.
> > 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).
- Davide
Index: linux-2.6.22-rc1-git1.mod/fs/eventfd.c
===
--- linux-2.6.22-rc1-git1.mod.orig/fs/eventfd.c 2007-05-16 15:10:26.0
-0700
+++ linux-2.6.22-rc1-git1.mod/fs/eventfd.c 2007-05-17 10:53:38.0
-0700
@@ -16,9 +16,15 @@
#include
#include
+#define EVENTFD_FLG_SHUTDOWN (1 << 0)
+
+#define EVENTFD_FILE(f)((f)->f_op == &eventfd_fops)
+
struct eventfd_ctx {
spinlock_t lock;
wait_queue_head_t wqh;
+ unsigned long flags;
+
/*
* Every time that a write(2) is performed on an eventfd, the
* value of the __u64 being written is added to "count" and a
@@ -28,37 +34,72 @@
* issue a wakeup.
*/
__u64 count;
+
+ /*
+* List of callbacks to be invo
Re: [kvm-devel] [PATCH 5/9] KVM: Adds ability to signal userspace using a file-descriptor
On Thu, 17 May 2007, Davide Libenzi wrote:
> /* 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);
+ fput(c->evfile);
> /* Then return fd to userspace in some way */
> ...
> }
You need an fput() there, to bring f_count back to 1, of course ;)
- Davide
-
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
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
[CC-ing Al too] On Wed, 27 Feb 2008, Roland Dreier wrote: > The anonymous inodes interface anon_inode_getfd() calls fd_install() > for the newly created fd, which does not work for some use cases where > the caller must do futher initialization before exposing the file to > userspace. This is also probably not the safest interface, since the > caller must be sure that it is OK if userspace closes the fd before > anon_inode_getfd() even returns. I believe Al changed the interface to not give out inode* and file*, *and* call fd_install() inside it. I'd slightly prefer Al version, although I don't see any major problems in this one too. - Davide - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
On Wed, 27 Feb 2008, Roland Dreier wrote: > > > The anonymous inodes interface anon_inode_getfd() calls fd_install() > > > for the newly created fd, which does not work for some use cases where > > > the caller must do futher initialization before exposing the file to > > > userspace. This is also probably not the safest interface, since the > > > caller must be sure that it is OK if userspace closes the fd before > > > anon_inode_getfd() even returns. > > > > I believe Al changed the interface to not give out inode* and file*, *and* > > call fd_install() inside it. I'd slightly prefer Al version, although I > > don't see any major problems in this one too. > > Any pointer to that patch? A web search for "viro" and > "anon_inode_getfd" doesn't turn up anything likely looking. It's inside his vfs git tree: http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commit;h=49be4f8114e6ff0efdab10ebba2493fb67bc3034 I'm fine with both approaches. - Davide - This SF.net email is sponsored by: Microsoft Defy all challenges. Microsoft(R) Visual Studio 2008. http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/ ___ kvm-devel mailing list kvm-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
On Wed, 27 Feb 2008, Roland Dreier wrote:
> >
> http://git.kernel.org/?p=linux/kernel/git/viro/vfs-2.6.git;a=commit;h=49be4f8114e6ff0efdab10ebba2493fb67bc3034
>
> Actually, looking closer at the kvm changes here, I think that
> create_vcpu_fd() needs the same treatment as kvm_dev_ioctl_create_vm()
> gets in the patch because of the race I mentioned in the changelog
> for my patch: otherwise kvm_vcpu_release() could drop the last
> reference to vcpu->kvm->filp before the get_file() gets an extra
> reference.
>
> I'm beginning to think that moving the fd_install() out of
> anon_inode_getfd() really is worth it to make a safer interface.
If we let the caller call fd_install(), then it may be messed up WRT
cleanup (fd, file, inode).
How about removing the inode pointer handout altogether, and *doing*
fd_install() inside anon_inode_getfd() like:
if (pfile != NULL) {
get_file(file);
*pfile = file;
}
fd_install(fd, file);
In this way, if the caller want the file* back, he gets the reference
bumped before fd_install().
- Davide
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
Re: [kvm-devel] [PATCH/RFC 1/2] anon-inodes: Remove fd_install() from anon_inode_getfd()
On Thu, 28 Feb 2008, Roland Dreier wrote:
> > If we let the caller call fd_install(), then it may be messed up WRT
> > cleanup (fd, file, inode).
>
> Yes, that is a tiny bit tricky (need to call put_unused_fd() if you
> don't install the fd).
>
> > How about removing the inode pointer handout altogether, and *doing*
> > fd_install() inside anon_inode_getfd() like:
> >
> >if (pfile != NULL) {
> >get_file(file);
> >*pfile = file;
> >}
> >fd_install(fd, file);
> >
> > In this way, if the caller want the file* back, he gets the reference
> > bumped before fd_install().
>
> I think that may be a bit cleaner than Al's approach, but it still
> leaves the same trap that create_vcpu_fd() falls into. The current
> code is:
>
> static int create_vcpu_fd(struct kvm_vcpu *vcpu)
> {
> int fd, r;
> struct inode *inode;
> struct file *file;
>
> r = anon_inode_getfd(&fd, &inode, &file,
>"kvm-vcpu", &kvm_vcpu_fops, vcpu);
> if (r)
> return r;
> atomic_inc(&vcpu->kvm->filp->f_count);
> return fd;
> }
>
> and with your proposal, the natural way to write that becomes:
>
> static int create_vcpu_fd(struct kvm_vcpu *vcpu)
> {
> int fd, r;
>
> r = anon_inode_getfd(&fd, NULL,
>"kvm-vcpu", &kvm_vcpu_fops, vcpu);
> if (r)
> return r;
> atomic_inc(&vcpu->kvm->filp->f_count);
> return fd;
> }
I don't know KVM code, but can't the "private_data" setup be completed
before calling anon_inode_getfd()?
Or ...
static int create_vcpu_fd(struct kvm_vcpu *vcpu)
{
int fd, r;
get_file(vcpu->kvm->filp);
r = anon_inode_getfd(&fd, NULL,
"kvm-vcpu", &kvm_vcpu_fops, vcpu);
if (r) {
fput(vcpu->kvm->filp);
return r;
}
return fd;
}
Hmm...?
- Davide
-
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse012070mrt/direct/01/
___
kvm-devel mailing list
kvm-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/kvm-devel
