[Qemu-devel] [PULL 0/1] virtio: quick fix

[Qemu-devel] [PULL 0/1] virtio: quick fix

[Michael S. Tsirkin]

This is just a quick bugfix so people can make progress.
Will send a pull req with more patches queues later this week.

The following changes since commit 52483b067cce4a88ffbf8fbeea26de7549d2ad23:

  Merge remote-tracking branch 'remotes/huth/tags/pull-request-2018-01-22' into staging (2018-01-23 10:15:09 +0000)

are available in the git repository at:

  git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream

for you to fetch changes up to 3e59a4745a1668eeec94d142b0f5a7847b866a87:

  kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call (2018-01-24 00:41:50 +0200)

----------------------------------------------------------------
virtio: quick fix

Fixes a regression in virtio that's causing issues
for many people.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

----------------------------------------------------------------
Jose Ricardo Ziviani (1):
      kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

 accel/kvm/kvm-all.c | 4 ----
 1 file changed, 4 deletions(-)

[Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Greg Kurz]

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,

Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Greg Kurz]

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,  
> 
> 

Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Paolo Bonzini]

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

Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Greg Kurz]

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

Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Paolo Bonzini]

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

Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Gal Hammer]

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.

Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Michael S. Tsirkin]

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?

Re: [Qemu-devel] [PULL 1/1] kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call

[Gal Hammer]

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.

Re: [Qemu-devel] [PULL 0/1] virtio: quick fix

[Peter Maydell]

On 23 January 2018 at 22:44, Michael S. Tsirkin <mst@redhat.com> wrote:
> This is just a quick bugfix so people can make progress.
> Will send a pull req with more patches queues later this week.
>
> The following changes since commit 52483b067cce4a88ffbf8fbeea26de7549d2ad23:
>
>   Merge remote-tracking branch 'remotes/huth/tags/pull-request-2018-01-22' into staging (2018-01-23 10:15:09 +0000)
>
> are available in the git repository at:
>
>   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
>
> for you to fetch changes up to 3e59a4745a1668eeec94d142b0f5a7847b866a87:
>
>   kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call (2018-01-24 00:41:50 +0200)
>
> ----------------------------------------------------------------
> virtio: quick fix
>
> Fixes a regression in virtio that's causing issues
> for many people.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> ----------------------------------------------------------------
> Jose Ricardo Ziviani (1):
>       kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call
>
>  accel/kvm/kvm-all.c | 4 ----
>  1 file changed, 4 deletions(-)

Hi -- this pullreq is superseded by your later one, right?

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/1] virtio: quick fix

[Michael S. Tsirkin]

On Wed, Jan 24, 2018 at 08:17:08PM +0000, Peter Maydell wrote:
> On 23 January 2018 at 22:44, Michael S. Tsirkin <mst@redhat.com> wrote:
> > This is just a quick bugfix so people can make progress.
> > Will send a pull req with more patches queues later this week.
> >
> > The following changes since commit 52483b067cce4a88ffbf8fbeea26de7549d2ad23:
> >
> >   Merge remote-tracking branch 'remotes/huth/tags/pull-request-2018-01-22' into staging (2018-01-23 10:15:09 +0000)
> >
> > are available in the git repository at:
> >
> >   git://git.kernel.org/pub/scm/virt/kvm/mst/qemu.git tags/for_upstream
> >
> > for you to fetch changes up to 3e59a4745a1668eeec94d142b0f5a7847b866a87:
> >
> >   kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call (2018-01-24 00:41:50 +0200)
> >
> > ----------------------------------------------------------------
> > virtio: quick fix
> >
> > Fixes a regression in virtio that's causing issues
> > for many people.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > ----------------------------------------------------------------
> > Jose Ricardo Ziviani (1):
> >       kvm-all: Partially reverts 4fe6d78b2e to remove the cleanup call
> >
> >  accel/kvm/kvm-all.c | 4 ----
> >  1 file changed, 4 deletions(-)
> 
> Hi -- this pullreq is superseded by your later one, right?
> 
> thanks
> -- PMM

Exactly, thanks!

-- 
MST