[2/3] xen-bus: allow AioContext to be specified for each event channel
diff mbox series

Message ID 20190408151617.13025-3-paul.durrant@citrix.com
State New
Headers show
Series
  • Support IOThread polling for PV shared rings
Related show

Commit Message

Paul Durrant April 8, 2019, 3:16 p.m. UTC
This patch adds an AioContext parameter to xen_device_bind_event_channel()
and then uses aio_set_fd_handler() to set the callback rather than
qemu_set_fd_handler().

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Stefan Hajnoczi <stefanha@redhat.com>
Cc: Kevin Wolf <kwolf@redhat.com>
Cc: Max Reitz <mreitz@redhat.com>
---
 hw/block/dataplane/xen-block.c |  2 +-
 hw/xen/xen-bus.c               | 10 +++++++---
 include/hw/xen/xen-bus.h       |  1 +
 3 files changed, 9 insertions(+), 4 deletions(-)

Comments

Anthony PERARD April 10, 2019, 12:57 p.m. UTC | #1
On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote:
> This patch adds an AioContext parameter to xen_device_bind_event_channel()
> and then uses aio_set_fd_handler() to set the callback rather than
> qemu_set_fd_handler().
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> ---
> @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque)
>  }
>  
>  XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> +                                               AioContext *ctx,
>                                                 unsigned int port,
>                                                 XenEventHandler handler,
>                                                 void *opaque, Error **errp)
> @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
>      channel->handler = handler;
>      channel->opaque = opaque;
>  
> -    qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
> -                        channel);
> +    channel->ctx = ctx;
> +    aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
> +                       xen_device_event, NULL, NULL, channel);

I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
`true' here, instead. That flag seems to be used when making a snapshot
of a blockdev, for example.

That was introduced by:
dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586

What do you think?
Paul Durrant April 10, 2019, 3:20 p.m. UTC | #2
> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 10 April 2019 13:57
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max
> Reitz <mreitz@redhat.com>
> Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
> 
> On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote:
> > This patch adds an AioContext parameter to xen_device_bind_event_channel()
> > and then uses aio_set_fd_handler() to set the callback rather than
> > qemu_set_fd_handler().
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > ---
> > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque)
> >  }
> >
> >  XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > +                                               AioContext *ctx,
> >                                                 unsigned int port,
> >                                                 XenEventHandler handler,
> >                                                 void *opaque, Error **errp)
> > @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> >      channel->handler = handler;
> >      channel->opaque = opaque;
> >
> > -    qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
> > -                        channel);
> > +    channel->ctx = ctx;
> > +    aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
> > +                       xen_device_event, NULL, NULL, channel);
> 
> I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
> `true' here, instead. That flag seems to be used when making a snapshot
> of a blockdev, for example.
> 
> That was introduced by:
> dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586
> 
> What do you think?

Interesting. I admit I was merely transcribing what qemu_set_fd_handler() passes without really looking into the values. Looking at the arguments that virtio-blk passes to aio_set_event_notifier() though, and what 'is_external' means, it would appear that setting it to true is probably the right thing to do. Do you want me to send a v2 of the series or can you fix it up?

  Cheers,

    Paul

