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

[Jason Wang <jasowang@redhat.com>]

On Tue, Apr 28, 2015 at 6:30 PM, Michael S. Tsirkin <mst@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 
>> <mst@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 <mst@redhat.com>
>>  >> Signed-off-by: Jason Wang <jasowang@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.