Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 01:41:05PM -0400, Gregory Haskins wrote: And having close not clean up the state unless you do an ioctl first is very messy IMO - I don't think you'll find any such examples in kernel. I agree, and that is why I am advocating this POLLHUP solution. It was only this other way to begin with because the technology didn't exist until Davide showed me the light. Problem with your request is that I already looked into what is essentially a bi-directional reference problem (for a different reason) when I started the POLLHUP series. Its messy to do this in a way that doesn't negatively impact the fast path (introducing locking, etc) or make my head explode making sure it doesn't race. Afaict, we would need to solve this problem to do what you are proposing (patches welcome). If this hybrid decoupled-deassign + unified-close is indeed an important feature set, I suggest that we still consider this POLLHUP series for inclusion, and then someone can re-introduce DEASSIGN support in the future as a CAP bit extension. That way we at least get the desirable close() properties that we both seem in favor of, and get this advanced use case when we need it (and can figure out the locking design). FWIW, I took a look and yes, it is non-trivial. I concur, we can always add the deassign ioctl later. I agree that deassign is needed for reasons of symmetry, and that it can be added later. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
Avi Kivity wrote: Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 01:41:05PM -0400, Gregory Haskins wrote: And having close not clean up the state unless you do an ioctl first is very messy IMO - I don't think you'll find any such examples in kernel. I agree, and that is why I am advocating this POLLHUP solution. It was only this other way to begin with because the technology didn't exist until Davide showed me the light. Problem with your request is that I already looked into what is essentially a bi-directional reference problem (for a different reason) when I started the POLLHUP series. Its messy to do this in a way that doesn't negatively impact the fast path (introducing locking, etc) or make my head explode making sure it doesn't race. Afaict, we would need to solve this problem to do what you are proposing (patches welcome). If this hybrid decoupled-deassign + unified-close is indeed an important feature set, I suggest that we still consider this POLLHUP series for inclusion, and then someone can re-introduce DEASSIGN support in the future as a CAP bit extension. That way we at least get the desirable close() properties that we both seem in favor of, and get this advanced use case when we need it (and can figure out the locking design). FWIW, I took a look and yes, it is non-trivial. I concur, we can always add the deassign ioctl later. I agree that deassign is needed for reasons of symmetry, and that it can be added later. Cool. FYI: Davide's patch has been accepted into -mm (Andrew CC'd). I am not sure of the protocol here, but I assume this means you can now safely pull it from -mm into kvm.git so the prerequisite for 2/2 is properly met. -Greg signature.asc Description: OpenPGP digital signature
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
Avi Kivity wrote: Gregory Haskins wrote: I agree that deassign is needed for reasons of symmetry, and that it can be added later. Cool. FYI: Davide's patch has been accepted into -mm (Andrew CC'd). I am not sure of the protocol here, but I assume this means you can now safely pull it from -mm into kvm.git so the prerequisite for 2/2 is properly met. I'm not sure either. But I think I saw a Thanks for catching that for 2/2? Ah, right! I queued that fix up eons ago after David's feedback and forgot that it was there waiting for me ;) Since Paul ok'd (I think?) the srcu design, and the only other feedback was the key-bitmap thing from Davide, I will go ahead and push a v2 with just that one fix (unless there is any other feedback?) -Greg signature.asc Description: OpenPGP digital signature
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
Gregory Haskins wrote: Since Paul ok'd (I think?) the srcu design, and the only other feedback was the key-bitmap thing from Davide, I will go ahead and push a v2 with just that one fix (unless there is any other feedback?) I'll do a detailed review on your next posting. When I see a long thread I go hide under the bed, where there is no Internet access. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
On Tue, Jun 02, 2009 at 01:41:05PM -0400, Gregory Haskins wrote: And having close not clean up the state unless you do an ioctl first is very messy IMO - I don't think you'll find any such examples in kernel. I agree, and that is why I am advocating this POLLHUP solution. It was only this other way to begin with because the technology didn't exist until Davide showed me the light. Problem with your request is that I already looked into what is essentially a bi-directional reference problem (for a different reason) when I started the POLLHUP series. Its messy to do this in a way that doesn't negatively impact the fast path (introducing locking, etc) or make my head explode making sure it doesn't race. Afaict, we would need to solve this problem to do what you are proposing (patches welcome). If this hybrid decoupled-deassign + unified-close is indeed an important feature set, I suggest that we still consider this POLLHUP series for inclusion, and then someone can re-introduce DEASSIGN support in the future as a CAP bit extension. That way we at least get the desirable close() properties that we both seem in favor of, and get this advanced use case when we need it (and can figure out the locking design). FWIW, I took a look and yes, it is non-trivial. I concur, we can always add the deassign ioctl later. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 01:41:05PM -0400, Gregory Haskins wrote: And having close not clean up the state unless you do an ioctl first is very messy IMO - I don't think you'll find any such examples in kernel. I agree, and that is why I am advocating this POLLHUP solution. It was only this other way to begin with because the technology didn't exist until Davide showed me the light. Problem with your request is that I already looked into what is essentially a bi-directional reference problem (for a different reason) when I started the POLLHUP series. Its messy to do this in a way that doesn't negatively impact the fast path (introducing locking, etc) or make my head explode making sure it doesn't race. Afaict, we would need to solve this problem to do what you are proposing (patches welcome). If this hybrid decoupled-deassign + unified-close is indeed an important feature set, I suggest that we still consider this POLLHUP series for inclusion, and then someone can re-introduce DEASSIGN support in the future as a CAP bit extension. That way we at least get the desirable close() properties that we both seem in favor of, and get this advanced use case when we need it (and can figure out the locking design). FWIW, I took a look and yes, it is non-trivial. I concur, we can always add the deassign ioctl later. Sounds good, Michael. Thanks! -Greg signature.asc Description: OpenPGP digital signature
[KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
(Applies to kvm.git/master:25deed73) Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly. I have it as an RFC for now because it needs Davide's official submission/SOB for patch 1/2, and it should get some eyeballs/acks on my SRCU usage before going in. I will submit the updated irqfd userspace which eschews the deassign() verb since we can now just use the close(fd) method alone. I will also address the userspace review comments from Avi. --- Davide Libenzi (1): eventfd: send POLLHUP on f_ops-release Gregory Haskins (1): kvm: use POLLHUP to close an irqfd instead of an explicit ioctl fs/eventfd.c| 10 +++ include/linux/kvm.h |2 - virt/kvm/eventfd.c | 177 +++ virt/kvm/kvm_main.c |3 + 4 files changed, 90 insertions(+), 102 deletions(-) -- Signature -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
On Tue, Jun 02, 2009 at 11:15:28AM -0400, Gregory Haskins wrote: (Applies to kvm.git/master:25deed73) Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly. I have it as an RFC for now because it needs Davide's official submission/SOB for patch 1/2, and it should get some eyeballs/acks on my SRCU usage before going in. I will submit the updated irqfd userspace which eschews the deassign() verb since we can now just use the close(fd) method alone. I will also address the userspace review comments from Avi. We are not killing the deassign though, do we? It's good to have that option e.g. for when we pass the fd to another process. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 11:15:28AM -0400, Gregory Haskins wrote: (Applies to kvm.git/master:25deed73) Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly. I have it as an RFC for now because it needs Davide's official submission/SOB for patch 1/2, and it should get some eyeballs/acks on my SRCU usage before going in. I will submit the updated irqfd userspace which eschews the deassign() verb since we can now just use the close(fd) method alone. I will also address the userspace review comments from Avi. We are not killing the deassign though, do we? Yes, it is not needed any more now that we have proper release-notification from eventfd. It's good to have that option e.g. for when we pass the fd to another process. Passing the fd to another app should up the underlying file reference count. If the producer app wants to deassign it simply calls close(fd) (as opposed to today where it calls DEASSIGN+close), but the reference count will allow the consuming app to leave the eventfd's file open. Or am I misunderstanding you? -Greg signature.asc Description: OpenPGP digital signature
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
On Tue, Jun 02, 2009 at 12:14:15PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 11:15:28AM -0400, Gregory Haskins wrote: (Applies to kvm.git/master:25deed73) Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly. I have it as an RFC for now because it needs Davide's official submission/SOB for patch 1/2, and it should get some eyeballs/acks on my SRCU usage before going in. I will submit the updated irqfd userspace which eschews the deassign() verb since we can now just use the close(fd) method alone. I will also address the userspace review comments from Avi. We are not killing the deassign though, do we? Yes, it is not needed any more now that we have proper release-notification from eventfd. It's good to have that option e.g. for when we pass the fd to another process. Passing the fd to another app should up the underlying file reference count. If the producer app wants to deassign it simply calls close(fd) (as opposed to today where it calls DEASSIGN+close), but the reference count will allow the consuming app to leave the eventfd's file open. Or am I misunderstanding you? -Greg I think we want to keep supporting the deassign ioctl. This, even though close overlaps with it functionally somewhat. This allows qemu to pass eventfd to another process/device, and then block/unblock interrupts as seen by that process by assigning/deassigning irq to it. This is much easier and lightweight than asking another process to close the fd and passing another fd later. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 12:14:15PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 11:15:28AM -0400, Gregory Haskins wrote: (Applies to kvm.git/master:25deed73) Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly. I have it as an RFC for now because it needs Davide's official submission/SOB for patch 1/2, and it should get some eyeballs/acks on my SRCU usage before going in. I will submit the updated irqfd userspace which eschews the deassign() verb since we can now just use the close(fd) method alone. I will also address the userspace review comments from Avi. We are not killing the deassign though, do we? Yes, it is not needed any more now that we have proper release-notification from eventfd. It's good to have that option e.g. for when we pass the fd to another process. Passing the fd to another app should up the underlying file reference count. If the producer app wants to deassign it simply calls close(fd) (as opposed to today where it calls DEASSIGN+close), but the reference count will allow the consuming app to leave the eventfd's file open. Or am I misunderstanding you? -Greg I think we want to keep supporting the deassign ioctl. This, even though close overlaps with it functionally somewhat. This allows qemu to pass eventfd to another process/device, and then block/unblock interrupts as seen by that process by assigning/deassigning irq to it. This is much easier and lightweight than asking another process to close the fd and passing another fd later. Perhaps, but if that is the case we should just ignore this series and continue with the DEASSIGN+close methodology since it already provides that separation. Trying to do a hybrid is just messy. But in any case, I think that approach is flawed. DEASSIGN shouldn't be used as a mask in my opinion, and we shouldn't be reassigning a channel's meaning under the covers like that. If this is in fact a valid use case, we should have a separate GSI_MASK type operation that is independent of irqfd. Likewise, we really should pass a new fd if the gsi-routing is changing. Today there is a tight coupling of fd-to-gsi, and I think that makes sense to continue this association. -Greg signature.asc Description: OpenPGP digital signature
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
On Tue, Jun 02, 2009 at 12:34:12PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 12:14:15PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 11:15:28AM -0400, Gregory Haskins wrote: (Applies to kvm.git/master:25deed73) Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly. I have it as an RFC for now because it needs Davide's official submission/SOB for patch 1/2, and it should get some eyeballs/acks on my SRCU usage before going in. I will submit the updated irqfd userspace which eschews the deassign() verb since we can now just use the close(fd) method alone. I will also address the userspace review comments from Avi. We are not killing the deassign though, do we? Yes, it is not needed any more now that we have proper release-notification from eventfd. It's good to have that option e.g. for when we pass the fd to another process. Passing the fd to another app should up the underlying file reference count. If the producer app wants to deassign it simply calls close(fd) (as opposed to today where it calls DEASSIGN+close), but the reference count will allow the consuming app to leave the eventfd's file open. Or am I misunderstanding you? -Greg I think we want to keep supporting the deassign ioctl. This, even though close overlaps with it functionally somewhat. This allows qemu to pass eventfd to another process/device, and then block/unblock interrupts as seen by that process by assigning/deassigning irq to it. This is much easier and lightweight than asking another process to close the fd and passing another fd later. Perhaps, but if that is the case we should just ignore this series and continue with the DEASSIGN+close methodology since it already provides that separation. Trying to do a hybrid is just messy. As I see it, it's the least evil. One-way ioctl operations on file descriptors are messier still. What's another example of an ioctl that can't be undone without closing the fd? And having close not clean up the state unless you do an ioctl first is very messy IMO - I don't think you'll find any such examples in kernel. But in any case, I think that approach is flawed. DEASSIGN shouldn't be used as a mask in my opinion, and we shouldn't be reassigning a channel's meaning under the covers like that. If this is in fact a valid use case, we should have a separate GSI_MASK type operation that is independent of irqfd. Likewise, we really should pass a new fd if the gsi-routing is changing. Today there is a tight coupling of fd-to-gsi, and I think that makes sense to continue this association. -Greg I'm not arguing that this use-case is not theoretical. Just that if you don't create the fd to connect to GSI, you shouln't ask the user to destroy it to disconnect. Who knows what else this eventfd descriptor can be used for? -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
On Tue, Jun 02, 2009 at 07:59:49PM +0300, Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 12:34:12PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 12:14:15PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 11:15:28AM -0400, Gregory Haskins wrote: (Applies to kvm.git/master:25deed73) Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly. I have it as an RFC for now because it needs Davide's official submission/SOB for patch 1/2, and it should get some eyeballs/acks on my SRCU usage before going in. I will submit the updated irqfd userspace which eschews the deassign() verb since we can now just use the close(fd) method alone. I will also address the userspace review comments from Avi. We are not killing the deassign though, do we? Yes, it is not needed any more now that we have proper release-notification from eventfd. It's good to have that option e.g. for when we pass the fd to another process. Passing the fd to another app should up the underlying file reference count. If the producer app wants to deassign it simply calls close(fd) (as opposed to today where it calls DEASSIGN+close), but the reference count will allow the consuming app to leave the eventfd's file open. Or am I misunderstanding you? -Greg I think we want to keep supporting the deassign ioctl. This, even though close overlaps with it functionally somewhat. This allows qemu to pass eventfd to another process/device, and then block/unblock interrupts as seen by that process by assigning/deassigning irq to it. This is much easier and lightweight than asking another process to close the fd and passing another fd later. Perhaps, but if that is the case we should just ignore this series and continue with the DEASSIGN+close methodology since it already provides that separation. Trying to do a hybrid is just messy. As I see it, it's the least evil. One-way ioctl operations on file descriptors are messier still. What's another example of an ioctl that can't be undone without closing the fd? And having close not clean up the state unless you do an ioctl first is very messy IMO - I don't think you'll find any such examples in kernel. But in any case, I think that approach is flawed. DEASSIGN shouldn't be used as a mask in my opinion, and we shouldn't be reassigning a channel's meaning under the covers like that. If this is in fact a valid use case, we should have a separate GSI_MASK type operation that is independent of irqfd. Likewise, we really should pass a new fd if the gsi-routing is changing. Today there is a tight coupling of fd-to-gsi, and I think that makes sense to continue this association. -Greg I'm not arguing that this use-case is not theoretical. Just that if you don't create the fd to connect to GSI, you shouln't ask the user to destroy it to disconnect. Who knows what else this eventfd descriptor can be used for? As a follow-up, here's another example: imagine an application that handles interrupt events from a thread by blocking on eventfd. To wake up this thread, we could reuse the same eventfd just by writing there. I might want to do this even after I don't get any interrupts anymore. -- MST -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [KVM-RFC PATCH 0/2] irqfd: use POLLHUP notification for close()
Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 12:34:12PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 12:14:15PM -0400, Gregory Haskins wrote: Michael S. Tsirkin wrote: On Tue, Jun 02, 2009 at 11:15:28AM -0400, Gregory Haskins wrote: (Applies to kvm.git/master:25deed73) Please see the header for 2/2 for a description. This patch series has been fully tested and appears to be working correctly. I have it as an RFC for now because it needs Davide's official submission/SOB for patch 1/2, and it should get some eyeballs/acks on my SRCU usage before going in. I will submit the updated irqfd userspace which eschews the deassign() verb since we can now just use the close(fd) method alone. I will also address the userspace review comments from Avi. We are not killing the deassign though, do we? Yes, it is not needed any more now that we have proper release-notification from eventfd. It's good to have that option e.g. for when we pass the fd to another process. Passing the fd to another app should up the underlying file reference count. If the producer app wants to deassign it simply calls close(fd) (as opposed to today where it calls DEASSIGN+close), but the reference count will allow the consuming app to leave the eventfd's file open. Or am I misunderstanding you? -Greg I think we want to keep supporting the deassign ioctl. This, even though close overlaps with it functionally somewhat. This allows qemu to pass eventfd to another process/device, and then block/unblock interrupts as seen by that process by assigning/deassigning irq to it. This is much easier and lightweight than asking another process to close the fd and passing another fd later. Perhaps, but if that is the case we should just ignore this series and continue with the DEASSIGN+close methodology since it already provides that separation. Trying to do a hybrid is just messy. As I see it, it's the least evil. Which? Leaving the code as is, or a hybrid? One-way ioctl operations on file descriptors are messier still. What's another example of an ioctl that can't be undone without closing the fd? -ENOPARSE And having close not clean up the state unless you do an ioctl first is very messy IMO - I don't think you'll find any such examples in kernel. I agree, and that is why I am advocating this POLLHUP solution. It was only this other way to begin with because the technology didn't exist until Davide showed me the light. Problem with your request is that I already looked into what is essentially a bi-directional reference problem (for a different reason) when I started the POLLHUP series. Its messy to do this in a way that doesn't negatively impact the fast path (introducing locking, etc) or make my head explode making sure it doesn't race. Afaict, we would need to solve this problem to do what you are proposing (patches welcome). If this hybrid decoupled-deassign + unified-close is indeed an important feature set, I suggest that we still consider this POLLHUP series for inclusion, and then someone can re-introduce DEASSIGN support in the future as a CAP bit extension. That way we at least get the desirable close() properties that we both seem in favor of, and get this advanced use case when we need it (and can figure out the locking design). But in any case, I think that approach is flawed. DEASSIGN shouldn't be used as a mask in my opinion, and we shouldn't be reassigning a channel's meaning under the covers like that. If this is in fact a valid use case, we should have a separate GSI_MASK type operation that is independent of irqfd. Likewise, we really should pass a new fd if the gsi-routing is changing. Today there is a tight coupling of fd-to-gsi, and I think that makes sense to continue this association. -Greg I'm not arguing that this use-case is not theoretical. Just that if you don't create the fd to connect to GSI, you shouln't ask the user to destroy it to disconnect. Well, thats just it. Today, you *do* create the eventfd to bundle with the gsi (take a look at my userspace patches..I posted some new ones today). The eventfd is returned after you specify the GSI via kvm_irqfd(). Thats why I am arguing that it is natural for close() to terminate the assignment. To me, this is consistent with other interfaces that return an fd (socket(), open(), etc). That said, if we are going to support your proposal going forward, we should probably change libkvm::kvm_irqfd() to take the fd as a parameter, instead of returning it. Who knows what else this eventfd descriptor can be used for? Perhaps, but you are exceeding the original design specifications of irqfd as it is, so we can't really predict what
