Re: [Qemu-devel] [PATCH] RFC: qio: Improve corking of TLS sessions

[Daniel P . Berrangé]

On Fri, Jun 07, 2019 at 05:14:14PM -0500, Eric Blake wrote:
> Our current implementation of qio_channel_set_cork() is pointless for
> TLS sessions: we block the underlying channel, but still hand things
> piecemeal to gnutls which then produces multiple encryption packets.
> Better is to directly use gnutls corking, which collects multiple
> inputs into a single encryption packet.
> 
> Signed-off-by: Eric Blake <address@hidden>
> 
> ---
> 
> RFC because unfortunately, I'm not sure I like the results.  My test
> case (using latest nbdkit.git) involves sending 10G of random data
> over NBD using parallel writes (and doing nothing on the server end;
> this is all about timing the data transfer):
> 
> $ dd if=/dev/urandom of=rand bs=1M count=10k
> $ time nbdkit -p 10810 --tls=require --tls-verify-peer \
>    --tls-psk=/tmp/keys.psk --filter=stats null 10g statsfile=/dev/stderr \
>    --run '~/qemu/qemu-img convert -f raw -W -n --target-image-opts \
>      --object tls-creds-psk,id=tls0,endpoint=client,dir=/tmp,username=eblake \
>      rand 
> driver=nbd,server.type=inet,server.host=localhost,server.port=10810,tls-creds=tls0'
> 
> Pre-patch, I measured:
> real  0m34.536s
> user  0m29.264s
> sys   0m4.014s
> 
> while post-patch, it changed to:
> real  0m36.055s
> user  0m27.685s
> sys   0m10.138s
> 
> Less time spent in user space, but for this particular qemu-img
> behavior (writing 2M chunks at a time), gnutls is now uncorking huge
> packets and the kernel is doing enough extra work that the overall
> program actually takes longer. :(

I wonder if the problem is that we're hitting a tradeoff between
time spent in encryption vs time spent in network I/O.

When we don't cork, the kernel has already sent the first packet
during the time gnutls is burning CPU encrypting the second packet.

When we do cork gnutls has to encrypt both packets before the kernel
can send anything.

> For smaller I/O patterns, the effects of corking are more likely to be
> beneficial, but I don't have a ready test case to produce that pattern
> (short of creating a guest running fio on a device backed by nbd).

I think it would probably be useful to see guest kernels with fio
and definitely see results for something closer to sector sized
I/O.

2 MB chunks is pretty unrealistic for a guest workload where there
will be lots of sector sized I/O. Sure QEMU / guest OS can merge
sectors, but it still feels like most I/O is going to be much smaller
than 2 MB.


> diff --git a/io/channel.c b/io/channel.c
> index 2a26c2a2c0b9..3510912465ac 100644
> --- a/io/channel.c
> +++ b/io/channel.c
> @@ -379,7 +379,16 @@ void qio_channel_set_cork(QIOChannel *ioc,
>      QIOChannelClass *klass = QIO_CHANNEL_GET_CLASS(ioc);
> 
>      if (klass->io_set_cork) {
> -        klass->io_set_cork(ioc, enabled);
> +        int r = klass->io_set_cork(ioc, enabled);
> +
> +        while (r == QIO_CHANNEL_ERR_BLOCK) {
> +            if (qemu_in_coroutine()) {
> +                qio_channel_yield(ioc, G_IO_OUT);
> +            } else {
> +                qio_channel_wait(ioc, G_IO_OUT);
> +            }
> +            r = klass->io_set_cork(ioc, enabled);
> +        }
>      }

Interesting - did you actually hit this EAGAIN behaviour ? I wouldn't
have expected anything to be pending in gnutls that needs retry. 


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 :|