From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33808) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1eeH9J-0003DS-Oz for qemu-devel@nongnu.org; Wed, 24 Jan 2018 04:15:19 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1eeH9F-0008Jw-Lh for qemu-devel@nongnu.org; Wed, 24 Jan 2018 04:15:17 -0500 Received: from mx1.redhat.com ([209.132.183.28]:37220) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1eeH9F-0008JN-DO for qemu-devel@nongnu.org; Wed, 24 Jan 2018 04:15:13 -0500 References: <1516747387-31760-1-git-send-email-mst@redhat.com> <1516747387-31760-2-git-send-email-mst@redhat.com> <20180124094621.46ff2f43@bahia.lan> <20180124100500.1f9bf54a@bahia.lan> From: Paolo Bonzini Message-ID: Date: Wed, 24 Jan 2018 10:14:57 +0100 MIME-Version: 1.0 In-Reply-To: <20180124100500.1f9bf54a@bahia.lan> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Greg Kurz , "Michael S. Tsirkin" Cc: Peter Maydell , kvm@vger.kernel.org, Jose Ricardo Ziviani , qemu-devel@nongnu.org, 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 wrote: >=20 >> 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" wrote: >> >>> From: Jose Ricardo Ziviani >>> >>> 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=3Dkvm -netdev type=3Duser,id=3Dnet0 \ >>> -device virtio-net-pci,netdev=3Dnet0 -drive file=3D../disk.qcow2,if= =3Dvirtio >>> >>> 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=3Dutil.qcow2,if=3D= 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/msg= 05457.html >>> >>> Reported-by: Anton Blanchard >>> Signed-off-by: Jose Ricardo Ziviani >>> Reviewed-by: Daniel Henrique Barboza >>> Tested-by: Daniel Henrique Barboza >>> Reviewed-by: Michael S. Tsirkin >>> Signed-off-by: Michael S. Tsirkin >>> --- >>> 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); >>> - } =20 >> >> 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 !=3D e->wfd) { >> close(e->rfd); >> } >> close(e->wfd); >> e->rfd =3D -1; >> e->wfd =3D -1; >> e->cleanup =3D NULL; >> } >> >> And indeed, with this patch applied, QEMU leaks eventfds on every mach= ine >> reset. >> >=20 > Reverting 4fe6d78b2e entirely isn't even enough and QEMU aborts at the > next machine reset. The following commit must be reverted as well: >=20 > commit 6f0bb230722931d17fb284eee8efd40b9d653822 > Author: Gal Hammer > Date: Sun Jan 14 12:06:56 2018 +0200 >=20 > 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