Replace GSource with AioContext for chardev

Replace GSource with AioContext for chardev

[Coiby Xu]

Hi,

I'm now implementing vhost-user block device backend
https://patchew.org/QEMU/20200309100342.14921-1-coiby.xu@gmail.com/
and want to use chardev to help manage vhost-user client connections
and read socket message. However there are two issues that need to be
addressed.

Firstly, chardev isn't suitable for the case when exported drive is
run in an IOThread because for mow chardev use GSource to dispatch
socket fd events. So I have to specify which IOThread the exported
drive is using when launching vhost-user block device backend,
for example, the following syntax will be used,

   -drive file=file.img,id=disk -device virtio-blk,drive=disk,iothread=iothread0 \
    -object vhost-user-blk-server,node-name=disk,chardev=mon1,iothread=iothread0 \
    -object iothread,id=iothread0 \
    -chardev socket,id=mon1,path=/tmp/vhost-user-blk_vhost.socket,server,nowait

then iothread_get_g_main_context(IOThread *iothread) has to be called
to run the gcontext in IOThread. If we use AioContext to dispatch socket
fd events, we needn't to specify IOThread twice. Besides aio_poll is faster
than g_main_loop_run.

Secondly, socket chardev's async read handler (set through
qemu_chr_fe_set_handlers) doesn't take the case of socket short read
into consideration.  I plan to add one which will make use qio_channel_yield.

According to
[1] Improving the QEMU Event Loop - Linux Foundation Events
http://events17.linuxfoundation.org/sites/events/files/slides/Improving%20the%20QEMU%20Event%20Loop%20-%203.pdf

"Convert chardev GSource to aio or an equivalent source" (p.30) should have
been finished. I'm curious why the plan didn't continue. If it's desirable,
I'm going to finish the leftover work to resolve the aforementioned two issues.

Any suggestion will be appreciated.
Thank you!

Re: Replace GSource with AioContext for chardev

[Marc-André Lureau]

Hi

On Thu, Apr 9, 2020 at 2:46 PM Coiby Xu <coiby.xu@gmail.com> wrote:
>
>
> Hi,
>
> I'm now implementing vhost-user block device backend
> https://patchew.org/QEMU/20200309100342.14921-1-coiby.xu@gmail.com/
> and want to use chardev to help manage vhost-user client connections
> and read socket message. However there are two issues that need to be
> addressed.
>
> Firstly, chardev isn't suitable for the case when exported drive is
> run in an IOThread because for mow chardev use GSource to dispatch
> socket fd events. So I have to specify which IOThread the exported
> drive is using when launching vhost-user block device backend,
> for example, the following syntax will be used,
>
>    -drive file=file.img,id=disk -device virtio-blk,drive=disk,iothread=iothread0 \
>     -object vhost-user-blk-server,node-name=disk,chardev=mon1,iothread=iothread0 \
>     -object iothread,id=iothread0 \
>     -chardev socket,id=mon1,path=/tmp/vhost-user-blk_vhost.socket,server,nowait
>
> then iothread_get_g_main_context(IOThread *iothread) has to be called
> to run the gcontext in IOThread. If we use AioContext to dispatch socket
> fd events, we needn't to specify IOThread twice. Besides aio_poll is faster
> than g_main_loop_run.
>
> Secondly, socket chardev's async read handler (set through
> qemu_chr_fe_set_handlers) doesn't take the case of socket short read
> into consideration.  I plan to add one which will make use qio_channel_yield.
>
> According to
> [1] Improving the QEMU Event Loop - Linux Foundation Events
> http://events17.linuxfoundation.org/sites/events/files/slides/Improving%20the%20QEMU%20Event%20Loop%20-%203.pdf
>
> "Convert chardev GSource to aio or an equivalent source" (p.30) should have
> been finished. I'm curious why the plan didn't continue. If it's desirable,
> I'm going to finish the leftover work to resolve the aforementioned two issues.

Converting all chardevs to Aio might be challenging, and doesn't bring
much benefits imho.

Perhaps a better approach would be to rely on a new chardev API to
steal the chardev underlying fd or QIO... (mostly keeping -chardev for
CLI/QMP compatibility reason - although breaking some chardev features
that imho aren't compatible with all use cases, like replay, muxing,
swapping etc). The chardev should probably be removed after that...

Re: Replace GSource with AioContext for chardev

