Re: [Qemu-devel] [PATCH v2 0/3] virtio: proposal to optimize accesses to VQs

[Paolo Bonzini <pbonzini@redhat.com>]

On 16/12/2015 11:39, Vincenzo Maffione wrote:
> No problems.
> 
> I have some additional (orthogonal) curiosities:
> 
>   1) Assuming "hw/virtio/dataplane/vring.c" is what I think it is (VQ
> data structures directly accessible in the host virtual memory, with
> guest-phyisical-to-host-virtual mapping done statically at setup time)
> why isn't QEMU using this approach also for virtio-net? I see it is
> used by virtio-blk only.

Yes, it's a good idea.  vring.c's roots are in the old "virtio-blk
dataplane" code, which bypassed all of QEMU.  It had a separate event
loop, a separate I/O queue, a separate implementation of the memory map,
and a separate implementation of virtio.  virtio-blk dataplane is plenty
fast, which is why there isn't a vhost-blk.

Right now the only part that survives is the last one, which is vring.c.
 The event loop has been reconciled with AioContext, the I/O queue uses
BlockDriverState, and the memory map uses memory_region_find.

virtio-net dataplane also existed, as a possible competitor of
vhost-net.  However, vhost-net actually had better performance, so
virtio-net dataplane was never committed.  As Michael mentioned, in
practice on Linux you use vhost, and non-Linux hypervisors you do not
use QEMU. :)

Indeed the implementation in virtio.c does kind of suck.  On the other
hand, vring.c doesn't support migration because it doesn't know how to
mark guest memory as dirty.  And virtio.c is used by plenty of
devices---including virtio-blk and virtio-scsi unless you enable usage
of a separate I/O thread---and converting them to vring.c is bug-prone.
This is why I would like to drop vring.c and improve virtio.c, rather
than use vring.c even more.

The main optimization that vring.c has is to cache the translation of
the rings.  Using address_space_map/unmap for rings in virtio.c would be
a noticeable improvement, as your numbers for patch 3 show.  However, by
caching translations you also conveniently "forget" to promptly mark the
pages as dirty.  As you pointed out this is obviously an issue for
migration.  You can then add a notifier for runstate changes.  When
entering RUN_STATE_FINISH_MIGRATE or RUN_STATE_SAVE_VM the rings would
be unmapped, and then remapped the next time the VM starts running again.

You also guessed right that there are consistency issues; for these you
can add a MemoryListener that invalidates all mappings.

That said, I'm wondering where the cost of address translation lies---is
it cache-unfriendly data structures, locked operations, or simply too
much code to execute?  It was quite surprising to me that on virtio-blk
benchmarks we were spending 5% of the time doing memcpy! (I have just
extracted from my branch the patches to remove that, and sent them to
qemu-devel).

Examples of missing optimizations in exec.c include:

* caching enough information in RAM MemoryRegions to avoid the calls to
qemu_get_ram_block (e.g. replace mr->ram_addr with a RAMBlock pointer);

* adding a MRU cache to address_space_lookup_region.

In particular, the former should be easy if you want to give it a
try---easier than caching ring translations in virtio.c.

Paolo