Re: [Qemu-devel] [PATCH V2] vhost: logs sharing

[Jason Wang]

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. Then vhost_log_put() will drop the reference 
count to the old log.


 >
 >> +    } else {
 >> +        ++vhost_log->refcnt;
 >> +    }
 >> +
 >> +    return vhost_log;
 >> +}
 >> +
 >> +static void vhost_log_put(struct vhost_log *log)
 >> +{
 >> +    if (!log) {
 >> +        return;
 >> +    }
 >> +
 >> +    --log->refcnt;
 >> +    if (log->refcnt == 0) {
 >> +        if (vhost_log == log) {
 >> +            vhost_log = NULL;
 >> +        }
 >> +        g_free(log);
 >> +    }
 >> +}
 >>     static inline void vhost_dev_log_resize(struct vhost_dev* 
dev,
 >>uint64_t size)
 >>  {
 >> -    vhost_log_chunk_t *log;
 >> -    uint64_t log_base;
 >> +    struct vhost_log *log = vhost_log_get(size);
 >
 >At this point next device will try to use the
 >new log, but there is nothing there.
 
 vhost_log_get() will either allocate and return a new log if size 
is change
 or just increase the refcnt and use current log. So it works in 
fact?

But old log is lost and is not synced.

As I replied above, it was not lost and kept in vhost_log->log.