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

[Jason Wang <jasowang@redhat.com>]

On Thu, Apr 30, 2015 at 4:09 PM, Michael S. Tsirkin <mst@redhat.com> 
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 
>> <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.
> 
> 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.