Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Cornelia Huck]

On Wed, 29 Jun 2016 09:41:50 +0800
Jason Wang  wrote:
 
> On 2016年06月27日 17:44, Peter Lieven wrote:
> > Hi, with the above patch applied:
> >
> > commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
> > Author: Cornelia Huck 
> > Date:   Fri Jun 10 11:04:12 2016 +0200
> >
> > virtio-pci: convert to ioeventfd callbacks
> >
> > a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if 
> > there is only one queue.
> >
> > Peter
> >
> >
> 
> In fact, I notice vhost-net does not work for master, look like we are 
> trying to set host notifier without initialization which seems a bug

Yes, that's the problem: We switch handlers, but the notifier has not
yet been setup (that's different from the dataplane call sequence). I
think we need to setup the notifier without touching the handler for
that case. I'm working on a patch.

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Cornelia Huck]

On Wed, 29 Jun 2016 16:21:07 +0800
Jason Wang  wrote:

> 
> 
> On 2016年06月29日 15:23, Cornelia Huck wrote:
> > On Wed, 29 Jun 2016 09:41:50 +0800
> > Jason Wang  wrote:
> >
> >>
> >> On 2016年06月27日 17:44, Peter Lieven wrote:
> >>> Hi, with the above patch applied:
> >>>
> >>> commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
> >>> Author: Cornelia Huck 
> >>> Date:   Fri Jun 10 11:04:12 2016 +0200
> >>>
> >>>  virtio-pci: convert to ioeventfd callbacks
> >>>
> >>> a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if
> >>> there is only one queue.
> >>>
> >>> Peter
> >>>
> >>>
> >> In fact, I notice vhost-net does not work for master, look like we are
> >> trying to set host notifier without initialization which seems a bug
> >>
> > Does the patch in <20160628101618.57bdb1e0.cornelia.h...@de.ibm.com> help?
> >
> >
> 
> It doesn't help.

Thanks for checking.

Debugging. This is broken here (virtio-ccw) as well.

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Jason Wang]

On 2016年06月29日 15:23, Cornelia Huck wrote:

On Wed, 29 Jun 2016 09:41:50 +0800
Jason Wang  wrote:



On 2016年06月27日 17:44, Peter Lieven wrote:

Hi, with the above patch applied:

commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
Author: Cornelia Huck 
Date:   Fri Jun 10 11:04:12 2016 +0200

 virtio-pci: convert to ioeventfd callbacks

a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if
there is only one queue.

Peter



In fact, I notice vhost-net does not work for master, look like we are
trying to set host notifier without initialization which seems a bug


Does the patch in <20160628101618.57bdb1e0.cornelia.h...@de.ibm.com> help?




It doesn't help.

Thanks

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Cornelia Huck]

On Wed, 29 Jun 2016 09:41:50 +0800
Jason Wang  wrote:

> 
> 
> On 2016年06月27日 17:44, Peter Lieven wrote:
> > Hi, with the above patch applied:
> >
> > commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
> > Author: Cornelia Huck 
> > Date:   Fri Jun 10 11:04:12 2016 +0200
> >
> > virtio-pci: convert to ioeventfd callbacks
> >
> > a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if 
> > there is only one queue.
> >
> > Peter
> >
> >
> 
> In fact, I notice vhost-net does not work for master, look like we are 
> trying to set host notifier without initialization which seems a bug
> 

Does the patch in <20160628101618.57bdb1e0.cornelia.h...@de.ibm.com> help?

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Jason Wang]

On 2016年06月27日 17:44, Peter Lieven wrote:

Hi, with the above patch applied:

commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
Author: Cornelia Huck 
Date:   Fri Jun 10 11:04:12 2016 +0200

virtio-pci: convert to ioeventfd callbacks

a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if 
there is only one queue.


Peter




In fact, I notice vhost-net does not work for master, look like we are 
trying to set host notifier without initialization which seems a bug

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Peter Lieven]

Am 28.06.2016 um 10:16 schrieb Cornelia Huck:

On Tue, 28 Jun 2016 09:47:01 +0200
Peter Lieven  wrote:


The problem goes away, but its horribly slow. Maybe the lost notifications
you were thinking off.

I have the following patch (works for me on ccw as well). I'm worried
about the slowness you're seeing, though. Is this just with an iscsi
backing?


Strange enough now neither my nor your version of the fix work for
me anymore...

I can only reproduce with iSCSI. Neither QCOW2, RAW or NBD work.