> 
> 
> --
> Anthony PERARD
Anthony PERARD April 10, 2019, 3:22 p.m. UTC | #3
On Wed, Apr 10, 2019 at 04:20:05PM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > Sent: 10 April 2019 13:57
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini
> > <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max
> > Reitz <mreitz@redhat.com>
> > Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
> > 
> > On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote:
> > > This patch adds an AioContext parameter to xen_device_bind_event_channel()
> > > and then uses aio_set_fd_handler() to set the callback rather than
> > > qemu_set_fd_handler().
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > ---
> > > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque)
> > >  }
> > >
> > >  XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > > +                                               AioContext *ctx,
> > >                                                 unsigned int port,
> > >                                                 XenEventHandler handler,
> > >                                                 void *opaque, Error **errp)
> > > @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > >      channel->handler = handler;
> > >      channel->opaque = opaque;
> > >
> > > -    qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
> > > -                        channel);
> > > +    channel->ctx = ctx;
> > > +    aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
> > > +                       xen_device_event, NULL, NULL, channel);
> > 
> > I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
> > `true' here, instead. That flag seems to be used when making a snapshot
> > of a blockdev, for example.
> > 
> > That was introduced by:
> > dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586
> > 
> > What do you think?
> 
> Interesting. I admit I was merely transcribing what qemu_set_fd_handler() passes without really looking into the values. Looking at the arguments that virtio-blk passes to aio_set_event_notifier() though, and what 'is_external' means, it would appear that setting it to true is probably the right thing to do. Do you want me to send a v2 of the series or can you fix it up?

I'll fix that up.

Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>

Thanks,
Paul Durrant April 10, 2019, 3:56 p.m. UTC | #4
> -----Original Message-----
> From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> Sent: 10 April 2019 16:23
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini
> <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max
> Reitz <mreitz@redhat.com>
> Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
> 
> On Wed, Apr 10, 2019 at 04:20:05PM +0100, Paul Durrant wrote:
> > > -----Original Message-----
> > > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > > Sent: 10 April 2019 13:57
> > > To: Paul Durrant <Paul.Durrant@citrix.com>
> > > Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Stefano
> Stabellini
> > > <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>;
> Max
> > > Reitz <mreitz@redhat.com>
> > > Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
> > >
> > > On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote:
> > > > This patch adds an AioContext parameter to xen_device_bind_event_channel()
> > > > and then uses aio_set_fd_handler() to set the callback rather than
> > > > qemu_set_fd_handler().
> > > >
> > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > > ---
> > > > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque)
> > > >  }
> > > >
> > > >  XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > > > +                                               AioContext *ctx,
> > > >                                                 unsigned int port,
> > > >                                                 XenEventHandler handler,
> > > >                                                 void *opaque, Error **errp)
> > > > @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > > >      channel->handler = handler;
> > > >      channel->opaque = opaque;
> > > >
> > > > -    qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
> > > > -                        channel);
> > > > +    channel->ctx = ctx;
> > > > +    aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
> > > > +                       xen_device_event, NULL, NULL, channel);
> > >
> > > I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
> > > `true' here, instead. That flag seems to be used when making a snapshot
> > > of a blockdev, for example.
> > >
> > > That was introduced by:
> > > dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586
> > >
> > > What do you think?
> >
> > Interesting. I admit I was merely transcribing what qemu_set_fd_handler() passes without really
> looking into the values. Looking at the arguments that virtio-blk passes to aio_set_event_notifier()
> though, and what 'is_external' means, it would appear that setting it to true is probably the right
> thing to do. Do you want me to send a v2 of the series or can you fix it up?
> 
> I'll fix that up.

Thanks,

  Paul

