Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Tue, 22 Mar 2016 13:11:05 +0100
Paolo Bonzini  wrote:

> On 22/03/2016 12:59, Cornelia Huck wrote:
> >> > They can be fixed with just an extra object_ref/object_unref.
> >> > 
> >> > I didn't understand that Tu Bo also needed the BH fix, and with that
> >> > information it makes sense.  Passing the assign value ensures that
> >> > ioeventfd remains always assigned.  With the CPU threads out of the
> >> > picture, the BH becomes enough to make everything thread-safe.
> > Yes, this makes sense.
> > 
> > Might we still have a hole somewhere in dataplane teardown? Probably
> > not, from reading the code, even if it runs in cpu thread context.
> 
> The bug arises when the main thread sets started = true, a CPU thread
> comes along while the ioeventfd is reset, and as soon as the BQL is
> released by the main thread the CPU thread thinks it is a dataplane
> thread.  This does horrible things to non-reentrant code.  For stop we
> should be safe because the CPU thread is the one that sets started = false.
> 
> IOW, we should be safe as long as the ioeventfd is never unassigned
> (your fix) _and_ we ensure serialization between threads that touch
> dataplane_started (Fam's fix).

We should really add something like that explanation to the changelog
so that future generations may understand what's going on here :)

So, what do we do for 2.6? A respin of Fam's fix + my refactoring (with
some interface doc added)? I'd still like some reviews and maybe a test
on virtio-mmio.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 22/03/2016 12:59, Cornelia Huck wrote:
>> > They can be fixed with just an extra object_ref/object_unref.
>> > 
>> > I didn't understand that Tu Bo also needed the BH fix, and with that
>> > information it makes sense.  Passing the assign value ensures that
>> > ioeventfd remains always assigned.  With the CPU threads out of the
>> > picture, the BH becomes enough to make everything thread-safe.
> Yes, this makes sense.
> 
> Might we still have a hole somewhere in dataplane teardown? Probably
> not, from reading the code, even if it runs in cpu thread context.

The bug arises when the main thread sets started = true, a CPU thread
comes along while the ioeventfd is reset, and as soon as the BQL is
released by the main thread the CPU thread thinks it is a dataplane
thread.  This does horrible things to non-reentrant code.  For stop we
should be safe because the CPU thread is the one that sets started = false.

IOW, we should be safe as long as the ioeventfd is never unassigned
(your fix) _and_ we ensure serialization between threads that touch
dataplane_started (Fam's fix).

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Tue, 22 Mar 2016 10:46:58 +0100
Paolo Bonzini  wrote:

> On 22/03/2016 10:07, Cornelia Huck wrote:
> > So far, we had the best results with my refactoring + the mutex/bh
> > change. Two problems:
> > 
> > - We don't really understand yet why my refactoring helps, but passing
> > the assign value is a good canditate (and it's agreed that this fixes a
> > bug, I think.)
> > - There's some problem with the bh, if I understood Stefan correctly.
> 
> They can be fixed with just an extra object_ref/object_unref.
> 
> I didn't understand that Tu Bo also needed the BH fix, and with that
> information it makes sense.  Passing the assign value ensures that
> ioeventfd remains always assigned.  With the CPU threads out of the
> picture, the BH becomes enough to make everything thread-safe.

Yes, this makes sense.

Might we still have a hole somewhere in dataplane teardown? Probably
not, from reading the code, even if it runs in cpu thread context.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 22/03/2016 10:07, Cornelia Huck wrote:
> So far, we had the best results with my refactoring + the mutex/bh
> change. Two problems:
> 
> - We don't really understand yet why my refactoring helps, but passing
> the assign value is a good canditate (and it's agreed that this fixes a
> bug, I think.)
> - There's some problem with the bh, if I understood Stefan correctly.

They can be fixed with just an extra object_ref/object_unref.

I didn't understand that Tu Bo also needed the BH fix, and with that
information it makes sense.  Passing the assign value ensures that
ioeventfd remains always assigned.  With the CPU threads out of the
picture, the BH becomes enough to make everything thread-safe.

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

(re-adding cc:s)

On Tue, 22 Mar 2016 15:18:05 +0800
Fam Zheng  wrote:

> On Tue, 03/22 15:10, tu bo wrote:
> > Hi Fam:
> > 
> > On 03/21/2016 06:57 PM, Fam Zheng wrote:
> > >On Thu, 03/17 19:03, tu bo wrote:
> > >>
> > >>On 03/17/2016 08:39 AM, Fam Zheng wrote:
> > >>>On Wed, 03/16 14:45, Paolo Bonzini wrote:
> > 
> > 
> > On 16/03/2016 14:38, Christian Borntraeger wrote:
> > >>If you just remove the calls to virtio_queue_host_notifier_read, here
> > >>and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
> > >>(keeping patches 2-4 in)?
> > >
> > >With these changes and patch 2-4 it does no longer locks up.
> > >I keep it running some hour to check if a crash happens.
> > >
> > >Tu Bo, your setup is currently better suited for reproducing. Can you 
> > >also check?
> > 
> > Great, I'll prepare a patch to virtio then sketching the solution that
> > Conny agreed with.
> > 
> > While Fam and I agreed that patch 1 is not required, I'm not sure if the
> > mutex is necessary in the end.
> > >>>
> > >>>If we can fix this from the virtio_queue_host_notifier_read side, the 
> > >>>mutex/BH
> > >>>are not necessary; but OTOH the mutex does catch such bugs, so maybe 
> > >>>it's good
> > >>>to have it. I'm not sure about the BH.
> > >>>
> > >>>And on a hindsight I realize we don't want patches 2-3 too. Actually the
> > >>>begin/end pair won't work as expected because of the blk_set_aio_context.
> > >>>
> > >>>Let's hold on this series.
> > >>>
> > 
> > So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
> > and both with/without Fam's patches, it would be great.
> > >>>
> > >>>Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the 
> > >>>noise.
> > >>>
> > >>1. without the virtio_queue_host_notifier_read calls,  without patch 4
> > >>
> > >>crash happens very often,
> > >>
> > >>(gdb) bt
> > >>#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> > >>#1  0x02aa165da37e in coroutine_trampoline (i0=,
> > >>i1=1812051552) at util/coroutine-ucontext.c:79
> > >>#2  0x03ff7dd5150a in __makecontext_ret () from /lib64/libc.so.6
> > >>
> > >>
> > >>2. without the virtio_queue_host_notifier_read calls, with patch 4
> > >>
> > >>crash happens very often,
> > >>
> > >>(gdb) bt
> > >>#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> > >>#1  0x02aa39dda43e in coroutine_trampoline (i0=,
> > >>i1=-1677715600) at util/coroutine-ucontext.c:79
> > >>#2  0x03ffab6d150a in __makecontext_ret () from /lib64/libc.so.6
> > >>
> > >>
> > >
> > >Tu Bo,
> > >
> > >Could you help test this patch (on top of master, without patch 4)?
> > >
> > >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > >index 08275a9..47f8043 100644
> > >--- a/hw/virtio/virtio.c
> > >+++ b/hw/virtio/virtio.c
> > >@@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > >
> > >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > >  {
> > >-virtio_queue_notify_vq(>vq[n]);
> > >+VirtQueue *vq = >vq[n];
> > >+EventNotifier *n;
> > >+n = virtio_queue_get_host_notifier(vq);
> > >+if (n) {
> > >+event_notifier_set(n);
> > >+} else {
> > >+virtio_queue_notify_vq(vq);
> > >+}
> > >  }
> > >
> > >  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> > >
> > >
> > 
> > I got a build error as below,
> > 
> > /BUILD/qemu-2.5.50/hw/virtio/virtio.c: In function 'virtio_queue_notify':
> > /BUILD/qemu-2.5.50/hw/virtio/virtio.c:1102:20: error: 'n' redeclared
> > as different kind of symbol
> >  EventNotifier *n;
> > ^
> > /BUILD/qemu-2.5.50/hw/virtio/virtio.c:1099:50: note: previous
> > definition of 'n' was here
> >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > 
> > 
> > Then I did some change for your patch as below,
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 08275a9..a10da39 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > 
> >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> >  {
> > -virtio_queue_notify_vq(>vq[n]);
> > +VirtQueue *vq = >vq[n];
> > +EventNotifier *en;
> > +en = virtio_queue_get_host_notifier(vq);
> > +if (en) {
> > +event_notifier_set(en);
> > +} else {
> > +virtio_queue_notify_vq(vq);
> > +}
> >  }
> > 
> >  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> > 
> > With qemu master + modified patch above(without patch 4, without
> > Conny's patches), I did NOT get crash so far.  thanks
> 
> Yes, it was a mistake.  Thanks for the testing!

I'm wondering what we learn from this. Bypassing notify_vq() removes
the race, but that's not the solution here.

So far, we had the best results with my refactoring + the mutex/bh
change. Two problems:

- We don't really understand yet why my refactoring helps, but passing
the assign value is 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Tue, 22 Mar 2016 07:45:19 +0800
Fam Zheng  wrote:

> On Mon, 03/21 14:02, Cornelia Huck wrote:
> > On Mon, 21 Mar 2016 20:45:27 +0800
> > Fam Zheng  wrote:
> > 
> > > On Mon, 03/21 12:15, Cornelia Huck wrote:
> > > > On Mon, 21 Mar 2016 18:57:18 +0800
> > > > Fam Zheng  wrote:
> > > > 
> > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > > index 08275a9..47f8043 100644
> > > > > --- a/hw/virtio/virtio.c
> > > > > +++ b/hw/virtio/virtio.c
> > > > > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > > > > 
> > > > >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > > > >  {
> > > > > -virtio_queue_notify_vq(>vq[n]);
> > > > > +VirtQueue *vq = >vq[n];
> > > > > +EventNotifier *n;
> > > > > +n = virtio_queue_get_host_notifier(vq);
> > > > > +if (n) {
> > > > 
> > > > Isn't that always true, even if the notifier has not been setup?
> > > 
> > > You are right, this doesn't make a correct fix. But we can still do a 
> > > quick
> > > test with this as the else branch should never be used with ioeventfd=on. 
> > > Am I
> > > right?
> > > 
> > > Fam
> > 
> > Won't we come through here for the very first kick, when we haven't
> > registered the ioeventfd with the kernel yet?
> > 
> 
> The ioeventfd in virtio-ccw is registered in the main loop when
> VIRTIO_CONFIG_S_DRIVER_OK is set, so I think the first kick is okay.

You're right, for well-behaved drivers this will be done from
set_status, so for testing that's fine.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Tue, 03/22 15:10, tu bo wrote:
> Hi Fam:
> 
> On 03/21/2016 06:57 PM, Fam Zheng wrote:
> >On Thu, 03/17 19:03, tu bo wrote:
> >>
> >>On 03/17/2016 08:39 AM, Fam Zheng wrote:
> >>>On Wed, 03/16 14:45, Paolo Bonzini wrote:
> 
> 
> On 16/03/2016 14:38, Christian Borntraeger wrote:
> >>If you just remove the calls to virtio_queue_host_notifier_read, here
> >>and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
> >>(keeping patches 2-4 in)?
> >
> >With these changes and patch 2-4 it does no longer locks up.
> >I keep it running some hour to check if a crash happens.
> >
> >Tu Bo, your setup is currently better suited for reproducing. Can you 
> >also check?
> 
> Great, I'll prepare a patch to virtio then sketching the solution that
> Conny agreed with.
> 
> While Fam and I agreed that patch 1 is not required, I'm not sure if the
> mutex is necessary in the end.
> >>>
> >>>If we can fix this from the virtio_queue_host_notifier_read side, the 
> >>>mutex/BH
> >>>are not necessary; but OTOH the mutex does catch such bugs, so maybe it's 
> >>>good
> >>>to have it. I'm not sure about the BH.
> >>>
> >>>And on a hindsight I realize we don't want patches 2-3 too. Actually the
> >>>begin/end pair won't work as expected because of the blk_set_aio_context.
> >>>
> >>>Let's hold on this series.
> >>>
> 
> So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
> and both with/without Fam's patches, it would be great.
> >>>
> >>>Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the 
> >>>noise.
> >>>
> >>1. without the virtio_queue_host_notifier_read calls,  without patch 4
> >>
> >>crash happens very often,
> >>
> >>(gdb) bt
> >>#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> >>#1  0x02aa165da37e in coroutine_trampoline (i0=,
> >>i1=1812051552) at util/coroutine-ucontext.c:79
> >>#2  0x03ff7dd5150a in __makecontext_ret () from /lib64/libc.so.6
> >>
> >>
> >>2. without the virtio_queue_host_notifier_read calls, with patch 4
> >>
> >>crash happens very often,
> >>
> >>(gdb) bt
> >>#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> >>#1  0x02aa39dda43e in coroutine_trampoline (i0=,
> >>i1=-1677715600) at util/coroutine-ucontext.c:79
> >>#2  0x03ffab6d150a in __makecontext_ret () from /lib64/libc.so.6
> >>
> >>
> >
> >Tu Bo,
> >
> >Could you help test this patch (on top of master, without patch 4)?
> >
> >diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> >index 08275a9..47f8043 100644
> >--- a/hw/virtio/virtio.c
> >+++ b/hw/virtio/virtio.c
> >@@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> >
> >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> >  {
> >-virtio_queue_notify_vq(>vq[n]);
> >+VirtQueue *vq = >vq[n];
> >+EventNotifier *n;
> >+n = virtio_queue_get_host_notifier(vq);
> >+if (n) {
> >+event_notifier_set(n);
> >+} else {
> >+virtio_queue_notify_vq(vq);
> >+}
> >  }
> >
> >  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> >
> >
> 
> I got a build error as below,
> 
> /BUILD/qemu-2.5.50/hw/virtio/virtio.c: In function 'virtio_queue_notify':
> /BUILD/qemu-2.5.50/hw/virtio/virtio.c:1102:20: error: 'n' redeclared
> as different kind of symbol
>  EventNotifier *n;
> ^
> /BUILD/qemu-2.5.50/hw/virtio/virtio.c:1099:50: note: previous
> definition of 'n' was here
>  void virtio_queue_notify(VirtIODevice *vdev, int n)
> 
> 
> Then I did some change for your patch as below,
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 08275a9..a10da39 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> 
>  void virtio_queue_notify(VirtIODevice *vdev, int n)
>  {
> -virtio_queue_notify_vq(>vq[n]);
> +VirtQueue *vq = >vq[n];
> +EventNotifier *en;
> +en = virtio_queue_get_host_notifier(vq);
> +if (en) {
> +event_notifier_set(en);
> +} else {
> +virtio_queue_notify_vq(vq);
> +}
>  }
> 
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> 
> With qemu master + modified patch above(without patch 4, without
> Conny's patches), I did NOT get crash so far.  thanks

Yes, it was a mistake.  Thanks for the testing!

Fam

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[tu bo]

Hi Fam:

On 03/21/2016 06:57 PM, Fam Zheng wrote:

On Thu, 03/17 19:03, tu bo wrote:


On 03/17/2016 08:39 AM, Fam Zheng wrote:

On Wed, 03/16 14:45, Paolo Bonzini wrote:



On 16/03/2016 14:38, Christian Borntraeger wrote:

If you just remove the calls to virtio_queue_host_notifier_read, here
and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
(keeping patches 2-4 in)?


With these changes and patch 2-4 it does no longer locks up.
I keep it running some hour to check if a crash happens.

Tu Bo, your setup is currently better suited for reproducing. Can you also 
check?


Great, I'll prepare a patch to virtio then sketching the solution that
Conny agreed with.

While Fam and I agreed that patch 1 is not required, I'm not sure if the
mutex is necessary in the end.


If we can fix this from the virtio_queue_host_notifier_read side, the mutex/BH
are not necessary; but OTOH the mutex does catch such bugs, so maybe it's good
to have it. I'm not sure about the BH.

And on a hindsight I realize we don't want patches 2-3 too. Actually the
begin/end pair won't work as expected because of the blk_set_aio_context.

Let's hold on this series.



So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
and both with/without Fam's patches, it would be great.


Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the noise.


1. without the virtio_queue_host_notifier_read calls,  without patch 4

crash happens very often,

(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa165da37e in coroutine_trampoline (i0=,
i1=1812051552) at util/coroutine-ucontext.c:79
#2  0x03ff7dd5150a in __makecontext_ret () from /lib64/libc.so.6


2. without the virtio_queue_host_notifier_read calls, with patch 4

crash happens very often,

(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa39dda43e in coroutine_trampoline (i0=,
i1=-1677715600) at util/coroutine-ucontext.c:79
#2  0x03ffab6d150a in __makecontext_ret () from /lib64/libc.so.6




Tu Bo,

Could you help test this patch (on top of master, without patch 4)?

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 08275a9..47f8043 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)

  void virtio_queue_notify(VirtIODevice *vdev, int n)
  {
-virtio_queue_notify_vq(>vq[n]);
+VirtQueue *vq = >vq[n];
+EventNotifier *n;
+n = virtio_queue_get_host_notifier(vq);
+if (n) {
+event_notifier_set(n);
+} else {
+virtio_queue_notify_vq(vq);
+}
  }

  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)




I got a build error as below,

/BUILD/qemu-2.5.50/hw/virtio/virtio.c: In function 'virtio_queue_notify':
/BUILD/qemu-2.5.50/hw/virtio/virtio.c:1102:20: error: 'n' redeclared as 
different kind of symbol

 EventNotifier *n;
^
/BUILD/qemu-2.5.50/hw/virtio/virtio.c:1099:50: note: previous definition 
of 'n' was here

 void virtio_queue_notify(VirtIODevice *vdev, int n)


Then I did some change for your patch as below,
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 08275a9..a10da39 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)

 void virtio_queue_notify(VirtIODevice *vdev, int n)
 {
-virtio_queue_notify_vq(>vq[n]);
+VirtQueue *vq = >vq[n];
+EventNotifier *en;
+en = virtio_queue_get_host_notifier(vq);
+if (en) {
+event_notifier_set(en);
+} else {
+virtio_queue_notify_vq(vq);
+}
 }

 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)