I think it could have to do with the iSCSI event handler.

Peter

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Cornelia Huck]

On Tue, 28 Jun 2016 09:47:01 +0200
Peter Lieven  wrote:

> The problem goes away, but its horribly slow. Maybe the lost notifications
> you were thinking off.

I have the following patch (works for me on ccw as well). I'm worried
about the slowness you're seeing, though. Is this just with an iscsi
backing?

From a62e1518fa9c8a68c27d6997d8ca8fccf6152d1e Mon Sep 17 00:00:00 2001
From: Cornelia Huck 
Date: Tue, 28 Jun 2016 10:04:52 +0200
Subject: [PATCH] virtio: deassign ioeventfd before resetting handler

We should unassign the transport's ioeventfd backing before
resetting the handler, or we may use notifications.

Pointed-out-by: Michael S. Tsirkin 
Reported-by: Peter Lieven 
Signed-off-by: Cornelia Huck 
---
 hw/virtio/virtio-bus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1313760..32fcc64 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -176,8 +176,8 @@ static int set_host_notifier_internal(DeviceState *proxy, 
VirtioBusState *bus,
 return r;
 }
 } else {
-virtio_queue_set_host_notifier_fd_handler(vq, false, false);
 k->ioeventfd_assign(proxy, notifier, n, assign);
+virtio_queue_set_host_notifier_fd_handler(vq, false, false);
 event_notifier_cleanup(notifier);
 }
 return r;
-- 
2.9.0

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Cornelia Huck]

On Tue, 28 Jun 2016 09:47:01 +0200
Peter Lieven  wrote:

> Am 28.06.2016 um 09:42 schrieb Cornelia Huck:
> > On Tue, 28 Jun 2016 10:03:21 +0300
> > "Michael S. Tsirkin"  wrote:
> >
> >> I notice cleanup is a bit weird:
> >>
> >>  virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> >>  k->ioeventfd_assign(proxy, notifier, n, assign);
> >>  event_notifier_cleanup(notifier);
> >>
> >> I think virtio_queue_set_host_notifier_fd_handler should happen
> >> after ioeventfd_assign for symmetry with init?
> > Looking at the pre-rework code, ccw used the order now in common code,
> > while pci and mmio used the order you suggest.
> >
> > "Switch the handler back, then unassign the transport's ioeventfd
> > backing" made more sense to me (regardless of symmetry) - but we might
> > lose a notification?
> >
> > Peter: Can you check whether your problem goes away if you switch the
> > two lines around?
> >
> 
> The problem goes away, but its horribly slow. Maybe the lost notifications
> you were thinking off.

It points into that direction.

> 
> diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
> index 1313760..7924a59 100644
> --- a/hw/virtio/virtio-bus.c
> +++ b/hw/virtio/virtio-bus.c
> @@ -176,9 +176,9 @@ static int set_host_notifier_internal(DeviceState *proxy, 
> VirtioBusState *bus,
>   return r;
>   }
>   } else {
> -virtio_queue_set_host_notifier_fd_handler(vq, false, false);
>   k->ioeventfd_assign(proxy, notifier, n, assign);

The virtio_queue_...() should go here.

>   event_notifier_cleanup(notifier);
> +virtio_queue_set_host_notifier_fd_handler(vq, false, false);
>   }
>   return r;
>   }
> 
> Peter
> 

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Peter Lieven]

Am 28.06.2016 um 09:42 schrieb Cornelia Huck:

On Tue, 28 Jun 2016 10:03:21 +0300
"Michael S. Tsirkin"  wrote:


I notice cleanup is a bit weird:

 virtio_queue_set_host_notifier_fd_handler(vq, false, false);
 k->ioeventfd_assign(proxy, notifier, n, assign);
 event_notifier_cleanup(notifier);

I think virtio_queue_set_host_notifier_fd_handler should happen
after ioeventfd_assign for symmetry with init?

Looking at the pre-rework code, ccw used the order now in common code,
while pci and mmio used the order you suggest.

"Switch the handler back, then unassign the transport's ioeventfd
backing" made more sense to me (regardless of symmetry) - but we might
lose a notification?

Peter: Can you check whether your problem goes away if you switch the
two lines around?



The problem goes away, but its horribly slow. Maybe the lost notifications
you were thinking off.

