* [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call [not found] <1516747387-31760-1-git-send-email-mst@redhat.com> @ 2018-01-23 22:44 ` Michael S. Tsirkin 2018-01-24 8:46 ` [Qemu-devel] " Greg Kurz 0 siblings, 1 reply; 10+ messages in thread From: Michael S. Tsirkin @ 2018-01-23 22:44 UTC (permalink / raw) To: qemu-devel Cc: Peter Maydell, Jose Ricardo Ziviani, Anton Blanchard, Daniel Henrique Barboza, Paolo Bonzini, kvm From: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> This commit partially reverts the commit 4fe6d78b2e because of issues reported in the virtio. Examples: $ qemu-system-ppc64 -cpu POWER8 -nographic -vga none -m 4G \ -M pseries,accel=kvm -netdev type=user,id=net0 \ -device virtio-net-pci,netdev=net0 -drive file=../disk.qcow2,if=virtio Populating /vdevice/nvram@71000001 Populating /vdevice/v-scsi@71000002 SCSI: Looking for devices 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+" Populating /pci@800000020000000 00 0000 (D) : 1af4 1000 virtio [ net ] Aborted $ qemu-system-x86_64 -m 4G -enable-kvm -drive file=util.qcow2,if=virtio Running QEMU with GTK 2.x is deprecated, and will be removed in a future release. Please switch to GTK 3.x instead [1] 5282 abort Reference http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html Reported-by: Anton Blanchard <anton@samba.org> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- accel/kvm/kvm-all.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c index 071f4f5..f290f48 100644 --- a/accel/kvm/kvm-all.c +++ b/accel/kvm/kvm-all.c @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener, if (r < 0) { abort(); } - - if (e->cleanup) { - e->cleanup(e); - } } static void kvm_io_ioeventfd_add(MemoryListener *listener, -- MST ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call 2018-01-23 22:44 ` [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call Michael S. Tsirkin @ 2018-01-24 8:46 ` Greg Kurz 2018-01-24 9:05 ` Greg Kurz 0 siblings, 1 reply; 10+ messages in thread From: Greg Kurz @ 2018-01-24 8:46 UTC (permalink / raw) To: Michael S. Tsirkin Cc: qemu-devel, Peter Maydell, kvm, Jose Ricardo Ziviani, Anton Blanchard, Paolo Bonzini, Daniel Henrique Barboza Please note that Peter usually doesn't work on Wednesdays. The master branch might remain broken for everyone until tomorrow... :-\ And I don't think this is the right fix anyway. See below. On Wed, 24 Jan 2018 00:44:14 +0200 "Michael S. Tsirkin" <mst@redhat.com> wrote: > From: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> > > This commit partially reverts the commit 4fe6d78b2e because of issues > reported in the virtio. > > Examples: > > $ qemu-system-ppc64 -cpu POWER8 -nographic -vga none -m 4G \ > -M pseries,accel=kvm -netdev type=user,id=net0 \ > -device virtio-net-pci,netdev=net0 -drive file=../disk.qcow2,if=virtio > > Populating /vdevice/nvram@71000001 > Populating /vdevice/v-scsi@71000002 > SCSI: Looking for devices > 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+" > Populating /pci@800000020000000 > 00 0000 (D) : 1af4 1000 virtio [ net ] > Aborted > > $ qemu-system-x86_64 -m 4G -enable-kvm -drive file=util.qcow2,if=virtio > > Running QEMU with GTK 2.x is deprecated, and will be removed > in a future release. Please switch to GTK 3.x instead > [1] 5282 abort > > Reference http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html > > Reported-by: Anton Blanchard <anton@samba.org> > Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> > Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > accel/kvm/kvm-all.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > index 071f4f5..f290f48 100644 > --- a/accel/kvm/kvm-all.c > +++ b/accel/kvm/kvm-all.c > @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener, > if (r < 0) { > abort(); > } > - > - if (e->cleanup) { > - e->cleanup(e); > - } This looks wrong as the cleanup is expected to do things like closing fds: static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier) { /* Test and clear notifier after disabling event, * in case poll callback didn't have time to run. */ virtio_queue_host_notifier_read(notifier); event_notifier_cleanup(notifier); } void event_notifier_cleanup(EventNotifier *e) { if (e->rfd != e->wfd) { close(e->rfd); } close(e->wfd); e->rfd = -1; e->wfd = -1; e->cleanup = NULL; } And indeed, with this patch applied, QEMU leaks eventfds on every machine reset. > } > > static void kvm_io_ioeventfd_add(MemoryListener *listener, ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call 2018-01-24 8:46 ` [Qemu-devel] " Greg Kurz @ 2018-01-24 9:05 ` Greg Kurz 2018-01-24 9:14 ` Paolo Bonzini 0 siblings, 1 reply; 10+ messages in thread From: Greg Kurz @ 2018-01-24 9:05 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Peter Maydell, kvm, Jose Ricardo Ziviani, qemu-devel, Anton Blanchard, Paolo Bonzini, Daniel Henrique Barboza On Wed, 24 Jan 2018 09:46:21 +0100 Greg Kurz <groug@kaod.org> wrote: > Please note that Peter usually doesn't work on Wednesdays. The master branch > might remain broken for everyone until tomorrow... :-\ > > And I don't think this is the right fix anyway. See below. > > On Wed, 24 Jan 2018 00:44:14 +0200 > "Michael S. Tsirkin" <mst@redhat.com> wrote: > > > From: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> > > > > This commit partially reverts the commit 4fe6d78b2e because of issues > > reported in the virtio. > > > > Examples: > > > > $ qemu-system-ppc64 -cpu POWER8 -nographic -vga none -m 4G \ > > -M pseries,accel=kvm -netdev type=user,id=net0 \ > > -device virtio-net-pci,netdev=net0 -drive file=../disk.qcow2,if=virtio > > > > Populating /vdevice/nvram@71000001 > > Populating /vdevice/v-scsi@71000002 > > SCSI: Looking for devices > > 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+" > > Populating /pci@800000020000000 > > 00 0000 (D) : 1af4 1000 virtio [ net ] > > Aborted > > > > $ qemu-system-x86_64 -m 4G -enable-kvm -drive file=util.qcow2,if=virtio > > > > Running QEMU with GTK 2.x is deprecated, and will be removed > > in a future release. Please switch to GTK 3.x instead > > [1] 5282 abort > > > > Reference http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html > > > > Reported-by: Anton Blanchard <anton@samba.org> > > Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> > > Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > > Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > > Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > accel/kvm/kvm-all.c | 4 ---- > > 1 file changed, 4 deletions(-) > > > > diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > index 071f4f5..f290f48 100644 > > --- a/accel/kvm/kvm-all.c > > +++ b/accel/kvm/kvm-all.c > > @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener, > > if (r < 0) { > > abort(); > > } > > - > > - if (e->cleanup) { > > - e->cleanup(e); > > - } > > This looks wrong as the cleanup is expected to do things like closing fds: > > static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier) > { > /* Test and clear notifier after disabling event, > * in case poll callback didn't have time to run. > */ > virtio_queue_host_notifier_read(notifier); > event_notifier_cleanup(notifier); > } > > void event_notifier_cleanup(EventNotifier *e) > { > if (e->rfd != e->wfd) { > close(e->rfd); > } > close(e->wfd); > e->rfd = -1; > e->wfd = -1; > e->cleanup = NULL; > } > > And indeed, with this patch applied, QEMU leaks eventfds on every machine > reset. > Reverting 4fe6d78b2e entirely isn't even enough and QEMU aborts at the next machine reset. The following commit must be reverted as well: commit 6f0bb230722931d17fb284eee8efd40b9d653822 Author: Gal Hammer <ghammer@redhat.com> Date: Sun Jan 14 12:06:56 2018 +0200 virtio: improve virtio devices initialization time > > } > > > > static void kvm_io_ioeventfd_add(MemoryListener *listener, > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call 2018-01-24 9:05 ` Greg Kurz @ 2018-01-24 9:14 ` Paolo Bonzini 2018-01-24 9:34 ` Greg Kurz 0 siblings, 1 reply; 10+ messages in thread From: Paolo Bonzini @ 2018-01-24 9:14 UTC (permalink / raw) To: Greg Kurz, Michael S. Tsirkin Cc: Peter Maydell, kvm, Jose Ricardo Ziviani, qemu-devel, Anton Blanchard, Daniel Henrique Barboza On 24/01/2018 10:05, Greg Kurz wrote: > On Wed, 24 Jan 2018 09:46:21 +0100 > Greg Kurz <groug@kaod.org> wrote: > >> Please note that Peter usually doesn't work on Wednesdays. The master branch >> might remain broken for everyone until tomorrow... :-\ >> >> And I don't think this is the right fix anyway. See below. >> >> On Wed, 24 Jan 2018 00:44:14 +0200 >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> >>> From: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> >>> >>> This commit partially reverts the commit 4fe6d78b2e because of issues >>> reported in the virtio. >>> >>> Examples: >>> >>> $ qemu-system-ppc64 -cpu POWER8 -nographic -vga none -m 4G \ >>> -M pseries,accel=kvm -netdev type=user,id=net0 \ >>> -device virtio-net-pci,netdev=net0 -drive file=../disk.qcow2,if=virtio >>> >>> Populating /vdevice/nvram@71000001 >>> Populating /vdevice/v-scsi@71000002 >>> SCSI: Looking for devices >>> 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+" >>> Populating /pci@800000020000000 >>> 00 0000 (D) : 1af4 1000 virtio [ net ] >>> Aborted >>> >>> $ qemu-system-x86_64 -m 4G -enable-kvm -drive file=util.qcow2,if=virtio >>> >>> Running QEMU with GTK 2.x is deprecated, and will be removed >>> in a future release. Please switch to GTK 3.x instead >>> [1] 5282 abort >>> >>> Reference http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html >>> >>> Reported-by: Anton Blanchard <anton@samba.org> >>> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> >>> Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >>> Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>> --- >>> accel/kvm/kvm-all.c | 4 ---- >>> 1 file changed, 4 deletions(-) >>> >>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>> index 071f4f5..f290f48 100644 >>> --- a/accel/kvm/kvm-all.c >>> +++ b/accel/kvm/kvm-all.c >>> @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener, >>> if (r < 0) { >>> abort(); >>> } >>> - >>> - if (e->cleanup) { >>> - e->cleanup(e); >>> - } >> >> This looks wrong as the cleanup is expected to do things like closing fds: >> >> static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier) >> { >> /* Test and clear notifier after disabling event, >> * in case poll callback didn't have time to run. >> */ >> virtio_queue_host_notifier_read(notifier); >> event_notifier_cleanup(notifier); >> } >> >> void event_notifier_cleanup(EventNotifier *e) >> { >> if (e->rfd != e->wfd) { >> close(e->rfd); >> } >> close(e->wfd); >> e->rfd = -1; >> e->wfd = -1; >> e->cleanup = NULL; >> } >> >> And indeed, with this patch applied, QEMU leaks eventfds on every machine >> reset. >> > > Reverting 4fe6d78b2e entirely isn't even enough and QEMU aborts at the > next machine reset. The following commit must be reverted as well: > > commit 6f0bb230722931d17fb284eee8efd40b9d653822 > Author: Gal Hammer <ghammer@redhat.com> > Date: Sun Jan 14 12:06:56 2018 +0200 > > virtio: improve virtio devices initialization time I'm a bit confused by this patch. The basic idea of wrapping with transaction_begin/commit is clear (without it you have quadratic behavior from removing one ioeventfd at a time), but I don't understand why the new ->cleanup member is needed. Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call 2018-01-24 9:14 ` Paolo Bonzini @ 2018-01-24 9:34 ` Greg Kurz 2018-01-24 9:36 ` Paolo Bonzini 2018-01-24 17:08 ` Michael S. Tsirkin 0 siblings, 2 replies; 10+ messages in thread From: Greg Kurz @ 2018-01-24 9:34 UTC (permalink / raw) To: Paolo Bonzini Cc: Michael S. Tsirkin, Peter Maydell, kvm, Jose Ricardo Ziviani, qemu-devel, Anton Blanchard, Daniel Henrique Barboza On Wed, 24 Jan 2018 10:14:57 +0100 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 24/01/2018 10:05, Greg Kurz wrote: > > On Wed, 24 Jan 2018 09:46:21 +0100 > > Greg Kurz <groug@kaod.org> wrote: > > > >> Please note that Peter usually doesn't work on Wednesdays. The master branch > >> might remain broken for everyone until tomorrow... :-\ > >> > >> And I don't think this is the right fix anyway. See below. > >> > >> On Wed, 24 Jan 2018 00:44:14 +0200 > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > >> > >>> From: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> > >>> > >>> This commit partially reverts the commit 4fe6d78b2e because of issues > >>> reported in the virtio. > >>> > >>> Examples: > >>> > >>> $ qemu-system-ppc64 -cpu POWER8 -nographic -vga none -m 4G \ > >>> -M pseries,accel=kvm -netdev type=user,id=net0 \ > >>> -device virtio-net-pci,netdev=net0 -drive file=../disk.qcow2,if=virtio > >>> > >>> Populating /vdevice/nvram@71000001 > >>> Populating /vdevice/v-scsi@71000002 > >>> SCSI: Looking for devices > >>> 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+" > >>> Populating /pci@800000020000000 > >>> 00 0000 (D) : 1af4 1000 virtio [ net ] > >>> Aborted > >>> > >>> $ qemu-system-x86_64 -m 4G -enable-kvm -drive file=util.qcow2,if=virtio > >>> > >>> Running QEMU with GTK 2.x is deprecated, and will be removed > >>> in a future release. Please switch to GTK 3.x instead > >>> [1] 5282 abort > >>> > >>> Reference http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html > >>> > >>> Reported-by: Anton Blanchard <anton@samba.org> > >>> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> > >>> Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > >>> Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >>> --- > >>> accel/kvm/kvm-all.c | 4 ---- > >>> 1 file changed, 4 deletions(-) > >>> > >>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > >>> index 071f4f5..f290f48 100644 > >>> --- a/accel/kvm/kvm-all.c > >>> +++ b/accel/kvm/kvm-all.c > >>> @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener, > >>> if (r < 0) { > >>> abort(); > >>> } > >>> - > >>> - if (e->cleanup) { > >>> - e->cleanup(e); > >>> - } > >> > >> This looks wrong as the cleanup is expected to do things like closing fds: > >> > >> static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier) > >> { > >> /* Test and clear notifier after disabling event, > >> * in case poll callback didn't have time to run. > >> */ > >> virtio_queue_host_notifier_read(notifier); > >> event_notifier_cleanup(notifier); > >> } > >> > >> void event_notifier_cleanup(EventNotifier *e) > >> { > >> if (e->rfd != e->wfd) { > >> close(e->rfd); > >> } > >> close(e->wfd); > >> e->rfd = -1; > >> e->wfd = -1; > >> e->cleanup = NULL; > >> } > >> > >> And indeed, with this patch applied, QEMU leaks eventfds on every machine > >> reset. > >> > > > > Reverting 4fe6d78b2e entirely isn't even enough and QEMU aborts at the > > next machine reset. The following commit must be reverted as well: > > > > commit 6f0bb230722931d17fb284eee8efd40b9d653822 > > Author: Gal Hammer <ghammer@redhat.com> > > Date: Sun Jan 14 12:06:56 2018 +0200 > > > > virtio: improve virtio devices initialization time > > I'm a bit confused by this patch. The basic idea of wrapping with > transaction_begin/commit is clear (without it you have quadratic > behavior from removing one ioeventfd at a time), but I don't understand > why the new ->cleanup member is needed. > I don't understand either... the cover letter of the series says: "The patch wraps all the changes made to the Memory Regions during the eventfd registrations in a memory regions transaction. I had to add a cleanup callback function to the EventNotifier struct, so it will be possible to use a transaction in the shutdown code path as well." Until this is sorted out, maybe best to revert both commits or even the full series (ie, f87d72f5c5bf as well), no ? > Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call 2018-01-24 9:34 ` Greg Kurz @ 2018-01-24 9:36 ` Paolo Bonzini 2018-01-24 17:08 ` Michael S. Tsirkin 1 sibling, 0 replies; 10+ messages in thread From: Paolo Bonzini @ 2018-01-24 9:36 UTC (permalink / raw) To: Greg Kurz Cc: Michael S. Tsirkin, Peter Maydell, kvm, Jose Ricardo Ziviani, qemu-devel, Anton Blanchard, Daniel Henrique Barboza On 24/01/2018 10:34, Greg Kurz wrote: > On Wed, 24 Jan 2018 10:14:57 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 24/01/2018 10:05, Greg Kurz wrote: >>> On Wed, 24 Jan 2018 09:46:21 +0100 >>> Greg Kurz <groug@kaod.org> wrote: >>> >>>> Please note that Peter usually doesn't work on Wednesdays. The master branch >>>> might remain broken for everyone until tomorrow... :-\ >>>> >>>> And I don't think this is the right fix anyway. See below. >>>> >>>> On Wed, 24 Jan 2018 00:44:14 +0200 >>>> "Michael S. Tsirkin" <mst@redhat.com> wrote: >>>> >>>>> From: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> >>>>> >>>>> This commit partially reverts the commit 4fe6d78b2e because of issues >>>>> reported in the virtio. >>>>> >>>>> Examples: >>>>> >>>>> $ qemu-system-ppc64 -cpu POWER8 -nographic -vga none -m 4G \ >>>>> -M pseries,accel=kvm -netdev type=user,id=net0 \ >>>>> -device virtio-net-pci,netdev=net0 -drive file=../disk.qcow2,if=virtio >>>>> >>>>> Populating /vdevice/nvram@71000001 >>>>> Populating /vdevice/v-scsi@71000002 >>>>> SCSI: Looking for devices >>>>> 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+" >>>>> Populating /pci@800000020000000 >>>>> 00 0000 (D) : 1af4 1000 virtio [ net ] >>>>> Aborted >>>>> >>>>> $ qemu-system-x86_64 -m 4G -enable-kvm -drive file=util.qcow2,if=virtio >>>>> >>>>> Running QEMU with GTK 2.x is deprecated, and will be removed >>>>> in a future release. Please switch to GTK 3.x instead >>>>> [1] 5282 abort >>>>> >>>>> Reference http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html >>>>> >>>>> Reported-by: Anton Blanchard <anton@samba.org> >>>>> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> >>>>> Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >>>>> Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >>>>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >>>>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >>>>> --- >>>>> accel/kvm/kvm-all.c | 4 ---- >>>>> 1 file changed, 4 deletions(-) >>>>> >>>>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >>>>> index 071f4f5..f290f48 100644 >>>>> --- a/accel/kvm/kvm-all.c >>>>> +++ b/accel/kvm/kvm-all.c >>>>> @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener, >>>>> if (r < 0) { >>>>> abort(); >>>>> } >>>>> - >>>>> - if (e->cleanup) { >>>>> - e->cleanup(e); >>>>> - } >>>> >>>> This looks wrong as the cleanup is expected to do things like closing fds: >>>> >>>> static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier) >>>> { >>>> /* Test and clear notifier after disabling event, >>>> * in case poll callback didn't have time to run. >>>> */ >>>> virtio_queue_host_notifier_read(notifier); >>>> event_notifier_cleanup(notifier); >>>> } >>>> >>>> void event_notifier_cleanup(EventNotifier *e) >>>> { >>>> if (e->rfd != e->wfd) { >>>> close(e->rfd); >>>> } >>>> close(e->wfd); >>>> e->rfd = -1; >>>> e->wfd = -1; >>>> e->cleanup = NULL; >>>> } >>>> >>>> And indeed, with this patch applied, QEMU leaks eventfds on every machine >>>> reset. >>>> >>> >>> Reverting 4fe6d78b2e entirely isn't even enough and QEMU aborts at the >>> next machine reset. The following commit must be reverted as well: >>> >>> commit 6f0bb230722931d17fb284eee8efd40b9d653822 >>> Author: Gal Hammer <ghammer@redhat.com> >>> Date: Sun Jan 14 12:06:56 2018 +0200 >>> >>> virtio: improve virtio devices initialization time >> >> I'm a bit confused by this patch. The basic idea of wrapping with >> transaction_begin/commit is clear (without it you have quadratic >> behavior from removing one ioeventfd at a time), but I don't understand >> why the new ->cleanup member is needed. > > I don't understand either... the cover letter of the series says: > > "The patch wraps all the changes made to the Memory Regions during the > eventfd registrations in a memory regions transaction. I had to add a > cleanup callback function to the EventNotifier struct, so it will be > possible to use a transaction in the shutdown code path as well." > > Until this is sorted out, maybe best to revert both commits or even > the full series (ie, f87d72f5c5bf as well), no ? Yes, indeed. Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call 2018-01-24 9:34 ` Greg Kurz 2018-01-24 9:36 ` Paolo Bonzini @ 2018-01-24 17:08 ` Michael S. Tsirkin 2018-01-28 14:07 ` Gal Hammer 1 sibling, 1 reply; 10+ messages in thread From: Michael S. Tsirkin @ 2018-01-24 17:08 UTC (permalink / raw) To: Greg Kurz Cc: Paolo Bonzini, Peter Maydell, kvm, Jose Ricardo Ziviani, qemu-devel, Anton Blanchard, Daniel Henrique Barboza On Wed, Jan 24, 2018 at 10:34:02AM +0100, Greg Kurz wrote: > On Wed, 24 Jan 2018 10:14:57 +0100 > Paolo Bonzini <pbonzini@redhat.com> wrote: > > > On 24/01/2018 10:05, Greg Kurz wrote: > > > On Wed, 24 Jan 2018 09:46:21 +0100 > > > Greg Kurz <groug@kaod.org> wrote: > > > > > >> Please note that Peter usually doesn't work on Wednesdays. The master branch > > >> might remain broken for everyone until tomorrow... :-\ > > >> > > >> And I don't think this is the right fix anyway. See below. > > >> > > >> On Wed, 24 Jan 2018 00:44:14 +0200 > > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: > > >> > > >>> From: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> > > >>> > > >>> This commit partially reverts the commit 4fe6d78b2e because of issues > > >>> reported in the virtio. > > >>> > > >>> Examples: > > >>> > > >>> $ qemu-system-ppc64 -cpu POWER8 -nographic -vga none -m 4G \ > > >>> -M pseries,accel=kvm -netdev type=user,id=net0 \ > > >>> -device virtio-net-pci,netdev=net0 -drive file=../disk.qcow2,if=virtio > > >>> > > >>> Populating /vdevice/nvram@71000001 > > >>> Populating /vdevice/v-scsi@71000002 > > >>> SCSI: Looking for devices > > >>> 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+" > > >>> Populating /pci@800000020000000 > > >>> 00 0000 (D) : 1af4 1000 virtio [ net ] > > >>> Aborted > > >>> > > >>> $ qemu-system-x86_64 -m 4G -enable-kvm -drive file=util.qcow2,if=virtio > > >>> > > >>> Running QEMU with GTK 2.x is deprecated, and will be removed > > >>> in a future release. Please switch to GTK 3.x instead > > >>> [1] 5282 abort > > >>> > > >>> Reference http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html > > >>> > > >>> Reported-by: Anton Blanchard <anton@samba.org> > > >>> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> > > >>> Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > > >>> Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> > > >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> > > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > >>> --- > > >>> accel/kvm/kvm-all.c | 4 ---- > > >>> 1 file changed, 4 deletions(-) > > >>> > > >>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c > > >>> index 071f4f5..f290f48 100644 > > >>> --- a/accel/kvm/kvm-all.c > > >>> +++ b/accel/kvm/kvm-all.c > > >>> @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener, > > >>> if (r < 0) { > > >>> abort(); > > >>> } > > >>> - > > >>> - if (e->cleanup) { > > >>> - e->cleanup(e); > > >>> - } > > >> > > >> This looks wrong as the cleanup is expected to do things like closing fds: > > >> > > >> static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier) > > >> { > > >> /* Test and clear notifier after disabling event, > > >> * in case poll callback didn't have time to run. > > >> */ > > >> virtio_queue_host_notifier_read(notifier); > > >> event_notifier_cleanup(notifier); > > >> } > > >> > > >> void event_notifier_cleanup(EventNotifier *e) > > >> { > > >> if (e->rfd != e->wfd) { > > >> close(e->rfd); > > >> } > > >> close(e->wfd); > > >> e->rfd = -1; > > >> e->wfd = -1; > > >> e->cleanup = NULL; > > >> } > > >> > > >> And indeed, with this patch applied, QEMU leaks eventfds on every machine > > >> reset. > > >> > > > > > > Reverting 4fe6d78b2e entirely isn't even enough and QEMU aborts at the > > > next machine reset. The following commit must be reverted as well: > > > > > > commit 6f0bb230722931d17fb284eee8efd40b9d653822 > > > Author: Gal Hammer <ghammer@redhat.com> > > > Date: Sun Jan 14 12:06:56 2018 +0200 > > > > > > virtio: improve virtio devices initialization time > > > > I'm a bit confused by this patch. The basic idea of wrapping with > > transaction_begin/commit is clear (without it you have quadratic > > behavior from removing one ioeventfd at a time), but I don't understand > > why the new ->cleanup member is needed. > > > > I don't understand either... the cover letter of the series says: > > "The patch wraps all the changes made to the Memory Regions during the > eventfd registrations in a memory regions transaction. I had to add a > cleanup callback function to the EventNotifier struct, so it will be > possible to use a transaction in the shutdown code path as well." > > Until this is sorted out, maybe best to revert both commits or even > the full series (ie, f87d72f5c5bf as well), no ? OK I guess that's best. > > Paolo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call 2018-01-24 17:08 ` Michael S. Tsirkin @ 2018-01-28 14:07 ` Gal Hammer 2018-01-29 4:45 ` Michael S. Tsirkin 0 siblings, 1 reply; 10+ messages in thread From: Gal Hammer @ 2018-01-28 14:07 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Greg Kurz, Peter Maydell, kvm, Jose Ricardo Ziviani, qemu-devel, Anton Blanchard, Paolo Bonzini, Daniel Henrique Barboza Hi, Sorry for a bad patch and the problems it may have caused. On Wed, Jan 24, 2018 at 7:08 PM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Jan 24, 2018 at 10:34:02AM +0100, Greg Kurz wrote: >> On Wed, 24 Jan 2018 10:14:57 +0100 >> Paolo Bonzini <pbonzini@redhat.com> wrote: >> >> > On 24/01/2018 10:05, Greg Kurz wrote: >> > > On Wed, 24 Jan 2018 09:46:21 +0100 >> > > Greg Kurz <groug@kaod.org> wrote: >> > > >> > >> Please note that Peter usually doesn't work on Wednesdays. The master branch >> > >> might remain broken for everyone until tomorrow... :-\ >> > >> >> > >> And I don't think this is the right fix anyway. See below. >> > >> >> > >> On Wed, 24 Jan 2018 00:44:14 +0200 >> > >> "Michael S. Tsirkin" <mst@redhat.com> wrote: >> > >> >> > >>> From: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> >> > >>> >> > >>> This commit partially reverts the commit 4fe6d78b2e because of issues >> > >>> reported in the virtio. >> > >>> >> > >>> Examples: >> > >>> >> > >>> $ qemu-system-ppc64 -cpu POWER8 -nographic -vga none -m 4G \ >> > >>> -M pseries,accel=kvm -netdev type=user,id=net0 \ >> > >>> -device virtio-net-pci,netdev=net0 -drive file=../disk.qcow2,if=virtio >> > >>> >> > >>> Populating /vdevice/nvram@71000001 >> > >>> Populating /vdevice/v-scsi@71000002 >> > >>> SCSI: Looking for devices >> > >>> 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+" >> > >>> Populating /pci@800000020000000 >> > >>> 00 0000 (D) : 1af4 1000 virtio [ net ] >> > >>> Aborted >> > >>> >> > >>> $ qemu-system-x86_64 -m 4G -enable-kvm -drive file=util.qcow2,if=virtio >> > >>> >> > >>> Running QEMU with GTK 2.x is deprecated, and will be removed >> > >>> in a future release. Please switch to GTK 3.x instead >> > >>> [1] 5282 abort >> > >>> >> > >>> Reference http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg05457.html >> > >>> >> > >>> Reported-by: Anton Blanchard <anton@samba.org> >> > >>> Signed-off-by: Jose Ricardo Ziviani <joserz@linux.vnet.ibm.com> >> > >>> Reviewed-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >> > >>> Tested-by: Daniel Henrique Barboza <danielhb@linux.vnet.ibm.com> >> > >>> Reviewed-by: Michael S. Tsirkin <mst@redhat.com> >> > >>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> > >>> --- >> > >>> accel/kvm/kvm-all.c | 4 ---- >> > >>> 1 file changed, 4 deletions(-) >> > >>> >> > >>> diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c >> > >>> index 071f4f5..f290f48 100644 >> > >>> --- a/accel/kvm/kvm-all.c >> > >>> +++ b/accel/kvm/kvm-all.c >> > >>> @@ -812,10 +812,6 @@ static void kvm_mem_ioeventfd_del(MemoryListener *listener, >> > >>> if (r < 0) { >> > >>> abort(); >> > >>> } >> > >>> - >> > >>> - if (e->cleanup) { >> > >>> - e->cleanup(e); >> > >>> - } >> > >> >> > >> This looks wrong as the cleanup is expected to do things like closing fds: >> > >> >> > >> static void virtio_bus_cleanup_event_notifier(EventNotifier *notifier) >> > >> { >> > >> /* Test and clear notifier after disabling event, >> > >> * in case poll callback didn't have time to run. >> > >> */ >> > >> virtio_queue_host_notifier_read(notifier); >> > >> event_notifier_cleanup(notifier); >> > >> } >> > >> >> > >> void event_notifier_cleanup(EventNotifier *e) >> > >> { >> > >> if (e->rfd != e->wfd) { >> > >> close(e->rfd); >> > >> } >> > >> close(e->wfd); >> > >> e->rfd = -1; >> > >> e->wfd = -1; >> > >> e->cleanup = NULL; >> > >> } >> > >> >> > >> And indeed, with this patch applied, QEMU leaks eventfds on every machine >> > >> reset. >> > >> >> > > >> > > Reverting 4fe6d78b2e entirely isn't even enough and QEMU aborts at the >> > > next machine reset. The following commit must be reverted as well: >> > > >> > > commit 6f0bb230722931d17fb284eee8efd40b9d653822 >> > > Author: Gal Hammer <ghammer@redhat.com> >> > > Date: Sun Jan 14 12:06:56 2018 +0200 >> > > >> > > virtio: improve virtio devices initialization time >> > >> > I'm a bit confused by this patch. The basic idea of wrapping with >> > transaction_begin/commit is clear (without it you have quadratic >> > behavior from removing one ioeventfd at a time), but I don't understand >> > why the new ->cleanup member is needed. >> > Like the cover letter tried to explain, the reason I added the cleanup call back was to a try to close the event fd after the memory transaction is committed. Without it, the code in the virtio_bus_set_host_notifier function is calling event_notifier_cleanup (which results in a closed fd) before the ioeventfd is removed from kvm. A call to kvm_set_ioeventfd_* on a closed fd would fail and cause qemu to abort. So the idea was to add a way to run a cleanup code after the transaction is committed. The reason for the bug is something that I didn't see on the virtio-serial-pci device. With virtio-net-pci device, the vq's MemoryRegion is registered with the same EventNotifier's fd twice, both with kvm_set_ioeventfd_mmio and kvm_set_ioeventfd_pio. I'm not sure if this is the right behavior and maybe one of you can help here. That double registration causes the cleanup function to execute twice, and the second call that is using a closed fd, results in the described failure. >> >> I don't understand either... the cover letter of the series says: >> >> "The patch wraps all the changes made to the Memory Regions during the >> eventfd registrations in a memory regions transaction. I had to add a >> cleanup callback function to the EventNotifier struct, so it will be >> possible to use a transaction in the shutdown code path as well." >> >> Until this is sorted out, maybe best to revert both commits or even >> the full series (ie, f87d72f5c5bf as well), no ? > > OK I guess that's best. > >> > Paolo > Gal. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call 2018-01-28 14:07 ` Gal Hammer @ 2018-01-29 4:45 ` Michael S. Tsirkin 2018-01-29 7:48 ` Gal Hammer 0 siblings, 1 reply; 10+ messages in thread From: Michael S. Tsirkin @ 2018-01-29 4:45 UTC (permalink / raw) To: Gal Hammer Cc: Greg Kurz, Peter Maydell, kvm, Jose Ricardo Ziviani, qemu-devel, Anton Blanchard, Paolo Bonzini, Daniel Henrique Barboza On Sun, Jan 28, 2018 at 04:07:27PM +0200, Gal Hammer wrote: > Hi, > > Sorry for a bad patch and the problems it may have caused. Not at all, these things happen. Do you have short term plans to work on a new version? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call 2018-01-29 4:45 ` Michael S. Tsirkin @ 2018-01-29 7:48 ` Gal Hammer 0 siblings, 0 replies; 10+ messages in thread From: Gal Hammer @ 2018-01-29 7:48 UTC (permalink / raw) To: Michael S. Tsirkin Cc: Greg Kurz, Peter Maydell, kvm, Jose Ricardo Ziviani, qemu-devel, Anton Blanchard, Paolo Bonzini, Daniel Henrique Barboza On Mon, Jan 29, 2018 at 6:45 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Sun, Jan 28, 2018 at 04:07:27PM +0200, Gal Hammer wrote: >> Hi, >> >> Sorry for a bad patch and the problems it may have caused. > > Not at all, these things happen. Do you have short term plans to work > on a new version? I already have a solution for it (which I abandoned because I liked the "cleanup" option), I'll submit a patch for a review later today. Gal. ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-01-29 7:48 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1516747387-31760-1-git-send-email-mst@redhat.com>
2018-01-23 22:44 ` [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call Michael S. Tsirkin
2018-01-24 8:46 ` [Qemu-devel] " Greg Kurz
2018-01-24 9:05 ` Greg Kurz
2018-01-24 9:14 ` Paolo Bonzini
2018-01-24 9:34 ` Greg Kurz
2018-01-24 9:36 ` Paolo Bonzini
2018-01-24 17:08 ` Michael S. Tsirkin
2018-01-28 14:07 ` Gal Hammer
2018-01-29 4:45 ` Michael S. Tsirkin
2018-01-29 7:48 ` Gal Hammer
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.