> 
> Reviewed-by: Anthony PERARD <anthony.perard@citrix.com>
> 
> Thanks,
> 
> --
> Anthony PERARD
Stefan Hajnoczi April 15, 2019, 8:42 a.m. UTC | #5
On Wed, Apr 10, 2019 at 03:20:05PM +0000, Paul Durrant wrote:
> > -----Original Message-----
> > From: Anthony PERARD [mailto:anthony.perard@citrix.com]
> > Sent: 10 April 2019 13:57
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: qemu-devel@nongnu.org; qemu-block@nongnu.org; xen-devel@lists.xenproject.org; Stefano Stabellini
> > <sstabellini@kernel.org>; Stefan Hajnoczi <stefanha@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Max
> > Reitz <mreitz@redhat.com>
> > Subject: Re: [PATCH 2/3] xen-bus: allow AioContext to be specified for each event channel
> > 
> > On Mon, Apr 08, 2019 at 04:16:16PM +0100, Paul Durrant wrote:
> > > This patch adds an AioContext parameter to xen_device_bind_event_channel()
> > > and then uses aio_set_fd_handler() to set the callback rather than
> > > qemu_set_fd_handler().
> > >
> > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > > ---
> > > @@ -943,6 +944,7 @@ static void xen_device_event(void *opaque)
> > >  }
> > >
> > >  XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > > +                                               AioContext *ctx,
> > >                                                 unsigned int port,
> > >                                                 XenEventHandler handler,
> > >                                                 void *opaque, Error **errp)
> > > @@ -968,8 +970,9 @@ XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
> > >      channel->handler = handler;
> > >      channel->opaque = opaque;
> > >
> > > -    qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
> > > -                        channel);
> > > +    channel->ctx = ctx;
> > > +    aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
> > > +                       xen_device_event, NULL, NULL, channel);
> > 
> > I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
> > `true' here, instead. That flag seems to be used when making a snapshot
> > of a blockdev, for example.
> > 
> > That was introduced by:
> > dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586
> > 
> > What do you think?
> 
> Interesting. I admit I was merely transcribing what qemu_set_fd_handler() passes without really looking into the values. Looking at the arguments that virtio-blk passes to aio_set_event_notifier() though, and what 'is_external' means, it would appear that setting it to true is probably the right thing to do. Do you want me to send a v2 of the series or can you fix it up?

Hi,
Handlers are invoked by the aio_poll() event loop.  Some handlers are
considered "external" in the sense that they submit new I/O requests
from the guest or outside world.  Others are considered "internal" in
the sense that they are part of the block layer and not an entry point
into the block layer.

There are points where the block layer wants to run the event loop but
new requests must not be submitted.  In this case aio_disable_external()
will be called so that "external" handlers are not processed.

For example, see virtio's virtio_queue_aio_set_host_notifier_handler().
This is the virtqueue kick ioeventfd and it shouldn't be processed when
aio_disable_external() has been called.

Stefan
Paul Durrant April 16, 2019, 9:36 a.m. UTC | #6
> -----Original Message-----
[snip]
> > > I wonder if the `'is_external' parameter of aio_set_fd_handler shoud be
> > > `true' here, instead. That flag seems to be used when making a snapshot
> > > of a blockdev, for example.
> > >
> > > That was introduced by:
> > > dca21ef23ba48f6f1428c59f295a857e5dc203c8^..c07bc2c1658fffeee08eb46402b2f66d55b07586
> > >
> > > What do you think?
> >
> > Interesting. I admit I was merely transcribing what qemu_set_fd_handler() passes without really
> looking into the values. Looking at the arguments that virtio-blk passes to aio_set_event_notifier()
> though, and what 'is_external' means, it would appear that setting it to true is probably the right
> thing to do. Do you want me to send a v2 of the series or can you fix it up?
> 
> Hi,
> Handlers are invoked by the aio_poll() event loop.  Some handlers are
> considered "external" in the sense that they submit new I/O requests
> from the guest or outside world.  Others are considered "internal" in
> the sense that they are part of the block layer and not an entry point
> into the block layer.
> 
> There are points where the block layer wants to run the event loop but
> new requests must not be submitted.  In this case aio_disable_external()
> will be called so that "external" handlers are not processed.
> 
> For example, see virtio's virtio_queue_aio_set_host_notifier_handler().
> This is the virtqueue kick ioeventfd and it shouldn't be processed when
> aio_disable_external() has been called.
> 

Thanks for the explanation Stefan. Xen event channels/shared rings should indeed be considered as external sources.

  Cheers,

    Paul

Patch
diff mbox series

diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
index bb8f1186e4..1046f965c4 100644
--- a/hw/block/dataplane/xen-block.c
+++ b/hw/block/dataplane/xen-block.c
@@ -802,7 +802,7 @@  void xen_block_dataplane_start(XenBlockDataPlane *dataplane,
     }
 
     dataplane->event_channel =
-        xen_device_bind_event_channel(xendev, event_channel,
+        xen_device_bind_event_channel(xendev, dataplane->ctx, event_channel,
                                       xen_block_dataplane_event, dataplane,
                                       &local_err);
     if (local_err) {
diff --git a/hw/xen/xen-bus.c b/hw/xen/xen-bus.c
index 9e391492ac..4f634d1291 100644
--- a/hw/xen/xen-bus.c
+++ b/hw/xen/xen-bus.c
@@ -924,6 +924,7 @@  done:
 
 struct XenEventChannel {
     QLIST_ENTRY(XenEventChannel) list;
+    AioContext *ctx;
     xenevtchn_handle *xeh;
     evtchn_port_t local_port;
     XenEventHandler handler;
@@ -943,6 +944,7 @@  static void xen_device_event(void *opaque)
 }
 
 XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
+                                               AioContext *ctx,
                                                unsigned int port,
                                                XenEventHandler handler,
                                                void *opaque, Error **errp)
@@ -968,8 +970,9 @@  XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
     channel->handler = handler;
     channel->opaque = opaque;
 
-    qemu_set_fd_handler(xenevtchn_fd(channel->xeh), xen_device_event, NULL,
-                        channel);
+    channel->ctx = ctx;
+    aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
+                       xen_device_event, NULL, NULL, channel);
 
     QLIST_INSERT_HEAD(&xendev->event_channels, channel, list);
 
@@ -1010,7 +1013,8 @@  void xen_device_unbind_event_channel(XenDevice *xendev,
 
     QLIST_REMOVE(channel, list);
 
-    qemu_set_fd_handler(xenevtchn_fd(channel->xeh), NULL, NULL, NULL);
+    aio_set_fd_handler(channel->ctx, xenevtchn_fd(channel->xeh), false,
+                       NULL, NULL, NULL, NULL);
 
     if (xenevtchn_unbind(channel->xeh, channel->local_port) < 0) {
         error_setg_errno(errp, errno, "xenevtchn_unbind failed");
diff --git a/include/hw/xen/xen-bus.h b/include/hw/xen/xen-bus.h
index 3315f0de20..8183b98c7d 100644
--- a/include/hw/xen/xen-bus.h
+++ b/include/hw/xen/xen-bus.h
@@ -122,6 +122,7 @@  void xen_device_copy_grant_refs(XenDevice *xendev, bool to_domain,
 typedef void (*XenEventHandler)(void *opaque);
 
 XenEventChannel *xen_device_bind_event_channel(XenDevice *xendev,
+                                               AioContext *ctx,
                                                unsigned int port,
                                                XenEventHandler handler,
                                                void *opaque, Error **errp);