On 24/01/2018 10:34, Greg Kurz wrote: > On Wed, 24 Jan 2018 10:14:57 +0100 > Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote: > >> On 24/01/2018 10:05, Greg Kurz wrote: >>> On Wed, 24 Jan 2018 09:46:21 +0100 >>> Greg Kurz <groug@xxxxxxxx> 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@xxxxxxxxxx> wrote: >>>> >>>>> From: Jose Ricardo Ziviani <joserz@xxxxxxxxxxxxxxxxxx> >>>>> >>>>> 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@xxxxxxxxx> >>>>> Signed-off-by: Jose Ricardo Ziviani <joserz@xxxxxxxxxxxxxxxxxx> >>>>> Reviewed-by: Daniel Henrique Barboza <danielhb@xxxxxxxxxxxxxxxxxx> >>>>> Tested-by: Daniel Henrique Barboza <danielhb@xxxxxxxxxxxxxxxxxx> >>>>> Reviewed-by: Michael S. Tsirkin <mst@xxxxxxxxxx> >>>>> Signed-off-by: Michael S. Tsirkin <mst@xxxxxxxxxx> >>>>> --- >>>>> 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@xxxxxxxxxx> >>> 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
