On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin
<address@hidden> wrote:
>On Fri, Apr 10, 2015 at 05:33:35PM +0800, Jason Wang wrote:
>> Currently we allocate one vhost log per vhost device. This is sub
>> optimal when:
>> - Guest has several device with vhost as backend
>> - Guest has multiqueue devices
>> In the above cases, we can avoid the memory allocation by
sharing a
>> single vhost log among all the vhost devices. This is done
through:
>> - Introducing a new vhost_log structure with refcnt inside.
>> - Using a global pointer to vhost_log structure that will be
used. And
>> introduce helper to get the log with expected log size and
helper to
>> - drop the refcnt to the old log.
>> - Each vhost device still keep track of a pointer to the log
that was
>> used.
>> With above, if no resize happens, all vhost device will share a
>>single
>> vhost log. During resize, a new vhost_log structure will be
allocated
>> and made for the global pointer. And each vhost devices will
drop the
>> refcnt to the old log.
>> Tested by doing scp during migration for a 2 queues
virtio-net-pci.
>> Cc: Michael S. Tsirkin <address@hidden>
>> Signed-off-by: Jason Wang <address@hidden>
>> ---
>> Changes from V1:
>> - Drop the list of vhost log, instead, using a global pointer
instead
>
>I don't think it works like this. If you have a global pointer,
>you also need a global listener, have that sync all devices.
It doesn't conflict, see my comments below.
>
>
>
>> ---
>> hw/virtio/vhost.c | 66
>>++++++++++++++++++++++++++++++++++++++---------
>> include/hw/virtio/vhost.h | 9 ++++++-
>> 2 files changed, 62 insertions(+), 13 deletions(-)
>> diff --git a/hw/virtio/vhost.c b/hw/virtio/vhost.c
>> index 5a12861..e16c2db 100644
>> --- a/hw/virtio/vhost.c
>> +++ b/hw/virtio/vhost.c
>> @@ -22,15 +22,19 @@
>> #include "hw/virtio/virtio-bus.h"
>> #include "migration/migration.h"
>> +static struct vhost_log *vhost_log;
>> +
>> static void vhost_dev_sync_region(struct vhost_dev *dev,
>> MemoryRegionSection *section,
>> uint64_t mfirst, uint64_t
mlast,
>> uint64_t rfirst, uint64_t
rlast)
>> {
>> + vhost_log_chunk_t *log = dev->log->log;
>> +
>> uint64_t start = MAX(mfirst, rfirst);
>> uint64_t end = MIN(mlast, rlast);
>> - vhost_log_chunk_t *from = dev->log + start /
VHOST_LOG_CHUNK;
>> - vhost_log_chunk_t *to = dev->log + end / VHOST_LOG_CHUNK +
1;
>> + vhost_log_chunk_t *from = log + start / VHOST_LOG_CHUNK;
>> + vhost_log_chunk_t *to = log + end / VHOST_LOG_CHUNK + 1;
>> uint64_t addr = (start / VHOST_LOG_CHUNK) * VHOST_LOG_CHUNK;
>> if (end < start) {
>> @@ -280,22 +284,55 @@ static uint64_t vhost_get_log_size(struct
>>vhost_dev *dev)
>> }
>> return log_size;
>> }
>> +static struct vhost_log *vhost_log_alloc(uint64_t size)
>> +{
>> + struct vhost_log *log = g_malloc0(sizeof *log + size *
>>sizeof(*(log->log)));
>> +
>> + log->size = size;
>> + log->refcnt = 1;
>> +
>> + return log;
>> +}
>> +
>> +static struct vhost_log *vhost_log_get(uint64_t size)
>> +{
>> + if (!vhost_log || vhost_log->size != size) {
>> + vhost_log = vhost_log_alloc(size);
>
>This just leaks the old log if size != size.
But old log is reference counted and will be freed during
vhost_log_put() if
refcnt drops to zero.