| Message ID | 1516747387-31760-2-git-send-email-mst@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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,
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, > >
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
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
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
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
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.
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?
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.
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,