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

[Michael S. Tsirkin]

On Thu, Apr 30, 2015 at 04:05:09PM +0800, Jason Wang wrote:
> 
> 
> On Tue, Apr 28, 2015 at 6:30 PM, Michael S. Tsirkin <address@hidden> 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
> >><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.
> >
> >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.

vhost_dev_log_resize is per device, isn't it?
So the log is synced in device 1, but not in device 2.

> 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.