[Daniel P. Berrangé]

On Thu, Apr 09, 2020 at 03:16:01PM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Apr 9, 2020 at 2:46 PM Coiby Xu <coiby.xu@gmail.com> wrote:
> >
> >
> > Hi,
> >
> > I'm now implementing vhost-user block device backend
> > https://patchew.org/QEMU/20200309100342.14921-1-coiby.xu@gmail.com/
> > and want to use chardev to help manage vhost-user client connections
> > and read socket message. However there are two issues that need to be
> > addressed.
> >
> > Firstly, chardev isn't suitable for the case when exported drive is
> > run in an IOThread because for mow chardev use GSource to dispatch
> > socket fd events. So I have to specify which IOThread the exported
> > drive is using when launching vhost-user block device backend,
> > for example, the following syntax will be used,
> >
> >    -drive file=file.img,id=disk -device virtio-blk,drive=disk,iothread=iothread0 \
> >     -object vhost-user-blk-server,node-name=disk,chardev=mon1,iothread=iothread0 \
> >     -object iothread,id=iothread0 \
> >     -chardev socket,id=mon1,path=/tmp/vhost-user-blk_vhost.socket,server,nowait
> >
> > then iothread_get_g_main_context(IOThread *iothread) has to be called
> > to run the gcontext in IOThread. If we use AioContext to dispatch socket
> > fd events, we needn't to specify IOThread twice. Besides aio_poll is faster
> > than g_main_loop_run.
> >
> > Secondly, socket chardev's async read handler (set through
> > qemu_chr_fe_set_handlers) doesn't take the case of socket short read
> > into consideration.  I plan to add one which will make use qio_channel_yield.
> >
> > According to
> > [1] Improving the QEMU Event Loop - Linux Foundation Events
> > http://events17.linuxfoundation.org/sites/events/files/slides/Improving%20the%20QEMU%20Event%20Loop%20-%203.pdf
> >
> > "Convert chardev GSource to aio or an equivalent source" (p.30) should have
> > been finished. I'm curious why the plan didn't continue. If it's desirable,
> > I'm going to finish the leftover work to resolve the aforementioned two issues.
> 
> Converting all chardevs to Aio might be challenging, and doesn't bring
> much benefits imho.
> 
> Perhaps a better approach would be to rely on a new chardev API to
> steal the chardev underlying fd or QIO... (mostly keeping -chardev for
> CLI/QMP compatibility reason - although breaking some chardev features
> that imho aren't compatible with all use cases, like replay, muxing,
> swapping etc). The chardev should probably be removed after that...

Yeah, I feel like it was a mistake for us to wire up many of our features
to chardevs. We mostly did it because -chardev provides a pre-existing
syntax for TCP/UNIX sockets and we didn't want to invent new CLI args.
IMHO this was a mistake in retrospect.

Unfortunately the -chardev API is absolutely terrible for any usage that
actually cares about the connection based semantics. Witness the horrible
hacks we do for re-connect and re-try when failing to initially connect
in vhost-user net code.

For features in QEMU where the only desirable chardev backend is one with
connection based, socket semantics, I think we would be better off using
the QIOChannel APIs directly and completely avoiding the chardev code.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Replace GSource with AioContext for chardev

[Markus Armbruster]

Daniel P. Berrangé <berrange@redhat.com> writes:

> On Thu, Apr 09, 2020 at 03:16:01PM +0200, Marc-André Lureau wrote:
>> Hi
>> 
>> On Thu, Apr 9, 2020 at 2:46 PM Coiby Xu <coiby.xu@gmail.com> wrote:
>> >
>> >
>> > Hi,
>> >
>> > I'm now implementing vhost-user block device backend
>> > https://patchew.org/QEMU/20200309100342.14921-1-coiby.xu@gmail.com/
>> > and want to use chardev to help manage vhost-user client connections
>> > and read socket message. However there are two issues that need to be
>> > addressed.
>> >
>> > Firstly, chardev isn't suitable for the case when exported drive is
>> > run in an IOThread because for mow chardev use GSource to dispatch
>> > socket fd events. So I have to specify which IOThread the exported
>> > drive is using when launching vhost-user block device backend,
>> > for example, the following syntax will be used,
>> >
>> >    -drive file=file.img,id=disk -device virtio-blk,drive=disk,iothread=iothread0 \
>> >     -object vhost-user-blk-server,node-name=disk,chardev=mon1,iothread=iothread0 \
>> >     -object iothread,id=iothread0 \
>> >     -chardev socket,id=mon1,path=/tmp/vhost-user-blk_vhost.socket,server,nowait
>> >
>> > then iothread_get_g_main_context(IOThread *iothread) has to be called
>> > to run the gcontext in IOThread. If we use AioContext to dispatch socket
>> > fd events, we needn't to specify IOThread twice. Besides aio_poll is faster
>> > than g_main_loop_run.
>> >
>> > Secondly, socket chardev's async read handler (set through
>> > qemu_chr_fe_set_handlers) doesn't take the case of socket short read
>> > into consideration.  I plan to add one which will make use qio_channel_yield.
>> >
>> > According to
>> > [1] Improving the QEMU Event Loop - Linux Foundation Events
>> > http://events17.linuxfoundation.org/sites/events/files/slides/Improving%20the%20QEMU%20Event%20Loop%20-%203.pdf
>> >
>> > "Convert chardev GSource to aio or an equivalent source" (p.30) should have
>> > been finished. I'm curious why the plan didn't continue. If it's desirable,
>> > I'm going to finish the leftover work to resolve the aforementioned two issues.
>> 
>> Converting all chardevs to Aio might be challenging, and doesn't bring
>> much benefits imho.
>> 
>> Perhaps a better approach would be to rely on a new chardev API to
>> steal the chardev underlying fd or QIO... (mostly keeping -chardev for
>> CLI/QMP compatibility reason - although breaking some chardev features
>> that imho aren't compatible with all use cases, like replay, muxing,
>> swapping etc). The chardev should probably be removed after that...
>
> Yeah, I feel like it was a mistake for us to wire up many of our features
> to chardevs. We mostly did it because -chardev provides a pre-existing
> syntax for TCP/UNIX sockets and we didn't want to invent new CLI args.
> IMHO this was a mistake in retrospect.
>
> Unfortunately the -chardev API is absolutely terrible for any usage that
> actually cares about the connection based semantics. Witness the horrible
> hacks we do for re-connect and re-try when failing to initially connect
> in vhost-user net code.
>
> For features in QEMU where the only desirable chardev backend is one with
> connection based, socket semantics, I think we would be better off using
> the QIOChannel APIs directly and completely avoiding the chardev code.

How do we get from here to there?

Re: Replace GSource with AioContext for chardev

[Daniel P. Berrangé]

