[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to rem
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call |
Date: |
Wed, 24 Jan 2018 10:36:24 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 |
On 24/01/2018 10:34, Greg Kurz wrote:
> On Wed, 24 Jan 2018 10:14:57 +0100
> Paolo Bonzini <address@hidden> wrote:
>
>> On 24/01/2018 10:05, Greg Kurz wrote:
>>> On Wed, 24 Jan 2018 09:46:21 +0100
>>> Greg Kurz <address@hidden> 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" <address@hidden> wrote:
>>>>
>>>>> From: Jose Ricardo Ziviani <address@hidden>
>>>>>
>>>>> 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/address@hidden
>>>>> Populating /vdevice/address@hidden
>>>>> SCSI: Looking for devices
>>>>> 8200000000000000 CD-ROM : "QEMU QEMU CD-ROM 2.5+"
>>>>> Populating /address@hidden
>>>>> 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 <address@hidden>
>>>>> Signed-off-by: Jose Ricardo Ziviani <address@hidden>
>>>>> Reviewed-by: Daniel Henrique Barboza <address@hidden>
>>>>> Tested-by: Daniel Henrique Barboza <address@hidden>
>>>>> Reviewed-by: Michael S. Tsirkin <address@hidden>
>>>>> Signed-off-by: Michael S. Tsirkin <address@hidden>
>>>>> ---
>>>>> 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 <address@hidden>
>>> 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
- [Qemu-devel] [PULL 0/1] virtio: quick fix, Michael S. Tsirkin, 2018/01/23
- [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call, Michael S. Tsirkin, 2018/01/23
- Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call, Greg Kurz, 2018/01/24
- Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call, Greg Kurz, 2018/01/24
- Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call, Paolo Bonzini, 2018/01/24
- Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call, Greg Kurz, 2018/01/24
- Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call,
Paolo Bonzini <=
- Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call, Michael S. Tsirkin, 2018/01/24
- Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call, Gal Hammer, 2018/01/28
- Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call, Michael S. Tsirkin, 2018/01/28
- Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call, Gal Hammer, 2018/01/29
Re: [Qemu-devel] [PULL 0/1] virtio: quick fix, Peter Maydell, 2018/01/24