With qemu master + modified patch above(without patch 4, without Conny's 
patches), I did NOT get crash so far.  thanks

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Mon, 03/21 15:19, Cornelia Huck wrote:
> On Mon, 21 Mar 2016 14:54:04 +0100
> Paolo Bonzini  wrote:
> 
> > On 21/03/2016 14:47, TU BO wrote:
> > >> I'll see if I can produce something based on Conny's patches, which are
> > >> already a start.  Today I had a short day so I couldn't play with the
> > >> bug; out of curiosity, does the bug reproduce with her work + patch 4
> > >> from this series + the reentrancy assertion?
> > > I did NOT see crash with qemu master + "[PATCH RFC 0/6] virtio: refactor
> > > host notifiers" from Conny + patch 4 + assertion.  thx
> > 
> > That's unexpected, but I guess it only says that I didn't review her
> > patches well enough. :)
> 
> I'm also a bit surprised, the only thing that should really be
> different is passing the 'assign' argument in stop_ioeventfd(). Any
> other fixes are purely accidental :)
> 
> Would be interesting to see how this setup fares with virtio-pci.
> 

Seems to fix the assertion I'm hitting too.

Fam

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Mon, 03/21 14:02, Cornelia Huck wrote:
> On Mon, 21 Mar 2016 20:45:27 +0800
> Fam Zheng  wrote:
> 
> > On Mon, 03/21 12:15, Cornelia Huck wrote:
> > > On Mon, 21 Mar 2016 18:57:18 +0800
> > > Fam Zheng  wrote:
> > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index 08275a9..47f8043 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > > > 
> > > >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > > >  {
> > > > -virtio_queue_notify_vq(>vq[n]);
> > > > +VirtQueue *vq = >vq[n];
> > > > +EventNotifier *n;
> > > > +n = virtio_queue_get_host_notifier(vq);
> > > > +if (n) {
> > > 
> > > Isn't that always true, even if the notifier has not been setup?
> > 
> > You are right, this doesn't make a correct fix. But we can still do a quick
> > test with this as the else branch should never be used with ioeventfd=on. 
> > Am I
> > right?
> > 
> > Fam
> 
> Won't we come through here for the very first kick, when we haven't
> registered the ioeventfd with the kernel yet?
> 

The ioeventfd in virtio-ccw is registered in the main loop when
VIRTIO_CONFIG_S_DRIVER_OK is set, so I think the first kick is okay.

Fam

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Mon, 21 Mar 2016 14:54:04 +0100
Paolo Bonzini  wrote:

> On 21/03/2016 14:47, TU BO wrote:
> >> I'll see if I can produce something based on Conny's patches, which are
> >> already a start.  Today I had a short day so I couldn't play with the
> >> bug; out of curiosity, does the bug reproduce with her work + patch 4
> >> from this series + the reentrancy assertion?
> > I did NOT see crash with qemu master + "[PATCH RFC 0/6] virtio: refactor
> > host notifiers" from Conny + patch 4 + assertion.  thx
> 
> That's unexpected, but I guess it only says that I didn't review her
> patches well enough. :)

I'm also a bit surprised, the only thing that should really be
different is passing the 'assign' argument in stop_ioeventfd(). Any
other fixes are purely accidental :)

Would be interesting to see how this setup fares with virtio-pci.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 21/03/2016 14:47, TU BO wrote:
>> I'll see if I can produce something based on Conny's patches, which are
>> already a start.  Today I had a short day so I couldn't play with the
>> bug; out of curiosity, does the bug reproduce with her work + patch 4
>> from this series + the reentrancy assertion?
> I did NOT see crash with qemu master + "[PATCH RFC 0/6] virtio: refactor
> host notifiers" from Conny + patch 4 + assertion.  thx

That's unexpected, but I guess it only says that I didn't review her
patches well enough. :)

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[TU BO]

On 16/3/18 下午11:03, Paolo Bonzini wrote:


On 17/03/2016 17:08, Christian Borntraeger wrote:

Good (or bad?) news is the assert also triggers on F23, it just seems
to take longer.

I guess good news, because we can rule out the kernel (not that I
believed it was a kernel problem, but the thought is always there in
the background...).

The interaction between ioeventfd and dataplane is too complicated.  I
think if we get rid of the start/stop ioeventfd calls (just set up the
ioeventfd as soon as possible and then only set/clear the handlers)
things would be much simpler.

I'll see if I can produce something based on Conny's patches, which are
already a start.  Today I had a short day so I couldn't play with the
bug; out of curiosity, does the bug reproduce with her work + patch 4
from this series + the reentrancy assertion?
I did NOT see crash with qemu master + "[PATCH RFC 0/6] virtio: refactor 
host notifiers" from Conny + patch 4 + assertion.  thx


Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Mon, 21 Mar 2016 20:45:27 +0800
Fam Zheng  wrote:

> On Mon, 03/21 12:15, Cornelia Huck wrote:
> > On Mon, 21 Mar 2016 18:57:18 +0800
> > Fam Zheng  wrote:
> > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index 08275a9..47f8043 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > > 
> > >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> > >  {
> > > -virtio_queue_notify_vq(>vq[n]);
> > > +VirtQueue *vq = >vq[n];
> > > +EventNotifier *n;
> > > +n = virtio_queue_get_host_notifier(vq);
> > > +if (n) {
> > 
> > Isn't that always true, even if the notifier has not been setup?
> 
> You are right, this doesn't make a correct fix. But we can still do a quick
> test with this as the else branch should never be used with ioeventfd=on. Am I
> right?
> 
> Fam

Won't we come through here for the very first kick, when we haven't
registered the ioeventfd with the kernel yet?

> 
> > 
> > > +event_notifier_set(n);
> > > +} else {
> > > +virtio_queue_notify_vq(vq);
> > > +}
> > >  }
> > > 
> > >  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> > > 
> > > 
> > 
> 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Mon, 03/21 12:15, Cornelia Huck wrote:
> On Mon, 21 Mar 2016 18:57:18 +0800
> Fam Zheng  wrote:
> 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index 08275a9..47f8043 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> > 
> >  void virtio_queue_notify(VirtIODevice *vdev, int n)
> >  {
> > -virtio_queue_notify_vq(>vq[n]);
> > +VirtQueue *vq = >vq[n];
> > +EventNotifier *n;
> > +n = virtio_queue_get_host_notifier(vq);
> > +if (n) {
> 
> Isn't that always true, even if the notifier has not been setup?

You are right, this doesn't make a correct fix. But we can still do a quick
test with this as the else branch should never be used with ioeventfd=on. Am I
right?

Fam

> 
> > +event_notifier_set(n);
> > +} else {
> > +virtio_queue_notify_vq(vq);
> > +}
> >  }
> > 
> >  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> > 
> > 
> 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Mon, 21 Mar 2016 17:42:06 +0800
Fam Zheng  wrote:

> On Fri, 03/18 16:03, Paolo Bonzini wrote:
> >
> >
> > On 17/03/2016 17:08, Christian Borntraeger wrote:
> > > Good (or bad?) news is the assert also triggers on F23, it just seems
> > > to take longer.
> >
> > I guess good news, because we can rule out the kernel (not that I
> > believed it was a kernel problem, but the thought is always there in
> > the background...).
> >
> > The interaction between ioeventfd and dataplane is too complicated.  I
> > think if we get rid of the start/stop ioeventfd calls (just set up the
> > ioeventfd as soon as possible and then only set/clear the handlers)
> > things would be much simpler.
> >
> > I'll see if I can produce something based on Conny's patches, which are
> > already a start.  Today I had a short day so I couldn't play with the
> > bug; out of curiosity, does the bug reproduce with her work + patch 4
> > from this series + the reentrancy assertion?
> 
> The other half of the race condition is from ioport write in the vcpu thread. 
> I
> hit this by adding an extra assert(is_in_iothread()) in
> virtio_blk_handle_request(), at the same place with Paolo's atomic read of
> variable "test".
> 
> I haven't tried to find where this ioport write is from, but that is indeed an
> issue in virtio-pci.

Might this be a slow-path notification from before the ioeventfd got
registered on the io bus (IOW before qemu got control)? We have the
first notification that triggers dataplane setup (including registering
the ioeventfd). The second notification was already making its way to
qemu, and gets only handled when ->started had already been set.

Now I'm totally disregarding any possible locking between threads etc.
(perhaps somebody on cc: knows better?), but would the following logic
in handle_output make sense?

if (dataplane) {
   if (!started) {
  dataplane_start();
   }
   if (!disabled) {
  return;
   }
}
/* slow path processing */

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Mon, 21 Mar 2016 18:57:18 +0800
Fam Zheng  wrote:

> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index 08275a9..47f8043 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
> 
>  void virtio_queue_notify(VirtIODevice *vdev, int n)
>  {
> -virtio_queue_notify_vq(>vq[n]);
> +VirtQueue *vq = >vq[n];
> +EventNotifier *n;
> +n = virtio_queue_get_host_notifier(vq);
> +if (n) {

Isn't that always true, even if the notifier has not been setup?

> +event_notifier_set(n);
> +} else {
> +virtio_queue_notify_vq(vq);
> +}
>  }
> 
>  uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)
> 
> 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Christian Borntraeger]

On 03/21/2016 10:42 AM, Fam Zheng wrote:
> On Fri, 03/18 16:03, Paolo Bonzini wrote:
>>
>>
>> On 17/03/2016 17:08, Christian Borntraeger wrote:
>>> Good (or bad?) news is the assert also triggers on F23, it just seems
>>> to take longer.
>>
>> I guess good news, because we can rule out the kernel (not that I
>> believed it was a kernel problem, but the thought is always there in
>> the background...).
>>
>> The interaction between ioeventfd and dataplane is too complicated.  I
>> think if we get rid of the start/stop ioeventfd calls (just set up the
>> ioeventfd as soon as possible and then only set/clear the handlers)
>> things would be much simpler.
>>
>> I'll see if I can produce something based on Conny's patches, which are
>> already a start.  Today I had a short day so I couldn't play with the
>> bug; out of curiosity, does the bug reproduce with her work + patch 4
>> from this series + the reentrancy assertion?
> 
> The other half of the race condition is from ioport write in the vcpu thread. 
> I
> hit this by adding an extra assert(is_in_iothread()) in
> virtio_blk_handle_request(), at the same place with Paolo's atomic read of
> variable "test".

Thats good, that you can reproduce on x86.
the ioport write in the vcpu thread, is the equivalent of s390_virtio_hypercall 
on
s390 - a virtio kick that is usually handled by eventfd but here we  have a case
where we go the slow path. So the good thing is that this is not s390 specific,
which might help to find the issue more quickly.



