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

OK I guess that's best.

> > Paolo