| Message ID | 20190408151617.13025-3-paul.durrant@citrix.com |
|---|---|
| State | New |
| Headers | show |
| Series | Support IOThread polling for PV shared rings | expand |
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?
> -----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
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,
> -----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
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
> -----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
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);
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(-)