> 
> I haven't tried to find where this ioport write is from, but that is indeed an
> issue in virtio-pci.
> 
> (gdb) thread apply all bt
> 
> <...>
> 
> Thread 3 (Thread 0x7f9e8928b700 (LWP 30671)):
> #0  0x7f9e8bac65d7 in __GI_raise (sig=sig@entry=6) at 
> ../nptl/sysdeps/unix/sysv/linux/raise.c:56
> #1  0x7f9e8bac7cc8 in __GI_abort () at abort.c:90
> #2  0x7f9e8babf546 in __assert_fail_base (fmt=0x7f9e8bc0f128 "%s%s%s:%u: 
> %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7f9e8e04e9d1 
> "is_in_iothread()",
> file=file@entry=0x7f9e8e04e8e0 
> "/home/fam/work/qemu/hw/block/virtio-blk.c", line=line@entry=597,
> function=function@entry=0x7f9e8e04ec30 <__PRETTY_FUNCTION__.37148> 
> "virtio_blk_handle_output") at assert.c:92
> #3  0x7f9e8babf5f2 in __GI___assert_fail (assertion=0x7f9e8e04e9d1 
> "is_in_iothread()", file=0x7f9e8e04e8e0 
> "/home/fam/work/qemu/hw/block/virtio-blk.c", line=597,
> function=0x7f9e8e04ec30 <__PRETTY_FUNCTION__.37148> 
> "virtio_blk_handle_output") at assert.c:101
> #4  0x7f9e8dc9f414 in virtio_blk_handle_output (vdev=0x7f9e929d7b68, 
> vq=0x7f9e92a762f0) at /home/fam/work/qemu/hw/block/virtio-blk.c:597
> #5  0x7f9e8dcd6f53 in virtio_queue_notify_vq (vq=0x7f9e92a762f0) at 
> /home/fam/work/qemu/hw/virtio/virtio.c:1095
> #6  0x7f9e8dcd6f91 in virtio_queue_notify (vdev=0x7f9e929d7b68, n=0) at 
> /home/fam/work/qemu/hw/virtio/virtio.c:1101
> #7  0x7f9e8df03d2f in virtio_ioport_write (opaque=0x7f9e929cf840, 
> addr=16, val=0) at /home/fam/work/qemu/hw/virtio/virtio-pci.c:419
> #8  0x7f9e8df041be in virtio_pci_config_write (opaque=0x7f9e929cf840, 
> addr=16, val=0, size=2) at /home/fam/work/qemu/hw/virtio/virtio-pci.c:552
> #9  0x7f9e8dc7c8c9 in memory_region_write_accessor (mr=0x7f9e929d00c0, 
> addr=16, value=0x7f9e8928a988, size=2, shift=0, mask=65535, attrs=...)
> at /home/fam/work/qemu/memory.c:524
> #10 0x7f9e8dc7cad4 in access_with_adjusted_size (addr=16, 
> value=0x7f9e8928a988, size=2, access_size_min=1, access_size_max=4, access=
> 0x7f9e8dc7c7e8 , mr=0x7f9e929d00c0, 
> attrs=...) at /home/fam/work/qemu/memory.c:590
> #11 0x7f9e8dc7f71b in memory_region_dispatch_write (mr=0x7f9e929d00c0, 
> addr=16, data=0, size=2, attrs=...) at /home/fam/work/qemu/memory.c:1272
> #12 0x7f9e8dc32815 in address_space_write_continue (as=0x7f9e8e5834a0 
> , addr=49232, attrs=..., buf=0x7f9e8daa9000  0x7f9e8daa9000 out of bounds>,
> len=2, addr1=16, l=2, mr=0x7f9e929d00c0) at 
> /home/fam/work/qemu/exec.c:2607
> #13 0x7f9e8dc329c1 in address_space_write (as=0x7f9e8e5834a0 
> , addr=49232, attrs=..., buf=0x7f9e8daa9000  0x7f9e8daa9000 out of bounds>, len=2)
> at /home/fam/work/qemu/exec.c:2659
> #14 0x7f9e8dc32d78 in address_space_rw (as=0x7f9e8e5834a0 
> , addr=49232, attrs=..., buf=0x7f9e8daa9000  0x7f9e8daa9000 out of bounds>, len=2,
> is_write=true) at /home/fam/work/qemu/exec.c:2762
> #15 0x7f9e8dc79358 in kvm_handle_io (port=49232, attrs=..., 
> data=0x7f9e8daa9000, direction=1, size=2, count=1) at 
> /home/fam/work/qemu/kvm-all.c:1699
> #16 0x7f9e8dc79858 in kvm_cpu_exec (cpu=0x7f9e905a5250) at 
> /home/fam/work/qemu/kvm-all.c:1863
> #17 0x7f9e8dc619a3 in qemu_kvm_cpu_thread_fn (arg=0x7f9e905a5250) at 
> /home/fam/work/qemu/cpus.c:1056
> #18 0x7f9e8be59df5 in start_thread (arg=0x7f9e8928b700) at 
> pthread_create.c:308
> #19 0x7f9e8bb871ad in clone () at 
> 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Thu, 03/17 19:03, tu bo wrote:
> 
> On 03/17/2016 08:39 AM, Fam Zheng wrote:
> >On Wed, 03/16 14:45, Paolo Bonzini wrote:
> >>
> >>
> >>On 16/03/2016 14:38, Christian Borntraeger wrote:
> If you just remove the calls to virtio_queue_host_notifier_read, here
> and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
> (keeping patches 2-4 in)?
> >>>
> >>>With these changes and patch 2-4 it does no longer locks up.
> >>>I keep it running some hour to check if a crash happens.
> >>>
> >>>Tu Bo, your setup is currently better suited for reproducing. Can you also 
> >>>check?
> >>
> >>Great, I'll prepare a patch to virtio then sketching the solution that
> >>Conny agreed with.
> >>
> >>While Fam and I agreed that patch 1 is not required, I'm not sure if the
> >>mutex is necessary in the end.
> >
> >If we can fix this from the virtio_queue_host_notifier_read side, the 
> >mutex/BH
> >are not necessary; but OTOH the mutex does catch such bugs, so maybe it's 
> >good
> >to have it. I'm not sure about the BH.
> >
> >And on a hindsight I realize we don't want patches 2-3 too. Actually the
> >begin/end pair won't work as expected because of the blk_set_aio_context.
> >
> >Let's hold on this series.
> >
> >>
> >>So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
> >>and both with/without Fam's patches, it would be great.
> >
> >Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the noise.
> >
> 1. without the virtio_queue_host_notifier_read calls,  without patch 4
> 
> crash happens very often,
> 
> (gdb) bt
> #0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> #1  0x02aa165da37e in coroutine_trampoline (i0=,
> i1=1812051552) at util/coroutine-ucontext.c:79
> #2  0x03ff7dd5150a in __makecontext_ret () from /lib64/libc.so.6
> 
> 
> 2. without the virtio_queue_host_notifier_read calls, with patch 4
> 
> crash happens very often,
> 
> (gdb) bt
> #0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> #1  0x02aa39dda43e in coroutine_trampoline (i0=,
> i1=-1677715600) at util/coroutine-ucontext.c:79
> #2  0x03ffab6d150a in __makecontext_ret () from /lib64/libc.so.6
> 
> 

Tu Bo,

Could you help test this patch (on top of master, without patch 4)?

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index 08275a9..47f8043 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1098,7 +1098,14 @@ void virtio_queue_notify_vq(VirtQueue *vq)
 
 void virtio_queue_notify(VirtIODevice *vdev, int n)
 {
-virtio_queue_notify_vq(>vq[n]);
+VirtQueue *vq = >vq[n];
+EventNotifier *n;
+n = virtio_queue_get_host_notifier(vq);
+if (n) {
+event_notifier_set(n);
+} else {
+virtio_queue_notify_vq(vq);
+}
 }
 
 uint16_t virtio_queue_vector(VirtIODevice *vdev, int n)

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Fri, 03/18 16:03, Paolo Bonzini wrote:
>
>
> On 17/03/2016 17:08, Christian Borntraeger wrote:
> > Good (or bad?) news is the assert also triggers on F23, it just seems
> > to take longer.
>
> I guess good news, because we can rule out the kernel (not that I
> believed it was a kernel problem, but the thought is always there in
> the background...).
>
> The interaction between ioeventfd and dataplane is too complicated.  I
> think if we get rid of the start/stop ioeventfd calls (just set up the
> ioeventfd as soon as possible and then only set/clear the handlers)
> things would be much simpler.
>
> I'll see if I can produce something based on Conny's patches, which are
> already a start.  Today I had a short day so I couldn't play with the
> bug; out of curiosity, does the bug reproduce with her work + patch 4
> from this series + the reentrancy assertion?

The other half of the race condition is from ioport write in the vcpu thread. I
hit this by adding an extra assert(is_in_iothread()) in
virtio_blk_handle_request(), at the same place with Paolo's atomic read of
variable "test".

I haven't tried to find where this ioport write is from, but that is indeed an
issue in virtio-pci.

(gdb) thread apply all bt

<...>

Thread 3 (Thread 0x7f9e8928b700 (LWP 30671)):
#0  0x7f9e8bac65d7 in __GI_raise (sig=sig@entry=6) at 
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7f9e8bac7cc8 in __GI_abort () at abort.c:90
#2  0x7f9e8babf546 in __assert_fail_base (fmt=0x7f9e8bc0f128 "%s%s%s:%u: 
%s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7f9e8e04e9d1 
"is_in_iothread()",
file=file@entry=0x7f9e8e04e8e0 "/home/fam/work/qemu/hw/block/virtio-blk.c", 
line=line@entry=597,
function=function@entry=0x7f9e8e04ec30 <__PRETTY_FUNCTION__.37148> 
"virtio_blk_handle_output") at assert.c:92
#3  0x7f9e8babf5f2 in __GI___assert_fail (assertion=0x7f9e8e04e9d1 
"is_in_iothread()", file=0x7f9e8e04e8e0 
"/home/fam/work/qemu/hw/block/virtio-blk.c", line=597,
function=0x7f9e8e04ec30 <__PRETTY_FUNCTION__.37148> 
"virtio_blk_handle_output") at assert.c:101
#4  0x7f9e8dc9f414 in virtio_blk_handle_output (vdev=0x7f9e929d7b68, 
vq=0x7f9e92a762f0) at /home/fam/work/qemu/hw/block/virtio-blk.c:597
#5  0x7f9e8dcd6f53 in virtio_queue_notify_vq (vq=0x7f9e92a762f0) at 
/home/fam/work/qemu/hw/virtio/virtio.c:1095
#6  0x7f9e8dcd6f91 in virtio_queue_notify (vdev=0x7f9e929d7b68, n=0) at 
/home/fam/work/qemu/hw/virtio/virtio.c:1101
#7  0x7f9e8df03d2f in virtio_ioport_write (opaque=0x7f9e929cf840, addr=16, 
val=0) at /home/fam/work/qemu/hw/virtio/virtio-pci.c:419
#8  0x7f9e8df041be in virtio_pci_config_write (opaque=0x7f9e929cf840, 
addr=16, val=0, size=2) at /home/fam/work/qemu/hw/virtio/virtio-pci.c:552
#9  0x7f9e8dc7c8c9 in memory_region_write_accessor (mr=0x7f9e929d00c0, 
addr=16, value=0x7f9e8928a988, size=2, shift=0, mask=65535, attrs=...)
at /home/fam/work/qemu/memory.c:524
#10 0x7f9e8dc7cad4 in access_with_adjusted_size (addr=16, 
value=0x7f9e8928a988, size=2, access_size_min=1, access_size_max=4, access=
0x7f9e8dc7c7e8 , mr=0x7f9e929d00c0, 
attrs=...) at /home/fam/work/qemu/memory.c:590
#11 0x7f9e8dc7f71b in memory_region_dispatch_write (mr=0x7f9e929d00c0, 
addr=16, data=0, size=2, attrs=...) at /home/fam/work/qemu/memory.c:1272
#12 0x7f9e8dc32815 in address_space_write_continue (as=0x7f9e8e5834a0 
, addr=49232, attrs=..., buf=0x7f9e8daa9000 ,
len=2, addr1=16, l=2, mr=0x7f9e929d00c0) at /home/fam/work/qemu/exec.c:2607
#13 0x7f9e8dc329c1 in address_space_write (as=0x7f9e8e5834a0 
, addr=49232, attrs=..., buf=0x7f9e8daa9000 , len=2)
at /home/fam/work/qemu/exec.c:2659
#14 0x7f9e8dc32d78 in address_space_rw (as=0x7f9e8e5834a0 
, addr=49232, attrs=..., buf=0x7f9e8daa9000 , len=2,
is_write=true) at /home/fam/work/qemu/exec.c:2762
#15 0x7f9e8dc79358 in kvm_handle_io (port=49232, attrs=..., 
data=0x7f9e8daa9000, direction=1, size=2, count=1) at 
/home/fam/work/qemu/kvm-all.c:1699
#16 0x7f9e8dc79858 in kvm_cpu_exec (cpu=0x7f9e905a5250) at 
/home/fam/work/qemu/kvm-all.c:1863
#17 0x7f9e8dc619a3 in qemu_kvm_cpu_thread_fn (arg=0x7f9e905a5250) at 
/home/fam/work/qemu/cpus.c:1056
#18 0x7f9e8be59df5 in start_thread (arg=0x7f9e8928b700) at 
pthread_create.c:308
#19 0x7f9e8bb871ad in clone () at 
../sysdeps/unix/sysv/linux/x86_64/clone.S:113

<...>