On Tue, Apr 14, 2020 at 09:25:58AM +0200, Markus Armbruster wrote:
> Daniel P. Berrangé <berrange@redhat.com> writes:
> 
> > On Thu, Apr 09, 2020 at 03:16:01PM +0200, Marc-André Lureau wrote:
> >> Hi
> >> 
> >> On Thu, Apr 9, 2020 at 2:46 PM Coiby Xu <coiby.xu@gmail.com> wrote:
> >> >
> >> >
> >> > Hi,
> >> >
> >> > I'm now implementing vhost-user block device backend
> >> > https://patchew.org/QEMU/20200309100342.14921-1-coiby.xu@gmail.com/
> >> > and want to use chardev to help manage vhost-user client connections
> >> > and read socket message. However there are two issues that need to be
> >> > addressed.
> >> >
> >> > Firstly, chardev isn't suitable for the case when exported drive is
> >> > run in an IOThread because for mow chardev use GSource to dispatch
> >> > socket fd events. So I have to specify which IOThread the exported
> >> > drive is using when launching vhost-user block device backend,
> >> > for example, the following syntax will be used,
> >> >
> >> >    -drive file=file.img,id=disk -device virtio-blk,drive=disk,iothread=iothread0 \
> >> >     -object vhost-user-blk-server,node-name=disk,chardev=mon1,iothread=iothread0 \
> >> >     -object iothread,id=iothread0 \
> >> >     -chardev socket,id=mon1,path=/tmp/vhost-user-blk_vhost.socket,server,nowait
> >> >
> >> > then iothread_get_g_main_context(IOThread *iothread) has to be called
> >> > to run the gcontext in IOThread. If we use AioContext to dispatch socket
> >> > fd events, we needn't to specify IOThread twice. Besides aio_poll is faster
> >> > than g_main_loop_run.
> >> >
> >> > Secondly, socket chardev's async read handler (set through
> >> > qemu_chr_fe_set_handlers) doesn't take the case of socket short read
> >> > into consideration.  I plan to add one which will make use qio_channel_yield.
> >> >
> >> > According to
> >> > [1] Improving the QEMU Event Loop - Linux Foundation Events
> >> > http://events17.linuxfoundation.org/sites/events/files/slides/Improving%20the%20QEMU%20Event%20Loop%20-%203.pdf
> >> >
> >> > "Convert chardev GSource to aio or an equivalent source" (p.30) should have
> >> > been finished. I'm curious why the plan didn't continue. If it's desirable,
> >> > I'm going to finish the leftover work to resolve the aforementioned two issues.
> >> 
> >> Converting all chardevs to Aio might be challenging, and doesn't bring
> >> much benefits imho.
> >> 
> >> Perhaps a better approach would be to rely on a new chardev API to
> >> steal the chardev underlying fd or QIO... (mostly keeping -chardev for
> >> CLI/QMP compatibility reason - although breaking some chardev features
> >> that imho aren't compatible with all use cases, like replay, muxing,
> >> swapping etc). The chardev should probably be removed after that...
> >
> > Yeah, I feel like it was a mistake for us to wire up many of our features
> > to chardevs. We mostly did it because -chardev provides a pre-existing
> > syntax for TCP/UNIX sockets and we didn't want to invent new CLI args.
> > IMHO this was a mistake in retrospect.
> >
> > Unfortunately the -chardev API is absolutely terrible for any usage that
> > actually cares about the connection based semantics. Witness the horrible
> > hacks we do for re-connect and re-try when failing to initially connect
> > in vhost-user net code.
> >
> > For features in QEMU where the only desirable chardev backend is one with
> > connection based, socket semantics, I think we would be better off using
> > the QIOChannel APIs directly and completely avoiding the chardev code.
> 
> How do we get from here to there?

Ignoring back compat, what would be our ideal CLI syntax ?

Current syntax is

  -chardev socket,id=charnet1,path=/tmp/vhost1.sock
  -netdev vhost-user,chardev=charnet1,id=hostnet1 

Should we have an option that expresses a "SocketAddress" struct on the
CLI ?

   -socket type=unix,path=/tmp/vhost1.sock,id=sock0
   -netdev vhost-user,socket=sock0,id=hostnet1


IIUC, Marc-André is suggesting that we carry on using -chardev, but
detect when it is a socket chardev, and then ignore chardev APIs and
create a QIOChannel.  I can see some appeal in this as it provides a
way to get all existing usage switched over, but I feel uneasy about
sticking with -chardev forever, if we're not actually using a chardev.

We could do the magic -chardev -> -socket conversion though, for a
short period of time to ease the transition.

We would have to

 1. Introduce the new -socket and add "socket=$id" to devices that need it
 2. Deprecate -chardev with type != socket, with no repacement intended
 3. Deprecate -chardev with type == socket, translating to -socket
 ...wait 2 releases...
 4. Delete support for "chardev=$id" from devices with "socket=$id"

The hardest part is probably deciding exactly which set of devices can
be restricted to only sockets, and which must have the full range of
chardev backends available.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: Replace GSource with AioContext for chardev

[Paolo Bonzini]

On 14/04/20 12:27, Daniel P. Berrangé wrote:
> Ignoring back compat, what would be our ideal CLI syntax ?
> 
> Current syntax is
> 
>   -chardev socket,id=charnet1,path=/tmp/vhost1.sock
>   -netdev vhost-user,chardev=charnet1,id=hostnet1 
> 
> Should we have an option that expresses a "SocketAddress" struct on the
> CLI ?
> 
>    -socket type=unix,path=/tmp/vhost1.sock,id=sock0
>    -netdev vhost-user,socket=sock0,id=hostnet1

I think this should be just a "-object socket" that under the covers
creates a QIOChannel.  There are also ideas of switching "-chardev" to
"-object"; we could do the reverse of Marc-André's suggestion, and have
"chardev=" take both a "chardev-foo" object or a QIOChannel object
(converting the latter to a socket-based chardev).

IOW, the new "-object socket" QOM type can act as both a chardev or a
QIOChannel factory.  The C side of that should not be hard.

Paolo

