[Qemu-devel] [PATCH] virtio: Register host notifier handler as external

[Qemu-devel] [PATCH] virtio: Register host notifier handler as external

[Fam Zheng]

This ensures the bdrv_drained_begin() in block layer is effective and
fixes launchpad bug #1570134.

Signed-off-by: Fam Zheng <famz@redhat.com>
---
 hw/virtio/virtio.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
index f745c4a..002c2c6 100644
--- a/hw/virtio/virtio.c
+++ b/hw/virtio/virtio.c
@@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
                                                bool set_handler)
 {
     if (assign && set_handler) {
-        event_notifier_set_handler(&vq->host_notifier,
-                                   virtio_queue_host_notifier_read);
+        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
+                               true, virtio_queue_host_notifier_read);
     } else {
-        event_notifier_set_handler(&vq->host_notifier, NULL);
+        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
+                               true, NULL);
     }
     if (!assign) {
         /* Test and clear notifier before after disabling event,
-- 
2.8.0

Re: [Qemu-devel] [PATCH for-2.6] virtio: Register host notifier handler as external

[Fam Zheng]

On Thu, 04/21 10:04, Fam Zheng wrote:
> This ensures the bdrv_drained_begin() in block layer is effective and
> fixes launchpad bug #1570134.

Forgot to add to the subject, but this patch is for 2.6.

Fam

> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/virtio/virtio.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f745c4a..002c2c6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                 bool set_handler)
>  {
>      if (assign && set_handler) {
> -        event_notifier_set_handler(&vq->host_notifier,
> -                                   virtio_queue_host_notifier_read);
> +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> +                               true, virtio_queue_host_notifier_read);
>      } else {
> -        event_notifier_set_handler(&vq->host_notifier, NULL);
> +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> +                               true, NULL);
>      }
>      if (!assign) {
>          /* Test and clear notifier before after disabling event,
> -- 
> 2.8.0
> 
> 

Re: [Qemu-devel] [PATCH for-2.6] virtio: Register host notifier handler as external

[Kevin Wolf]

Am 21.04.2016 um 07:05 hat Fam Zheng geschrieben:
> On Thu, 04/21 10:04, Fam Zheng wrote:
> > This ensures the bdrv_drained_begin() in block layer is effective and
> > fixes launchpad bug #1570134.
> 
> Forgot to add to the subject, but this patch is for 2.6.

Also CCing qemu-stable (this affects 2.5) and qemu-block.

> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/virtio/virtio.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index f745c4a..002c2c6 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> >                                                 bool set_handler)
> >  {
> >      if (assign && set_handler) {
> > -        event_notifier_set_handler(&vq->host_notifier,
> > -                                   virtio_queue_host_notifier_read);
> > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > +                               true, virtio_queue_host_notifier_read);
> >      } else {
> > -        event_notifier_set_handler(&vq->host_notifier, NULL);
> > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > +                               true, NULL);
> >      }

This feels like fixing one special case of a more general problem. Are
we sure that event_notifier_set_handler() should ever result in an
internal handler? The default should probably be external, and block
drivers need to take care to use functions that have the is_external
parameter so they can mark themselves internal.

Kevin

Re: [Qemu-devel] [PATCH for-2.6] virtio: Register host notifier handler as external

[Fam Zheng]

On Thu, 04/21 10:07, Kevin Wolf wrote:
> Am 21.04.2016 um 07:05 hat Fam Zheng geschrieben:
> > On Thu, 04/21 10:04, Fam Zheng wrote:
> > > This ensures the bdrv_drained_begin() in block layer is effective and
> > > fixes launchpad bug #1570134.
> > 
> > Forgot to add to the subject, but this patch is for 2.6.
> 
> Also CCing qemu-stable (this affects 2.5) and qemu-block.
> 
> > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > ---
> > >  hw/virtio/virtio.c | 7 ++++---
> > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > index f745c4a..002c2c6 100644
> > > --- a/hw/virtio/virtio.c
> > > +++ b/hw/virtio/virtio.c
> > > @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> > >                                                 bool set_handler)
> > >  {
> > >      if (assign && set_handler) {
> > > -        event_notifier_set_handler(&vq->host_notifier,
> > > -                                   virtio_queue_host_notifier_read);
> > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > +                               true, virtio_queue_host_notifier_read);
> > >      } else {
> > > -        event_notifier_set_handler(&vq->host_notifier, NULL);
> > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > +                               true, NULL);
> > >      }
> 
> This feels like fixing one special case of a more general problem. Are
> we sure that event_notifier_set_handler() should ever result in an
> internal handler? The default should probably be external, and block
> drivers need to take care to use functions that have the is_external
> parameter so they can mark themselves internal.
> 

Looks like this is the only event_notifier_set_handler caller that
should be fixed.  If we see problem in other devices they can be
addressed case by case, I'm not sure marking event_notifier_set_handler
as external is a better idea for 2.6. (The other two callers are
hyperv.c, which seems never set the handler, and ccid-card-emulated.c,
which device I have no idea about.)

Fam

Re: [Qemu-devel] [PATCH for-2.6] virtio: Register host notifier handler as external

[Kevin Wolf]

Am 21.04.2016 um 10:57 hat Fam Zheng geschrieben:
> On Thu, 04/21 10:07, Kevin Wolf wrote:
> > Am 21.04.2016 um 07:05 hat Fam Zheng geschrieben:
> > > On Thu, 04/21 10:04, Fam Zheng wrote:
> > > > This ensures the bdrv_drained_begin() in block layer is effective and
> > > > fixes launchpad bug #1570134.
> > > 
> > > Forgot to add to the subject, but this patch is for 2.6.
> > 
> > Also CCing qemu-stable (this affects 2.5) and qemu-block.
> > 
> > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > > ---
> > > >  hw/virtio/virtio.c | 7 ++++---
> > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > index f745c4a..002c2c6 100644
> > > > --- a/hw/virtio/virtio.c
> > > > +++ b/hw/virtio/virtio.c
> > > > @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> > > >                                                 bool set_handler)
> > > >  {
> > > >      if (assign && set_handler) {
> > > > -        event_notifier_set_handler(&vq->host_notifier,
> > > > -                                   virtio_queue_host_notifier_read);
> > > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > > +                               true, virtio_queue_host_notifier_read);
> > > >      } else {
> > > > -        event_notifier_set_handler(&vq->host_notifier, NULL);
> > > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > > +                               true, NULL);
> > > >      }
> > 
> > This feels like fixing one special case of a more general problem. Are
> > we sure that event_notifier_set_handler() should ever result in an
> > internal handler? The default should probably be external, and block
> > drivers need to take care to use functions that have the is_external
> > parameter so they can mark themselves internal.
> > 
> 
> Looks like this is the only event_notifier_set_handler caller that
> should be fixed.  If we see problem in other devices they can be
> addressed case by case, I'm not sure marking event_notifier_set_handler
> as external is a better idea for 2.6. (The other two callers are
> hyperv.c, which seems never set the handler, and ccid-card-emulated.c,
> which device I have no idea about.)

As we don't have a lot of time for 2.6 any more, this local fix is
probably the right thing to do. It fixes what we know is broken and
has a very low risk of breaking other things.

But we should probably review all fd handlers during the 2.7 development
cycle. I didn't see anything obvious that could possibly call into the
block layer, but even unrelated and therefore harmless users should
probably be marked as external to avoid unnecessary traps for future
development.

Kevin

Re: [Qemu-devel] [PATCH for-2.6] virtio: Register host notifier handler as external

[Fam Zheng]

On Thu, 04/21 13:42, Kevin Wolf wrote:
> Am 21.04.2016 um 10:57 hat Fam Zheng geschrieben:
> > On Thu, 04/21 10:07, Kevin Wolf wrote:
> > > Am 21.04.2016 um 07:05 hat Fam Zheng geschrieben:
> > > > On Thu, 04/21 10:04, Fam Zheng wrote:
> > > > > This ensures the bdrv_drained_begin() in block layer is effective and
> > > > > fixes launchpad bug #1570134.
> > > > 
> > > > Forgot to add to the subject, but this patch is for 2.6.
> > > 
> > > Also CCing qemu-stable (this affects 2.5) and qemu-block.
> > > 
> > > > > Signed-off-by: Fam Zheng <famz@redhat.com>
> > > > > ---
> > > > >  hw/virtio/virtio.c | 7 ++++---
> > > > >  1 file changed, 4 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > > > > index f745c4a..002c2c6 100644
> > > > > --- a/hw/virtio/virtio.c
> > > > > +++ b/hw/virtio/virtio.c
> > > > > @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> > > > >                                                 bool set_handler)
> > > > >  {
> > > > >      if (assign && set_handler) {
> > > > > -        event_notifier_set_handler(&vq->host_notifier,
> > > > > -                                   virtio_queue_host_notifier_read);
> > > > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > > > +                               true, virtio_queue_host_notifier_read);
> > > > >      } else {
> > > > > -        event_notifier_set_handler(&vq->host_notifier, NULL);
> > > > > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > > > > +                               true, NULL);
> > > > >      }
> > > 
> > > This feels like fixing one special case of a more general problem. Are
> > > we sure that event_notifier_set_handler() should ever result in an
> > > internal handler? The default should probably be external, and block
> > > drivers need to take care to use functions that have the is_external
> > > parameter so they can mark themselves internal.
> > > 
> > 
> > Looks like this is the only event_notifier_set_handler caller that
> > should be fixed.  If we see problem in other devices they can be
> > addressed case by case, I'm not sure marking event_notifier_set_handler
> > as external is a better idea for 2.6. (The other two callers are
> > hyperv.c, which seems never set the handler, and ccid-card-emulated.c,
> > which device I have no idea about.)
> 
> As we don't have a lot of time for 2.6 any more, this local fix is
> probably the right thing to do. It fixes what we know is broken and
> has a very low risk of breaking other things.
> 
> But we should probably review all fd handlers during the 2.7 development
> cycle. I didn't see anything obvious that could possibly call into the
> block layer, but even unrelated and therefore harmless users should
> probably be marked as external to avoid unnecessary traps for future
> development.

Completely agreed. I'm adding this to my todo list for 2.7.

Fam

Re: [Qemu-devel] [PATCH] virtio: Register host notifier handler as external

[Michael S. Tsirkin]

On Thu, Apr 21, 2016 at 10:04:28AM +0800, Fam Zheng wrote:
> This ensures the bdrv_drained_begin() in block layer is effective and
> fixes launchpad bug #1570134.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/virtio/virtio.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f745c4a..002c2c6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                 bool set_handler)
>  {
>      if (assign && set_handler) {
> -        event_notifier_set_handler(&vq->host_notifier,
> -                                   virtio_queue_host_notifier_read);
> +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> +                               true, virtio_queue_host_notifier_read);
>      } else {
> -        event_notifier_set_handler(&vq->host_notifier, NULL);
> +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> +                               true, NULL);
>      }
>      if (!assign) {
>          /* Test and clear notifier before after disabling event,

Is this the right thing to do in all cases?
E.g. it doesn't look like event_notifier_set_handler previously called
aio_set_event_notifier on win32. Was there a reason for this?


> -- 
> 2.8.0

Re: [Qemu-devel] [PATCH] virtio: Register host notifier handler as external

[Fam Zheng]

On Thu, 04/21 15:20, Michael S. Tsirkin wrote:
> On Thu, Apr 21, 2016 at 10:04:28AM +0800, Fam Zheng wrote:
> > This ensures the bdrv_drained_begin() in block layer is effective and
> > fixes launchpad bug #1570134.
> > 
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > ---
> >  hw/virtio/virtio.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> > index f745c4a..002c2c6 100644
> > --- a/hw/virtio/virtio.c
> > +++ b/hw/virtio/virtio.c
> > @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
> >                                                 bool set_handler)
> >  {
> >      if (assign && set_handler) {
> > -        event_notifier_set_handler(&vq->host_notifier,
> > -                                   virtio_queue_host_notifier_read);
> > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > +                               true, virtio_queue_host_notifier_read);
> >      } else {
> > -        event_notifier_set_handler(&vq->host_notifier, NULL);
> > +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> > +                               true, NULL);
> >      }
> >      if (!assign) {
> >          /* Test and clear notifier before after disabling event,
> 
> Is this the right thing to do in all cases?
> E.g. it doesn't look like event_notifier_set_handler previously called
> aio_set_event_notifier on win32. Was there a reason for this?

It's a good catch. I'm not sure whether/how win32 goes with ioeventfd, but it
seems all the existing host notifier handling (fd handler) in vhost and data
plane mismatch with the event notifier implementation (WaitObject) in win32.

This is a subject understood best by Paolo but since he's on PTO I'll see if I
can figure out the answer by testing this code with wine.

Fam

Re: [Qemu-devel] [PATCH] virtio: Register host notifier handler as external

[Paolo Bonzini]

On 21/04/2016 16:28, Fam Zheng wrote:
>> > Is this the right thing to do in all cases?
>> > E.g. it doesn't look like event_notifier_set_handler previously called
>> > aio_set_event_notifier on win32. Was there a reason for this?
> It's a good catch. I'm not sure whether/how win32 goes with ioeventfd

Fam's patch is okay.  While POSIX has only file descriptors, Windows has
socket descriptors and handles.  EventNotifier is a handle, so it must
use aio_set_event_notifier or qemu_add_wait_object.

Fam changed qemu_add_wait_object to the corresponding AioContext-based
function.

qemu_add_wait_object and qemu_del_wait_object probably should go away,
replaced by a new function aio_set_wait_handler.  Windows's
aio_set_event_notifier then can simply become

    aio_set_wait_handler(ctx, event_notifier_get_handle(e), is_external,
                         io_notify, e);

similar to how POSIX implements it with

    aio_set_fd_handler(ctx, event_notifier_get_fd(e),
                       is_external, (IOHandler *)io_read, NULL, e);

Thanks,

Paolo

> , but it
> seems all the existing host notifier handling (fd handler) in vhost and data
> plane mismatch with the event notifier implementation (WaitObject) in win32.
> This is a subject understood best by Paolo but since he's on PTO I'll see if I
> can figure out the answer by testing this code with wine.

Re: [Qemu-devel] [PATCH] virtio: Register host notifier handler as external

[Fam Zheng]

On Thu, 04/21 10:04, Fam Zheng wrote:
> This ensures the bdrv_drained_begin() in block layer is effective and
> fixes launchpad bug #1570134.
> 
> Signed-off-by: Fam Zheng <famz@redhat.com>
> ---
>  hw/virtio/virtio.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c
> index f745c4a..002c2c6 100644
> --- a/hw/virtio/virtio.c
> +++ b/hw/virtio/virtio.c
> @@ -1829,10 +1829,11 @@ void virtio_queue_set_host_notifier_fd_handler(VirtQueue *vq, bool assign,
>                                                 bool set_handler)
>  {
>      if (assign && set_handler) {
> -        event_notifier_set_handler(&vq->host_notifier,
> -                                   virtio_queue_host_notifier_read);
> +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> +                               true, virtio_queue_host_notifier_read);
>      } else {
> -        event_notifier_set_handler(&vq->host_notifier, NULL);
> +        aio_set_event_notifier(qemu_get_aio_context(), &vq->host_notifier,
> +                               true, NULL);
>      }
>      if (!assign) {
>          /* Test and clear notifier before after disabling event,

This isn't the right fix, at least not in the intended way. The crash is gone
because in this patch the handler is moved from iohandler's AioContext to the
main loop AioContext, on which the mirror BH is scheduled, and consequently the
mirror BH handler always runs first than the VQ handler.

For 2.6 I think what we could do is reverting some changes in 5a7e7a0bad1 for
non-dataplane case, i.e. call mirror_exit directly in mirror_run.

It's too late today and I'll look into this tomorrow. The fix will probably be
left for -rc4.

Fam