Thread 1 (Thread 0x7f9e8b28f700 (LWP 30667)):
#0  0x7f9e8bac65d7 in __GI_raise (sig=sig@entry=6) at 
../nptl/sysdeps/unix/sysv/linux/raise.c:56
#1  0x7f9e8bac7cc8 in __GI_abort () at abort.c:90
#2  0x7f9e8babf546 in __assert_fail_base (fmt=0x7f9e8bc0f128 "%s%s%s:%u: 
%s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7f9e8e04e9e2 "x == 
0",
file=file@entry=0x7f9e8e04e8e0 "/home/fam/work/qemu/hw/block/virtio-blk.c", 
line=line@entry=598,
function=function@entry=0x7f9e8e04ec30 <__PRETTY_FUNCTION__.37148> 
"virtio_blk_handle_output") at assert.c:92
#3  0x7f9e8babf5f2 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 12:32, Cornelia Huck wrote:
> On Wed, 16 Mar 2016 12:09:02 +0100
> Paolo Bonzini  wrote:
> 
>> On 16/03/2016 11:49, Christian Borntraeger wrote:
> 
>>> #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
>>> /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
>>> #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
>>> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
>>> #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
>>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
>>> #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
>>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
>>> #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
>>> (vq=0xba305270, assign=false, set_handler=false) at 
>>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
>>> #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
>>> (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
>>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
>>> #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
>>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154
>>
>> One bug is here: virtio_ccw_stop_ioeventfd, in this case, should pass
>> assign=true to virtio_ccw_set_guest2host_notifier.  (Assuming my
>> understanding of assign=true is correct; I think it means "I'm going to
>> set up another host notifier handler").
> 
> Hm... I copied these semantics from virtio-pci, and they still seem to
> be the same. I wonder why we never saw this on virtio-pci?

Just because we never ran the right tests, probably.  The bug is indeed
in virtio-pci as well (I didn't check mmio).

>> In dataplane, instead, all calls to
>> virtio_queue_set_host_notifier_fd_handler and
>> virtio_queue_aio_set_host_notifier_handler should have assign=true.  The
>> ioeventfd is just being moved from one aiocontext to another.
> 
> How can a transport know where they are called from?

That's a bug in dataplane.  The calls should be changed in
hw/block/dataplane/virtio-blk.c

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 12:24, Christian Borntraeger wrote:
> On 03/16/2016 12:09 PM, Paolo Bonzini wrote:
>> On 16/03/2016 11:49, Christian Borntraeger wrote:
>>> #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
>>> /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
>>> #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
>>> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
>>> #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
>>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
>>> #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
>>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785

If you just remove the calls to virtio_queue_host_notifier_read, here
and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
(keeping patches 2-4 in)?

Paolo

>>> #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
>>> (vq=0xba305270, assign=false, set_handler=false) at 
>>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
>>> #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
>>> (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
>>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
>>> #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
>>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Christian Borntraeger]

On 03/16/2016 12:09 PM, Paolo Bonzini wrote:
> 
> 
> On 16/03/2016 11:49, Christian Borntraeger wrote:
>> Seems to lockup.
> 
> That's an improvement actually. :)
> 
>> Thread 3 (Thread 0x3ff888dc910 (LWP 88958)):
>> #0  0x03ff8ca90cd4 in __lll_lock_wait () at /lib64/libpthread.so.0
>> #1  0x03ff8ca93e74 in __lll_lock_elision () at /lib64/libpthread.so.0
> 
> Off-topic, I love how s390 backtraces always have a little bit of
> showing off in them! :)

So I understood your first remark (about _working_ transactional memory) but  I 
cannot parse your 2nd one. For Conny it is exactly the opposite, so I will let
Conny answer belows discussion ;-)



> 
>> #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
>> /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
>> #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
>> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
>> #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
>> #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
>> #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
>> (vq=0xba305270, assign=false, set_handler=false) at 
>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
>> #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
>> (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
>> #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154
> 
> One bug is here: virtio_ccw_stop_ioeventfd, in this case, should pass
> assign=true to virtio_ccw_set_guest2host_notifier.  (Assuming my
> understanding of assign=true is correct; I think it means "I'm going to
> set up another host notifier handler").
> 
> In dataplane, instead, all calls to
> virtio_queue_set_host_notifier_fd_handler and
> virtio_queue_aio_set_host_notifier_handler should have assign=true.  The
> ioeventfd is just being moved from one aiocontext to another.
> 
> Paolo
> 
>> #10 0x8010d5aa in virtio_ccw_set_host_notifier (d=0xb9eed6a0, n=0, 
>> assign=true) at /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:1211
>> #11 0x800b722c in virtio_blk_data_plane_start (s=0xba232d80) at 
>> /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:242
>> #12 0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
>> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
>> #13 0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
>> #14 0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
>> #15 0x802f1cd4 in aio_dispatch (ctx=0xb9e81c70) at 
>> /home/cborntra/REPOS/qemu/aio-posix.c:327
>> #16 0x802df31c in aio_ctx_dispatch (source=0xb9e81c70, callback=0x0, 
>> user_data=0x0) at /home/cborntra/REPOS/qemu/async.c:232
> 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 14:38, Christian Borntraeger wrote:
> > If you just remove the calls to virtio_queue_host_notifier_read, here
> > and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
> > (keeping patches 2-4 in)?
> 
> With these changes and patch 2-4 it does no longer locks up. 
> I keep it running some hour to check if a crash happens.
> 
> Tu Bo, your setup is currently better suited for reproducing. Can you also 
> check?

Great, I'll prepare a patch to virtio then sketching the solution that
Conny agreed with.

While Fam and I agreed that patch 1 is not required, I'm not sure if the
mutex is necessary in the end.

So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
and both with/without Fam's patches, it would be great.

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Christian Borntraeger]

Good (or bad?) news is the assert also triggers on F23, it just seems to take 
longer.

trace 1 (compiled on F20)

Thread 5 (Thread 0x3ff84b7f910 (LWP 33030)):
#0  0x03ff86a817b8 in ppoll () at /lib64/libc.so.6
#1  0x102c712e in qemu_poll_ns (fds=0x3ff80001e70, nfds=3, timeout=-1) 
at /home/cborntra/REPOS/qemu/qemu-timer.c:313
#2  0x102c96d6 in aio_poll (ctx=0x10a7b050, blocking=true) at 
/home/cborntra/REPOS/qemu/aio-posix.c:453
#3  0x1014fb1e in iothread_run (opaque=0x10a7ab10) at 
/home/cborntra/REPOS/qemu/iothread.c:46
#4  0x03ff86b87c2c in start_thread () at /lib64/libpthread.so.0
#5  0x03ff86a8ec9a in thread_start () at /lib64/libc.so.6

Thread 4 (Thread 0x3ff8537f910 (LWP 33029)):
#0  0x03ff86a8841e in syscall () at /lib64/libc.so.6
#1  0x1039cbe8 in futex_wait (ev=0x10a140ec , 
val=4294967295) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:292
#2  0x1039ce1e in qemu_event_wait (ev=0x10a140ec 
) at 
/home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:399
#3  0x103b9678 in call_rcu_thread (opaque=0x0) at 
/home/cborntra/REPOS/qemu/util/rcu.c:250
#4  0x03ff86b87c2c in start_thread () at /lib64/libpthread.so.0
#5  0x03ff86a8ec9a in thread_start () at /lib64/libc.so.6

Thread 3 (Thread 0x3ff66428910 (LWP 33041)):
#0  0x03ff86a8334a in ioctl () at /lib64/libc.so.6
#1  0x1007b97a in kvm_vcpu_ioctl (cpu=0x10b15970, type=44672) at 
/home/cborntra/REPOS/qemu/kvm-all.c:1984
#2  0x1007b2f0 in kvm_cpu_exec (cpu=0x10b15970) at 
/home/cborntra/REPOS/qemu/kvm-all.c:1834
#3  0x1005bd92 in qemu_kvm_cpu_thread_fn (arg=0x10b15970) at 
/home/cborntra/REPOS/qemu/cpus.c:1050
#4  0x03ff86b87c2c in start_thread () at /lib64/libpthread.so.0
#5  0x03ff86a8ec9a in thread_start () at /lib64/libc.so.6

Thread 2 (Thread 0x3ff885dbb90 (LWP 32970)):
#0  0x03ff86a817b8 in ppoll () at /lib64/libc.so.6
#1  0x102c7244 in qemu_poll_ns (fds=0x10a77a90, nfds=5, 
timeout=96500) at /home/cborntra/REPOS/qemu/qemu-timer.c:325
#2  0x102c5ef2 in os_host_main_loop_wait (timeout=96500) at 
/home/cborntra/REPOS/qemu/main-loop.c:251
#3  0x102c6006 in main_loop_wait (nonblocking=0) at 
/home/cborntra/REPOS/qemu/main-loop.c:505
#4  0x101667e4 in main_loop () at /home/cborntra/REPOS/qemu/vl.c:1933
#5  0x1016e8ea in main (argc=72, argv=0x3ffe977e978, 
envp=0x3ffe977ebc0) at /home/cborntra/REPOS/qemu/vl.c:4656

Thread 1 (Thread 0x3ff66c28910 (LWP 33033)):
#0  0x03ff869be2c0 in raise () at /lib64/libc.so.6
#1  0x03ff869bfc26 in abort () at /lib64/libc.so.6
#2  0x03ff869b5bce in __assert_fail_base () at /lib64/libc.so.6
#3  0x03ff869b5c5c in  () at /lib64/libc.so.6
#4  0x100ab8f2 in virtio_blk_handle_output (vdev=0x10ad57e8, 
vq=0x10eec270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:595
#5  0x100e18a4 in virtio_queue_notify_vq (vq=0x10eec270) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
#6  0x100e1906 in virtio_queue_notify (vdev=0x10ad57e8, n=0) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1101
#7  0x100f921c in virtio_ccw_hcall_notify (args=0x10e2aad0) at 
/home/cborntra/REPOS/qemu/hw/s390x/s390-virtio-ccw.c:66
#8  0x100ee518 in s390_virtio_hypercall (env=0x10e2aac0) at 
/home/cborntra/REPOS/qemu/hw/s390x/s390-virtio-hcall.c:35
#9  0x1014192e in handle_hypercall (cpu=0x10e227f0, run=0x3ff8428) 
at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:1283
#10 0x10141c36 in handle_diag (cpu=0x10e227f0, run=0x3ff8428, 
ipb=83886080) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:1352
#11 0x101431b6 in handle_instruction (cpu=0x10e227f0, 
run=0x3ff8428) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:1799
#12 0x101433ee in handle_intercept (cpu=0x10e227f0) at 
/home/cborntra/REPOS/qemu/target-s390x/kvm.c:1842
#13 0x10143c22 in kvm_arch_handle_exit (cs=0x10e227f0, 
run=0x3ff8428) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:2028
#14 0x1007b5aa in kvm_cpu_exec (cpu=0x10e227f0) at 
/home/cborntra/REPOS/qemu/kvm-all.c:1921
#15 0x1005bd92 in qemu_kvm_cpu_thread_fn (arg=0x10e227f0) at 
/home/cborntra/REPOS/qemu/cpus.c:1050
#16 0x03ff86b87c2c in start_thread () at /lib64/libpthread.so.0
#17 0x03ff86a8ec9a in thread_start () at /lib64/libc.so.6


trace 2 (compiled on F23)

Thread 5 (Thread 0x3ffb897f910 (LWP 37895)):
#0  0x03ffba00841e in syscall () at /lib64/libc.so.6
#1  0x803d5306 in futex_wait (ev=0x80a4a104 , 
val=4294967295) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:292
#2  0x803d5596 in qemu_event_wait (ev=0x80a4a104 
) at 
/home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:399
#3  0x803f2c3c in call_rcu_thread (opaque=0x0) at 
/home/cborntra/REPOS/qemu/util/rcu.c:250
#4  0x03ffba107c2c in start_thread () at /lib64/libpthread.so.0
#5  0x03ffba00ec9a in thread_start () at /lib64/libc.so.6

Thread 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 17/03/2016 13:39, Christian Borntraeger wrote:
> As an interesting side note, I updated my system from F20 to F23 some days ago
> (after the initial report). While To Bo is still on a F20 system. I was not 
> able
> to reproduce the original crash on f23. but going back to F20 made this
> problem re-appear.
>  
>   Stack trace of thread 26429:
> #0  0x802008aa tracked_request_begin 
> (qemu-system-s390x)
> #1  0x80203f3c bdrv_co_do_preadv (qemu-system-s390x)
> #2  0x8020567c bdrv_co_do_readv (qemu-system-s390x)
> #3  0x8025d0f4 coroutine_trampoline 
> (qemu-system-s390x)
> #4  0x03ff943d150a __makecontext_ret (libc.so.6)
> 
> this is with patch 2-4 plus the removal of virtio_queue_host_notifier_read.
> 
> Without removing virtio_queue_host_notifier_read, I get the same mutex lockup 
> (as expected).
> 
> Maybe we have two independent issues here and this is some old bug in glibc or
> whatever?

I'm happy to try and reproduce on x86 if you give me some instruction
(RHEL7 should be close enough to Fedora 20).

Can you add an assert in virtio_blk_handle_output to catch reentrancy, like

diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index a7ec572..96ea896 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -591,6 +591,8 @@ static void virtio_blk_handle_output(VirtIODevice
*vdev, VirtQueue *vq)
 return;
 }

+int x = atomic_fetch_inc(>test);
+assert(x == 0);
 blk_io_plug(s->blk);

 while ((req = virtio_blk_get_request(s))) {
@@ -602,6 +604,7 @@ static void virtio_blk_handle_output(VirtIODevice
*vdev, VirtQueue *vq)
 }

 blk_io_unplug(s->blk);
+atomic_dec(>test);
 }

 static void virtio_blk_dma_restart_bh(void *opaque)
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index ae84d92..6472503 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -48,6 +48,7 @@ typedef struct VirtIOBlock {
 BlockBackend *blk;
 VirtQueue *vq;
 void *rq;
+int test;
 QEMUBH *bh;
 VirtIOBlkConf conf;
 unsigned short sector_mask;

?

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Christian Borntraeger]

On 03/16/2016 01:55 PM, Paolo Bonzini wrote:
> 
> 
> On 16/03/2016 12:24, Christian Borntraeger wrote:
>> On 03/16/2016 12:09 PM, Paolo Bonzini wrote:
>>> On 16/03/2016 11:49, Christian Borntraeger wrote:
 #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
 /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
 #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
 vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
 #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
 /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
 #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) 
 at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
> 
> If you just remove the calls to virtio_queue_host_notifier_read, here
> and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
> (keeping patches 2-4 in)?

With these changes and patch 2-4 it does no longer locks up. 
I keep it running some hour to check if a crash happens.

Tu Bo, your setup is currently better suited for reproducing. Can you also 
check?

