On Tue, Apr 28, 2015 at 6:30 PM, Michael S. Tsirkin
<m...@redhat.com> wrote:
>On Tue, Apr 28, 2015 at 05:58:28PM +0800, Jason Wang wrote:
>> On Tue, Apr 28, 2015 at 5:37 PM, Michael S. Tsirkin
>><m...@redhat.com> 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 <m...@redhat.com>
>> >> Signed-off-by: Jason Wang <jasow...@redhat.com>
>> >> ---
>> >> 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.
>
>You need a pointer to reference-count it.
>You return pointer to new object, no one references the old one.
The pointer is just vhost_log->log. The old pointer will be kept in
vhost_dev_log_resize() until 1) new log was set through ioctl and
2) the old
log was synced.
