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 <address@hidden> 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 <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.
>> >

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.