> 
> Paolo
> 
 #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
 (vq=0xba305270, assign=false, set_handler=false) at 
 /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
 #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
 (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
 /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
 #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
 /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154
> 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Wed, 16 Mar 2016 12:32:13 +0100
Cornelia Huck  wrote:

> On Wed, 16 Mar 2016 12:09:02 +0100
> Paolo Bonzini  wrote:
> 
> > On 16/03/2016 11:49, Christian Borntraeger wrote:
> 
> > > #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
> > > /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
> > > #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
> > > vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
> > > #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
> > > /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
> > > #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) 
> > > at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
> > > #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
> > > (vq=0xba305270, assign=false, set_handler=false) at 
> > > /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
> > > #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
> > > (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
> > > /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
> > > #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
> > > /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154
> > 
> > One bug is here: virtio_ccw_stop_ioeventfd, in this case, should pass
> > assign=true to virtio_ccw_set_guest2host_notifier.  (Assuming my
> > understanding of assign=true is correct; I think it means "I'm going to
> > set up another host notifier handler").
> 
> Hm... I copied these semantics from virtio-pci, and they still seem to
> be the same. I wonder why we never saw this on virtio-pci?
> 
> > In dataplane, instead, all calls to
> > virtio_queue_set_host_notifier_fd_handler and
> > virtio_queue_aio_set_host_notifier_handler should have assign=true.  The
> > ioeventfd is just being moved from one aiocontext to another.
> 
> How can a transport know where they are called from?

Hm^2... I looked at virtio-scsi dataplane, and I noticed that it
acquires the aio context prior to calling ->set_host_notifiers(). Does
virtio-blk dataplane need to do this as well, or is virtio-scsi
dataplane wrong/different?

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Thu, 17 Mar 2016 13:39:18 +0100
Christian Borntraeger  wrote:

> On 03/17/2016 01:22 PM, tu bo wrote:
> > 
> > On 03/16/2016 09:38 PM, Christian Borntraeger wrote:
> >> On 03/16/2016 01:55 PM, Paolo Bonzini wrote:
> >>>
> >>>
> >>> On 16/03/2016 12:24, Christian Borntraeger wrote:
>  On 03/16/2016 12:09 PM, Paolo Bonzini wrote:
> > On 16/03/2016 11:49, Christian Borntraeger wrote:
> >> #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) 
> >> at /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
> >> #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
> >> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
> >> #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
> >> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
> >> #6  0x800f1c9c in virtio_queue_host_notifier_read 
> >> (n=0xba3052c8) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
> >>>
> >>> If you just remove the calls to virtio_queue_host_notifier_read, here
> >>> and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
> >>> (keeping patches 2-4 in)?
> >>
> >> With these changes and patch 2-4 it does no longer locks up.
> >> I keep it running some hour to check if a crash happens.
> >>
> >> Tu Bo, your setup is currently better suited for reproducing. Can you also 
> >> check?
> > 
> > remove the calls to virtio_queue_host_notifier_read, and keeping patches 
> > 2-4 in,
> > 
> > I got same crash as before,
> > (gdb) bt
> > #0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> > #1  0x02aa0c65d786 in coroutine_trampoline (i0=, 
> > i1=-2013204784) at util/coroutine-ucontext.c:79
> > #2  0x03ff99ad150a in __makecontext_ret () from /lib64/libc.so.6
> > 
> 
> As an interesting side note, I updated my system from F20 to F23 some days ago
> (after the initial report). While To Bo is still on a F20 system. I was not 
> able
> to reproduce the original crash on f23. but going back to F20 made this
> problem re-appear.
> 
>   Stack trace of thread 26429:
> #0  0x802008aa tracked_request_begin 
> (qemu-system-s390x)
> #1  0x80203f3c bdrv_co_do_preadv (qemu-system-s390x)
> #2  0x8020567c bdrv_co_do_readv (qemu-system-s390x)
> #3  0x8025d0f4 coroutine_trampoline 
> (qemu-system-s390x)
> #4  0x03ff943d150a __makecontext_ret (libc.so.6)
> 
> this is with patch 2-4 plus the removal of virtio_queue_host_notifier_read.
> 
> Without removing virtio_queue_host_notifier_read, I get the same mutex lockup 
> (as expected).
> 
> Maybe we have two independent issues here and this is some old bug in glibc or
> whatever?

Doesn't sound unlikely.

But the notifier_read removal makes sense. Fix this now and continue
searching for the root cause of the f20 breakage? We should try to
understand this even if it has been fixed in the meantime.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Christian Borntraeger]

On 03/17/2016 04:15 PM, Christian Borntraeger wrote:
> On 03/17/2016 04:02 PM, Paolo Bonzini wrote:
>>
>>
>> On 17/03/2016 13:39, Christian Borntraeger wrote:
>>> As an interesting side note, I updated my system from F20 to F23 some days 
>>> ago
>>> (after the initial report). While To Bo is still on a F20 system. I was not 
>>> able
>>> to reproduce the original crash on f23. but going back to F20 made this
>>> problem re-appear.
>>>  
>>>   Stack trace of thread 26429:
>>> #0  0x802008aa tracked_request_begin 
>>> (qemu-system-s390x)
>>> #1  0x80203f3c bdrv_co_do_preadv (qemu-system-s390x)
>>> #2  0x8020567c bdrv_co_do_readv (qemu-system-s390x)
>>> #3  0x8025d0f4 coroutine_trampoline 
>>> (qemu-system-s390x)
>>> #4  0x03ff943d150a __makecontext_ret (libc.so.6)
>>>
>>> this is with patch 2-4 plus the removal of virtio_queue_host_notifier_read.
>>>
>>> Without removing virtio_queue_host_notifier_read, I get the same mutex 
>>> lockup (as expected).
>>>
>>> Maybe we have two independent issues here and this is some old bug in glibc 
>>> or
>>> whatever?
>>
>> I'm happy to try and reproduce on x86 if you give me some instruction
>> (RHEL7 should be close enough to Fedora 20).
>>
>> Can you add an assert in virtio_blk_handle_output to catch reentrancy, like
> 
> that was quick (let me know if I should recompile with debugging)
> 
> (gdb) thread apply all bt
> 
> Thread 5 (Thread 0x3ff7b8ff910 (LWP 236419)):
> #0  0x03ff7cdfcf56 in syscall () from /lib64/libc.so.6
> #1  0x1022452e in futex_wait (val=, ev= out>) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:292
> #2  qemu_event_wait (ev=ev@entry=0x1082b5c4 ) at 
> /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:399
> #3  0x1023353a in call_rcu_thread (opaque=) at 
> /home/cborntra/REPOS/qemu/util/rcu.c:250
> #4  0x03ff7cf084c6 in start_thread () from /lib64/libpthread.so.0
> #5  0x03ff7ce02ec2 in thread_start () from /lib64/libc.so.6
> 
> Thread 4 (Thread 0x3ff78eca910 (LWP 236426)):
> #0  0x03ff7cdf819a in ioctl () from /lib64/libc.so.6
> #1  0x1005ddf8 in kvm_vcpu_ioctl (cpu=cpu@entry=0x10c27d40, 
> type=type@entry=44672) at /home/cborntra/REPOS/qemu/kvm-all.c:1984
> #2  0x1005df1c in kvm_cpu_exec (cpu=cpu@entry=0x10c27d40) at 
> /home/cborntra/REPOS/qemu/kvm-all.c:1834
> #3  0x1004b1be in qemu_kvm_cpu_thread_fn (arg=0x10c27d40) at 
> /home/cborntra/REPOS/qemu/cpus.c:1050
> #4  0x03ff7cf084c6 in start_thread () from /lib64/libpthread.so.0
> #5  0x03ff7ce02ec2 in thread_start () from /lib64/libc.so.6
> 
> Thread 3 (Thread 0x3ff7e8dcbb0 (LWP 236395)):
> #0  0x03ff7cdf66e6 in ppoll () from /lib64/libc.so.6
> #1  0x101a5e08 in ppoll (__ss=0x0, __timeout=0x3ffd6afe8a0, 
> __nfds=, __fds=) at /usr/include/bits/poll2.h:77
> #2  qemu_poll_ns (fds=, nfds=, 
> timeout=timeout@entry=103400) at 
> /home/cborntra/REPOS/qemu/qemu-timer.c:325
> #3  0x101a56f2 in os_host_main_loop_wait (timeout=103400) at 
> /home/cborntra/REPOS/qemu/main-loop.c:251
> #4  main_loop_wait (nonblocking=) at 
> /home/cborntra/REPOS/qemu/main-loop.c:505
> #5  0x100136d6 in main_loop () at /home/cborntra/REPOS/qemu/vl.c:1933
> #6  main (argc=, argv=, envp=) 
> at /home/cborntra/REPOS/qemu/vl.c:4656
> 
> Thread 2 (Thread 0x3ff7b0ff910 (LWP 236421)):
> #0  0x03ff7cdf66e6 in ppoll () from /lib64/libc.so.6
> #1  0x101a5e28 in ppoll (__ss=0x0, __timeout=0x0, __nfds= out>, __fds=) at /usr/include/bits/poll2.h:77
> #2  qemu_poll_ns (fds=, nfds=, 
> timeout=timeout@entry=-1) at /home/cborntra/REPOS/qemu/qemu-timer.c:313
> #3  0x101a727c in aio_poll (ctx=0x10880560, blocking=) 
> at /home/cborntra/REPOS/qemu/aio-posix.c:453
> #4  0x100d39f0 in iothread_run (opaque=0x10880020) at 
> /home/cborntra/REPOS/qemu/iothread.c:46
> #5  0x03ff7cf084c6 in start_thread () from /lib64/libpthread.so.0
> #6  0x03ff7ce02ec2 in thread_start () from /lib64/libc.so.6
> 
> Thread 1 (Thread 0x3ff57fff910 (LWP 236427)):
> #0  0x03ff7cd3b650 in raise () from /lib64/libc.so.6
> #1  0x03ff7cd3ced8 in abort () from /lib64/libc.so.6
> #2  0x03ff7cd33666 in __assert_fail_base () from /lib64/libc.so.6
> #3  0x03ff7cd336f4 in __assert_fail () from /lib64/libc.so.6
> #4  0x1007a3c4 in virtio_blk_handle_output (vdev=, 
> vq=) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:595
> #5  0x1009390e in virtio_queue_notify_vq (vq=0x10d77c70) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
> #6  0x10095894 in virtio_queue_notify_vq (vq=) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1091
> #7  virtio_queue_notify (vdev=, n=n@entry=0) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1101
> #8  0x100a17c8 in virtio_ccw_hcall_notify (args=) at 
> /home/cborntra/REPOS/qemu/hw/s390x/s390-virtio-ccw.c:66
> #9  

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Wed, 16 Mar 2016 13:49:10 +0100
Paolo Bonzini  wrote:

> On 16/03/2016 13:42, Cornelia Huck wrote:
> > On Wed, 16 Mar 2016 13:32:59 +0100
> > Paolo Bonzini  wrote:
> > 
> >> On 16/03/2016 13:22, Cornelia Huck wrote:
> > Yeah, it doesn't help that the functions are underdocumented (as in the
> > "assign" parameter above).
> >>> My understanding is:
> >>>
> >>> - 'assign': set up a new notifier (true) or disable it (false)
> >>> - 'set_handler': use our handler (true) or have it handled elsewhere
> >>>   (false)
> >>
> >> Right.  So if we're setting up a new notifier in
> >> virtio_queue_aio_set_host_notifier_handler, virtio_pci_stop_ioeventfd 
> >> should
> 
> ... not call virtio_queue_host_notifier_read.

This needs to be cascaded into
virtio_queue_set_host_notifier_fd_handler(), I guess.

> 
> > I don't think the ->set_host_notifiers() api really allows for this.
> 
>  I think it does, assign is the last argument to k->set_host_notifier().
> >>>
> >>> This depends on whether we want 'assign' to clean up any old notifiers
> >>> before setting up new ones. I think we want different behaviour for
> >>> dataplane and vhost.
> >>
> >> I think dataplane and vhost are the same.
> >>
> >> The question is whether ioeventfd=off,vhost=on or
> >> ioeventfd=off,dataplane=on are valid combinations; I think they aren't.
> > 
> > We should disallow that even temporary, then.
> > 
> > (This would imply that we need to drop the _stop_ioeventfd() call in
> > ->set_host_notifier(), correct?)
> 
> No, it would not.  ioeventfd=off,vhost=on would mean: "when vhost is
> off, use vCPU thread notification".

*confused*

Is ioeventfd=off supposed to mean "don't talk to the kernel, do
everything in qemu"?

> 
> When turning on vhost you'd still stop ioeventfd (i.e. stop processing
> the virtqueue in QEMU's main iothread), but you don't need to do
> anything to the event notifier.  vhost will pick it up and work on the
> virtqueue if necessary.  Likewise for dataplane.

So "disassociate the handler and switch over to the new one"?