diff --git a/hw/virtio/virtio-bus.c b/hw/virtio/virtio-bus.c
index 1313760..7924a59 100644
--- a/hw/virtio/virtio-bus.c
+++ b/hw/virtio/virtio-bus.c
@@ -176,9 +176,9 @@ static int set_host_notifier_internal(DeviceState *proxy, 
VirtioBusState *bus,
 return r;
 }
 } else {
-virtio_queue_set_host_notifier_fd_handler(vq, false, false);
 k->ioeventfd_assign(proxy, notifier, n, assign);
 event_notifier_cleanup(notifier);
+virtio_queue_set_host_notifier_fd_handler(vq, false, false);
 }
 return r;
 }

Peter

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Cornelia Huck]

On Tue, 28 Jun 2016 10:03:21 +0300
"Michael S. Tsirkin"  wrote:

> I notice cleanup is a bit weird:
> 
> virtio_queue_set_host_notifier_fd_handler(vq, false, false);
> k->ioeventfd_assign(proxy, notifier, n, assign);
> event_notifier_cleanup(notifier);
> 
> I think virtio_queue_set_host_notifier_fd_handler should happen
> after ioeventfd_assign for symmetry with init?

Looking at the pre-rework code, ccw used the order now in common code,
while pci and mmio used the order you suggest.

"Switch the handler back, then unassign the transport's ioeventfd
backing" made more sense to me (regardless of symmetry) - but we might
lose a notification?

Peter: Can you check whether your problem goes away if you switch the
two lines around?

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Cornelia Huck]

On Tue, 28 Jun 2016 08:22:05 +0200
Peter Lieven  wrote:

> Am 27.06.2016 um 17:09 schrieb Cornelia Huck:
> > On Mon, 27 Jun 2016 11:44:47 +0200
> > Peter Lieven  wrote:
> >
> >> Hi, with the above patch applied:
> >>
> >> commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
> >> Author: Cornelia Huck 
> >> Date:   Fri Jun 10 11:04:12 2016 +0200
> >>
> >>   virtio-pci: convert to ioeventfd callbacks
> >>
> >> a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if there 
> >> is only one queue.
> > OK, I played around a bit with Stefan's block branch rebased upon
> > current master (and the conflict fixed up)... and I can't seem to hit
> > any error with virtio-ccw (will see if I find time to play around with
> > virtio-pci; but unless I've messed up, this should be the same code
> > path). VIRTIO_BLK_F_MQ is correctly negotiated if num_queues > 1, and I
> > can use the disk.
> >
> > So, two questions:
> > - Which branch are you using? Any chance of a mis-merge there?
> > - Can you share the interesting portion of your command line?
> >
> 
> Hi Cornelia,
> 
> sorry I should have been a little more precise. I am using current master for 
> git.qemu.org.
> 
> I tried around and I can only reproduce the regression with iSCSI *and* 
> dataplane enabled.
> Here is the reproducing commandline for me.
> 
> ./x86_64-softmmu/qemu-system-x86_64 -enable-kvm -nodefaults -drive 
> if=none,format=raw,file=iscsi://172.21.200.56/iqn.2001-05.com.equallogic:0-8a0906-98f384e0a-7d2004ee0a85767a-00lieven-test/0,cache=none,aio=native,id=disk0
>  -object iothread,id=iothread0 
> -device virtio-blk-pci,drive=disk0,iothread=iothread0 -vga vmware -monitor 
> stdio

Nothing really exciting there, maybe a timing issue triggered by the
iscsi backing?

> 
> Have you the possiblity to install Ubuntu 14.04 on an iSCSI target?

No, I don't have any iscsi target available.

> 
> Let me know if you need further information. Maybe the regression is an 
> incompatiblity between the iSCSI driver and ioeventfd?

Running in gdb and looking at the backchains may be helpful. Or
checking in a guest dump whether there's any hang there instead. (Does
this happen with guests other than Ubuntu 14.04, maybe with a similar
kernel level?)

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Michael S. Tsirkin]

On Mon, Jun 27, 2016 at 05:09:38PM +0200, Cornelia Huck wrote:
> On Mon, 27 Jun 2016 11:44:47 +0200
> Peter Lieven  wrote:
> 
> > Hi, with the above patch applied:
> > 
> > commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
> > Author: Cornelia Huck 
> > Date:   Fri Jun 10 11:04:12 2016 +0200
> > 
> >  virtio-pci: convert to ioeventfd callbacks
> > 
> > a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if there 
> > is only one queue.
> 
> OK, I played around a bit with Stefan's block branch rebased upon
> current master (and the conflict fixed up)... and I can't seem to hit
> any error with virtio-ccw (will see if I find time to play around with
> virtio-pci; but unless I've messed up, this should be the same code
> path). VIRTIO_BLK_F_MQ is correctly negotiated if num_queues > 1, and I
> can use the disk.
> 
> So, two questions:
> - Which branch are you using? Any chance of a mis-merge there?
> - Can you share the interesting portion of your command line?