> IIUC, Marc-André is suggesting that we carry on using -chardev, but
> detect when it is a socket chardev, and then ignore chardev APIs and
> create a QIOChannel.  I can see some appeal in this as it provides a
> way to get all existing usage switched over, but I feel uneasy about
> sticking with -chardev forever, if we're not actually using a chardev.
> 
> We could do the magic -chardev -> -socket conversion though, for a
> short period of time to ease the transition.
> 
> We would have to
> 
>  1. Introduce the new -socket and add "socket=$id" to devices that need it
>  2. Deprecate -chardev with type != socket, with no repacement intended
>  3. Deprecate -chardev with type == socket, translating to -socket
>  ...wait 2 releases...
>  4. Delete support for "chardev=$id" from devices with "socket=$id"
> 
> The hardest part is probably deciding exactly which set of devices can
> be restricted to only sockets, and which must have the full range of
> chardev backends available.
> 
> Regards,
> Daniel
> 

Re: Replace GSource with AioContext for chardev

[Kevin Wolf]

Am 14.04.2020 um 12:54 hat Paolo Bonzini geschrieben:
> On 14/04/20 12:27, Daniel P. Berrangé wrote:
> > Ignoring back compat, what would be our ideal CLI syntax ?
> > 
> > Current syntax is
> > 
> >   -chardev socket,id=charnet1,path=/tmp/vhost1.sock
> >   -netdev vhost-user,chardev=charnet1,id=hostnet1 
> > 
> > Should we have an option that expresses a "SocketAddress" struct on the
> > CLI ?
> > 
> >    -socket type=unix,path=/tmp/vhost1.sock,id=sock0
> >    -netdev vhost-user,socket=sock0,id=hostnet1
> 
> I think this should be just a "-object socket" that under the covers
> creates a QIOChannel.  There are also ideas of switching "-chardev" to
> "-object"; we could do the reverse of Marc-André's suggestion, and have
> "chardev=" take both a "chardev-foo" object or a QIOChannel object
> (converting the latter to a socket-based chardev).

Is this just an attempt to avoid nesting on the command line? Because I
don't see much use in socket objects that need to be managed separately
and require separate object-add/del commands.

While dotted syntax can make SocketAddress specifications a bit lengthy,
the obvious syntax is:

-netdev vhost-user,socket.type=unix,socket.path=/tmp/vhost1.sock,id=hostnet1

Assuming that this would be a QAPIfied -netdev, JSON is a readily
available alternative syntax.

Kevin

Re: Replace GSource with AioContext for chardev

[Daniel P. Berrangé]

On Tue, Apr 14, 2020 at 02:13:45PM +0200, Kevin Wolf wrote:
> Am 14.04.2020 um 12:54 hat Paolo Bonzini geschrieben:
> > On 14/04/20 12:27, Daniel P. Berrangé wrote:
> > > Ignoring back compat, what would be our ideal CLI syntax ?
> > > 
> > > Current syntax is
> > > 
> > >   -chardev socket,id=charnet1,path=/tmp/vhost1.sock
> > >   -netdev vhost-user,chardev=charnet1,id=hostnet1 
> > > 
> > > Should we have an option that expresses a "SocketAddress" struct on the
> > > CLI ?
> > > 
> > >    -socket type=unix,path=/tmp/vhost1.sock,id=sock0
> > >    -netdev vhost-user,socket=sock0,id=hostnet1
> > 
> > I think this should be just a "-object socket" that under the covers
> > creates a QIOChannel.  There are also ideas of switching "-chardev" to
> > "-object"; we could do the reverse of Marc-André's suggestion, and have
> > "chardev=" take both a "chardev-foo" object or a QIOChannel object
> > (converting the latter to a socket-based chardev).
> 
> Is this just an attempt to avoid nesting on the command line? Because I
> don't see much use in socket objects that need to be managed separately
> and require separate object-add/del commands.
> 
> While dotted syntax can make SocketAddress specifications a bit lengthy,
> the obvious syntax is:
> 
> -netdev vhost-user,socket.type=unix,socket.path=/tmp/vhost1.sock,id=hostnet1
> 
> Assuming that this would be a QAPIfied -netdev, JSON is a readily
> available alternative syntax.

I'd be fine with this approach too.  I'm not bothered about the dotted
syntax being verbose, as that battle is already lost, and can only be
solved by switching to a different config approach, such as reading
an actual json file.  IOW, we'd not be making life worse than it already
is.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|