> 
> >> If they aren't, it should be okay to remove the
> >> virtio_queue_host_notifier_read call in
> >> virtio_queue_set_host_notifier_fd_handler and
> >> virtio_queue_aio_set_host_notifier_handler.  That's because a handler
> >> for the notifier will always be set _somewhere_.  It could be the usual
> >> ioeventfd handler, the vhost handler or the dataplane handler, but one
> >> will be there.
> > 
> > It should; but we probably need to do a final read when we stop the
> > ioeventfd.
> 
> I was thinking of handing the final read directly to the next guy who
> polls the event notifier instead.  So, when called from vhost or
> dataplane, virtio_pci_stop_ioeventfd would use
> assign=true/set_handler=false ("a new notifier is going to be set up by
> the caller").

OK, then we'd need to pass a new parameter for this.

> 
> The host notifier API unfortunately is full of indirections.  I'm not
> sure how many of them are actually necessary.

Oh yes, it's very hard to follow, especially with not-very-well defined
parameters.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Wed, 16 Mar 2016 12:48:22 +0100
Paolo Bonzini  wrote:

> 
> 
> On 16/03/2016 12:32, Cornelia Huck wrote:
> > On Wed, 16 Mar 2016 12:09:02 +0100
> > Paolo Bonzini  wrote:
> > 
> >> On 16/03/2016 11:49, Christian Borntraeger wrote:
> > 
> >>> #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
> >>> /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
> >>> #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
> >>> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
> >>> #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
> >>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
> >>> #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) 
> >>> at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
> >>> #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
> >>> (vq=0xba305270, assign=false, set_handler=false) at 
> >>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
> >>> #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
> >>> (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
> >>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
> >>> #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
> >>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154
> >>
> >> One bug is here: virtio_ccw_stop_ioeventfd, in this case, should pass
> >> assign=true to virtio_ccw_set_guest2host_notifier.  (Assuming my
> >> understanding of assign=true is correct; I think it means "I'm going to
> >> set up another host notifier handler").
> > 
> > Hm... I copied these semantics from virtio-pci, and they still seem to
> > be the same. I wonder why we never saw this on virtio-pci?
> 
> Just because we never ran the right tests, probably.  The bug is indeed
> in virtio-pci as well (I didn't check mmio).

Somebody should probably do a writeup on the expected behaviour, as
this always ends in mental gymnastics (at least for me :)

> 
> >> In dataplane, instead, all calls to
> >> virtio_queue_set_host_notifier_fd_handler and
> >> virtio_queue_aio_set_host_notifier_handler should have assign=true.  The
> >> ioeventfd is just being moved from one aiocontext to another.
> > 
> > How can a transport know where they are called from?
> 
> That's a bug in dataplane.  The calls should be changed in
> hw/block/dataplane/virtio-blk.c

I don't think the ->set_host_notifiers() api really allows for this.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 11:49, Christian Borntraeger wrote:
> Seems to lockup.

That's an improvement actually. :)

> Thread 3 (Thread 0x3ff888dc910 (LWP 88958)):
> #0  0x03ff8ca90cd4 in __lll_lock_wait () at /lib64/libpthread.so.0
> #1  0x03ff8ca93e74 in __lll_lock_elision () at /lib64/libpthread.so.0

Off-topic, I love how s390 backtraces always have a little bit of
showing off in them! :)

> #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
> /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
> #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
> #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
> #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
> #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
> (vq=0xba305270, assign=false, set_handler=false) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
> #8  0x80109c50 in virtio_ccw_set_guest2host_notifier (dev=0xb9eed6a0, 
> n=0, assign=false, set_handler=false) at 
> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
> #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154

One bug is here: virtio_ccw_stop_ioeventfd, in this case, should pass
assign=true to virtio_ccw_set_guest2host_notifier.  (Assuming my
understanding of assign=true is correct; I think it means "I'm going to
set up another host notifier handler").

In dataplane, instead, all calls to
virtio_queue_set_host_notifier_fd_handler and
virtio_queue_aio_set_host_notifier_handler should have assign=true.  The
ioeventfd is just being moved from one aiocontext to another.

Paolo

> #10 0x8010d5aa in virtio_ccw_set_host_notifier (d=0xb9eed6a0, n=0, 
> assign=true) at /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:1211
> #11 0x800b722c in virtio_blk_data_plane_start (s=0xba232d80) at 
> /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:242
> #12 0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
> #13 0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
> #14 0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
> #15 0x802f1cd4 in aio_dispatch (ctx=0xb9e81c70) at 
> /home/cborntra/REPOS/qemu/aio-posix.c:327
> #16 0x802df31c in aio_ctx_dispatch (source=0xb9e81c70, callback=0x0, 
> user_data=0x0) at /home/cborntra/REPOS/qemu/async.c:232

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 14:04, Cornelia Huck wrote:
> > No, it would not.  ioeventfd=off,vhost=on would mean: "when vhost is
> > off, use vCPU thread notification".
> 
> *confused*
> 
> Is ioeventfd=off supposed to mean "don't talk to the kernel, do
> everything in qemu"?

For KVM, it means do everything in the QEMU vCPU thread (using userspace
vmexits).

>> > When turning on vhost you'd still stop ioeventfd (i.e. stop processing
>> > the virtqueue in QEMU's main iothread), but you don't need to do
>> > anything to the event notifier.  vhost will pick it up and work on the
>> > virtqueue if necessary.  Likewise for dataplane.
> 
> So "disassociate the handler and switch over to the new one"?

Yes, if we prohibit combinations which switch from vCPU thread
notification to vhost or dataplane (such as ioeventfd=off,vhost=on).  If
we always use an eventfd, we always have a handler to switch to.

> > > > If they aren't, it should be okay to remove the
> > > > virtio_queue_host_notifier_read call in
> > > > virtio_queue_set_host_notifier_fd_handler and
> > > > virtio_queue_aio_set_host_notifier_handler.  That's because a handler
> > > > for the notifier will always be set _somewhere_.  It could be the usual
> > > > ioeventfd handler, the vhost handler or the dataplane handler, but one
> > > > will be there.
> > > 
> > > It should; but we probably need to do a final read when we stop the
> > > ioeventfd.
> > 
> > I was thinking of handing the final read directly to the next guy who
> > polls the event notifier instead.  So, when called from vhost or
> > dataplane, virtio_pci_stop_ioeventfd would use
> > assign=true/set_handler=false ("a new notifier is going to be set up by
> > the caller").
> 
> OK, then we'd need to pass a new parameter for this.

Yes, agreed.

> > The host notifier API unfortunately is full of indirections.  I'm not
> > sure how many of them are actually necessary.
> 
> Oh yes, it's very hard to follow, especially with not-very-well defined
> parameters.

And full of duplicate code.  If copied code were moved to the virtio bus
level, it would be easier to change too.

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[tu bo]

On 03/17/2016 08:39 AM, Fam Zheng wrote:

On Wed, 03/16 14:45, Paolo Bonzini wrote:



On 16/03/2016 14:38, Christian Borntraeger wrote:

If you just remove the calls to virtio_queue_host_notifier_read, here
and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
(keeping patches 2-4 in)?


With these changes and patch 2-4 it does no longer locks up.
I keep it running some hour to check if a crash happens.

Tu Bo, your setup is currently better suited for reproducing. Can you also 
check?


Great, I'll prepare a patch to virtio then sketching the solution that
Conny agreed with.

While Fam and I agreed that patch 1 is not required, I'm not sure if the
mutex is necessary in the end.


If we can fix this from the virtio_queue_host_notifier_read side, the mutex/BH
are not necessary; but OTOH the mutex does catch such bugs, so maybe it's good
to have it. I'm not sure about the BH.

And on a hindsight I realize we don't want patches 2-3 too. Actually the
begin/end pair won't work as expected because of the blk_set_aio_context.

Let's hold on this series.



So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
and both with/without Fam's patches, it would be great.


Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the noise.


1. without the virtio_queue_host_notifier_read calls,  without patch 4

crash happens very often,

(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa165da37e in coroutine_trampoline (i0=, 
i1=1812051552) at util/coroutine-ucontext.c:79

#2  0x03ff7dd5150a in __makecontext_ret () from /lib64/libc.so.6


2. without the virtio_queue_host_notifier_read calls, with patch 4

crash happens very often,

(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa39dda43e in coroutine_trampoline (i0=, 
i1=-1677715600) at util/coroutine-ucontext.c:79

#2  0x03ffab6d150a in __makecontext_ret () from /lib64/libc.so.6



Thanks,
Fam

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

On Wed, 03/16 14:45, Paolo Bonzini wrote:
> 
> 
> On 16/03/2016 14:38, Christian Borntraeger wrote:
> > > If you just remove the calls to virtio_queue_host_notifier_read, here
> > > and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
> > > (keeping patches 2-4 in)?
> > 
> > With these changes and patch 2-4 it does no longer locks up. 
> > I keep it running some hour to check if a crash happens.
> > 
> > Tu Bo, your setup is currently better suited for reproducing. Can you also 
> > check?
> 
> Great, I'll prepare a patch to virtio then sketching the solution that
> Conny agreed with.
> 
> While Fam and I agreed that patch 1 is not required, I'm not sure if the
> mutex is necessary in the end.

If we can fix this from the virtio_queue_host_notifier_read side, the mutex/BH
are not necessary; but OTOH the mutex does catch such bugs, so maybe it's good
to have it. I'm not sure about the BH.

And on a hindsight I realize we don't want patches 2-3 too. Actually the
begin/end pair won't work as expected because of the blk_set_aio_context.

Let's hold on this series.

> 
> So if Tu Bo can check without the virtio_queue_host_notifier_read calls,
> and both with/without Fam's patches, it would be great.

Tu Bo, only with/withoug patch 4, if you want to check. Sorry for the noise.

Thanks,
Fam

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Christian Borntraeger]

On 03/17/2016 04:02 PM, Paolo Bonzini wrote:
> 
> 
> On 17/03/2016 13:39, Christian Borntraeger wrote:
>> As an interesting side note, I updated my system from F20 to F23 some days 
>> ago
>> (after the initial report). While To Bo is still on a F20 system. I was not 
>> able
>> to reproduce the original crash on f23. but going back to F20 made this
>> problem re-appear.
>>  
>>   Stack trace of thread 26429:
>> #0  0x802008aa tracked_request_begin 
>> (qemu-system-s390x)
>> #1  0x80203f3c bdrv_co_do_preadv (qemu-system-s390x)
>> #2  0x8020567c bdrv_co_do_readv (qemu-system-s390x)
>> #3  0x8025d0f4 coroutine_trampoline 
>> (qemu-system-s390x)
>> #4  0x03ff943d150a __makecontext_ret (libc.so.6)
>>
>> this is with patch 2-4 plus the removal of virtio_queue_host_notifier_read.
>>
>> Without removing virtio_queue_host_notifier_read, I get the same mutex 
>> lockup (as expected).
>>
>> Maybe we have two independent issues here and this is some old bug in glibc 
>> or
>> whatever?
> 
> I'm happy to try and reproduce on x86 if you give me some instruction
> (RHEL7 should be close enough to Fedora 20).
> 
> Can you add an assert in virtio_blk_handle_output to catch reentrancy, like

that was quick (let me know if I should recompile with debugging)

(gdb) thread apply all bt

Thread 5 (Thread 0x3ff7b8ff910 (LWP 236419)):
#0  0x03ff7cdfcf56 in syscall () from /lib64/libc.so.6
#1  0x1022452e in futex_wait (val=, ev=) 
at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:292
#2  qemu_event_wait (ev=ev@entry=0x1082b5c4 ) at 
/home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:399
#3  0x1023353a in call_rcu_thread (opaque=) at 
/home/cborntra/REPOS/qemu/util/rcu.c:250
#4  0x03ff7cf084c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff7ce02ec2 in thread_start () from /lib64/libc.so.6

Thread 4 (Thread 0x3ff78eca910 (LWP 236426)):
#0  0x03ff7cdf819a in ioctl () from /lib64/libc.so.6
#1  0x1005ddf8 in kvm_vcpu_ioctl (cpu=cpu@entry=0x10c27d40, 
type=type@entry=44672) at /home/cborntra/REPOS/qemu/kvm-all.c:1984
#2  0x1005df1c in kvm_cpu_exec (cpu=cpu@entry=0x10c27d40) at 
/home/cborntra/REPOS/qemu/kvm-all.c:1834
#3  0x1004b1be in qemu_kvm_cpu_thread_fn (arg=0x10c27d40) at 
/home/cborntra/REPOS/qemu/cpus.c:1050
#4  0x03ff7cf084c6 in start_thread () from /lib64/libpthread.so.0
#5  0x03ff7ce02ec2 in thread_start () from /lib64/libc.so.6

Thread 3 (Thread 0x3ff7e8dcbb0 (LWP 236395)):
#0  0x03ff7cdf66e6 in ppoll () from /lib64/libc.so.6
#1  0x101a5e08 in ppoll (__ss=0x0, __timeout=0x3ffd6afe8a0, 
__nfds=, __fds=) at /usr/include/bits/poll2.h:77
#2  qemu_poll_ns (fds=, nfds=, 
timeout=timeout@entry=103400) at /home/cborntra/REPOS/qemu/qemu-timer.c:325
#3  0x101a56f2 in os_host_main_loop_wait (timeout=103400) at 
/home/cborntra/REPOS/qemu/main-loop.c:251
#4  main_loop_wait (nonblocking=) at 
/home/cborntra/REPOS/qemu/main-loop.c:505
#5  0x100136d6 in main_loop () at /home/cborntra/REPOS/qemu/vl.c:1933
#6  main (argc=, argv=, envp=) at 
/home/cborntra/REPOS/qemu/vl.c:4656

Thread 2 (Thread 0x3ff7b0ff910 (LWP 236421)):
#0  0x03ff7cdf66e6 in ppoll () from /lib64/libc.so.6
#1  0x101a5e28 in ppoll (__ss=0x0, __timeout=0x0, __nfds=, __fds=) at /usr/include/bits/poll2.h:77
#2  qemu_poll_ns (fds=, nfds=, 
timeout=timeout@entry=-1) at /home/cborntra/REPOS/qemu/qemu-timer.c:313
#3  0x101a727c in aio_poll (ctx=0x10880560, blocking=) 
at /home/cborntra/REPOS/qemu/aio-posix.c:453
#4  0x100d39f0 in iothread_run (opaque=0x10880020) at 
/home/cborntra/REPOS/qemu/iothread.c:46
#5  0x03ff7cf084c6 in start_thread () from /lib64/libpthread.so.0
#6  0x03ff7ce02ec2 in thread_start () from /lib64/libc.so.6

Thread 1 (Thread 0x3ff57fff910 (LWP 236427)):
#0  0x03ff7cd3b650 in raise () from /lib64/libc.so.6
#1  0x03ff7cd3ced8 in abort () from /lib64/libc.so.6
#2  0x03ff7cd33666 in __assert_fail_base () from /lib64/libc.so.6
#3  0x03ff7cd336f4 in __assert_fail () from /lib64/libc.so.6
#4  0x1007a3c4 in virtio_blk_handle_output (vdev=, 
vq=) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:595
#5  0x1009390e in virtio_queue_notify_vq (vq=0x10d77c70) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
#6  0x10095894 in virtio_queue_notify_vq (vq=) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1091
#7  virtio_queue_notify (vdev=, n=n@entry=0) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1101
#8  0x100a17c8 in virtio_ccw_hcall_notify (args=) at 
/home/cborntra/REPOS/qemu/hw/s390x/s390-virtio-ccw.c:66
#9  0x1009c210 in s390_virtio_hypercall (env=env@entry=0x10c75aa0) at 
/home/cborntra/REPOS/qemu/hw/s390x/s390-virtio-hcall.c:35
#10 0x100cb4e8 in handle_hypercall (run=, 
cpu=0x10c6d7d0) at 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Christian Borntraeger]

On 03/17/2016 01:22 PM, tu bo wrote:
> 
> On 03/16/2016 09:38 PM, Christian Borntraeger wrote:
>> On 03/16/2016 01:55 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 16/03/2016 12:24, Christian Borntraeger wrote:
 On 03/16/2016 12:09 PM, Paolo Bonzini wrote:
> On 16/03/2016 11:49, Christian Borntraeger wrote:
>> #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
>> /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
>> #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
>> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
>> #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
>> #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) 
>> at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
>>>
>>> If you just remove the calls to virtio_queue_host_notifier_read, here
>>> and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
>>> (keeping patches 2-4 in)?
>>
>> With these changes and patch 2-4 it does no longer locks up.
>> I keep it running some hour to check if a crash happens.
>>
>> Tu Bo, your setup is currently better suited for reproducing. Can you also 
>> check?
> 
> remove the calls to virtio_queue_host_notifier_read, and keeping patches 2-4 
> in,
> 
> I got same crash as before,
> (gdb) bt
> #0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
> #1  0x02aa0c65d786 in coroutine_trampoline (i0=, 
> i1=-2013204784) at util/coroutine-ucontext.c:79
> #2  0x03ff99ad150a in __makecontext_ret () from /lib64/libc.so.6
> 

