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

[Michael S. Tsirkin]

On Thu, Apr 30, 2015 at 05:22:33PM +0800, Jason Wang wrote:
> 
> 
> On Thu, Apr 30, 2015 at 4:09 PM, Michael S. Tsirkin <address@hidden> wrote:
> >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?
> 
> Yes.
> 
> >
> >So the log is synced in device 1, but not in device 2.
> 
> But we will do resizing one by one for all listeners. So the sync of device
> 2 will happen soon afterwards.

yes but it will get the new log from vhost_log_get, not the old one,
and sync the new buffer not the old one.

-- 
MSt