I notice cleanup is a bit weird:

virtio_queue_set_host_notifier_fd_handler(vq, false, false);
k->ioeventfd_assign(proxy, notifier, n, assign);
event_notifier_cleanup(notifier);

I think virtio_queue_set_host_notifier_fd_handler should happen
after ioeventfd_assign for symmetry with init?


-- 
MST

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Peter Lieven]

Am 27.06.2016 um 17:09 schrieb Cornelia Huck:

On Mon, 27 Jun 2016 11:44:47 +0200
Peter Lieven  wrote:


Hi, with the above patch applied:

commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
Author: Cornelia Huck 
Date:   Fri Jun 10 11:04:12 2016 +0200

  virtio-pci: convert to ioeventfd callbacks

a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if there is 
only one queue.

OK, I played around a bit with Stefan's block branch rebased upon
current master (and the conflict fixed up)... and I can't seem to hit
any error with virtio-ccw (will see if I find time to play around with
virtio-pci; but unless I've messed up, this should be the same code
path). VIRTIO_BLK_F_MQ is correctly negotiated if num_queues > 1, and I
can use the disk.

So, two questions:
- Which branch are you using? Any chance of a mis-merge there?
- Can you share the interesting portion of your command line?



Hi Cornelia,

sorry I should have been a little more precise. I am using current master for 
git.qemu.org.

I tried around and I can only reproduce the regression with iSCSI *and* 
dataplane enabled.
Here is the reproducing commandline for me.

./x86_64-softmmu/qemu-system-x86_64 -enable-kvm -nodefaults -drive if=none,format=raw,file=iscsi://172.21.200.56/iqn.2001-05.com.equallogic:0-8a0906-98f384e0a-7d2004ee0a85767a-00lieven-test/0,cache=none,aio=native,id=disk0 -object iothread,id=iothread0 
-device virtio-blk-pci,drive=disk0,iothread=iothread0 -vga vmware -monitor stdio


Have you the possiblity to install Ubuntu 14.04 on an iSCSI target?

Let me know if you need further information. Maybe the regression is an 
incompatiblity between the iSCSI driver and ioeventfd?

Thanks,
Peter

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Cornelia Huck]

On Mon, 27 Jun 2016 11:44:47 +0200
Peter Lieven  wrote:

> Hi, with the above patch applied:
> 
> commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
> Author: Cornelia Huck 
> Date:   Fri Jun 10 11:04:12 2016 +0200
> 
>  virtio-pci: convert to ioeventfd callbacks
> 
> a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if there is 
> only one queue.

OK, I played around a bit with Stefan's block branch rebased upon
current master (and the conflict fixed up)... and I can't seem to hit
any error with virtio-ccw (will see if I find time to play around with
virtio-pci; but unless I've messed up, this should be the same code
path). VIRTIO_BLK_F_MQ is correctly negotiated if num_queues > 1, and I
can use the disk.

So, two questions:
- Which branch are you using? Any chance of a mis-merge there?
- Can you share the interesting portion of your command line?

Re: [Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Cornelia Huck]

On Mon, 27 Jun 2016 11:44:47 +0200
Peter Lieven  wrote:

> Hi, with the above patch applied:
> 
> commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
> Author: Cornelia Huck 
> Date:   Fri Jun 10 11:04:12 2016 +0200
> 
>  virtio-pci: convert to ioeventfd callbacks
> 
> a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if there is 
> only one queue.

Hm, I had not played around with blk-mq...

Any chance you could run this in gdb and get some backtraces?

[Qemu-devel] Regression: virtio-pci: convert to ioeventfd callbacks

[Peter Lieven]

Hi, with the above patch applied:

commit 9f06e71a567ba5ee8b727e65a2d5347fd331d2aa
Author: Cornelia Huck 
Date:   Fri Jun 10 11:04:12 2016 +0200

virtio-pci: convert to ioeventfd callbacks

a Ubuntu 14.04 VM freezes at startup when blk-mq is set up - even if there is 
only one queue.

Peter