As an interesting side note, I updated my system from F20 to F23 some days ago
(after the initial report). While To Bo is still on a F20 system. I was not able
to reproduce the original crash on f23. but going back to F20 made this
problem re-appear.
 
  Stack trace of thread 26429:
#0  0x802008aa tracked_request_begin (qemu-system-s390x)
#1  0x80203f3c bdrv_co_do_preadv (qemu-system-s390x)
#2  0x8020567c bdrv_co_do_readv (qemu-system-s390x)
#3  0x8025d0f4 coroutine_trampoline (qemu-system-s390x)
#4  0x03ff943d150a __makecontext_ret (libc.so.6)

this is with patch 2-4 plus the removal of virtio_queue_host_notifier_read.

Without removing virtio_queue_host_notifier_read, I get the same mutex lockup 
(as expected).

Maybe we have two independent issues here and this is some old bug in glibc or
whatever?

> 
>>
>>>
>>> Paolo
>>>
>> #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
>> (vq=0xba305270, assign=false, set_handler=false) at 
>> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
>> #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
>> (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
>> #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
>> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154
>>>
>>

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Christian Borntraeger]

On 03/17/2016 04:02 PM, Paolo Bonzini wrote:
> 
> 
> On 17/03/2016 13:39, Christian Borntraeger wrote:
>> As an interesting side note, I updated my system from F20 to F23 some days 
>> ago
>> (after the initial report). While To Bo is still on a F20 system. I was not 
>> able
>> to reproduce the original crash on f23. but going back to F20 made this
>> problem re-appear.
>>  
>>   Stack trace of thread 26429:
>> #0  0x802008aa tracked_request_begin 
>> (qemu-system-s390x)
>> #1  0x80203f3c bdrv_co_do_preadv (qemu-system-s390x)
>> #2  0x8020567c bdrv_co_do_readv (qemu-system-s390x)
>> #3  0x8025d0f4 coroutine_trampoline 
>> (qemu-system-s390x)
>> #4  0x03ff943d150a __makecontext_ret (libc.so.6)
>>
>> this is with patch 2-4 plus the removal of virtio_queue_host_notifier_read.
>>
>> Without removing virtio_queue_host_notifier_read, I get the same mutex 
>> lockup (as expected).
>>
>> Maybe we have two independent issues here and this is some old bug in glibc 
>> or
>> whatever?
> 
> I'm happy to try and reproduce on x86 if you give me some instruction
> (RHEL7 should be close enough to Fedora 20).

Tu Bo has a standard guest that he starting multiple times. I can trigger some
issues by starting 20 guests that only have a kernel (with virtio-blk and bus
driver compiled in) and a busybox ramdisk that simply calls reboot. Sooner or
later a qemu crashes.
This guest has several virtio devices attached as well (partition detection will
do minimal reads)

ala 

  



































  





> 
> Can you add an assert in virtio_blk_handle_output to catch reentrancy, like

Will do.


> 
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index a7ec572..96ea896 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -591,6 +591,8 @@ static void virtio_blk_handle_output(VirtIODevice
> *vdev, VirtQueue *vq)
>  return;
>  }
> 
> +int x = atomic_fetch_inc(>test);
> +assert(x == 0);
>  blk_io_plug(s->blk);
> 
>  while ((req = virtio_blk_get_request(s))) {
> @@ -602,6 +604,7 @@ static void virtio_blk_handle_output(VirtIODevice
> *vdev, VirtQueue *vq)
>  }
> 
>  blk_io_unplug(s->blk);
> +atomic_dec(>test);
>  }
> 
>  static void virtio_blk_dma_restart_bh(void *opaque)
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index ae84d92..6472503 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -48,6 +48,7 @@ typedef struct VirtIOBlock {
>  BlockBackend *blk;
>  VirtQueue *vq;
>  void *rq;
> +int test;
>  QEMUBH *bh;
>  VirtIOBlkConf conf;
>  unsigned short sector_mask;
> 
> ?
> 
> Paolo
> 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 12:52, Cornelia Huck wrote:
> > Hm... I copied these semantics from virtio-pci, and they still seem to
> > be the same. I wonder why we never saw this on virtio-pci?
> > 
> > > In dataplane, instead, all calls to
> > > virtio_queue_set_host_notifier_fd_handler and
> > > virtio_queue_aio_set_host_notifier_handler should have assign=true.  The
> > > ioeventfd is just being moved from one aiocontext to another.
> > 
> > How can a transport know where they are called from?
> 
> Hm^2... I looked at virtio-scsi dataplane, and I noticed that it
> acquires the aio context prior to calling ->set_host_notifiers(). Does
> virtio-blk dataplane need to do this as well, or is virtio-scsi
> dataplane wrong/different?

I cannot really answer, but my plan was to solve this bug and then
ensure that virtio-scsi dataplane does exactly the same thing...  I
would ignore virtio-scsi dataplane for now.

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 14:14, Cornelia Huck wrote:
> > And full of duplicate code.  If copied code were moved to the virtio bus
> > level, it would be easier to change too.
> 
> Yes, pci, ccw and mmio basically all do the same things. The only
> difference is actually wiring up the eventfd.
> 
> I can attempt to do some refactoring.

That would be great, in the meanwhile I've asked Christian to try
removing the calls that cause the reentrancy.

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 13:22, Cornelia Huck wrote:
>> > Yeah, it doesn't help that the functions are underdocumented (as in the
>> > "assign" parameter above).
> My understanding is:
> 
> - 'assign': set up a new notifier (true) or disable it (false)
> - 'set_handler': use our handler (true) or have it handled elsewhere
>   (false)

Right.  So if we're setting up a new notifier in
virtio_queue_aio_set_host_notifier_handler, virtio_pci_stop_ioeventfd should

> > > I don't think the ->set_host_notifiers() api really allows for this.
> > 
> > I think it does, assign is the last argument to k->set_host_notifier().
> 
> This depends on whether we want 'assign' to clean up any old notifiers
> before setting up new ones. I think we want different behaviour for
> dataplane and vhost.

I think dataplane and vhost are the same.

The question is whether ioeventfd=off,vhost=on or
ioeventfd=off,dataplane=on are valid combinations; I think they aren't.

If they aren't, it should be okay to remove the
virtio_queue_host_notifier_read call in
virtio_queue_set_host_notifier_fd_handler and
virtio_queue_aio_set_host_notifier_handler.  That's because a handler
for the notifier will always be set _somewhere_.  It could be the usual
ioeventfd handler, the vhost handler or the dataplane handler, but one
will be there.

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Wed, 16 Mar 2016 12:59:37 +0100
Paolo Bonzini  wrote:

> On 16/03/2016 12:56, Cornelia Huck wrote:
> > On Wed, 16 Mar 2016 12:48:22 +0100
> > Paolo Bonzini  wrote:
> > 
> >>
> >>
> >> On 16/03/2016 12:32, Cornelia Huck wrote:
> >>> On Wed, 16 Mar 2016 12:09:02 +0100
> >>> Paolo Bonzini  wrote:
> >>>
>  On 16/03/2016 11:49, Christian Borntraeger wrote:
> >>>
> > #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
> > /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
> > #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
> > vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
> > #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
> > /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
> > #6  0x800f1c9c in virtio_queue_host_notifier_read 
> > (n=0xba3052c8) at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
> > #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
> > (vq=0xba305270, assign=false, set_handler=false) at 
> > /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
> > #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
> > (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
> > /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
> > #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
> > /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154
> 
>  One bug is here: virtio_ccw_stop_ioeventfd, in this case, should pass
>  assign=true to virtio_ccw_set_guest2host_notifier.  (Assuming my
>  understanding of assign=true is correct; I think it means "I'm going to
>  set up another host notifier handler").
> >>>
> >>> Hm... I copied these semantics from virtio-pci, and they still seem to
> >>> be the same. I wonder why we never saw this on virtio-pci?
> >>
> >> Just because we never ran the right tests, probably.  The bug is indeed
> >> in virtio-pci as well (I didn't check mmio).
> > 
> > Somebody should probably do a writeup on the expected behaviour, as
> > this always ends in mental gymnastics (at least for me :)
> 
> Yeah, it doesn't help that the functions are underdocumented (as in the
> "assign" parameter above).

My understanding is:

- 'assign': set up a new notifier (true) or disable it (false)
- 'set_handler': use our handler (true) or have it handled elsewhere
  (false)

> 
> >>
>  In dataplane, instead, all calls to
>  virtio_queue_set_host_notifier_fd_handler and
>  virtio_queue_aio_set_host_notifier_handler should have assign=true.  The
>  ioeventfd is just being moved from one aiocontext to another.
> >>>
> >>> How can a transport know where they are called from?
> >>
> >> That's a bug in dataplane.  The calls should be changed in
> >> hw/block/dataplane/virtio-blk.c
> > 
> > I don't think the ->set_host_notifiers() api really allows for this.
> 
> I think it does, assign is the last argument to k->set_host_notifier().

This depends on whether we want 'assign' to clean up any old notifiers
before setting up new ones. I think we want different behaviour for
dataplane and vhost.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Wed, 16 Mar 2016 14:10:29 +0100
Paolo Bonzini  wrote:

> On 16/03/2016 14:04, Cornelia Huck wrote:
> > > No, it would not.  ioeventfd=off,vhost=on would mean: "when vhost is
> > > off, use vCPU thread notification".
> > 
> > *confused*
> > 
> > Is ioeventfd=off supposed to mean "don't talk to the kernel, do
> > everything in qemu"?
> 
> For KVM, it means do everything in the QEMU vCPU thread (using userspace
> vmexits).

OK, we're on the same page then.

> 
> >> > When turning on vhost you'd still stop ioeventfd (i.e. stop processing
> >> > the virtqueue in QEMU's main iothread), but you don't need to do
> >> > anything to the event notifier.  vhost will pick it up and work on the
> >> > virtqueue if necessary.  Likewise for dataplane.
> > 
> > So "disassociate the handler and switch over to the new one"?
> 
> Yes, if we prohibit combinations which switch from vCPU thread
> notification to vhost or dataplane (such as ioeventfd=off,vhost=on).  If
> we always use an eventfd, we always have a handler to switch to.
> 
> > > > > If they aren't, it should be okay to remove the
> > > > > virtio_queue_host_notifier_read call in
> > > > > virtio_queue_set_host_notifier_fd_handler and
> > > > > virtio_queue_aio_set_host_notifier_handler.  That's because a handler
> > > > > for the notifier will always be set _somewhere_.  It could be the 
> > > > > usual
> > > > > ioeventfd handler, the vhost handler or the dataplane handler, but one
> > > > > will be there.
> > > > 
> > > > It should; but we probably need to do a final read when we stop the
> > > > ioeventfd.
> > > 
> > > I was thinking of handing the final read directly to the next guy who
> > > polls the event notifier instead.  So, when called from vhost or
> > > dataplane, virtio_pci_stop_ioeventfd would use
> > > assign=true/set_handler=false ("a new notifier is going to be set up by
> > > the caller").
> > 
> > OK, then we'd need to pass a new parameter for this.
> 
> Yes, agreed.
> 
> > > The host notifier API unfortunately is full of indirections.  I'm not
> > > sure how many of them are actually necessary.
> > 
> > Oh yes, it's very hard to follow, especially with not-very-well defined
> > parameters.
> 
> And full of duplicate code.  If copied code were moved to the virtio bus
> level, it would be easier to change too.

Yes, pci, ccw and mmio basically all do the same things. The only
difference is actually wiring up the eventfd.

I can attempt to do some refactoring.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Wed, 16 Mar 2016 12:09:02 +0100
Paolo Bonzini  wrote:

> On 16/03/2016 11:49, Christian Borntraeger wrote:

> > #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
> > /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
> > #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
> > vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
> > #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
> > /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
> > #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
> > /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
> > #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
> > (vq=0xba305270, assign=false, set_handler=false) at 
> > /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
> > #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
> > (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
> > /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
> > #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
> > /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154
> 
> One bug is here: virtio_ccw_stop_ioeventfd, in this case, should pass
> assign=true to virtio_ccw_set_guest2host_notifier.  (Assuming my
> understanding of assign=true is correct; I think it means "I'm going to
> set up another host notifier handler").

Hm... I copied these semantics from virtio-pci, and they still seem to
be the same. I wonder why we never saw this on virtio-pci?

> In dataplane, instead, all calls to
> virtio_queue_set_host_notifier_fd_handler and
> virtio_queue_aio_set_host_notifier_handler should have assign=true.  The
> ioeventfd is just being moved from one aiocontext to another.

How can a transport know where they are called from?

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Cornelia Huck]

On Wed, 16 Mar 2016 13:32:59 +0100
Paolo Bonzini  wrote:

> On 16/03/2016 13:22, Cornelia Huck wrote:
> >> > Yeah, it doesn't help that the functions are underdocumented (as in the
> >> > "assign" parameter above).
> > My understanding is:
> > 
> > - 'assign': set up a new notifier (true) or disable it (false)
> > - 'set_handler': use our handler (true) or have it handled elsewhere
> >   (false)
> 
> Right.  So if we're setting up a new notifier in
> virtio_queue_aio_set_host_notifier_handler, virtio_pci_stop_ioeventfd should

...do what? ;)

> 
> > > > I don't think the ->set_host_notifiers() api really allows for this.
> > > 
> > > I think it does, assign is the last argument to k->set_host_notifier().
> > 
> > This depends on whether we want 'assign' to clean up any old notifiers
> > before setting up new ones. I think we want different behaviour for
> > dataplane and vhost.
> 
> I think dataplane and vhost are the same.
> 
> The question is whether ioeventfd=off,vhost=on or
> ioeventfd=off,dataplane=on are valid combinations; I think they aren't.

We should disallow that even temporary, then.

(This would imply that we need to drop the _stop_ioeventfd() call in
->set_host_notifier(), correct?)

> If they aren't, it should be okay to remove the
> virtio_queue_host_notifier_read call in
> virtio_queue_set_host_notifier_fd_handler and
> virtio_queue_aio_set_host_notifier_handler.  That's because a handler
> for the notifier will always be set _somewhere_.  It could be the usual
> ioeventfd handler, the vhost handler or the dataplane handler, but one
> will be there.

It should; but we probably need to do a final read when we stop the
ioeventfd.

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 17/03/2016 17:08, Christian Borntraeger wrote:
> Good (or bad?) news is the assert also triggers on F23, it just seems
> to take longer.

I guess good news, because we can rule out the kernel (not that I
believed it was a kernel problem, but the thought is always there in
the background...).

The interaction between ioeventfd and dataplane is too complicated.  I
think if we get rid of the start/stop ioeventfd calls (just set up the
ioeventfd as soon as possible and then only set/clear the handlers)
things would be much simpler.

I'll see if I can produce something based on Conny's patches, which are
already a start.  Today I had a short day so I couldn't play with the
bug; out of curiosity, does the bug reproduce with her work + patch 4
from this series + the reentrancy assertion?

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 13:42, Cornelia Huck wrote:
> On Wed, 16 Mar 2016 13:32:59 +0100
> Paolo Bonzini  wrote:
> 
>> On 16/03/2016 13:22, Cornelia Huck wrote:
> Yeah, it doesn't help that the functions are underdocumented (as in the
> "assign" parameter above).
>>> My understanding is:
>>>
>>> - 'assign': set up a new notifier (true) or disable it (false)
>>> - 'set_handler': use our handler (true) or have it handled elsewhere
>>>   (false)
>>
>> Right.  So if we're setting up a new notifier in
>> virtio_queue_aio_set_host_notifier_handler, virtio_pci_stop_ioeventfd should

... not call virtio_queue_host_notifier_read.

> I don't think the ->set_host_notifiers() api really allows for this.

 I think it does, assign is the last argument to k->set_host_notifier().
>>>
>>> This depends on whether we want 'assign' to clean up any old notifiers
>>> before setting up new ones. I think we want different behaviour for
>>> dataplane and vhost.
>>
>> I think dataplane and vhost are the same.
>>
>> The question is whether ioeventfd=off,vhost=on or
>> ioeventfd=off,dataplane=on are valid combinations; I think they aren't.
> 
> We should disallow that even temporary, then.
> 
> (This would imply that we need to drop the _stop_ioeventfd() call in
> ->set_host_notifier(), correct?)

No, it would not.  ioeventfd=off,vhost=on would mean: "when vhost is
off, use vCPU thread notification".

When turning on vhost you'd still stop ioeventfd (i.e. stop processing
the virtqueue in QEMU's main iothread), but you don't need to do
anything to the event notifier.  vhost will pick it up and work on the
virtqueue if necessary.  Likewise for dataplane.

>> If they aren't, it should be okay to remove the
>> virtio_queue_host_notifier_read call in
>> virtio_queue_set_host_notifier_fd_handler and
>> virtio_queue_aio_set_host_notifier_handler.  That's because a handler
>> for the notifier will always be set _somewhere_.  It could be the usual
>> ioeventfd handler, the vhost handler or the dataplane handler, but one
>> will be there.
> 
> It should; but we probably need to do a final read when we stop the
> ioeventfd.

I was thinking of handing the final read directly to the next guy who
polls the event notifier instead.  So, when called from vhost or
dataplane, virtio_pci_stop_ioeventfd would use
assign=true/set_handler=false ("a new notifier is going to be set up by
the caller").

The host notifier API unfortunately is full of indirections.  I'm not
sure how many of them are actually necessary.

Paolo

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 12:56, Cornelia Huck wrote:
> On Wed, 16 Mar 2016 12:48:22 +0100
> Paolo Bonzini  wrote:
> 
>>
>>
>> On 16/03/2016 12:32, Cornelia Huck wrote:
>>> On Wed, 16 Mar 2016 12:09:02 +0100
>>> Paolo Bonzini  wrote:
>>>
 On 16/03/2016 11:49, Christian Borntraeger wrote:
>>>
> #3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
> /home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
> #4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
> vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
> #5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
> #6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) 
> at /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
> #7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
> (vq=0xba305270, assign=false, set_handler=false) at 
> /home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
> #8  0x80109c50 in virtio_ccw_set_guest2host_notifier 
> (dev=0xb9eed6a0, n=0, assign=false, set_handler=false) at 
> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
> #9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
> /home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154

 One bug is here: virtio_ccw_stop_ioeventfd, in this case, should pass
 assign=true to virtio_ccw_set_guest2host_notifier.  (Assuming my
 understanding of assign=true is correct; I think it means "I'm going to
 set up another host notifier handler").
>>>
>>> Hm... I copied these semantics from virtio-pci, and they still seem to
>>> be the same. I wonder why we never saw this on virtio-pci?
>>
>> Just because we never ran the right tests, probably.  The bug is indeed
>> in virtio-pci as well (I didn't check mmio).
> 
> Somebody should probably do a writeup on the expected behaviour, as
> this always ends in mental gymnastics (at least for me :)

Yeah, it doesn't help that the functions are underdocumented (as in the
"assign" parameter above).

>>
 In dataplane, instead, all calls to
 virtio_queue_set_host_notifier_fd_handler and
 virtio_queue_aio_set_host_notifier_handler should have assign=true.  The
 ioeventfd is just being moved from one aiocontext to another.
>>>
>>> How can a transport know where they are called from?
>>
>> That's a bug in dataplane.  The calls should be changed in
>> hw/block/dataplane/virtio-blk.c
> 
> I don't think the ->set_host_notifiers() api really allows for this.

I think it does, assign is the last argument to k->set_host_notifier().

Paolo

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[tu bo]

On 03/16/2016 09:38 PM, Christian Borntraeger wrote:

On 03/16/2016 01:55 PM, Paolo Bonzini wrote:



On 16/03/2016 12:24, Christian Borntraeger wrote:

On 03/16/2016 12:09 PM, Paolo Bonzini wrote:

On 16/03/2016 11:49, Christian Borntraeger wrote:

#3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
/home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
#4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
#5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
#6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785


If you just remove the calls to virtio_queue_host_notifier_read, here
and in virtio_queue_aio_set_host_notifier_fd_handler, does it work
(keeping patches 2-4 in)?


With these changes and patch 2-4 it does no longer locks up.
I keep it running some hour to check if a crash happens.

Tu Bo, your setup is currently better suited for reproducing. Can you also 
check?


remove the calls to virtio_queue_host_notifier_read, and keeping patches 
2-4 in,


I got same crash as before,
(gdb) bt
#0  bdrv_co_do_rw (opaque=0x0) at block/io.c:2172
#1  0x02aa0c65d786 in coroutine_trampoline (i0=, 
i1=-2013204784) at util/coroutine-ucontext.c:79

#2  0x03ff99ad150a in __makecontext_ret () from /lib64/libc.so.6






Paolo


#7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
(vq=0xba305270, assign=false, set_handler=false) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
#8  0x80109c50 in virtio_ccw_set_guest2host_notifier (dev=0xb9eed6a0, 
n=0, assign=false, set_handler=false) at 
/home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
#9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
/home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Christian Borntraeger]

On 03/16/2016 11:28 AM, Paolo Bonzini wrote:
> 
> 
> On 16/03/2016 11:10, Fam Zheng wrote:
>> These are some ideas originated from analyzing the Christian's crash report 
>> on
>> virtio-blk dataplane torture test:
>>
>> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg02093.html
>>
>> The ideas are mostly inspired/suggested by Paolo. This doesn't fix the bug, 
>> but
>> the first and the last patches seem to make the crash less frequent.  Also
>> thanks Cornelia Huck for reviewing the draft version posted in that thread.
> 
> I see you have fixed the mutex and started check in patch 4, so perhaps
> this fixes the bug. :)  Bo or Christian, could you try it out---and if
> it works try patches 2 to 4 only?
> 
> Thanks,
> 
> Paolo
> 
Seems to lockup.

Thread 5 (Thread 0x3ff8b2ff910 (LWP 88956)):
#0  0x03ff8c97f13e in syscall () at /lib64/libc.so.6
#1  0x803d52fe in futex_wait (ev=0x80a4a104 , 
val=4294967295) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:292
#2  0x803d558e in qemu_event_wait (ev=0x80a4a104 
) at 
/home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:399
#3  0x803f2c34 in call_rcu_thread (opaque=0x0) at 
/home/cborntra/REPOS/qemu/util/rcu.c:250
#4  0x03ff8ca87c2c in start_thread () at /lib64/libpthread.so.0
#5  0x03ff8c984c7a in thread_start () at /lib64/libc.so.6

Thread 4 (Thread 0x3ff8aaff910 (LWP 88957)):
#0  0x03ff8c9784d8 in ppoll () at /lib64/libc.so.6
#1  0x802efdca in qemu_poll_ns (fds=0x3ff84002240, nfds=2, timeout=-1) 
at /home/cborntra/REPOS/qemu/qemu-timer.c:313
#2  0x802f2528 in aio_poll (ctx=0xb9e94050, blocking=true) at 
/home/cborntra/REPOS/qemu/aio-posix.c:453
#3  0x8016392a in iothread_run (opaque=0xb9e93b10) at 
/home/cborntra/REPOS/qemu/iothread.c:46
#4  0x03ff8ca87c2c in start_thread () at /lib64/libpthread.so.0
#5  0x03ff8c984c7a in thread_start () at /lib64/libc.so.6

Thread 3 (Thread 0x3ff888dc910 (LWP 88958)):
#0  0x03ff8ca90cd4 in __lll_lock_wait () at /lib64/libpthread.so.0
#1  0x03ff8ca93e74 in __lll_lock_elision () at /lib64/libpthread.so.0
#2  0x803d49ce in qemu_mutex_lock (mutex=0x8061f260 
) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:64
#3  0x80060ef4 in qemu_mutex_lock_iothread () at 
/home/cborntra/REPOS/qemu/cpus.c:1226
#4  0x80156af6 in kvm_arch_handle_exit (cs=0xba23b7f0, 
run=0x3ff8a20) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:2024
#5  0x800815de in kvm_cpu_exec (cpu=0xba23b7f0) at 
/home/cborntra/REPOS/qemu/kvm-all.c:1921
#6  0x8006074c in qemu_kvm_cpu_thread_fn (arg=0xba23b7f0) at 
/home/cborntra/REPOS/qemu/cpus.c:1050
#7  0x03ff8ca87c2c in start_thread () at /lib64/libpthread.so.0
#8  0x03ff8c984c7a in thread_start () at /lib64/libc.so.6

Thread 2 (Thread 0x3ff67fff910 (LWP 88959)):
#0  0x03ff8ca90d04 in __lll_lock_wait () at /lib64/libpthread.so.0
#1  0x03ff8ca93e74 in __lll_lock_elision () at /lib64/libpthread.so.0
#2  0x803d49ce in qemu_mutex_lock (mutex=0x8061f260 
) at /home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:64
#3  0x80060ef4 in qemu_mutex_lock_iothread () at 
/home/cborntra/REPOS/qemu/cpus.c:1226
#4  0x80156af6 in kvm_arch_handle_exit (cs=0xb9f2e970, 
run=0x3ff8808) at /home/cborntra/REPOS/qemu/target-s390x/kvm.c:2024
#5  0x800815de in kvm_cpu_exec (cpu=0xb9f2e970) at 
/home/cborntra/REPOS/qemu/kvm-all.c:1921
#6  0x8006074c in qemu_kvm_cpu_thread_fn (arg=0xb9f2e970) at 
/home/cborntra/REPOS/qemu/cpus.c:1050
#7  0x03ff8ca87c2c in start_thread () at /lib64/libpthread.so.0
#8  0x03ff8c984c7a in thread_start () at /lib64/libc.so.6

Thread 1 (Thread 0x3ff8e55bb90 (LWP 88953)):
#0  0x03ff8ca90cd4 in __lll_lock_wait () at /lib64/libpthread.so.0
#1  0x03ff8ca93e74 in __lll_lock_elision () at /lib64/libpthread.so.0
#2  0x803d49ce in qemu_mutex_lock (mutex=0xba232df8) at 
/home/cborntra/REPOS/qemu/util/qemu-thread-posix.c:64
#3  0x800b713e in virtio_blk_data_plane_start (s=0xba232d80) at 
/home/cborntra/REPOS/qemu/hw/block/dataplane/virtio-blk.c:224
#4  0x800b4ea0 in virtio_blk_handle_output (vdev=0xb9eee7e8, 
vq=0xba305270) at /home/cborntra/REPOS/qemu/hw/block/virtio-blk.c:590
#5  0x800ef3dc in virtio_queue_notify_vq (vq=0xba305270) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1095
#6  0x800f1c9c in virtio_queue_host_notifier_read (n=0xba3052c8) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1785
#7  0x800f1e14 in virtio_queue_set_host_notifier_fd_handler 
(vq=0xba305270, assign=false, set_handler=false) at 
/home/cborntra/REPOS/qemu/hw/virtio/virtio.c:1817
#8  0x80109c50 in virtio_ccw_set_guest2host_notifier (dev=0xb9eed6a0, 
n=0, assign=false, set_handler=false) at 
/home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:97
#9  0x80109ef2 in virtio_ccw_stop_ioeventfd (dev=0xb9eed6a0) at 
/home/cborntra/REPOS/qemu/hw/s390x/virtio-ccw.c:154
#10 

Re: [Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Paolo Bonzini]

On 16/03/2016 11:10, Fam Zheng wrote:
> These are some ideas originated from analyzing the Christian's crash report on
> virtio-blk dataplane torture test:
> 
> https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg02093.html
> 
> The ideas are mostly inspired/suggested by Paolo. This doesn't fix the bug, 
> but
> the first and the last patches seem to make the crash less frequent.  Also
> thanks Cornelia Huck for reviewing the draft version posted in that thread.

I see you have fixed the mutex and started check in patch 4, so perhaps
this fixes the bug. :)  Bo or Christian, could you try it out---and if
it works try patches 2 to 4 only?

Thanks,

Paolo

[Qemu-devel] [PATCH 0/4] Tweaks around virtio-blk start/stop

[Fam Zheng]

These are some ideas originated from analyzing the Christian's crash report on
virtio-blk dataplane torture test:

https://lists.gnu.org/archive/html/qemu-devel/2016-03/msg02093.html

The ideas are mostly inspired/suggested by Paolo. This doesn't fix the bug, but
the first and the last patches seem to make the crash less frequent.  Also
thanks Cornelia Huck for reviewing the draft version posted in that thread.


Fam Zheng (4):
  block: Use drained section in bdrv_set_aio_context
  block-backend: Introduce blk_drained_begin/end
  virtio-blk: Use blk_drained_begin/end around dataplane stop
  virtio-blk: Clean up start/stop with mutex and BH

 block.c |  4 ++-
 block/block-backend.c   | 14 ++
 hw/block/dataplane/virtio-blk.c | 58 +++--
 hw/block/virtio-blk.c   |  3 ++-
 include/sysemu/block-backend.h  |  2 ++
 5 files changed, 65 insertions(+), 16 deletions(-)

-- 
2.4.3